Re: [PATCH v5 1/3] xen/device-tree: Let DT reserve map entries overlap reserved-memory

[Grygorii Strashko <grygorii_strashko@xxxxxxxx>]

Hi All,

On 04.11.24 12:49, Michal Orzel wrote:
> On 27/09/2024 00:24, Shawn Anastasio wrote:
> > Commit 53dc37829c31 ("xen/arm: Add DT reserve map regions to
> > bootinfo.reserved_mem") changes the way reserve map regions are tracked,
> > and as a result broke bootfdt's ability to handle device trees in which
> > the reserve map and the `reserved-memory` node contain the same entries
> > as each other, as is the case on PPC when booted by skiboot.
> >
> > Fix this behavior by moving the reserve map check to after the DT has
> > been parsed and by explicitly allowing overlap with entries created by
> > `reserved-memory` nodes.
> >
> > Fixes: 53dc37829c31 ("xen/arm: Add DT reserve map regions to 
> > bootinfo.reserved_mem")
> > Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   xen/common/device-tree/bootfdt.c  | 28 +++++++++++++++++++++++-----
> >   xen/common/device-tree/bootinfo.c | 11 +++++++++--
> >   xen/include/xen/bootfdt.h         |  3 ++-
> >   3 files changed, 34 insertions(+), 8 deletions(-)
> >
> > diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c
> > index 911a630e7d..2a51ee44a3 100644
> > --- a/xen/common/device-tree/bootfdt.c
> > +++ b/xen/common/device-tree/bootfdt.c
> > @@ -177,7 +177,7 @@ static int __init device_tree_get_meminfo(const void *fdt, 
> > int node,
> >       {
> >           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> >           if ( mem == bootinfo_get_reserved_mem() &&
> > -             check_reserved_regions_overlap(start, size) )
> > +             check_reserved_regions_overlap(start, size, NULL) )
> >               return -EINVAL;
> >           /* Some DT may describe empty bank, ignore them */
> >           if ( !size )
> > @@ -590,14 +590,36 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t 
> > paddr)
> >       if ( nr_rsvd < 0 )
> >           panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd);
> >
> > +    ret = device_tree_for_each_node(fdt, 0, early_scan_node, NULL);
> This should be moved before fdt_num_mem_rsv so that the program flow makes 
> sense. In your case nr_rsvd is
> not used immediately after.
>
> > +    if ( ret )
> > +        panic("Early FDT parsing failed (%d)\n", ret);
> > +
> >       for ( i = 0; i < nr_rsvd; i++ )
> >       {
> > +        const struct membanks *overlap = NULL;
> >           struct membank *bank;
> >           paddr_t s, sz;
> >
> >           if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, &s, &sz) < 0 )
> >               continue;
> >
> > +        if ( check_reserved_regions_overlap(s, sz, &overlap) )
> > +        {
> > +            if ( overlap == bootinfo_get_reserved_mem() )
> > +            {
> > +                /*
> > +                 * Some valid device trees, such as those generated by 
> > OpenPOWER
> > +                 * skiboot firmware, expose all reserved memory regions in the
> > +                 * FDT memory reservation block (here) AND in the
> > +                 * reserved-memory node which has already been parsed. Thus, 
> > any
> > +                 * overlaps in the mem_reserved banks should be ignored.
> > +                 */
> > +                 continue;
> I think this is incorrect. Imagine this scenario:
> /memreserve/ 0x40000000 0x40000000;
> and /reserved-memory/foo with:
> reg = <0x0 0x7FFFF000 0x0 0x1000>;
>
> You would ignore the entire region described with /memreserve/ even though it 
> overlaps just the last page.
>
> The problem you're describing is about regions that match 1:1 in /memreserve/ 
> and /reserved-memory/.
> Therefore I think you should check that the overlapped regions match exactly.
I've also discovered an issue with Commit 53dc37829c31 ("xen/arm: Add DT 
reserve map
regions to bootinfo.reserved_mem") - the bootloader adds Initrd in
FDT reserved map which then conflicts with Initrd module (ARM64).

This patch, as is, doesn't fix an issue for me:

(XEN) Checking for initrd in /chosen
(XEN) Initrd 0000000084000040-0000000086152ac6
(XEN) Region: [0x00000084000040, 0x00000086152ac6) overlapping with mod[2]: 
[0x00000084000040, 0x00000086152ac6)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) FDT reserve map overlapped with membanks/modules
(XEN) ****************************************

So I did fast try of Michal Orzel suggestion and it seems working for me.
And if it's working for PPC - may be that's it (feel free to incorporate). Diff 
below.

(XEN) Checking for initrd in /chosen
(XEN) Initrd 0000000084000040-0000000086152ac6
(XEN) RAM: 0000000048000000 - 00000000bfffffff
(XEN) RAM: 0000000480000000 - 00000004ffffffff
(XEN) RAM: 0000000600000000 - 00000006ffffffff
(XEN)
(XEN) MODULE[0]: 0000000048080000 - 00000000481ec000 Xen
(XEN) MODULE[1]: 0000000048000000 - 000000004801e080 Device Tree
(XEN) MODULE[2]: 0000000084000040 - 0000000086152ac6 Ramdisk
(XEN) MODULE[3]: 0000000048300000 - 000000004a300000 Kernel
(XEN) MODULE[4]: 0000000048070000 - 0000000048080000 XSM
(XEN)  RESVD[0]: 0000000060000000 - 000000007fffffff
(XEN)  RESVD[1]: 00000000b0000000 - 00000000bfffffff
(XEN)  RESVD[2]: 00000000a0000000 - 00000000afffffff
...
(XEN) *** LOADING DOMAIN 0 ***
(XEN) Loading d0 kernel from boot module @ 0000000048300000
(XEN) Loading ramdisk from boot module @ 0000000084000040
(XEN) Allocating 1:1 mappings totalling 256MB for dom0:
(XEN) BANK[0] 0x00000050000000-0x00000060000000 (256MB)
...


---
diff --git a/xen/common/device-tree/bootinfo.c 
b/xen/common/device-tree/bootinfo.c
index f2e6a1145b7c..10e997eeca8d 100644
--- a/xen/common/device-tree/bootinfo.c
+++ b/xen/common/device-tree/bootinfo.c
@@ -124,6 +124,30 @@ static bool __init meminfo_overlap_check(const struct 
membanks *mem,
     return false;
 }
 
+static bool __init meminfo_is_exist(const struct membanks *mem,
+                                         paddr_t region_start,
+                                         paddr_t region_size)
+{
+    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
+    paddr_t region_end = region_start + region_size;
+    unsigned int i, bank_num = mem->nr_banks;
+
+    for ( i = 0; i < bank_num; i++ )
+    {
+        bank_start = mem->bank[i].start;
+        bank_end = bank_start + mem->bank[i].size;
+
+        if ( INVALID_PADDR == bank_start || region_end <= bank_start ||
+             region_start >= bank_end )
+            continue;
+
+        if ( region_start == bank_start && region_end  == bank_end)
+            return true;
+    }
+
+    return false;
+}
+
 /*
  * TODO: '*_end' could be 0 if the module/region is at the end of the physical
  * address space. This is for now not handled as it requires more rework.
@@ -154,6 +178,29 @@ static bool __init bootmodules_overlap_check(struct 
bootmodules *bootmodules,
     return false;
 }
 
+static bool __init bootmodules_is_exist(struct bootmodules *bootmodules,
+                                             paddr_t region_start,
+                                             paddr_t region_size)
+{
+    paddr_t mod_start = INVALID_PADDR, mod_end = 0;
+    paddr_t region_end = region_start + region_size;
+    unsigned int i, mod_num = bootmodules->nr_mods;
+
+    for ( i = 0; i < mod_num; i++ )
+    {
+        mod_start = bootmodules->module[i].start;
+        mod_end = mod_start + bootmodules->module[i].size;
+
+        if ( region_end <= mod_start || region_start >= mod_end )
+            continue;
+
+        if (region_start == mod_start && region_end == mod_end)
+            return true;
+    }
+
+    return false;
+}
+
 void __init fw_unreserved_regions(paddr_t s, paddr_t e,
                                   void (*cb)(paddr_t ps, paddr_t pe),
                                   unsigned int first)
@@ -201,6 +248,37 @@ bool __init check_reserved_regions_overlap(paddr_t 
region_start,
     return false;
 }
 
+bool __init check_reserved_regions_is_exist(paddr_t region_start,
+                                            paddr_t region_size)
+{
+    const struct membanks *mem_banks[] = {
+        bootinfo_get_reserved_mem(),
+#ifdef CONFIG_ACPI
+        bootinfo_get_acpi(),
+#endif
+#ifdef CONFIG_STATIC_SHM
+        bootinfo_get_shmem(),
+#endif
+    };
+    unsigned int i;
+
+    /*
+     * Check if input region is overlapping with reserved memory banks or
+     * ACPI EfiACPIReclaimMemory (when ACPI feature is enabled) or static
+     * shared memory banks (when static shared memory feature is enabled)
+     */
+    for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ )
+        if ( meminfo_is_exist(mem_banks[i], region_start, region_size) )
+            return true;
+
+    /* Check if input region is overlapping with bootmodules */
+    if ( bootmodules_is_exist(&bootinfo.modules,
+                                   region_start, region_size) )
+        return true;
+
+    return false;
+}
+
 struct bootmodule __init *add_boot_module(bootmodule_kind kind,
                                           paddr_t start, paddr_t size,
                                           bool domU)
diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
index 16fa05f38f38..b8db1335be6c 100644
--- a/xen/include/xen/bootfdt.h
+++ b/xen/include/xen/bootfdt.h
@@ -159,6 +159,8 @@ extern struct bootinfo bootinfo;
 
 bool check_reserved_regions_overlap(paddr_t region_start,
                                     paddr_t region_size);
+bool check_reserved_regions_is_exist(paddr_t region_start,
+                                     paddr_t region_size);
 
 struct bootmodule *add_boot_module(bootmodule_kind kind,
                                    paddr_t start, paddr_t size, bool domU);