| Message ID | 20240308015435.4044339-5-xin.wang2@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | DOMCTL-based guest magic regions allocation for dom0less | expand |
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
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
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
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 >
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,
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(-)