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

[Wei Liu <wl@xxxxxxx>]

On Thu, Jan 30, 2020 at 01:11:59PM +0100, Roger Pau Monné wrote:
> 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.

I like this idea. I will post a new version shortly.

Wei.

> 
> Thanks, Roger.

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