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

[Daniel Kiper]

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>
> ---
> 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)
> V5: Use XEN_HVM_DEBUGCONS_IOPORT from Xen unstable (Roger Pau Monné)
>     Issue "System halted!" in panic (Daniel Kiper)
>     Clear interrupts and loop for halting (Roger Pau Monné, Daniel Kiper)
>     Use only one dummy variable for hypercall asm statement
> ---
>  grub-core/kern/i386/xen/pvh.c | 79 
> +++++++++++++++++++++++++++++++++++++++++++
>  include/xen/arch-x86/xen.h    |  7 ++++
>  2 files changed, 86 insertions(+)
>
> diff --git a/grub-core/kern/i386/xen/pvh.c b/grub-core/kern/i386/xen/pvh.c
> index 4f629b15e..478cef0d1 100644
> --- a/grub-core/kern/i386/xen/pvh.c
> +++ b/grub-core/kern/i386/xen/pvh.c
> @@ -20,15 +20,94 @@
>  #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 <xen/hvm/start_info.h>
>  #include <grub/machine/kernel.h>
>
>  grub_uint64_t grub_rsdp_addr;
>
> +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_cons_msg (const char *msg)
> +{
> +  const char *c;
> +
> +  for (c = msg; *c; c++)
> +    grub_outb (*c, XEN_HVM_DEBUGCONS_IOPORT);
> +}
> +
> +static void
> +grub_xen_panic (const char *msg)
> +{
> +  grub_xen_cons_msg (msg);
> +  grub_xen_cons_msg ("System halted!\n");
> +
> +  asm volatile ("cli");
> +
> +  while (1)
> +    {
> +      asm volatile ("hlt");
> +    }
> +}
> +
> +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");
> +}
> +
> +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);

Could not you use a constant instead of plain 2 here? Is there anything
like that in Xen headers? If not please add a comment what grub_cpuid ()
and wrmsr do. One line is sufficient.

> +  msr = ebx;
> +  pfn = (grub_uint32_t) (&hypercall_page);

Is it PFN? Really? I do not think so.

> +
> +  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, 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", ...?

Daniel