[PATCH v2 0/3] Memory region overlap check in device tree

[PATCH v2 0/3] Memory region overlap check in device tree

[Henry Wang]

As we are having more and more types of memory region defined in the
device tree, it is necessary to add the overlap check of these memory
regions in Xen, because such check will help user to identify the
misconfiguration in the device tree at the early stage of boot time.

The first patch introduces the basic memory overlap check mechanism,
and does the memory check for memory regions in bootinfo.reserved_mem.
Following patches extend the overlap check to include bootmodules and
EfiACPIReclaimMemory.

v1 -> v2:
- Split original `overlap_check()` to `meminfo_overlap_check()` and
  `bootmodules_overlap_check()`.
- Rework commit message.

Henry Wang (3):
  xen/arm: Add memory overlap check for bootinfo.reserved_mem
  xen/arm: Extend the memory overlap check to include bootmodules
  xen/arm: Extend the memory overlap check to include
    EfiACPIReclaimMemory

 xen/arch/arm/bootfdt.c           | 13 +++--
 xen/arch/arm/efi/efi-boot.h      | 10 +++-
 xen/arch/arm/include/asm/setup.h |  2 +
 xen/arch/arm/setup.c             | 82 ++++++++++++++++++++++++++++++++
 4 files changed, 98 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Henry Wang]

As we are having more and more types of static region, and all of
these static regions are defined in bootinfo.reserved_mem, it is
necessary to add the overlap check of reserved memory regions in Xen,
because such check will help user to identify the misconfiguration in
the device tree at the early stage of boot time.

Currently we have 3 types of static region, namely
(1) static memory
(2) static heap
(3) static shared memory

(1) and (2) are parsed by the function `device_tree_get_meminfo()` and
(3) is parsed using its own logic. All of parsed information of these
types will be stored in `struct meminfo`.

Therefore, to unify the overlap checking logic for all of these types,
this commit firstly introduces a helper `meminfo_overlap_check()` and
a function `check_reserved_regions_overlap()` to check if an input
physical address range is overlapping with the existing memory regions
defined in bootinfo. After that, use `check_reserved_regions_overlap()`
in `device_tree_get_meminfo()` to do the overlap check of (1) and (2)
and replace the original overlap check of (3) with
`check_reserved_regions_overlap()`.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v1 -> v2:
1. Split original `overlap_check()` to `meminfo_overlap_check()`.
2. Rework commit message.
---
 xen/arch/arm/bootfdt.c           | 13 +++++-----
 xen/arch/arm/include/asm/setup.h |  2 ++
 xen/arch/arm/setup.c             | 42 ++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 0085c28d74..e2f6c7324b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -88,6 +88,9 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
     for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
     {
         device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
+        if ( mem == &bootinfo.reserved_mem &&
+             check_reserved_regions_overlap(start, size) )
+            return -EINVAL;
         /* Some DT may describe empty bank, ignore them */
         if ( !size )
             continue;
@@ -482,7 +485,9 @@ static int __init process_shm_node(const void *fdt, int node,
                 return -EINVAL;
             }
 
-            if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
+            if ( check_reserved_regions_overlap(paddr, size) )
+                return -EINVAL;
+            else
             {
                 if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
                     continue;
@@ -493,12 +498,6 @@ static int __init process_shm_node(const void *fdt, int node,
                     return -EINVAL;
                 }
             }
-            else
-            {
-                printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
-                        mem->bank[i].start, bank_end);
-                return -EINVAL;
-            }
         }
     }
 
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index fdbf68aadc..6a9f88ecbb 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e,
 size_t boot_fdt_info(const void *fdt, paddr_t paddr);
 const char *boot_fdt_cmdline(const void *fdt);
 
+int check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size);
+
 struct bootmodule *add_boot_module(bootmodule_kind kind,
                                    paddr_t start, paddr_t size, bool domU);
 struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 1f26f67b90..e6eeb3a306 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -261,6 +261,31 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
     cb(s, e);
 }
 
