Re: [PATCH] xen/device-tree: Allow exact match for overlapping regions

[Michal Orzel <michal.orzel@xxxxxxx>]

On 06/11/2024 14:41, Luca Fancellu wrote:
> 
> 
> There are some cases where the device tree exposes a memory range
> in both /memreserve/ and reserved-memory node, in this case the
> current code will stop Xen to boot since it will find that the
> latter range is clashing with the already recorded /memreserve/
> ranges.
> 
> Furthermore, u-boot lists boot modules ranges, such as ramdisk,
> in the /memreserve/ part and even in this case this will prevent
> Xen to boot since it will see that the module memory range that
> it is going to add in 'add_boot_module' clashes with a /memreserve/
> range.
> 
> When Xen populate the data structure that tracks the memory ranges,
> it also adds a memory type described in 'enum membank_type', so
> in order to fix this behavior, allow the 'check_reserved_regions_overlap'
> function to check for exact memory range match given a specific memory
> type; allowing reserved-memory node ranges and boot modules to have an
> exact match with ranges from /memreserve/.
> 
> While there, set a type for the memory recorded during ACPI boot.
> 
> Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to 
> bootinfo.reserved_mem")
> Reported-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> Reported-by: Grygorii Strashko <grygorii_strashko@xxxxxxxx>
> Signed-off-by: Luca Fancellu <luca.fancellu@xxxxxxx>
> ---
> I tested this patch adding the same range in a /memreserve/ entry and
> /reserved-memory node, and by letting u-boot pass a ramdisk.
> I've also tested that a configuration running static shared memory still works
> fine.
> ---
So we have 2 separate issues. I don't particularly like the concept of 
introducing MEMBANK_NONE
and the changes below look a bit too much for me, given that for boot modules 
we can only have
/memreserve/ matching initrd.

Shawn patch fixes the first issue. AFAICT the second issue can be fixed by 
below simple patch:
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
index 927f59c64b0d..d8bd8c44bd35 100644
--- a/xen/common/device-tree/bootfdt.c
+++ b/xen/common/device-tree/bootfdt.c
@@ -586,6 +586,10 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)

     add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);

+    ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
+    if ( ret )
+        panic("Early FDT parsing failed (%d)\n", ret);
+
     nr_rsvd = fdt_num_mem_rsv(fdt);
     if ( nr_rsvd < 0 )
         panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
@@ -594,10 +598,14 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
paddr)
     {
         struct membank *bank;
         paddr_t s, sz;
+        const struct bootmodule *mod = 
boot_module_find_by_kind(BOOTMOD_RAMDISK);

         if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
             continue;

+        if ( mod && (mod->start == s) && (mod->size == sz) )
+            continue;
+
         if ( reserved_mem->nr_banks < reserved_mem->max_banks )
         {
             bank = &reserved_mem->bank[reserved_mem->nr_banks];
@@ -610,10 +618,6 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
             panic("Cannot allocate reserved memory bank\n");
     }

-    ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
-    if ( ret )
-        panic("Early FDT parsing failed (%d)\n", ret);
-
     /*
      * On Arm64 setup_directmap_mappings() expects to be called with the lowest
      * bank in memory first. There is no requirement that the DT will provide

I'll let other DT maintainers voice their opinion.

~Michal