1. Patchew
  2. Xen
  3. View series

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

Juergen Gross posted 1 patch 1 year, 9 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20220624092806.27700-1-jgross@suse.com
There is a newer version of this series
tools/helpers/init-xenstore-domain.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
[PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom
Posted by Juergen Gross 1 year, 9 months ago 
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@suse.com>
---
 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
@@ -71,8 +71,9 @@ static int build(xc_interface *xch)
     char cmdline[512];
     int rv, xs_fd;
     struct xc_dom_image *dom = NULL;
-    int limit_kb = (maxmem ? : (memory + 1)) * 1024;
+    int limit_kb = (maxmem ? : memory) * 1024 + X86_HVM_NR_SPECIAL_PAGES * 4;
     uint64_t mem_size = MB(memory);
+    uint64_t max_size = MB(maxmem);
     struct e820entry e820[3];
     struct xen_domctl_createdomain config = {
         .ssidref = SECINITSID_DOMU,
@@ -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;
         e820[1].size = dom->mmio_size;
         e820[1].type = E820_RESERVED;
         e820[2].addr = GB(4);
-        e820[2].size = dom->highmem_end - GB(4);
+        e820[2].size = (max_size > LAPIC_BASE_ADDRESS) ?
+                       max_size - LAPIC_BASE_ADDRESS : 0;
         e820[2].type = E820_RAM;
     }
 
-- 
2.35.3
Re: [PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom
Posted by Anthony PERARD 1 year, 9 months ago 
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@suse.com>
> ---
>  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)

Thanks,

-- 
Anthony PERARD
Re: [PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom
Posted by Juergen Gross 1 year, 9 months ago 
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@suse.com>
>> ---
>>   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.

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


Juergen
Re: [PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom
Posted by Anthony PERARD 1 year, 9 months ago 
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@suse.com>
> > > ---
> > >   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
Re: [PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom
Posted by Juergen Gross 1 year, 9 months ago 
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@suse.com>
>>>> ---
>>>>    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
Re: [PATCH] tools/init-xenstore-domain: fix memory map for PVH stubdom
Posted by Juergen Gross 1 year, 9 months ago 
Ping?

On 24.06.22 11:28, 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@suse.com>
> ---
>   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
> @@ -71,8 +71,9 @@ static int build(xc_interface *xch)
>       char cmdline[512];
>       int rv, xs_fd;
>       struct xc_dom_image *dom = NULL;
> -    int limit_kb = (maxmem ? : (memory + 1)) * 1024;
> +    int limit_kb = (maxmem ? : memory) * 1024 + X86_HVM_NR_SPECIAL_PAGES * 4;
>       uint64_t mem_size = MB(memory);
> +    uint64_t max_size = MB(maxmem);
>       struct e820entry e820[3];
>       struct xen_domctl_createdomain config = {
>           .ssidref = SECINITSID_DOMU,
> @@ -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;
>           e820[1].size = dom->mmio_size;
>           e820[1].type = E820_RESERVED;
>           e820[2].addr = GB(4);
> -        e820[2].size = dom->highmem_end - GB(4);
> +        e820[2].size = (max_size > LAPIC_BASE_ADDRESS) ?
> +                       max_size - LAPIC_BASE_ADDRESS : 0;
>           e820[2].type = E820_RAM;
>       }
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.