Mailing List Archive: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU

[PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU

Mar 7, 2024, 5:54 PM

Post #1 of 5 (15 views)

For 1:1 direct-mapped dom0less DomUs, the magic pages should not clash
with any RAM region. To find a proper region for guest magic pages,
we can reuse the logic of finding domain extended regions.

Extract the logic of finding domain extended regions to a helper
function named find_unused_memory() and use it to find unallocated
spaces for magic pages before make_hypervisor_node(). The result magic
page region is added to the reserved memory section of the bootinfo so
that it is carved out from the extended regions.

Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
Signed-off-by: Henry Wang <xin.wang2@amd.com>
---
v2:
- New patch
---
xen/arch/arm/dom0less-build.c | 43 +++++++++++++++++++++++++
xen/arch/arm/domain_build.c | 30 ++++++++++-------
xen/arch/arm/include/asm/domain_build.h | 2 ++
3 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index 1e1c8d83ae..99447bfb0c 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)

if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
{
+ if ( is_domain_direct_mapped(d) )
+ {
+ struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
+ struct meminfo *rsrv_mem = &bootinfo.reserved_mem;
+ struct mem_map_domain *mem_map = &d->arch.mem_map;
+ uint64_t magic_region_start = INVALID_PADDR;
+ uint64_t magic_region_size = GUEST_MAGIC_SIZE;
+ unsigned int i;
+
+ if ( !avail_magic_regions )
+ return -ENOMEM;
+
+ ret = find_unused_memory(d, kinfo, avail_magic_regions);
+ if ( ret )
+ {
+ printk(XENLOG_WARNING
+ "%pd: failed to find a region for domain magic pages\n",
+ d);
+ goto err;
+ }
+
+ magic_region_start = avail_magic_regions->bank[0].start;
+
+ /*
+ * Register the magic region as reserved mem to make sure this
+ * region will not be counted when allocating extended regions.
+ */
+ rsrv_mem->bank[rsrv_mem->nr_banks].start = magic_region_start;
+ rsrv_mem->bank[rsrv_mem->nr_banks].size = magic_region_size;
+ rsrv_mem->bank[rsrv_mem->nr_banks].type = MEMBANK_DEFAULT;
+ rsrv_mem->nr_banks++;
+
+ /* Update the domain memory map. */
+ for ( i = 0; i < mem_map->nr_mem_regions; i++ )
+ {
+ if ( mem_map->regions[i].type == GUEST_MEM_REGION_MAGIC )
+ {
+ mem_map->regions[i].start = magic_region_start;
+ mem_map->regions[i].size = magic_region_size;
+ }
+ }
+ }
+
ret = make_hypervisor_node(d, kinfo, addrcells, sizecells);
if ( ret )
goto err;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 085d88671e..b36b98ee7d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1110,6 +1110,24 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
return res;
}