+static int __init meminfo_overlap_check(struct meminfo *meminfo,
+                                        paddr_t region_start,
+                                        paddr_t region_end)
+{
+    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
+    unsigned int i, bank_num = meminfo->nr_banks;
+
+    for ( i = 0; i < bank_num; i++ )
+    {
+        bank_start = meminfo->bank[i].start;
+        bank_end = bank_start + meminfo->bank[i].size;
+
+        if ( region_end <= bank_start || region_start >= bank_end )
+            continue;
+        else
+        {
+            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",
+                   region_start, region_end, i, bank_start, bank_end);
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 void __init fw_unreserved_regions(paddr_t s, paddr_t e,
                                   void (*cb)(paddr_t, paddr_t),
                                   unsigned int first)
@@ -271,7 +296,24 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
         cb(s, e);
 }
 
+/*
+ * Given an input physical address range, check if this range is overlapping
+ * with the existing reserved memory regions defined in bootinfo.
+ * Return 0 if the input physical address range is not overlapping with any
+ * existing reserved memory regions, otherwise -EINVAL.
+ */
+int __init check_reserved_regions_overlap(paddr_t region_start,
+                                          paddr_t region_size)
+{
+    paddr_t region_end = region_start + region_size;
+
+    /* Check if input region is overlapping with bootinfo.reserved_mem banks */
+    if ( meminfo_overlap_check(&bootinfo.reserved_mem,
+                               region_start, region_end) )
+        return -EINVAL;
 
+    return 0;
+}
 
 struct bootmodule __init *add_boot_module(bootmodule_kind kind,
                                           paddr_t start, paddr_t size,
-- 
2.25.1

[PATCH v2 2/3] xen/arm: Extend the memory overlap check to include bootmodules

[Henry Wang]

Similarly as the static regions defined in bootinfo.reserved_mem,
the bootmodule regions defined in bootinfo.modules should also not
be overlapping with memory regions in either bootinfo.reserved_mem
or bootinfo.modules.

Therefore, this commit introduces a helper `bootmodules_overlap_check()`
and uses this helper to extend the check in function
`check_reserved_regions_overlap()` so that memory regions in
bootinfo.modules are included. Use `check_reserved_regions_overlap()`
in `add_boot_module()` to return early if any error occurs.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v1 -> v2:
1. Split original `overlap_check()` to `bootmodules_overlap_check()`.
2. Rework commit message.
---
 xen/arch/arm/setup.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index e6eeb3a306..ba0152f868 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -286,6 +286,31 @@ static int __init meminfo_overlap_check(struct meminfo *meminfo,
     return 0;
 }
 
+static int __init bootmodules_overlap_check(struct bootmodules *bootmodules,
+                                            paddr_t region_start,
+                                            paddr_t region_end)
+{
+    paddr_t mod_start = INVALID_PADDR, mod_end = 0;
+    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;
+        else
+        {
+            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with mod[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",
+                   region_start, region_end, i, mod_start, mod_end);
+            return -EINVAL;
+        }
+    }
+
+    return 0;
+}
+
 void __init fw_unreserved_regions(paddr_t s, paddr_t e,
                                   void (*cb)(paddr_t, paddr_t),
                                   unsigned int first)
@@ -312,6 +337,11 @@ int __init check_reserved_regions_overlap(paddr_t region_start,
                                region_start, region_end) )
         return -EINVAL;
 
+    /* Check if input region is overlapping with bootmodules */
+    if ( bootmodules_overlap_check(&bootinfo.modules,
+                                   region_start, region_end) )
+        return -EINVAL;
+
     return 0;
 }
 
@@ -329,6 +359,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind,
                boot_module_kind_as_string(kind), start, start + size);
         return NULL;
     }
