Re: [PATCH v4 10/19] xen: setup hypercall page for PVH

[Juergen Gross]

On 09/11/2018 19:12, Daniel Kiper wrote:
> On Fri, Nov 02, 2018 at 01:37:29PM +0100, Juergen Gross wrote:
>> Add the needed code to setup the hypercall page for calling into the
>> Xen hypervisor.
>>
>> Signed-off-by: Juergen Gross <address@hidden>
>> ---
>> V3: grub_xen_early_halt->grub_xen_panic (Roger Pau Monné)
>>     issue panic message (Roger Pau Monné)
>>     rewrite grub_xen_hypercall to avoid register variables (Daniel Kiper)
>> ---
>>  grub-core/kern/i386/xen/pvh.c | 69 
>> +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 69 insertions(+)
>>
>> diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c
>> index ac6181f4e..7e90a4538 100644
>> --- a/grub-core/kern/i386/xen/pvh.c
>> +++ b/grub-core/kern/i386/xen/pvh.c
>> @@ -20,14 +20,83 @@
>>  #include <grub/misc.h>
>>  #include <grub/memory.h>
>>  #include <grub/mm.h>
>> +#include <grub/i386/cpuid.h>
>> +#include <grub/i386/io.h>
>>  #include <grub/xen.h>
>>  #include <grub/machine/kernel.h>
>>
>> +#define XEN_CONSOLE_PORT   0xe9
> 
> I think that this is not PVH specific thing. Could you move this to
> more generic Xen header?
> 
>>  grub_uint64_t grub_rsdp_addr;
> 
> Hmmm... It seems to me that immediately after patch #8 GRUB2 build is
> broken. Is not it?

No. xen-pvh isn't yet selectable.

> 
>> +static char hypercall_page[GRUB_XEN_PAGE_SIZE]
>> +  __attribute__ ((aligned (GRUB_XEN_PAGE_SIZE)));
>> +
>> +static grub_uint32_t xen_cpuid_base;
>> +
>> +static void
>> +grub_xen_panic (const char *msg)
>> +{
>> +  const char *c;
>> +
>> +  for (c = msg; *c; c++)
>> +    grub_outb (*c, XEN_CONSOLE_PORT);
>> +
>> +  asm volatile ("hlt");
> 
> Should not you do something similar to grub-core/lib/i386/halt.c:stop() here?

Yes, as already replied to Roger.

> 
>> +}
>> +
>> +static void
>> +grub_xen_cpuid_base (void)
>> +{
>> +  grub_uint32_t base, eax, signature[3];
>> +
>> +  for (base = 0x40000000; base < 0x40010000; base += 0x100)
>> +    {
>> +      grub_cpuid (base, eax, signature[0], signature[1], signature[2]);
>> +      if (!grub_memcmp ("XenVMMXenVMM", signature, 12) && (eax - base) >= 2)
>> +    {
>> +      xen_cpuid_base = base;
>> +      return;
>> +    }
>> +    }
>> +
>> +  grub_xen_panic ("Found no Xen signature.\n");
> 
> "Found no Xen signature!\nSystem halted!\n"
> 
> Or maybe grub_xen_panic() should always add "System halted!\n".

Okay (the latter).

> 
>> +}
>> +
>> +static void
>> +grub_xen_setup_hypercall_page (void)
>> +{
>> +  grub_uint32_t msr, pfn, eax, ebx, ecx, edx;
>> +
>> +  grub_cpuid (xen_cpuid_base + 2, eax, ebx, ecx, edx);
>> +  msr = ebx;
>> +  pfn = (grub_uint32_t) (&hypercall_page[0]);
> 
> Could not you use hypercall_page alone here?

Hmm, yes.

> 
>> +
>> +  asm volatile ("wrmsr" : : "c" (msr), "a" (pfn), "d" (0) : "memory");
>> +}
>> +
>> +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, __ign0, __ign1, __ign2, __ign3, __ign4;
>> +
>> +  asm volatile ("call *%[callno]"
>> +            : "=a" (__res), "=b" (__ign0), "=c" (__ign1), "=d" (__ign2),
>> +              "=S" (__ign3), "=D" (__ign4)
>> +            : "1" (a0), "2" (a1), "3" (a2), "4" (a3), "5" (a4),
> 
> I think that you can drop all __ign* variables if you specify proper
> registers in input argument. If this does not work you can use "+"
> modifier instead of "=" in the output argument.

I'll have a try.


Juergen