+int __init find_unused_memory(struct domain *d, const struct kernel_info *kinfo,
+ struct meminfo *mem_region)
+{
+ int res;
+
+ if ( is_domain_direct_mapped(d) )
+ {
+ if ( !is_iommu_enabled(d) )
+ res = find_unallocated_memory(kinfo, mem_region);
+ else
+ res = find_memory_holes(kinfo, mem_region);
+ }
+ else
+ res = find_domU_holes(kinfo, mem_region);
+
+ return res;
+}
+
int __init make_hypervisor_node(struct domain *d,
const struct kernel_info *kinfo,
int addrcells, int sizecells)
@@ -1161,17 +1179,7 @@ int __init make_hypervisor_node(struct domain *d,
if ( !ext_regions )
return -ENOMEM;

- if ( is_domain_direct_mapped(d) )
- {
- if ( !is_iommu_enabled(d) )
- res = find_unallocated_memory(kinfo, ext_regions);
- else
- res = find_memory_holes(kinfo, ext_regions);
- }
- else
- {
- res = find_domU_holes(kinfo, ext_regions);
- }
+ res = find_unused_memory(d, kinfo, ext_regions);

if ( res )
printk(XENLOG_WARNING "%pd: failed to allocate extended regions\n",
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index da9e6025f3..4458012644 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -10,6 +10,8 @@ bool allocate_bank_memory(struct domain *d, struct kernel_info *kinfo,
gfn_t sgfn, paddr_t tot_size);
int construct_domain(struct domain *d, struct kernel_info *kinfo);
int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
+int find_unused_memory(struct domain *d, const struct kernel_info *kinfo,
+ struct meminfo *mem_region);
int make_chosen_node(const struct kernel_info *kinfo);
int make_cpus_node(const struct domain *d, void *fdt);
int make_hypervisor_node(struct domain *d, const struct kernel_info *kinfo,
--
2.34.1

Re: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU [ In reply to ]

michal.orzel at amd

Mar 11, 2024, 6:46 AM

Post #2 of 5 (11 views)

Permalink

Hi Henry,

On 08/03/2024 02:54, Henry Wang wrote:
> For 1:1 direct-mapped dom0less DomUs, the magic pages should not clash
> with any RAM region. To find a proper region for guest magic pages,
> we can reuse the logic of finding domain extended regions.
>
> Extract the logic of finding domain extended regions to a helper
> function named find_unused_memory() and use it to find unallocated
> spaces for magic pages before make_hypervisor_node(). The result magic
> page region is added to the reserved memory section of the bootinfo so
> that it is carved out from the extended regions.
>
> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
> Signed-off-by: Henry Wang <xin.wang2@amd.com>
> ---
> v2:
> - New patch
> ---
> xen/arch/arm/dom0less-build.c | 43 +++++++++++++++++++++++++
> xen/arch/arm/domain_build.c | 30 ++++++++++-------
> xen/arch/arm/include/asm/domain_build.h | 2 ++
> 3 files changed, 64 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 1e1c8d83ae..99447bfb0c 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>
> if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
> {
> + if ( is_domain_direct_mapped(d) )
> + {
This whole block is dependent on static memory feature that is compiled out by default.
Shouldn't you move it to static-memory.c ?

> + struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
I can't see corresponding xfree(avail_magic_regions). It's not going to be used after unused memory
regions are retrieved.

> + struct meminfo *rsrv_mem = &bootinfo.reserved_mem;
> + struct mem_map_domain *mem_map = &d->arch.mem_map;
> + uint64_t magic_region_start = INVALID_PADDR;
What's the purpose of this initialization? magic_region_start is going to be re-assigned before making use of this value.

> + uint64_t magic_region_size = GUEST_MAGIC_SIZE;
Why not paddr_t?

> + unsigned int i;
> +
> + if ( !avail_magic_regions )
> + return -ENOMEM;
What about memory allocated for kinfo->fdt? You should goto err;

> +
> + ret = find_unused_memory(d, kinfo, avail_magic_regions);
> + if ( ret )
> + {
> + printk(XENLOG_WARNING
> + "%pd: failed to find a region for domain magic pages\n",
> + d);
> + goto err;
What about memory allocated for avail_magic_regions? You should free it.

> + }
> +
> + magic_region_start = avail_magic_regions->bank[0].start;
> +
> + /*
> + * Register the magic region as reserved mem to make sure this
> + * region will not be counted when allocating extended regions.
Well, this is only true in case find_unallocated_memory() is used to retrieve free regions.
What if our direct mapped domU used partial dtb and IOMMU is in use? In this case,
find_memory_holes() will be used and the behavior will be different.

Also, I'm not sure if it is a good idea to call find_unused_memory twice (with lots of steps inside)
just to retrieve 16MB (btw. add_ext_regions will only return 64MB+ regions) region for magic pages.
I'll let other maintainers share their opinion.

Also, CCing Carlo since he was in a need of retrieving free memory regions as well for cache coloring with dom0.

~Michal

Re: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU [ In reply to ]

michal.orzel at amd

Mar 11, 2024, 6:50 AM

Post #3 of 5 (11 views)

Permalink

On 11/03/2024 14:46, Michal Orzel wrote:
>
>
> Hi Henry,
>
> On 08/03/2024 02:54, Henry Wang wrote:
>> For 1:1 direct-mapped dom0less DomUs, the magic pages should not clash
>> with any RAM region. To find a proper region for guest magic pages,
>> we can reuse the logic of finding domain extended regions.
>>
>> Extract the logic of finding domain extended regions to a helper
>> function named find_unused_memory() and use it to find unallocated
>> spaces for magic pages before make_hypervisor_node(). The result magic
>> page region is added to the reserved memory section of the bootinfo so
>> that it is carved out from the extended regions.
>>
>> Reported-by: Alec Kwapis <alec.kwapis@medtronic.com>
>> Signed-off-by: Henry Wang <xin.wang2@amd.com>
>> ---
>> v2:
>> - New patch
>> ---
>> xen/arch/arm/dom0less-build.c | 43 +++++++++++++++++++++++++
>> xen/arch/arm/domain_build.c | 30 ++++++++++-------
>> xen/arch/arm/include/asm/domain_build.h | 2 ++
>> 3 files changed, 64 insertions(+), 11 deletions(-)
>>
>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
>> index 1e1c8d83ae..99447bfb0c 100644
>> --- a/xen/arch/arm/dom0less-build.c
>> +++ b/xen/arch/arm/dom0less-build.c
>> @@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>>
>> if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
>> {
>> + if ( is_domain_direct_mapped(d) )
>> + {
> This whole block is dependent on static memory feature that is compiled out by default.
> Shouldn't you move it to static-memory.c ?
>
>> + struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
> I can't see corresponding xfree(avail_magic_regions). It's not going to be used after unused memory
> regions are retrieved.
>
>> + struct meminfo *rsrv_mem = &bootinfo.reserved_mem;
>> + struct mem_map_domain *mem_map = &d->arch.mem_map;
>> + uint64_t magic_region_start = INVALID_PADDR;
> What's the purpose of this initialization? magic_region_start is going to be re-assigned before making use of this value.
>
>> + uint64_t magic_region_size = GUEST_MAGIC_SIZE;
> Why not paddr_t?
>
>> + unsigned int i;
>> +
>> + if ( !avail_magic_regions )
>> + return -ENOMEM;
> What about memory allocated for kinfo->fdt? You should goto err;
>
>> +
>> + ret = find_unused_memory(d, kinfo, avail_magic_regions);
>> + if ( ret )
>> + {
>> + printk(XENLOG_WARNING
>> + "%pd: failed to find a region for domain magic pages\n",
>> + d);
>> + goto err;
> What about memory allocated for avail_magic_regions? You should free it.
>
>> + }
>> +
>> + magic_region_start = avail_magic_regions->bank[0].start;
>> +
>> + /*
>> + * Register the magic region as reserved mem to make sure this
>> + * region will not be counted when allocating extended regions.
> Well, this is only true in case find_unallocated_memory() is used to retrieve free regions.
> What if our direct mapped domU used partial dtb and IOMMU is in use? In this case,
> find_memory_holes() will be used and the behavior will be different.
>
> Also, I'm not sure if it is a good idea to call find_unused_memory twice (with lots of steps inside)
> just to retrieve 16MB (btw. add_ext_regions will only return 64MB+ regions) region for magic pages.
> I'll let other maintainers share their opinion.
>
> Also, CCing Carlo since he was in a need of retrieving free memory regions as well for cache coloring with dom0.
In the end, I forgot to CC Carlo. Adding him now.

~Michal

Re: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU [ In reply to ]

xin.wang2 at amd

Mar 11, 2024, 8:25 PM

Post #4 of 5 (10 views)

Permalink

Hi Michal,

On 3/11/2024 9:46 PM, Michal Orzel wrote:
> Hi Henry,
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index 1e1c8d83ae..99447bfb0c 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
>
> if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
> {
> + if ( is_domain_direct_mapped(d) )
> + {
> This whole block is dependent on static memory feature that is compiled out by default.
> Shouldn't you move it to static-memory.c ?

This makes sense. I will convert this block to a function then move to
static-memory.c in v3.

>> + struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
> I can't see corresponding xfree(avail_magic_regions). It's not going to be used after unused memory
> regions are retrieved.

Hmmm I realized I've made a mess here and below. I will do the proper
error handling in v3.

>> + struct meminfo *rsrv_mem = &bootinfo.reserved_mem;
>> + struct mem_map_domain *mem_map = &d->arch.mem_map;
>> + uint64_t magic_region_start = INVALID_PADDR;
> What's the purpose of this initialization? magic_region_start is going to be re-assigned before making use of this value.

Personally for variables holding an address, I would like to init the
local variable to a poison value before use. But you are right it does
not make a difference here I think. I can drop the initialization if you
prefer not having it, sure.

>> + uint64_t magic_region_size = GUEST_MAGIC_SIZE;
> Why not paddr_t?

Good catch, I mixed struct meminfo with the newly added struct. Will use
paddr_t.
>> +
>> + magic_region_start = avail_magic_regions->bank[0].start;
>> +
>> + /*
>> + * Register the magic region as reserved mem to make sure this
>> + * region will not be counted when allocating extended regions.
> Well, this is only true in case find_unallocated_memory() is used to retrieve free regions.
> What if our direct mapped domU used partial dtb and IOMMU is in use? In this case,
> find_memory_holes() will be used and the behavior will be different.
>
> Also, I'm not sure if it is a good idea to call find_unused_memory twice (with lots of steps inside)
> just to retrieve 16MB (btw. add_ext_regions will only return 64MB+ regions) region for magic pages.
> I'll let other maintainers share their opinion.

I agree with your point. Let's wait a bit longer for more
ideas/comments. If no other inputs, I think I will drop the
"adding to reserved_mem" part of logic and record the found unused
memory in kinfo, then use rangeset_remove_range() to remove this range
in both

find_unallocated_memory() and find_memory_holes().

> Also, CCing Carlo since he was in a need of retrieving free memory regions as well for cache coloring with dom0.

(+ Carlo)
Any inputs from your side for this topic Carlo?

Kind regards,
Henry
> ~Michal

Re: [PATCH v2 4/5] xen/arm: Find unallocated spaces for magic pages of direct-mapped domU [ In reply to ]

carlo.nonato at minervasys

Mar 13, 2024, 4:09 AM

Post #5 of 5 (10 views)

Permalink

Hi Henry,

On Tue, Mar 12, 2024 at 4:25?AM Henry Wang <xin.wang2@amd.com> wrote:
>
> Hi Michal,
>
> On 3/11/2024 9:46 PM, Michal Orzel wrote:
> > Hi Henry,
> >
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index 1e1c8d83ae..99447bfb0c 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -682,6 +682,49 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
> >
> > if ( kinfo->dom0less_feature & DOM0LESS_ENHANCED_NO_XS )
> > {
> > + if ( is_domain_direct_mapped(d) )
> > + {
> > This whole block is dependent on static memory feature that is compiled out by default.
> > Shouldn't you move it to static-memory.c ?
>
> This makes sense. I will convert this block to a function then move to
> static-memory.c in v3.
>
> >> + struct meminfo *avail_magic_regions = xzalloc(struct meminfo);
> > I can't see corresponding xfree(avail_magic_regions). It's not going to be used after unused memory
> > regions are retrieved.
>
> Hmmm I realized I've made a mess here and below. I will do the proper
> error handling in v3.
>
> >> + struct meminfo *rsrv_mem = &bootinfo.reserved_mem;
> >> + struct mem_map_domain *mem_map = &d->arch.mem_map;
> >> + uint64_t magic_region_start = INVALID_PADDR;
> > What's the purpose of this initialization? magic_region_start is going to be re-assigned before making use of this value.
>
> Personally for variables holding an address, I would like to init the
> local variable to a poison value before use. But you are right it does
> not make a difference here I think. I can drop the initialization if you
> prefer not having it, sure.
>
> >> + uint64_t magic_region_size = GUEST_MAGIC_SIZE;
> > Why not paddr_t?
>
> Good catch, I mixed struct meminfo with the newly added struct. Will use
> paddr_t.
> >> +
> >> + magic_region_start = avail_magic_regions->bank[0].start;
> >> +
> >> + /*
> >> + * Register the magic region as reserved mem to make sure this
> >> + * region will not be counted when allocating extended regions.
> > Well, this is only true in case find_unallocated_memory() is used to retrieve free regions.
> > What if our direct mapped domU used partial dtb and IOMMU is in use? In this case,
> > find_memory_holes() will be used and the behavior will be different.
> >
> > Also, I'm not sure if it is a good idea to call find_unused_memory twice (with lots of steps inside)
> > just to retrieve 16MB (btw. add_ext_regions will only return 64MB+ regions) region for magic pages.
> > I'll let other maintainers share their opinion.
>
> I agree with your point. Let's wait a bit longer for more
> ideas/comments. If no other inputs, I think I will drop the
> "adding to reserved_mem" part of logic and record the found unused
> memory in kinfo, then use rangeset_remove_range() to remove this range
> in both
>
> find_unallocated_memory() and find_memory_holes().
>
> > Also, CCing Carlo since he was in a need of retrieving free memory regions as well for cache coloring with dom0.
>
> (+ Carlo)
> Any inputs from your side for this topic Carlo?

Nothing at the moment.

Thanks.

> Kind regards,
> Henry
> > ~Michal
>