+
+    if ( check_reserved_regions_overlap(start, size) )
+        return NULL;
+
     for ( i = 0 ; i < mods->nr_mods ; i++ )
     {
         mod = &mods->module[i];
-- 
2.25.1

[PATCH v2 3/3] xen/arm: Extend the memory overlap check to include EfiACPIReclaimMemory

[Henry Wang]

Similarly as the static regions and boot modules, memory regions with
EfiACPIReclaimMemory type (defined in bootinfo.acpi if CONFIG_ACPI is
enabled) should also not be overlapping with memory regions in
bootinfo.reserved_mem and bootinfo.modules.

Therefore, this commit reuses the `meminfo_overlap_check()` to further
extends the check in function `check_reserved_regions_overlap()` so that
memory regions in bootinfo.acpi are included. If any error occurs in the
extended `check_reserved_regions_overlap()`, the `meminfo_add_bank()`
defined in `efi-boot.h` will return early.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v1 -> v2:
1. Rebase on top of patch #1 and #2.
---
 xen/arch/arm/efi/efi-boot.h | 10 ++++++++--
 xen/arch/arm/setup.c        |  6 ++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 43a836c3a7..6121ba1f2f 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -161,13 +161,19 @@ static bool __init meminfo_add_bank(struct meminfo *mem,
                                     EFI_MEMORY_DESCRIPTOR *desc)
 {
     struct membank *bank;
+    paddr_t start = desc->PhysicalStart;
+    paddr_t size = desc->NumberOfPages * EFI_PAGE_SIZE;
 
     if ( mem->nr_banks >= NR_MEM_BANKS )
         return false;
+#ifdef CONFIG_ACPI
+    if ( check_reserved_regions_overlap(start, size) )
+        return false;
+#endif
 
     bank = &mem->bank[mem->nr_banks];
-    bank->start = desc->PhysicalStart;
-    bank->size = desc->NumberOfPages * EFI_PAGE_SIZE;
+    bank->start = start;
+    bank->size = size;
 
     mem->nr_banks++;
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index ba0152f868..a0cb2dd588 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -342,6 +342,12 @@ int __init check_reserved_regions_overlap(paddr_t region_start,
                                    region_start, region_end) )
         return -EINVAL;
 
+#ifdef CONFIG_ACPI
+    /* Check if input region is overlapping with ACPI EfiACPIReclaimMemory */
+    if ( meminfo_overlap_check(&bootinfo.acpi, region_start, region_end) )
+        return -EINVAL;
+#endif
+
     return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Stefano Stabellini]

