Re: [PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Jul 07, 2022 at 05:01:38PM +0200, Juergen Gross wrote:
> On 07.07.22 16:45, Anthony PERARD wrote:
> > On Fri, Jun 24, 2022 at 11:28:06AM +0200, Juergen Gross wrote:
> > > In case of maxmem != memsize the E820 map of the PVH stubdom is wrong,
> > > as it is missing the RAM above memsize.
> > > 
> > > Additionally the MMIO area should only cover the HVM special pages.
> > > 
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > >   tools/helpers/init-xenstore-domain.c | 16 ++++++++++------
> > >   1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tools/helpers/init-xenstore-domain.c 
> > > b/tools/helpers/init-xenstore-domain.c
> > > index b4f3c65a8a..dad8e43c42 100644
> > > --- a/tools/helpers/init-xenstore-domain.c
> > > +++ b/tools/helpers/init-xenstore-domain.c
> > > @@ -157,21 +158,24 @@ static int build(xc_interface *xch)
> > >           config.flags |= XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap;
> > >           config.arch.emulation_flags = XEN_X86_EMU_LAPIC;
> > >           dom->target_pages = mem_size >> XC_PAGE_SHIFT;
> > > -        dom->mmio_size = GB(4) - LAPIC_BASE_ADDRESS;
> > > +        dom->mmio_size = X86_HVM_NR_SPECIAL_PAGES << XC_PAGE_SHIFT;
> > >           dom->lowmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
> > >                             LAPIC_BASE_ADDRESS : mem_size;
> > >           dom->highmem_end = (mem_size > LAPIC_BASE_ADDRESS) ?
> > >                              GB(4) + mem_size - LAPIC_BASE_ADDRESS : 0;
> > > -        dom->mmio_start = LAPIC_BASE_ADDRESS;
> > > +        dom->mmio_start = (X86_HVM_END_SPECIAL_REGION -
> > > +                           X86_HVM_NR_SPECIAL_PAGES) << XC_PAGE_SHIFT;
> > >           dom->max_vcpus = 1;
> > >           e820[0].addr = 0;
> > > -        e820[0].size = dom->lowmem_end;
> > > +        e820[0].size = (max_size > LAPIC_BASE_ADDRESS) ?
> > > +                       LAPIC_BASE_ADDRESS : max_size;
> > >           e820[0].type = E820_RAM;
> > > -        e820[1].addr = LAPIC_BASE_ADDRESS;
> > > +        e820[1].addr = dom->mmio_start;
> > 
> > 
> > So, it isn't expected to have an entry covering the LAPIC addresses,
> > right? I guess not as seen in df1ca1dfe20.
> > 
> > But based on that same commit info, shouldn't the LAPIC address part of
> > `dom->mmio_start, dom->mmio_size`? (I don't know how dom->mmio_start is
> > used, yet, but maybe it's used by Xen or xen libraries to avoid
> > allocations in the wrong places)
> 
> In my understanding this is the purpose of lowmem_end.

It looks like dom->mmio_start is used in libxenguest.meminit_hvm() to
check if a super page can be allocated, but lowmem_end would probably
prevent the check from been done in the first place. So I guess
mmio_start value doesn't matter in this case.

There is another use of mmio_start, it's in module_init_one(), which I
think is used to blobs to the guest like the firmware or the acpi
tables. It's an other check to be sure the new "module" doesn't overlap
with the mmio region. So based on that, I kind of think that mmio_start
should include the LAPIC. But on the other end, I don't think
"init-xenstore-domain" have any "module", so again, the value of
mmio_start probably doesn't matter at this point.

But maybe we should keep LAPIC in [mmio_start,mmio_size] just in case it
matter later.

> OTOH I can modify the patch to be along the lines of df1ca1dfe20.
> 
> 
> Juergen

-- 
Anthony PERARD