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:47:52AM +0000, Wei Liu wrote:
> 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.

Oh, OK, from your previous reply I understood that
hypervisor_reserve_top_pages would unconditionally return 1 for
HyperV, so that would end up always subtracting 1 page from the last
RAM region, even when not overlapping with (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.
> 
> That works for me too. Let's see what other people think.

I think that's the safest way, as you can assure there's nothing in
the region to be used by the hypercall page, and you can actually mark
it as reserved in the e820.

Thanks, Roger.

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