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

[Juergen Gross]

On 28/11/2018 12:28, Daniel Kiper wrote:
> 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.

Okay.


Juergen