Re: [PATCH v5 11/20] xen: setup hypercall page for PVH

[Daniel Kiper]

On Tue, Nov 27, 2018 at 09:31:10PM +0100, Daniel Kiper wrote:
> On Wed, Nov 21, 2018 at 03:28:46PM +0100, Juergen Gross wrote:
> > Add the needed code to setup the hypercall page for calling into the
> > Xen hypervisor.
> >
> > Import the XEN_HVM_DEBUGCONS_IOPORT define from Xen unstable into
> > include/xen/arch-x86/xen.h
> >
> > Signed-off-by: Juergen Gross <address@hidden>

[...]

> > +int
> > +grub_xen_hypercall (grub_uint32_t callno, grub_uint32_t a0,
> > +               grub_uint32_t a1, grub_uint32_t a2,
> > +               grub_uint32_t a3, grub_uint32_t a4,
> > +               grub_uint32_t a5 __attribute__ ((unused)))
> > +{
> > +  grub_uint32_t __res, dummy;
> > +
> > +  asm volatile ("call *%[callno]"
> > +           : "=a" (__res), "=b" (dummy), "=c" (dummy), "=d" (dummy),
> > +             "=S" (dummy), "=D" (dummy)
> > +           : "1" (a0), "2" (a1), "3" (a2), "4" (a3), "5" (a4),
> > +             [callno] "a" (&hypercall_page[callno * 32])
> > +           : "memory");
>
> Have you tried "+b", "+c", ... instead of "=b", "=c", ...?

I have done some experiments. Your code:

  grub_uint32_t __res, dummy;

  asm volatile ("call *%[callno]"
                : "=a" (__res), "=b" (dummy), "=c" (dummy), "=d" (dummy),
                  "=S" (dummy), "=D" (dummy)
                : "1" (a0), "2" (a1), "3" (a2), "4" (a3), "5" (a4),
                  [callno] "a" (&hypercall_page[callno * 32])
                : "memory");

... generates this assembly:

0000048e <grub_xen_hypercall>:
 48e:   55                      push   %ebp
 48f:   89 e5                   mov    %esp,%ebp
 491:   57                      push   %edi
 492:   56                      push   %esi
 493:   53                      push   %ebx
 494:   c1 e0 05                shl    $0x5,%eax
 497:   05 00 10 00 00          add    $0x1000,%eax
 49c:   89 d3                   mov    %edx,%ebx
 49e:   8b 55 08                mov    0x8(%ebp),%edx
 4a1:   8b 75 0c                mov    0xc(%ebp),%esi
 4a4:   8b 7d 10                mov    0x10(%ebp),%edi
 4a7:   ff d0                   call   *%eax
 4a9:   5b                      pop    %ebx
 4aa:   5e                      pop    %esi
 4ab:   5f                      pop    %edi
 4ac:   5d                      pop    %ebp
 4ad:   c2 10 00                ret    $0x10

Mine code:

  grub_uint32_t __res;

  asm volatile ("call *%[callno]"
                : "=a" (__res), "+b" (a0), "+c" (a1), "+d" (a2), "+S" (a3), 
"+D" (a4)
                : [callno] "rm" (&hypercall_page[callno * 32])
                : "memory");

... generates this assembly:

0000048e <grub_xen_hypercall>:
 48e:   55                      push   %ebp
 48f:   89 e5                   mov    %esp,%ebp
 491:   57                      push   %edi
 492:   56                      push   %esi
 493:   53                      push   %ebx
 494:   c1 e0 05                shl    $0x5,%eax
 497:   05 00 10 00 00          add    $0x1000,%eax
 49c:   89 d3                   mov    %edx,%ebx
 49e:   8b 55 08                mov    0x8(%ebp),%edx
 4a1:   8b 75 0c                mov    0xc(%ebp),%esi
 4a4:   8b 7d 10                mov    0x10(%ebp),%edi
 4a7:   ff d0                   call   *%eax
 4a9:   5b                      pop    %ebx
 4aa:   5e                      pop    %esi
 4ab:   5f                      pop    %edi
 4ac:   5d                      pop    %ebp
 4ad:   c2 10 00                ret    $0x10

So, both are equal but mine seems a bit simpler.

And I think that you can drop "__" from __res variable.

Daniel