On Wed, 14 Dec 2022, Henry Wang wrote:
> As we are having more and more types of static region, and all of
> these static regions are defined in bootinfo.reserved_mem, it is
> necessary to add the overlap check of reserved memory regions in Xen,
> because such check will help user to identify the misconfiguration in
> the device tree at the early stage of boot time.
> 
> Currently we have 3 types of static region, namely
> (1) static memory
> (2) static heap
> (3) static shared memory
> 
> (1) and (2) are parsed by the function `device_tree_get_meminfo()` and
> (3) is parsed using its own logic. All of parsed information of these
> types will be stored in `struct meminfo`.
> 
> Therefore, to unify the overlap checking logic for all of these types,
> this commit firstly introduces a helper `meminfo_overlap_check()` and
> a function `check_reserved_regions_overlap()` to check if an input
> physical address range is overlapping with the existing memory regions
> defined in bootinfo. After that, use `check_reserved_regions_overlap()`
> in `device_tree_get_meminfo()` to do the overlap check of (1) and (2)
> and replace the original overlap check of (3) with
> `check_reserved_regions_overlap()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> v1 -> v2:
> 1. Split original `overlap_check()` to `meminfo_overlap_check()`.
> 2. Rework commit message.
> ---
>  xen/arch/arm/bootfdt.c           | 13 +++++-----
>  xen/arch/arm/include/asm/setup.h |  2 ++
>  xen/arch/arm/setup.c             | 42 ++++++++++++++++++++++++++++++++
>  3 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..e2f6c7324b 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -88,6 +88,9 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>      for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
>      {
>          device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        if ( mem == &bootinfo.reserved_mem &&
> +             check_reserved_regions_overlap(start, size) )
> +            return -EINVAL;
>          /* Some DT may describe empty bank, ignore them */
>          if ( !size )
>              continue;
> @@ -482,7 +485,9 @@ static int __init process_shm_node(const void *fdt, int node,
>                  return -EINVAL;
>              }
>  
> -            if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
> +            if ( check_reserved_regions_overlap(paddr, size) )
> +                return -EINVAL;
> +            else
>              {
>                  if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
>                      continue;
> @@ -493,12 +498,6 @@ static int __init process_shm_node(const void *fdt, int node,
>                      return -EINVAL;
>                  }
>              }
> -            else
> -            {
> -                printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
> -                        mem->bank[i].start, bank_end);
> -                return -EINVAL;
> -            }
>          }
>      }
>  
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index fdbf68aadc..6a9f88ecbb 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e,
>  size_t boot_fdt_info(const void *fdt, paddr_t paddr);
>  const char *boot_fdt_cmdline(const void *fdt);
>  
> +int check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size);
> +
>  struct bootmodule *add_boot_module(bootmodule_kind kind,
>                                     paddr_t start, paddr_t size, bool domU);
>  struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..e6eeb3a306 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -261,6 +261,31 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>      cb(s, e);
>  }
>  
> +static int __init meminfo_overlap_check(struct meminfo *meminfo,
> +                                        paddr_t region_start,
> +                                        paddr_t region_end)
> +{
> +    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> +    unsigned int i, bank_num = meminfo->nr_banks;
> +
> +    for ( i = 0; i < bank_num; i++ )
> +    {
> +        bank_start = meminfo->bank[i].start;
> +        bank_end = bank_start + meminfo->bank[i].size;
> +
> +        if ( region_end <= bank_start || region_start >= bank_end )
> +            continue;
> +        else
> +        {
> +            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",
> +                   region_start, region_end, i, bank_start, bank_end);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>                                    void (*cb)(paddr_t, paddr_t),
>                                    unsigned int first)
> @@ -271,7 +296,24 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>          cb(s, e);
>  }
>  
> +/*
> + * Given an input physical address range, check if this range is overlapping
> + * with the existing reserved memory regions defined in bootinfo.
> + * Return 0 if the input physical address range is not overlapping with any
> + * existing reserved memory regions, otherwise -EINVAL.
> + */
> +int __init check_reserved_regions_overlap(paddr_t region_start,
> +                                          paddr_t region_size)
> +{
> +    paddr_t region_end = region_start + region_size;
> +
> +    /* Check if input region is overlapping with bootinfo.reserved_mem banks */
> +    if ( meminfo_overlap_check(&bootinfo.reserved_mem,
> +                               region_start, region_end) )
> +        return -EINVAL;
>  
> +    return 0;
> +}
>  
>  struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                                            paddr_t start, paddr_t size,
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/3] xen/arm: Extend the memory overlap check to include bootmodules

[Stefano Stabellini]

On Wed, 14 Dec 2022, Henry Wang wrote:
> Similarly as the static regions defined in bootinfo.reserved_mem,
> the bootmodule regions defined in bootinfo.modules should also not
> be overlapping with memory regions in either bootinfo.reserved_mem
> or bootinfo.modules.
> 
> Therefore, this commit introduces a helper `bootmodules_overlap_check()`
> and uses this helper to extend the check in function
> `check_reserved_regions_overlap()` so that memory regions in
> bootinfo.modules are included. Use `check_reserved_regions_overlap()`
> in `add_boot_module()` to return early if any error occurs.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> v1 -> v2:
> 1. Split original `overlap_check()` to `bootmodules_overlap_check()`.
> 2. Rework commit message.
> ---
>  xen/arch/arm/setup.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index e6eeb3a306..ba0152f868 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -286,6 +286,31 @@ static int __init meminfo_overlap_check(struct meminfo *meminfo,
>      return 0;
>  }
>  
> +static int __init bootmodules_overlap_check(struct bootmodules *bootmodules,
> +                                            paddr_t region_start,
> +                                            paddr_t region_end)
> +{
> +    paddr_t mod_start = INVALID_PADDR, mod_end = 0;
> +    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;
> +        else
> +        {
> +            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with mod[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",
> +                   region_start, region_end, i, mod_start, mod_end);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>                                    void (*cb)(paddr_t, paddr_t),
>                                    unsigned int first)
> @@ -312,6 +337,11 @@ int __init check_reserved_regions_overlap(paddr_t region_start,
>                                 region_start, region_end) )
>          return -EINVAL;
>  
> +    /* Check if input region is overlapping with bootmodules */
> +    if ( bootmodules_overlap_check(&bootinfo.modules,
> +                                   region_start, region_end) )
> +        return -EINVAL;
> +
>      return 0;
>  }
>  
> @@ -329,6 +359,10 @@ struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                 boot_module_kind_as_string(kind), start, start + size);
>          return NULL;
>      }
> +
> +    if ( check_reserved_regions_overlap(start, size) )
> +        return NULL;
> +
>      for ( i = 0 ; i < mods->nr_mods ; i++ )
>      {
>          mod = &mods->module[i];
> -- 
> 2.25.1
> 

Re: [PATCH v2 3/3] xen/arm: Extend the memory overlap check to include EfiACPIReclaimMemory

[Stefano Stabellini]

On Wed, 14 Dec 2022, Henry Wang wrote:
> Similarly as the static regions and boot modules, memory regions with
> EfiACPIReclaimMemory type (defined in bootinfo.acpi if CONFIG_ACPI is
> enabled) should also not be overlapping with memory regions in
> bootinfo.reserved_mem and bootinfo.modules.
> 
> Therefore, this commit reuses the `meminfo_overlap_check()` to further
> extends the check in function `check_reserved_regions_overlap()` so that
> memory regions in bootinfo.acpi are included. If any error occurs in the
> extended `check_reserved_regions_overlap()`, the `meminfo_add_bank()`
> defined in `efi-boot.h` will return early.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> v1 -> v2:
> 1. Rebase on top of patch #1 and #2.
> ---
>  xen/arch/arm/efi/efi-boot.h | 10 ++++++++--
>  xen/arch/arm/setup.c        |  6 ++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index 43a836c3a7..6121ba1f2f 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -161,13 +161,19 @@ static bool __init meminfo_add_bank(struct meminfo *mem,
>                                      EFI_MEMORY_DESCRIPTOR *desc)
>  {
>      struct membank *bank;
> +    paddr_t start = desc->PhysicalStart;
> +    paddr_t size = desc->NumberOfPages * EFI_PAGE_SIZE;
>  
>      if ( mem->nr_banks >= NR_MEM_BANKS )
>          return false;
> +#ifdef CONFIG_ACPI
> +    if ( check_reserved_regions_overlap(start, size) )
> +        return false;
> +#endif
>  
>      bank = &mem->bank[mem->nr_banks];
> -    bank->start = desc->PhysicalStart;
> -    bank->size = desc->NumberOfPages * EFI_PAGE_SIZE;
> +    bank->start = start;
> +    bank->size = size;
>  
>      mem->nr_banks++;
>  
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index ba0152f868..a0cb2dd588 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -342,6 +342,12 @@ int __init check_reserved_regions_overlap(paddr_t region_start,
>                                     region_start, region_end) )
>          return -EINVAL;
>  
> +#ifdef CONFIG_ACPI
> +    /* Check if input region is overlapping with ACPI EfiACPIReclaimMemory */
> +    if ( meminfo_overlap_check(&bootinfo.acpi, region_start, region_end) )
> +        return -EINVAL;
> +#endif
> +
>      return 0;
>  }
>  
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Julien Grall]

Hi Henry,

On 14/12/2022 03:16, Henry Wang wrote:
> As we are having more and more types of static region, and all of
> these static regions are defined in bootinfo.reserved_mem, it is
> necessary to add the overlap check of reserved memory regions in Xen,
> because such check will help user to identify the misconfiguration in
> the device tree at the early stage of boot time.
> 
> Currently we have 3 types of static region, namely
> (1) static memory
> (2) static heap
> (3) static shared memory
> 
> (1) and (2) are parsed by the function `device_tree_get_meminfo()` and
> (3) is parsed using its own logic. All of parsed information of these
> types will be stored in `struct meminfo`.
> 
> Therefore, to unify the overlap checking logic for all of these types,
> this commit firstly introduces a helper `meminfo_overlap_check()` and
> a function `check_reserved_regions_overlap()` to check if an input
> physical address range is overlapping with the existing memory regions
> defined in bootinfo. After that, use `check_reserved_regions_overlap()`
> in `device_tree_get_meminfo()` to do the overlap check of (1) and (2)
> and replace the original overlap check of (3) with
> `check_reserved_regions_overlap()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> v1 -> v2:
> 1. Split original `overlap_check()` to `meminfo_overlap_check()`.
> 2. Rework commit message.
> ---
>   xen/arch/arm/bootfdt.c           | 13 +++++-----
>   xen/arch/arm/include/asm/setup.h |  2 ++
>   xen/arch/arm/setup.c             | 42 ++++++++++++++++++++++++++++++++
>   3 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..e2f6c7324b 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -88,6 +88,9 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>       for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
>       {
>           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        if ( mem == &bootinfo.reserved_mem &&
> +             check_reserved_regions_overlap(start, size) )
> +            return -EINVAL;
>           /* Some DT may describe empty bank, ignore them */
>           if ( !size )
>               continue;
> @@ -482,7 +485,9 @@ static int __init process_shm_node(const void *fdt, int node,
>                   return -EINVAL;
>               }
>   
> -            if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
> +            if ( check_reserved_regions_overlap(paddr, size) )
> +                return -EINVAL;
> +            else
>               {
>                   if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
>                       continue;
> @@ -493,12 +498,6 @@ static int __init process_shm_node(const void *fdt, int node,
>                       return -EINVAL;
>                   }
>               }
> -            else
> -            {
> -                printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
> -                        mem->bank[i].start, bank_end);
> -                return -EINVAL;
> -            }
>           }
>       }
>   
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index fdbf68aadc..6a9f88ecbb 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e,
>   size_t boot_fdt_info(const void *fdt, paddr_t paddr);
>   const char *boot_fdt_cmdline(const void *fdt);
>   
> +int check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size);
> +
>   struct bootmodule *add_boot_module(bootmodule_kind kind,
>                                      paddr_t start, paddr_t size, bool domU);
>   struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..e6eeb3a306 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -261,6 +261,31 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>       cb(s, e);
>   }
>   
> +static int __init meminfo_overlap_check(struct meminfo *meminfo,
> +                                        paddr_t region_start,
> +                                        paddr_t region_end)

