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

[Wei Liu <wl@xxxxxxx>]

On Thu, Jan 30, 2020 at 12:39:47PM +0100, Roger Pau Monné wrote:
> 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).
> 

Why? The adjustment will not be applied unless RAM overlaps with that
reserved region.

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

That works for me too. Let's see what other people think.

Wei.

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