Re: [Xen-devel] [PATCH 02/24] xen/arm: hypercalls

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2012-07-27 at 14:02 +0100, Stefano Stabellini wrote:
> On Fri, 27 Jul 2012, Ian Campbell wrote:
> > On Thu, 2012-07-26 at 17:33 +0100, Konrad Rzeszutek Wilk wrote:
> > > On Thu, Jul 26, 2012 at 04:33:44PM +0100, Stefano Stabellini wrote:
> > > > Use r12 to pass the hypercall number to the hypervisor.
> > > > 
> > > > We need a register to pass the hypercall number because we might not
> > > > know it at compile time and HVC only takes an immediate argument.
> > > > 
> > > > Among the available registers r12 seems to be the best choice because it
> > > > is defined as "intra-procedure call scratch register".
> > > > 
> > > > Use the ISS to pass an hypervisor specific tag.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > ---
> > > >  arch/arm/include/asm/xen/hypercall.h |   50 ++++++++++++++++++++++++++
> > > >  arch/arm/xen/Makefile                |    2 +-
> > > >  arch/arm/xen/hypercall.S             |   65 ++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 116 insertions(+), 1 deletions(-)
> > > >  create mode 100644 arch/arm/include/asm/xen/hypercall.h
> > > >  create mode 100644 arch/arm/xen/hypercall.S
> > > > 
> > > > diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
> > > > new file mode 100644
> > > > index 0000000..4ac0624
> > > > --- /dev/null
> > > > +++ b/arch/arm/include/asm/xen/hypercall.h
> > > > @@ -0,0 +1,50 @@
> > > > +/******************************************************************************
> > > > + * hypercall.h
> > > > + *
> > > > + * Linux-specific hypervisor handling.
> > > > + *
> > > > + * Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Citrix, 2012
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or
> > > > + * modify it under the terms of the GNU General Public License version 2
> > > > + * as published by the Free Software Foundation; or, when distributed
> > > > + * separately from the Linux kernel or incorporated into other
> > > > + * software packages, subject to the following license:
> > > > + *
> > > > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > > > + * of this source file (the "Software"), to deal in the Software without
> > > > + * restriction, including without limitation the rights to use, copy, modify,
> > > > + * merge, publish, distribute, sublicense, and/or sell copies of the Software,
> > > > + * and to permit persons to whom the Software is furnished to do so, subject to
> > > > + * the following conditions:
> > > > + *
> > > > + * The above copyright notice and this permission notice shall be included in
> > > > + * all copies or substantial portions of the Software.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> > > > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > > > + * IN THE SOFTWARE.
> > > > + */
> > > > +
> > > > +#ifndef _ASM_ARM_XEN_HYPERCALL_H
> > > > +#define _ASM_ARM_XEN_HYPERCALL_H
> > > > +
> > > > +#include <xen/interface/xen.h>
> > > > +
> > > > +long privcmd_call(unsigned call, unsigned long a1,
> > > > +		unsigned long a2, unsigned long a3,
> > > > +		unsigned long a4, unsigned long a5);
> > > > +int HYPERVISOR_xen_version(int cmd, void *arg);
> > > > +int HYPERVISOR_console_io(int cmd, int count, char *str);
> > > > +int HYPERVISOR_grant_table_op(unsigned int cmd, void *uop, unsigned int count);
> > > > +int HYPERVISOR_sched_op(int cmd, void *arg);
> > > > +int HYPERVISOR_event_channel_op(int cmd, void *arg);
> > > > +unsigned long HYPERVISOR_hvm_op(int op, void *arg);
> > > > +int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
> > > > +int HYPERVISOR_physdev_op(int cmd, void *arg);
> > > > +
> > > > +#endif /* _ASM_ARM_XEN_HYPERCALL_H */
> > > > diff --git a/arch/arm/xen/Makefile b/arch/arm/xen/Makefile
> > > > index 0bad594..b9d6acc 100644
> > > > --- a/arch/arm/xen/Makefile
> > > > +++ b/arch/arm/xen/Makefile
> > > > @@ -1 +1 @@
> > > > -obj-y		:= enlighten.o
> > > > +obj-y		:= enlighten.o hypercall.o
> > > > diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
> > > > new file mode 100644
> > > > index 0000000..038cc5b
> > > > --- /dev/null
> > > > +++ b/arch/arm/xen/hypercall.S
> > > > @@ -0,0 +1,65 @@
> > > > +/******************************************************************************
> > > > + * hypercall.S
> > > > + *
> > > > + * Xen hypercall wrappers
> > > > + *
> > > > + * The Xen hypercall calling convention is very similar to the ARM
> > > > + * procedure calling convention: the first paramter is passed in r0, the
> > > > + * second in r1, the third in r2 and the third in r3. Considering that
> > > 
> > > I think you meant 'and the fourth in r3'.
> > > 
> > > So where does the similarity end?  Just in that we use r12?
> > 
> > The standard ARM function calling convention is arguments 1-4 on r0-r3
> > and arguments 5+ on the stack. r12 is a scratch register which can be
> > clobbered by the *linker* on subroutine call (r12 is also called "ip"
> > the intra-procedure call scratch register).
> > 
> > The hypervisor doesn't want to be accessing hypercall arguments off the
> > guest stack, for obvious reasons, so we use r4 for the fifth argument
> > (and if we even implemented 6 argument hypercalls we'd use r5, etc).
> > There is no equivalent to the hypercall number in the procedure calling
> > convention so we picked r12 because it is up and out of the way and is
> > otherwise a scratch register. Obviously that you must not make a
> > procedure call between setting the hypercall number in r12 and calling
> > the hvc instruction.
> > 
> > > 
> > > > + * Xen hypercalls have 5 arguments at most, the fifth paramter is passed
> > > > + * in r4, differently from the procedure calling convention of using the
> > > 
> > > > + * stack for that case.
> > > > + *
> > > > + * The hypercall number is passed in r12.
> > > > + *
> > > > + * The return value is in r0.
> > > > + *
> > > > + * The hvc ISS is required to be 0xEA1, that is the Xen specific ARM
> > > > + * hypercall tag.
> > > > + *
> > > > + * Stefano Stabellini <stefano.stabellini@eu.citrix.com>, Citrix, 2012
> > > > + */
> > > > +
> > > > +#include <linux/linkage.h>
> > > > +#include <asm/assembler.h>
> > > > +#include <xen/interface/xen.h>
> > > > +
> > > > +
> > > > +/* HVC 0xEA1 */
> > > > +#ifdef CONFIG_THUMB2_KERNEL
> > > > +#define xen_hvc .word 0xf7e08ea1
> > > > +#else
> > > > +#define xen_hvc .word 0xe140ea71
> > > > +#endif
> > > > +
> > > > +/* We need to save and restore r4, because Xen clobbers it. */
> > > 
> > > Hmm, the comment says r4, but right below I see r12?
> > 
> > The ARM procedure calling convention allows a subroutine to clobber
> > r1..r3 (r0 is the return value) but not r4 which must be preserved. But
> > the hypervisor ABI clobbers all argument registers so the caller has to
> > specially preserve r4 in this context whenever there is a 5 argument
> > hypercall.
> > 
> > I presume that none of the hypercalls defined below have 5 arguments and
> > therefore we don't need to preserve r4 except in the generic
> > privcmd_call function. 
> > 
> > To be honest I prefer the style which we use on x86 which is to define
> > hypercall{0,1,2,3,4,5} macros and to wrap those with the specific names
> > using inline functions.
> > 
> > I find the x86 way more self documenting, and being in C prevents errors
> > around the number of arguments. It also allows for better in-lining and
> > exposes to gcc the actual clobbers, which might allow it to avoid saving
> > r4 on the stack at all etc.
> 
> Considering that we cannot do the same thing that we do on x86 (see this
> thread http://marc.info/?l=linux-kernel&m=133052035426427&w=2),

I'd forgotten all about this arm-gcc braindamage.

>  I
> decided to go for the assembly implementation because it is much shorter
> and easier to understand (for me at least, being just 3 lines of code in
> the generic case and just one macro) and this way we can exploit the
> code generated by gcc to put the arguments in the right registers.
> 
> Also I like the fact that it is the same strategy used by libc to issue
> syscalls.

Fair enough.

> As you can see it results in 3 lines of code for all the hypercalls
> except the ones that might take more than 4 arguments, that right now is
> just privcmd.
> 
> 
> 
> > > Should this comment be by 'privcmd_call'?
> > 
> > When we add a 5 argument hypercall I suppose we'll see the required
> > push/pop of r4 added to this macro too.
> 
> For performance and simplicity I would add a second macro that push/pop
> r4, only required for hypercalls with more than 4 arguments.

For clarity / documentation purposes it might actually be worthwhile to
define all of HYPERCALL{0,1,2,3,4} even if the {0,1,2,3} cases are all
just:
        #define HYPERCALL0(x) HYPERCALL_SIMPLE(x)

> > > > +#define HYPERCALL(hypercall)			\
> > > > +ENTRY(HYPERVISOR_##hypercall)			\
> > > > +	mov r12, #__HYPERVISOR_##hypercall;	\
> > > > +	xen_hvc;							\
> > > > +	mov pc, lr;							\
> > > > +ENDPROC(HYPERVISOR_##hypercall)
> > > > +
> > > > +                .text
> > > > +
> > > > +HYPERCALL(xen_version);
> > > > +HYPERCALL(console_io);
> > > > +HYPERCALL(grant_table_op);
> > > > +HYPERCALL(sched_op);
> > > > +HYPERCALL(event_channel_op);
> > > > +HYPERCALL(hvm_op);
> > > > +HYPERCALL(memory_op);
> > > > +HYPERCALL(physdev_op);
> > > > +
> > > > +ENTRY(privcmd_call)
> > > > +	stmdb	sp!, {r4}
> > > > +	mov r12, r0
> > > > +	mov r0, r1
> > > > +	mov r1, r2
> > > > +	mov r2, r3
> > > > +	ldr r3, [sp, #8]
> > > > +	ldr r4, [sp, #4]
> > > > +	xen_hvc
> > > > +	pop {r4}
> > 
> > Why not ldmdb for symmetry?
> 
> Yep, I can do that.