Re: [Xen-devel] [PATCH v5 07/12] x86/hyperv: setup hypercall page

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jan 30, 2020 at 11:18:21AM +0000, Wei Liu wrote:
> On Thu, Jan 30, 2020 at 11:41:43AM +0100, Roger Pau Monné wrote:
> > On Wed, Jan 29, 2020 at 08:20:29PM +0000, Wei Liu wrote:
> > > Hyper-V uses a technique called overlay page for its hypercall page. It
> > > will insert a backing page to the guest when the hypercall functionality
> > > is enabled. That means we can use a page that is not backed by real
> > > memory for hypercall page.
> > > 
> > > Use the top-most addressable page for that purpose. Adjust e820 code
> > > accordingly.
> > > 
> > > We also need to register Xen's guest OS ID to Hyper-V. Use 0x3 as the
> > > vendor ID. Fix the comment in hyperv-tlfs.h while at it.
> > > 
> > > Signed-off-by: Wei Liu <liuwe@xxxxxxxxxxxxx>
> > > ---
> > > v5:
> > > 1. use hypervisor_reserve_top_pages
> > > 2. add a macro for hypercall page mfn
> > > 3. address other misc comments
> > > 
> > > v4:
> > > 1. Use fixmap
> > > 2. Follow routines listed in TLFS
> > > ---
> > >  xen/arch/x86/e820.c                     |  5 +++
> > >  xen/arch/x86/guest/hyperv/hyperv.c      | 57 +++++++++++++++++++++++--
> > >  xen/include/asm-x86/guest/hyperv-tlfs.h |  5 ++-
> > >  xen/include/asm-x86/guest/hyperv.h      |  3 ++
> > >  4 files changed, 65 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/xen/arch/x86/e820.c b/xen/arch/x86/e820.c
> > > index 3892c9cfb7..99643f3ea0 100644
> > > --- a/xen/arch/x86/e820.c
> > > +++ b/xen/arch/x86/e820.c
> > > @@ -343,6 +343,7 @@ static unsigned long __init find_max_pfn(void)
> > >  {
> > >      unsigned int i;
> > >      unsigned long max_pfn = 0;
> > > +    unsigned long top_pfn = ((1ull << paddr_bits) - 1) >> PAGE_SHIFT;
> > >  
> > >      for (i = 0; i < e820.nr_map; i++) {
> > >          unsigned long start, end;
> > > @@ -357,6 +358,10 @@ static unsigned long __init find_max_pfn(void)
> > >              max_pfn = end;
> > >      }
> > >  
> > > +    top_pfn -= hypervisor_reserve_top_pages();
> > > +    if ( max_pfn >= top_pfn )
> > > +        max_pfn = top_pfn;
> > 
> > Hm, I'm not sure I see the point of this. The value returned by
> > find_max_pfn is the maximum RAM address found in the memory map, but
> > the physical address you are using to map the hypercall page is almost
> > certainly much higher than the maximum address found in the physmap
> > (and certainly not RAM), and hence I'm not sure what's the point of
> > this.
> 
> Yes, the keyword is "almost certainly". :-)
> 
> This is done for correctness's sake. I don't expect in practice there
> would be a configuration that has that much memory, but correctness is
> still important.
> 
> It also guards against weird configuration in which memory is put into
> that part of the physical address space for whatever reason. I don't
> know why anyone would do that, but again, we should be prepared for
> that.
> 
> 
> > 
> > Also you haven't introduced a HyperV implementation of
> > hypervisor_reserve_top_pages so far, so it's hard to tell the intend
> > of this.
> 
> D'oh. That was supposed to be in this patch. I guess I forgot to commit
> that hunk!
> 
> That function for Hyper-V is going to return 1 (page).

But that would likely be wrong, unless the memory map has a RAM
region that expands up to (1 << paddr_bits)?

Or else you are just removing a page from the last RAM region in
the memory map for no reason. max_pfn is almost certainly way below (1
<< paddr_bits).

I think what you need is a hook that modifies the memory map and adds
a reserved region at ((1 << paddr_bits) - PAGE_SIZE) of size
PAGE_SIZE. See where pv_shim_fixup_e820 is used, and I think you want
to make this a hypervisor hook and add the HyperV code to reserve the
hypercall page in the e820 there.

> > 
> > > +
> > >      return max_pfn;
> > >  }
> > >  
> [...]
> > > diff --git a/xen/include/asm-x86/guest/hyperv.h 
> > > b/xen/include/asm-x86/guest/hyperv.h
> > > index c7a7f32bd5..0dcd8082ad 100644
> > > --- a/xen/include/asm-x86/guest/hyperv.h
> > > +++ b/xen/include/asm-x86/guest/hyperv.h
> > > @@ -21,6 +21,9 @@
> > >  
> > >  #include <xen/types.h>
> > >  
> > > +/* Use top-most MFN for hypercall page */
> > > +#define HV_HCALL_MFN (((1ull << paddr_bits) - 1) >> HV_HYP_PAGE_SHIFT)
> > 
> > I think you should use PAGE_SHIFT here, or else you also need to make
> > sure the fixmap reserved entry is of size ((1 << HV_HYP_PAGE_SHIFT) -
> > 1), and the call to set_fixmap_x in setup_hypercall_page needs to take
> > this into account AFAICT and not use PAGE_SHIFT.
> 
> PAGE_SHIFT and HV_HYP_PAGE_SHIFT are in fact the same.
> 
> I can add a BUILD_BUG_ON somewhere.

Yes please.

Thanks, Roger.

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