I am starting to dislike the use of 'end' for a couple of reasons:
   1) It never clear whether this is inclusive or exclusive
   2) When it is exclusive, this doesn't properly work if the region 
finish at (2^64 - 1) as 'end' would be 0

I have started to clean-up the Arm code to avoid all those issues. So 
for new code, I would rather prefer if we use 'start' and 'size' to 
describe a region.

> +{
> +    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> +    unsigned int i, bank_num = meminfo->nr_banks;
> +
> +    for ( i = 0; i < bank_num; i++ )
> +    {
> +        bank_start = meminfo->bank[i].start;
> +        bank_end = bank_start + meminfo->bank[i].size;
> +
> +        if ( region_end <= bank_start || region_start >= bank_end )
> +            continue;
> +        else
> +        {
> +            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",
> +                   region_start, region_end, i, bank_start, bank_end);
> +            return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>   void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>                                     void (*cb)(paddr_t, paddr_t),
>                                     unsigned int first)
> @@ -271,7 +296,24 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e,
>           cb(s, e);
>   }
>   
> +/*
> + * Given an input physical address range, check if this range is overlapping
> + * with the existing reserved memory regions defined in bootinfo.
> + * Return 0 if the input physical address range is not overlapping with any
> + * existing reserved memory regions, otherwise -EINVAL.
> + */
> +int __init check_reserved_regions_overlap(paddr_t region_start,
> +                                          paddr_t region_size)

