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

[Juergen Gross <jgross@xxxxxxxx>]

On 11.07.22 16:26, Anthony PERARD wrote:
> 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.
Okay, will send an updated patch version.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature