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

[Wei Liu <wl@xxxxxxx>]

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).

> 
> > +
> >      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.

Wei.

> 
> Thanks, Roger.

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