None of the caller seems to care about the return (other than it is 
failing or not). So I would prefer if this returns a boolean to indicate 
whether the check pass or not.

> +{
> +    paddr_t region_end = region_start + region_size; > +
> +    /* Check if input region is overlapping with bootinfo.reserved_mem banks */
> +    if ( meminfo_overlap_check(&bootinfo.reserved_mem,
> +                               region_start, region_end) )
> +        return -EINVAL;
>   
> +    return 0;
> +}
>   
>   struct bootmodule __init *add_boot_module(bootmodule_kind kind,
>                                             paddr_t start, paddr_t size,

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Julien Grall]

On 14/12/2022 03:16, Henry Wang wrote:
> As we are having more and more types of static region, and all of
> these static regions are defined in bootinfo.reserved_mem, it is
> necessary to add the overlap check of reserved memory regions in Xen,
> because such check will help user to identify the misconfiguration in
> the device tree at the early stage of boot time.
> 
> Currently we have 3 types of static region, namely
> (1) static memory
> (2) static heap
> (3) static shared memory
> 
> (1) and (2) are parsed by the function `device_tree_get_meminfo()` and
> (3) is parsed using its own logic. All of parsed information of these
> types will be stored in `struct meminfo`.
> 
> Therefore, to unify the overlap checking logic for all of these types,
> this commit firstly introduces a helper `meminfo_overlap_check()` and
> a function `check_reserved_regions_overlap()` to check if an input
> physical address range is overlapping with the existing memory regions
> defined in bootinfo. After that, use `check_reserved_regions_overlap()`
> in `device_tree_get_meminfo()` to do the overlap check of (1) and (2)
> and replace the original overlap check of (3) with
> `check_reserved_regions_overlap()`.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> ---
> v1 -> v2:
> 1. Split original `overlap_check()` to `meminfo_overlap_check()`.
> 2. Rework commit message.
> ---
>   xen/arch/arm/bootfdt.c           | 13 +++++-----
>   xen/arch/arm/include/asm/setup.h |  2 ++
>   xen/arch/arm/setup.c             | 42 ++++++++++++++++++++++++++++++++
>   3 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0085c28d74..e2f6c7324b 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -88,6 +88,9 @@ static int __init device_tree_get_meminfo(const void *fdt, int node,
>       for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
>       {
>           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
> +        if ( mem == &bootinfo.reserved_mem &&
> +             check_reserved_regions_overlap(start, size) )
> +            return -EINVAL;
>           /* Some DT may describe empty bank, ignore them */
>           if ( !size )
>               continue;
> @@ -482,7 +485,9 @@ static int __init process_shm_node(const void *fdt, int node,
>                   return -EINVAL;
>               }
>   
> -            if ( (end <= mem->bank[i].start) || (paddr >= bank_end) )
> +            if ( check_reserved_regions_overlap(paddr, size) )
> +                return -EINVAL;
> +            else
>               {
>                   if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 )
>                       continue;
> @@ -493,12 +498,6 @@ static int __init process_shm_node(const void *fdt, int node,
>                       return -EINVAL;
>                   }
>               }
> -            else
> -            {
> -                printk("fdt: shared memory region overlap with an existing entry %#"PRIpaddr" - %#"PRIpaddr"\n",
> -                        mem->bank[i].start, bank_end);
> -                return -EINVAL;
> -            }
>           }
>       }
>   
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index fdbf68aadc..6a9f88ecbb 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -143,6 +143,8 @@ void fw_unreserved_regions(paddr_t s, paddr_t e,
>   size_t boot_fdt_info(const void *fdt, paddr_t paddr);
>   const char *boot_fdt_cmdline(const void *fdt);
>   
> +int check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size);
> +
>   struct bootmodule *add_boot_module(bootmodule_kind kind,
>                                      paddr_t start, paddr_t size, bool domU);
>   struct bootmodule *boot_module_find_by_kind(bootmodule_kind kind);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 1f26f67b90..e6eeb3a306 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -261,6 +261,31 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e,
>       cb(s, e);
>   }
>   
> +static int __init meminfo_overlap_check(struct meminfo *meminfo,
> +                                        paddr_t region_start,
> +                                        paddr_t region_end)
> +{
> +    paddr_t bank_start = INVALID_PADDR, bank_end = 0;
> +    unsigned int i, bank_num = meminfo->nr_banks;
> +
> +    for ( i = 0; i < bank_num; i++ )
> +    {
> +        bank_start = meminfo->bank[i].start;
> +        bank_end = bank_start + meminfo->bank[i].size;
> +
> +        if ( region_end <= bank_start || region_start >= bank_end )
> +            continue;
> +        else
> +        {
> +            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",

AFAICT, in messages, the end would be inclusive. But here...

> +                   region_start, region_end, i, bank_start, bank_end);

... it would be exclusive. I would suggest to print using the format 
[start, end[ or decrement the value by 1.

Cheers,

-- 
Julien Grall

RE: [PATCH v2 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: Re: [PATCH v2 1/3] xen/arm: Add memory overlap check for
> bootinfo.reserved_mem
> 
> Hi Henry,
> 
> On 14/12/2022 03:16, Henry Wang wrote:
> >
> > +static int __init meminfo_overlap_check(struct meminfo *meminfo,
> > +                                        paddr_t region_start,
> > +                                        paddr_t region_end)
> 
> I am starting to dislike the use of 'end' for a couple of reasons:
>    1) It never clear whether this is inclusive or exclusive
>    2) When it is exclusive, this doesn't properly work if the region
> finish at (2^64 - 1) as 'end' would be 0

Good points!

> 
> I have started to clean-up the Arm code to avoid all those issues. So
> for new code, I would rather prefer if we use 'start' and 'size' to
> describe a region.

So I will use 'start' and 'size' in v3.

> 
> > +/*
> > + * Given an input physical address range, check if this range is overlapping
> > + * with the existing reserved memory regions defined in bootinfo.
> > + * Return 0 if the input physical address range is not overlapping with any
> > + * existing reserved memory regions, otherwise -EINVAL.
> > + */
> > +int __init check_reserved_regions_overlap(paddr_t region_start,
> > +                                          paddr_t region_size)
> 
> None of the caller seems to care about the return (other than it is
> failing or not). So I would prefer if this returns a boolean to indicate
> whether the check pass or not.

Sure, will fix in v3.

Kind regards,
Henry

RE: [PATCH v2 1/3] xen/arm: Add memory overlap check for bootinfo.reserved_mem

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: Re: [PATCH v2 1/3] xen/arm: Add memory overlap check for
> bootinfo.reserved_mem
> > +            printk("Region %#"PRIpaddr" - %#"PRIpaddr" overlapping with
> bank[%u] %#"PRIpaddr" - %#"PRIpaddr"\n",
> 
> AFAICT, in messages, the end would be inclusive. But here...
> 
> > +                   region_start, region_end, i, bank_start, bank_end);
> 
> ... it would be exclusive. I would suggest to print using the format
> [start, end[ or decrement the value by 1.

Another good point, thanks! I will switch to the "[start, end]" format
in all 3 patches.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall