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

[Dave Martin <dave.martin@xxxxxxxxxx>]

On Thu, Aug 09, 2012 at 04:37:24PM +0100, Stefano Stabellini wrote:
> On Wed, 8 Aug 2012, Dave Martin wrote:
> > On Mon, Aug 06, 2012 at 03:27:05PM +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.
> > > 
> > > Changes in v2:
> > > - define an HYPERCALL macro for 5 arguments hypercall wrappers, even if
> > > at the moment is unused;
> > > - use ldm instead of pop;
> > > - fix up comments.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > ---
> > >  arch/arm/include/asm/xen/hypercall.h |   50 ++++++++++++++++
> > >  arch/arm/xen/Makefile                |    2 +-
> > >  arch/arm/xen/hypercall.S             |  106 
> > > ++++++++++++++++++++++++++++++++++
> > >  3 files changed, 157 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/xen/hypercall.S b/arch/arm/xen/hypercall.S
> > > new file mode 100644
> > > index 0000000..074f5ed
> > > --- /dev/null
> > > +++ b/arch/arm/xen/hypercall.S
> > > @@ -0,0 +1,106 @@
> > > +/******************************************************************************
> > > + * hypercall.S
> > > + *
> > > + * Xen hypercall wrappers
> > > + *
> > > + * Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>, 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.
> > > + */
> > > +
> > > +/*
> > > + * 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 fourth in r3. Considering that
> > > + * 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.
> > > + */
> > > +
> > > +#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
> > 
> > Consider using my opcode injection helpers patch for this (see
> > separate repost: [PATCH v2 REPOST 0/4] ARM: opcodes: Facilitate custom
> > opcode injection), assuming that nobody objects to it.  This should mean
> > that the right opcodes get generated when building a kernel for a big-
> > endian target for example.
> > 
> > I believe the __HVC(imm) macro which I put in <asm/opcodes-virt.h> as an
> > example should do what you need in this case.
> 
> Sure I can do that. Maybe I'll add another patch at the end of my series
> to replace xen_hvc with __HVC(0xEA1), so that it remains independent
> from your series.
> I have learned through experience that avoiding cross patch series
> dependencies help to reduce the amount of headaches during merge windows
> :)

I agree.  I'll let you know when my patch gets merged -- in the meantime,
it makes sense for you to keep your existing code.

> 
> 
> > > +
> > > +#define HYPERCALL_SIMPLE(hypercall)              \
> > > +ENTRY(HYPERVISOR_##hypercall)                    \
> > > + mov r12, #__HYPERVISOR_##hypercall;     \
> > > + xen_hvc;                                                        \
> > > + mov pc, lr;                                                     \
> > > +ENDPROC(HYPERVISOR_##hypercall)
> > > +
> > > +#define HYPERCALL0 HYPERCALL_SIMPLE
> > > +#define HYPERCALL1 HYPERCALL_SIMPLE
> > > +#define HYPERCALL2 HYPERCALL_SIMPLE
> > > +#define HYPERCALL3 HYPERCALL_SIMPLE
> > > +#define HYPERCALL4 HYPERCALL_SIMPLE
> > > +
> > > +#define HYPERCALL5(hypercall)                    \
> > > +ENTRY(HYPERVISOR_##hypercall)                    \
> > > + stmdb sp!, {r4}                                         \
> > > + ldr r4, [sp, #4]                                        \
> > > + mov r12, #__HYPERVISOR_##hypercall;     \
> > > + xen_hvc                                                         \
> > > + ldm sp!, {r4}                                           \
> > > + mov pc, lr                                                      \
> > > +ENDPROC(HYPERVISOR_##hypercall)
> > > +
> > > +                .text
> > > +
> > > +HYPERCALL2(xen_version);
> > > +HYPERCALL3(console_io);
> > > +HYPERCALL3(grant_table_op);
> > > +HYPERCALL2(sched_op);
> > > +HYPERCALL2(event_channel_op);
> > > +HYPERCALL2(hvm_op);
> > > +HYPERCALL2(memory_op);
> > > +HYPERCALL2(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
> > > + ldm sp!, {r4}
> > > + mov pc, lr
> > 
> > Note that the preferred entry/exit sequences in such cases are:
> > 
> >     stmfd   sp!, {r4,lr}
> >     ...
> >     ldmfd   sp!, {r4,pc}
> > 
> > ...but it works either way.  I would bother to change it unless you
> > have other changes to make too.
> 
> Wouldn't this needlessly save and restore one more register (lr) to the
> stack?
> I would try to keep the hypercall wrappers as small as possible...

Argh, ignore me -- I was hallucinating for some reason that we actually
needed to save lr, but we don't.

Using the stmfd/ldmfd mnemonics might still be nicer than stmdb/ldm, since
the fd suffix makes the stack semantics more obvious, and the code
looks more symmetrical.  This was the conventional way to write these
mnemonics before the "push" and "pop" mnemonics existed.

That's purely cosmetic, though.

Cheers
---Dave

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel