Re: [Xen-devel] [PATCH v2 4/5] arm: use r12 to pass the hypercall number

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 23 Feb 2012, Ian Campbell wrote:
> On Thu, 2012-02-23 at 18:21 +0000, Stefano Stabellini wrote:
> > Use r12 to pass the hypercall number and r0-r4 for the hypercall
> > arguments as usual.
> 
> Strictly speaking "as usual" in the ARM calling convention would be args
> in r0-r3 and the fifth (and subsequent) arguments on the stack. However
> we are free to implement our own convention for hypercalls and avoiding
> arguments on the stack is a good idea.

Yes, you are right. I meant "as before" here.


> Please could you add a comment describing the interface. X86 documents
> this in xen/include/public/arch-x86/xen-x86_{32,64}.h so I suppose
> xen/include/public/arch-arm.h is appropriate

Good idea.


> We should define precisely which registers are clobbered by a hypercall.
> X86 clobbers exactly those arguments registers which are used for that
> hypercall but perhaps we can simplify and always clobber r0..r4,r12
> (plus any other caller saved registers in the usual calling convention,
> just for sanity). r0..r3,r12 are already clobbered in the normal calling
> convention so the only difference would be clobbering r4 which is
> normally callee saved (but is also not normally used to pass arguments).
> 
> We should explicitly clobber whatever we say we will too in a debug
> build.

OK

> I've trimmed the quote already so I'll mention it here instead:
> XEN_HYPERCALL_TAG should be defined in the public interface too not in
> the private asm headers.

Sure


> > @@ -409,16 +408,17 @@ static void do_trap_hypercall(struct cpu_user_regs 
> > *regs, unsigned long iss)
> >  {
> >      local_irq_enable();
> >  
> > -    regs->r0 = arm_hypercall_table[iss](regs->r0,
> > +    if ( iss != XEN_HYPERCALL_TAG  ) {
> > +        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
> > +                __LINE__ , iss);
> 
>       regs->r0 = -EINVAL;
> 
> here I think.

yes, we need that


> > +        return;
> > +    }
> > +
> > +    regs->r0 = arm_hypercall_table[regs->r12](regs->r0,
> 
> It's an existing issue but we need to check that
> arm_hypercall_table[regs->r12] is non-NULL here and return -ENOSYS (by
> setting r0) if it is.

Yes, you are right.

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