[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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@xxxxxxx>
---
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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.