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.kwa...@medtronic.com>
> Signed-off-by: Henry Wang <xin.wa...@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
