Re: [Xen-devel] [PATCH v4 2/7] x86/hyperv: setup hypercall page

[Michael Kelley <mikelley@xxxxxxxxxxxxx>]

From: Jan Beulich <jbeulich@xxxxxxxx> Sent: Thursday, January 23, 2020 3:19 AM
> 
> On 22.01.2020 21:23, Wei Liu wrote:
> > --- a/xen/arch/x86/e820.c
> > +++ b/xen/arch/x86/e820.c
> > @@ -36,6 +36,22 @@ boolean_param("e820-verbose", e820_verbose);
> >  struct e820map e820;
> >  struct e820map __initdata e820_raw;
> >
> > +static unsigned int find_phys_addr_bits(void)
> > +{
> > +    uint32_t eax;
> > +    unsigned int phys_bits = 36;
> > +
> > +    eax = cpuid_eax(0x80000000);
> > +    if ( (eax >> 16) == 0x8000 && eax >= 0x80000008 )
> > +    {
> > +        phys_bits = (uint8_t)cpuid_eax(0x80000008);
> > +        if ( phys_bits > PADDR_BITS )
> > +            phys_bits = PADDR_BITS;
> > +    }
> > +
> > +    return phys_bits;
> > +}
> 
> Instead of this, how about pulling further ahead the call to
> early_cpu_init() in setup.c? (Otherwise the function wants to
> be __init at least.)
> 
> > @@ -357,6 +373,21 @@ static unsigned long __init find_max_pfn(void)
> >              max_pfn = end;
> >      }
> >
> > +#ifdef CONFIG_HYPERV_GUEST
> > +    {
> > +   /*
> > +    * We reserve the top-most page for hypercall page. Adjust
> > +    * max_pfn if necessary.
> > +    */
> > +        unsigned int phys_bits = find_phys_addr_bits();
> > +        unsigned long hcall_pfn =
> > +          ((1ull << phys_bits) - 1) >> PAGE_SHIFT;
> > +
> > +        if ( max_pfn >= hcall_pfn )
> > +          max_pfn = hcall_pfn - 1;
> > +    }
> > +#endif
> 
> This wants abstracting away: There shouldn't be Hyper-V specific
> code in here if at all possible, and the adjustment also shouldn't
> be made unless actually running on Hyper-V.
> 
> > --- a/xen/arch/x86/guest/hyperv/hyperv.c
> > +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> > @@ -18,17 +18,27 @@
> >   *
> >   * Copyright (c) 2019 Microsoft.
> >   */
> > +#include <xen/version.h>
> >  #include <xen/init.h>
> 
> Please sort alphabetically.
> 
> > +#include <asm/fixmap.h>
> >  #include <asm/guest.h>
> >  #include <asm/guest/hyperv-tlfs.h>
> > +#include <asm/processor.h>
> >
> >  struct ms_hyperv_info __read_mostly ms_hyperv;
> >
> > -static const struct hypervisor_ops ops = {
> > -    .name = "Hyper-V",
> > -};
> > +static uint64_t generate_guest_id(void)
> > +{
> > +    uint64_t id = 0;
> 
> Pointless initializer.
> 
> > +    id = (uint64_t)HV_XEN_VENDOR_ID << 48;
> > +    id |= (xen_major_version() << 16) | xen_minor_version();
> > +
> > +    return id;
> > +}
> >
> > +static const struct hypervisor_ops ops;
> >  const struct hypervisor_ops *__init hyperv_probe(void)
> 
> Blank line between these two please (if you can't get away without
> this declaration in the first place).
> 
> > @@ -72,6 +82,43 @@ const struct hypervisor_ops *__init hyperv_probe(void)
> >      return &ops;
> >  }
> >
> > +static void __init setup_hypercall_page(void)
> > +{
> > +    union hv_x64_msr_hypercall_contents hypercall_msr;
> > +    union hv_guest_os_id guest_id;
> > +    unsigned long mfn;
> > +
> > +    rdmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> > +    if ( !guest_id.raw )
> > +    {
> > +        guest_id.raw = generate_guest_id();
> > +        wrmsrl(HV_X64_MSR_GUEST_OS_ID, guest_id.raw);
> > +    }
> > +
> > +    rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> > +    if ( !hypercall_msr.enable )
> > +    {
> > +        mfn = ((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT;
> 
> Along the lines of the abstracting-away request above: How is
> anyone to notice what else needs changing if it is decided
> that this page gets moved elsewhere?
> 
> > +        hypercall_msr.enable = 1;
> > +        hypercall_msr.guest_physical_address = mfn;
> > +        wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> 
> So on Hyper-V the hypervisor magically populates this page as
> a side effect of the MSR write?
> 

Yes, that's exactly what happens. :-)  Hyper-V takes the guest
physical address and remaps it to a different physical page that
Hyper-V has set up with whatever is needed to execute the hypercall
sequence. The original physical page corresponding to the guest physical
address is not modified, but it remains unused and inaccessible to the
guest.  When/if the MSR is written again with the enable flag set to 0,
the mapping is flipped back, and the original physical page, with its
original contents, reappears. There are a few other pages with this
same behavior.  The Hyper-V TLFS refers to these "GPA Overlay
Pages".  See Section 5.2.1 of the TLFS.

Michael
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel