[Xen-devel] [PATCH v5 0/7] reserved-memory in dom0

[Xen-devel] [PATCH v5 0/7] reserved-memory in dom0

[Stefano Stabellini]

Hi all,

This patch series introduces partial reserved-memory support for dom0
only (no domU support for reserved-memory yet.)


The following changes since commit 762b9a2d990bba1f3aefe660cff0c37ad2e375bc:

  xen/page_alloc: Keep away MFN 0 from the buddy allocator (2019-08-09 11:12:55 -0700)

are available in the Git repository at:

  http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git reserved-mem-4 

for you to fetch changes up to 74d2088dbd793409587a20db124d004e944d23e8:

  xen/arm: add reserved-memory regions to the dom0 memory node (2019-08-12 12:25:45 -0700)

----------------------------------------------------------------
Stefano Stabellini (7):
      xen/arm: pass node to device_tree_for_each_node
      xen/arm: make process_memory_node a device_tree_node_func
      xen/arm: keep track of reserved-memory regions
      xen/arm: early_print_info print reserved_mem
      xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions
      xen/arm: don't iomem_permit_access for reserved-memory regions
      xen/arm: add reserved-memory regions to the dom0 memory node

 xen/arch/arm/acpi/boot.c      |  8 ++--
 xen/arch/arm/bootfdt.c        | 96 ++++++++++++++++++++++++++++++-------------
 xen/arch/arm/domain_build.c   | 46 ++++++++++++++-------
 xen/arch/arm/setup.c          | 53 ++++++++++++++++++++++--
 xen/include/asm-arm/setup.h   |  1 +
 xen/include/xen/device_tree.h |  6 +--
 6 files changed, 157 insertions(+), 53 deletions(-)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 1/7] xen/arm: pass node to device_tree_for_each_node

[Stefano Stabellini]

Add a new parameter to device_tree_for_each_node: node, the node to
start the search from. Passing 0 triggers the old behavior.

Set min_depth to depth of the current node + 1 and replace the for
loop with a do/while loop to avoid scanning siblings of the initial node
passed as an argument.

We need this change because in follow-up patches we want to be able to
use reuse device_tree_for_each_node to call a function for each children
nodes of a provided node and the node itself.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v5:
- go back to v3
- code style improvement in acpi/boot.c
- improve comments and commit message
- increase min_depth to avoid parsing siblings
- replace for with do/while loop and increase min_depth to avoid
  scanning siblings of the initial node
- pass only node, calculate depth

Changes in v3:
- improve commit message
- improve in-code comments
- improve code style

Changes in v2:
- new
---
 xen/arch/arm/acpi/boot.c      |  8 +++++---
 xen/arch/arm/bootfdt.c        | 19 ++++++++++---------
 xen/include/xen/device_tree.h |  6 +++---
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 9b29769a10..d4957cca06 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
      * - the device tree is not empty (it has more than just a /chosen node)
      *   and ACPI has not been force enabled (acpi=force)
      */
-    if ( param_acpi_off || ( !param_acpi_force
-                             && device_tree_for_each_node(device_tree_flattened,
-                                                   dt_scan_depth1_nodes, NULL)))
+    if ( param_acpi_off)
+        goto disable;
+	if ( !param_acpi_force &&
+		 device_tree_for_each_node(device_tree_flattened, 0,
+			                       dt_scan_depth1_nodes, NULL) )
         goto disable;
 
     /*
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 891b4b66ff..a872ea57d6 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
 /**
  * device_tree_for_each_node - iterate over all device tree nodes
  * @fdt: flat device tree.
+ * @node: node to start the search from
  * @func: function to call for each node.
  * @data: data to pass to @func.
  *
@@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
  * Returns 0 if all nodes were iterated over successfully.  If @func
  * returns a value different from 0, that value is returned immediately.
  */
-int __init device_tree_for_each_node(const void *fdt,
+int __init device_tree_for_each_node(const void *fdt, int node,
                                      device_tree_node_func func,
                                      void *data)
 {
-    int node;
-    int depth;
+    int depth = fdt_node_depth(fdt, node);
+    int min_depth = depth + 1;
     u32 address_cells[DEVICE_TREE_MAX_DEPTH];
     u32 size_cells[DEVICE_TREE_MAX_DEPTH];
     int ret;
 
-    for ( node = 0, depth = 0;
-          node >=0 && depth >= 0;
-          node = fdt_next_node(fdt, node, &depth) )
-    {
+    do {
         const char *name = fdt_get_name(fdt, node, NULL);
         u32 as, ss;
 
@@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt,
         ret = func(fdt, node, name, depth, as, ss, data);
         if ( ret != 0 )
             return ret;
-    }
+
+        node = fdt_next_node(fdt, node, &depth);
+    } while ( node >= 0 && depth >= min_depth );
+
     return 0;
 }
 
@@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
 
     add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
 
-    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
+    device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
     early_print_info();
 
     return fdt_totalsize(fdt);
diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
index 83156297e2..9a7a8f2dab 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
 
 extern const void *device_tree_flattened;
 
-int device_tree_for_each_node(const void *fdt,
-                                     device_tree_node_func func,
-                                     void *data);
+int device_tree_for_each_node(const void *fdt, int node,
+                              device_tree_node_func func,
+                              void *data);
 
 /**
  * dt_unflatten_host_device_tree - Unflatten the host device tree
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Stefano Stabellini]

Change the signature of process_memory_node to match
device_tree_node_func. Thanks to this change, the next patch will be
able to use device_tree_for_each_node to call process_memory_node on all
the children of a provided node.

Return error if there is no reg property or if nr_banks is reached. Let
the caller deal with the error.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v5:
- return -ENOENT if address_cells or size_cells are not properly set

Changes in v4:
- return error if there is no reg propery, remove printk
- return error if nr_banks is reached

Changes in v3:
- improve commit message
- check return value of process_memory_node

Changes in v2:
- new
---
 xen/arch/arm/bootfdt.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index a872ea57d6..590b14304c 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int node,
     return 0;
 }
 
-static void __init process_memory_node(const void *fdt, int node,
-                                       const char *name,
-                                       u32 address_cells, u32 size_cells)
+static int __init process_memory_node(const void *fdt, int node,
+                                      const char *name, int depth,
+                                      u32 address_cells, u32 size_cells,
+                                      void *data)
 {
     const struct fdt_property *prop;
     int i;
@@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, int node,
     u32 reg_cells = address_cells + size_cells;
 
     if ( address_cells < 1 || size_cells < 1 )
-    {
-        printk("fdt: node `%s': invalid #address-cells or #size-cells",
-               name);
-        return;
-    }
+        return -ENOENT;
 
     prop = fdt_get_property(fdt, node, "reg", NULL);
     if ( !prop )
-    {
-        printk("fdt: node `%s': missing `reg' property\n", name);
-        return;
-    }
+        return -ENOENT;
 
     cell = (const __be32 *)prop->data;
     banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
@@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
         bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
         bootinfo.mem.nr_banks++;
     }
+
+    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
+        return -ENOSPC;
+    return 0;
 }
 
 static void __init process_multiboot_node(const void *fdt, int node,
@@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt,
                                   u32 address_cells, u32 size_cells,
                                   void *data)
 {
+    int rc = 0;
+
     if ( device_tree_node_matches(fdt, node, "memory") )
-        process_memory_node(fdt, node, name, address_cells, size_cells);
+        rc = process_memory_node(fdt, node, name, depth,
+                                 address_cells, size_cells, NULL);
     else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
               device_tree_node_compatible(fdt, node, "multiboot,module" )))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
         process_chosen_node(fdt, node, name, address_cells, size_cells);
 
-    return 0;
+    return rc;
 }
 
 static void __init early_print_info(void)
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Stefano Stabellini]

As we parse the device tree in Xen, keep track of the reserved-memory
regions as they need special treatment (follow-up patches will make use
of the stored information.)

Reuse process_memory_node to add reserved-memory regions to the
bootinfo.reserved_mem array.

Refuse to continue once we reach the max number of reserved memory
regions to avoid accidentally mapping any portions of them into a VM.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

---
Changes in v5:
- remove unneeded cast
- remove unneeded strlen check
- don't pass address_cells, size_cells, depth to device_tree_for_each_node

Changes in v4:
- depth + 1 in process_reserved_memory_node
- pass address_cells and size_cells to device_tree_for_each_node
- pass struct meminfo * instead of a boolean to process_memory_node
- improve in-code comment
- use a separate process_reserved_memory_node (separate from
  process_memory_node) function wrapper to have different error handling

Changes in v3:
- match only /reserved-memory
- put the warning back in place for reg not present on a normal memory
  region
- refuse to continue once we reach the max number of reserved memory
  regions

Changes in v2:
- call process_memory_node from process_reserved_memory_node to avoid
  duplication
---
 xen/arch/arm/bootfdt.c      | 41 +++++++++++++++++++++++++++++++------
 xen/include/asm-arm/setup.h |  1 +
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 590b14304c..0b0e22a3d0 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -136,6 +136,7 @@ static int __init process_memory_node(const void *fdt, int node,
     const __be32 *cell;
     paddr_t start, size;
     u32 reg_cells = address_cells + size_cells;
+    struct meminfo *mem = data;
 
     if ( address_cells < 1 || size_cells < 1 )
         return -ENOENT;
@@ -147,21 +148,46 @@ static int __init process_memory_node(const void *fdt, int node,
     cell = (const __be32 *)prop->data;
     banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
 
-    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
+    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
     {
         device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
         if ( !size )
             continue;
-        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
-        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
-        bootinfo.mem.nr_banks++;
+        mem->bank[mem->nr_banks].start = start;
+        mem->bank[mem->nr_banks].size = size;
+        mem->nr_banks++;
     }
 
-    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
+    if ( mem->nr_banks == NR_MEM_BANKS )
         return -ENOSPC;
     return 0;
 }
 
+static int __init process_reserved_memory_node(const void *fdt, int node,
+                                               const char *name, int depth,
+                                               u32 address_cells,
+                                               u32 size_cells,
+                                               void *data)
+{
+    int rc = process_memory_node(fdt, node, name, depth, address_cells,
+                                 size_cells, data);
+
+    if ( rc == -ENOSPC )
+        panic("Max number of supported reserved-memory regions reached.");
+    else if ( rc != -ENOENT )
+        return rc;
+    return 0;
+}
+
+static int __init process_reserved_memory(const void *fdt, int node,
+                                          const char *name, int depth,
+                                          u32 address_cells, u32 size_cells)
+{
+    return device_tree_for_each_node(fdt, node,
+                                     process_reserved_memory_node,
+                                     &bootinfo.reserved_mem);
+}
+
 static void __init process_multiboot_node(const void *fdt, int node,
                                           const char *name,
                                           u32 address_cells, u32 size_cells)
@@ -295,7 +321,10 @@ static int __init early_scan_node(const void *fdt,
 
     if ( device_tree_node_matches(fdt, node, "memory") )
         rc = process_memory_node(fdt, node, name, depth,
-                                 address_cells, size_cells, NULL);
+                                 address_cells, size_cells, &bootinfo.mem);
+    else if ( depth == 1 && !strcmp(name, "reserved-memory") )
+        rc = process_reserved_memory(fdt, node, name, depth,
+                                     address_cells, size_cells);
     else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
               device_tree_node_compatible(fdt, node, "multiboot,module" )))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 8bf3d5910a..efcba545c2 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -66,6 +66,7 @@ struct bootcmdlines {
 
 struct bootinfo {
     struct meminfo mem;
+    struct meminfo reserved_mem;
     struct bootmodules modules;
     struct bootcmdlines cmdlines;
 #ifdef CONFIG_ACPI
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 4/7] xen/arm: early_print_info print reserved_mem

[Stefano Stabellini]

Improve early_print_info to also print the banks saved in
bootinfo.reserved_mem. Print them right after RESVD, increasing the same
index.

Since we are at it, also switch the existing RESVD print to use unsigned
int.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v5:
- switch to unsigned

Changes in v4:
- new patch
---
 xen/arch/arm/bootfdt.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 0b0e22a3d0..32153e6207 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -337,9 +337,10 @@ static int __init early_scan_node(const void *fdt,
 static void __init early_print_info(void)
 {
     struct meminfo *mi = &bootinfo.mem;
+    struct meminfo *mem_resv = &bootinfo.reserved_mem;
     struct bootmodules *mods = &bootinfo.modules;
     struct bootcmdlines *cmds = &bootinfo.cmdlines;
-    int i, nr_rsvd;
+    unsigned int i, j, nr_rsvd;
 
     for ( i = 0; i < mi->nr_banks; i++ )
         printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
@@ -361,9 +362,15 @@ static void __init early_print_info(void)
             continue;
         /* fdt_get_mem_rsv returns length */
         e += s;
-        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
+        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n",
                      i, s, e);
     }
+    for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
+    {
+        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
+                     mem_resv->bank[j].start,
+                     mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
+    }
     printk("\n");
     for ( i = 0 ; i < cmds->nr_mods; i++ )
         printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 5/7] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions

[Stefano Stabellini]

reserved-memory regions overlap with memory nodes. The overlapping
memory is reserved-memory and should be handled accordingly:
consider_modules and dt_unreserved_regions should skip these regions the
same way they are already skipping mem-reserve regions.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---

Changes in v4:
- code style
- add acked-by

Changes in v3:
- coding style
- in-code comments

Changes in v2:
- fix commit message: full overlap
- remove check_reserved_memory
- extend consider_modules and dt_unreserved_regions
---
 xen/arch/arm/setup.c | 53 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 215746a5c3..bc4082296e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -206,6 +206,28 @@ void __init dt_unreserved_regions(paddr_t s, paddr_t e,
         }
     }
 
+    /*
+     * i is the current bootmodule we are evaluating across all possible
+     * kinds.
+     *
+     * When retrieving the corresponding reserved-memory addresses
+     * below, we need to index the bootinfo.reserved_mem bank starting
+     * from 0, and only counting the reserved-memory modules. Hence,
+     * we need to use i - nr.
+     */
+    for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+    {
+        paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
+        paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
+
+        if ( s < r_e && r_s < e )
+        {
+            dt_unreserved_regions(r_e, e, cb, i + 1);
+            dt_unreserved_regions(s, r_s, cb, i + 1);
+            return;
+        }
+    }
+
     cb(s, e);
 }
 
@@ -392,7 +414,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
 {
     const struct bootmodules *mi = &bootinfo.modules;
     int i;
-    int nr_rsvd;
+    int nr;
 
     s = (s+align-1) & ~(align-1);
     e = e & ~(align-1);
@@ -418,9 +440,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
 
     /* Now check any fdt reserved areas. */
 
-    nr_rsvd = fdt_num_mem_rsv(device_tree_flattened);
+    nr = fdt_num_mem_rsv(device_tree_flattened);
 
-    for ( ; i < mi->nr_mods + nr_rsvd; i++ )
+    for ( ; i < mi->nr_mods + nr; i++ )
     {
         paddr_t mod_s, mod_e;
 
@@ -442,6 +464,31 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e,
             return consider_modules(s, mod_s, size, align, i+1);
         }
     }
+
+    /*
+     * i is the current bootmodule we are evaluating, across all
+     * possible kinds of bootmodules.
+     *
+     * When retrieving the corresponding reserved-memory addresses, we
+     * need to index the bootinfo.reserved_mem bank starting from 0, and
+     * only counting the reserved-memory modules. Hence, we need to use
+     * i - nr.
+     */
+    nr += mi->nr_mods;
+    for ( ; i - nr < bootinfo.reserved_mem.nr_banks; i++ )
+    {
+        paddr_t r_s = bootinfo.reserved_mem.bank[i - nr].start;
+        paddr_t r_e = r_s + bootinfo.reserved_mem.bank[i - nr].size;
+
+        if ( s < r_e && r_s < e )
+        {
+            r_e = consider_modules(r_e, e, size, align, i + 1);
+            if ( r_e )
+                return r_e;
+
+            return consider_modules(s, r_s, size, align, i + 1);
+        }
+    }
     return e;
 }
 #endif
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions

[Stefano Stabellini]

Don't allow reserved-memory regions to be remapped into any unprivileged
guests, until reserved-memory regions are properly supported in Xen. For
now, do not call iomem_permit_access on them, because giving
iomem_permit_access to dom0 means that the toolstack will be able to
assign the region to a domU.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---

Changes in v5:
- fix check condition
- use strnicmp
- return error
- improve commit message

Changes in v4:
- compare the parent name with reserved-memory
- use dt_node_cmp

Changes in v3:
- new patch
---
 xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4c8404155a..e0c0c01c88 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
     bool need_mapping = !dt_device_for_passthrough(dev);
     int res;
 
-    res = iomem_permit_access(d, paddr_to_pfn(addr),
-                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
-    if ( res )
+    /*
+     * Don't give iomem permissions for reserved-memory ranges to domUs
+     * until reserved-memory support is complete.
+     */
+    if ( strnicmp(dt_node_full_name(dev), "/reserved-memory",
+         strlen("/reserved-memory")) != 0 )
     {
-        printk(XENLOG_ERR "Unable to permit to dom%d access to"
-               " 0x%"PRIx64" - 0x%"PRIx64"\n",
-               d->domain_id,
-               addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
-        return res;
+        res = iomem_permit_access(d, paddr_to_pfn(addr),
+                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
+        if ( res )
+        {
+            printk(XENLOG_ERR "Unable to permit to dom%d access to"
+                    " 0x%"PRIx64" - 0x%"PRIx64"\n",
+                    d->domain_id,
+                    addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
+            return res;
+        }
     }
 
     if ( need_mapping )
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v5 7/7] xen/arm: add reserved-memory regions to the dom0 memory node

[Stefano Stabellini]

Reserved memory regions are automatically remapped to dom0. Their device
tree nodes are also added to dom0 device tree. However, the dom0 memory
node is not currently extended to cover the reserved memory regions
ranges as required by the spec.  This commit fixes it.

Change make_memory_node to take a  struct meminfo * instead of a
kernel_info. Call it twice for dom0, once to create the first regular
memory node, and the second time to create a second memory node with the
ranges covering reserved-memory regions.

Also, make a small code style fix in make_memory_node.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changes in v5:
- add acked-by

Changes in v4:
- pass struct meminfo * to make_memory_node
- call make_memory_node twice for dom0, once for normal memory, once for
  reserved-memory regions
---
 xen/arch/arm/domain_build.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e0c0c01c88..d11cd40ba1 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -639,11 +639,11 @@ static int __init fdt_property_interrupts(void *fdt, gic_interrupt_t *intr,
 static int __init make_memory_node(const struct domain *d,
                                    void *fdt,
                                    int addrcells, int sizecells,
-                                   const struct kernel_info *kinfo)
+                                   struct meminfo *mem)
 {
     int res, i;
     int reg_size = addrcells + sizecells;
-    int nr_cells = reg_size*kinfo->mem.nr_banks;
+    int nr_cells = reg_size * mem->nr_banks;
     __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
     __be32 *cells;
 
@@ -662,10 +662,10 @@ static int __init make_memory_node(const struct domain *d,
         return res;
 
     cells = &reg[0];
-    for ( i = 0 ; i < kinfo->mem.nr_banks; i++ )
+    for ( i = 0 ; i < mem->nr_banks; i++ )
     {
-        u64 start = kinfo->mem.bank[i].start;
-        u64 size = kinfo->mem.bank[i].size;
+        u64 start = mem->bank[i].start;
+        u64 size = mem->bank[i].size;
 
         dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
                    i, start, start + size);
@@ -1485,10 +1485,18 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
         if ( res )
             return res;
 
-        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
+        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
         if ( res )
             return res;
 
+        /*
+         * Create a second memory node to store the ranges covering
+         * reserved-memory regions.
+         */
+        res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
+                               &bootinfo.reserved_mem);
+        if ( res )
+            return res;
     }
 
     res = fdt_end_node(kinfo->fdt);
@@ -1744,7 +1752,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo)
     if ( ret )
         goto err;
 
-    ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
+    ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, &kinfo->mem);
     if ( ret )
         goto err;
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 1/7] xen/arm: pass node to device_tree_for_each_node

[Volodymyr Babchuk]

Hi Stefano,

Stefano Stabellini writes:

> Add a new parameter to device_tree_for_each_node: node, the node to
> start the search from. Passing 0 triggers the old behavior.
>
> Set min_depth to depth of the current node + 1 and replace the for
> loop with a do/while loop to avoid scanning siblings of the initial node
> passed as an argument.
>
> We need this change because in follow-up patches we want to be able to
> use reuse device_tree_for_each_node to call a function for each children
> nodes of a provided node and the node itself.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>

You can have my

Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

providing that you'll fix formatting issue below.

> ---
> Changes in v5:
> - go back to v3
> - code style improvement in acpi/boot.c
> - improve comments and commit message
> - increase min_depth to avoid parsing siblings
> - replace for with do/while loop and increase min_depth to avoid
>   scanning siblings of the initial node
> - pass only node, calculate depth
>
> Changes in v3:
> - improve commit message
> - improve in-code comments
> - improve code style
>
> Changes in v2:
> - new
> ---
>  xen/arch/arm/acpi/boot.c      |  8 +++++---
>  xen/arch/arm/bootfdt.c        | 19 ++++++++++---------
>  xen/include/xen/device_tree.h |  6 +++---
>  3 files changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 9b29769a10..d4957cca06 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
>       * - the device tree is not empty (it has more than just a /chosen node)
>       *   and ACPI has not been force enabled (acpi=force)
>       */
> -    if ( param_acpi_off || ( !param_acpi_force
> -                             && device_tree_for_each_node(device_tree_flattened,
> -                                                   dt_scan_depth1_nodes, NULL)))
> +    if ( param_acpi_off)
> +        goto disable;
> +	if ( !param_acpi_force &&
> +		 device_tree_for_each_node(device_tree_flattened, 0,
> +			                       dt_scan_depth1_nodes, NULL) )
There is 3 tabs, followed by spaces.

This file missed emacs magic at the end. I think, this is cause for this
formatting issue.

>          goto disable;
>  
>      /*
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 891b4b66ff..a872ea57d6 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
>  /**
>   * device_tree_for_each_node - iterate over all device tree nodes
>   * @fdt: flat device tree.
> + * @node: node to start the search from
>   * @func: function to call for each node.
>   * @data: data to pass to @func.
>   *
> @@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
>   * Returns 0 if all nodes were iterated over successfully.  If @func
>   * returns a value different from 0, that value is returned immediately.
>   */
> -int __init device_tree_for_each_node(const void *fdt,
> +int __init device_tree_for_each_node(const void *fdt, int node,
>                                       device_tree_node_func func,
>                                       void *data)
>  {
> -    int node;
> -    int depth;
> +    int depth = fdt_node_depth(fdt, node);
> +    int min_depth = depth + 1;
>      u32 address_cells[DEVICE_TREE_MAX_DEPTH];
>      u32 size_cells[DEVICE_TREE_MAX_DEPTH];
>      int ret;
>  
> -    for ( node = 0, depth = 0;
> -          node >=0 && depth >= 0;
> -          node = fdt_next_node(fdt, node, &depth) )
> -    {
> +    do {
>          const char *name = fdt_get_name(fdt, node, NULL);
>          u32 as, ss;
>  
> @@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt,
>          ret = func(fdt, node, name, depth, as, ss, data);
>          if ( ret != 0 )
>              return ret;
> -    }
> +
> +        node = fdt_next_node(fdt, node, &depth);
> +    } while ( node >= 0 && depth >= min_depth );
> +
>      return 0;
>  }
>  
> @@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
>  
>      add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
>  
> -    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> +    device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
>      early_print_info();
>  
>      return fdt_totalsize(fdt);
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 83156297e2..9a7a8f2dab 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
>  
>  extern const void *device_tree_flattened;
>  
> -int device_tree_for_each_node(const void *fdt,
> -                                     device_tree_node_func func,
> -                                     void *data);
> +int device_tree_for_each_node(const void *fdt, int node,
> +                              device_tree_node_func func,
> +                              void *data);
>  
>  /**
>   * dt_unflatten_host_device_tree - Unflatten the host device tree


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Volodymyr Babchuk]

Hi Stefano,

Stefano Stabellini writes:

> Change the signature of process_memory_node to match
> device_tree_node_func. Thanks to this change, the next patch will be
> able to use device_tree_for_each_node to call process_memory_node on all
> the children of a provided node.
>
> Return error if there is no reg property or if nr_banks is reached. Let
> the caller deal with the error.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v5:
> - return -ENOENT if address_cells or size_cells are not properly set
>
> Changes in v4:
> - return error if there is no reg propery, remove printk
> - return error if nr_banks is reached
>
> Changes in v3:
> - improve commit message
> - check return value of process_memory_node
>
> Changes in v2:
> - new
> ---
>  xen/arch/arm/bootfdt.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index a872ea57d6..590b14304c 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int node,
>      return 0;
>  }
>
> -static void __init process_memory_node(const void *fdt, int node,
> -                                       const char *name,
> -                                       u32 address_cells, u32 size_cells)
> +static int __init process_memory_node(const void *fdt, int node,
> +                                      const char *name, int depth,
> +                                      u32 address_cells, u32 size_cells,
> +                                      void *data)
>  {
>      const struct fdt_property *prop;
>      int i;
> @@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, int node,
>      u32 reg_cells = address_cells + size_cells;
>
>      if ( address_cells < 1 || size_cells < 1 )
> -    {
> -        printk("fdt: node `%s': invalid #address-cells or #size-cells",
> -               name);
> -        return;
> -    }
> +        return -ENOENT;
>
>      prop = fdt_get_property(fdt, node, "reg", NULL);
>      if ( !prop )
> -    {
> -        printk("fdt: node `%s': missing `reg' property\n", name);
> -        return;
> -    }
> +        return -ENOENT;
>
>      cell = (const __be32 *)prop->data;
>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
>          bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>          bootinfo.mem.nr_banks++;
>      }
> +
> +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> +        return -ENOSPC;
Are you sure that this logic is correct?

For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
in device tree, this function will fail. But it should not. I think you
want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS

> +    return 0;
>  }
>
>  static void __init process_multiboot_node(const void *fdt, int node,
> @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt,
>                                    u32 address_cells, u32 size_cells,
>                                    void *data)
>  {
> +    int rc = 0;
> +
>      if ( device_tree_node_matches(fdt, node, "memory") )
> -        process_memory_node(fdt, node, name, address_cells, size_cells);
> +        rc = process_memory_node(fdt, node, name, depth,
> +                                 address_cells, size_cells, NULL);
>      else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
>                device_tree_node_compatible(fdt, node, "multiboot,module" )))
>          process_multiboot_node(fdt, node, name, address_cells, size_cells);
>      else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>          process_chosen_node(fdt, node, name, address_cells, size_cells);
>
> -    return 0;
> +    return rc;
>  }
>
>  static void __init early_print_info(void)


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Volodymyr Babchuk]

Stefano Stabellini writes:

> As we parse the device tree in Xen, keep track of the reserved-memory
> regions as they need special treatment (follow-up patches will make use
> of the stored information.)
>
> Reuse process_memory_node to add reserved-memory regions to the
> bootinfo.reserved_mem array.
>
> Refuse to continue once we reach the max number of reserved memory
> regions to avoid accidentally mapping any portions of them into a VM.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>
> ---
> Changes in v5:
> - remove unneeded cast
> - remove unneeded strlen check
> - don't pass address_cells, size_cells, depth to device_tree_for_each_node
>
> Changes in v4:
> - depth + 1 in process_reserved_memory_node
> - pass address_cells and size_cells to device_tree_for_each_node
> - pass struct meminfo * instead of a boolean to process_memory_node
> - improve in-code comment
> - use a separate process_reserved_memory_node (separate from
>   process_memory_node) function wrapper to have different error handling
>
> Changes in v3:
> - match only /reserved-memory
> - put the warning back in place for reg not present on a normal memory
>   region
> - refuse to continue once we reach the max number of reserved memory
>   regions
>
> Changes in v2:
> - call process_memory_node from process_reserved_memory_node to avoid
>   duplication
> ---
>  xen/arch/arm/bootfdt.c      | 41 +++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/setup.h |  1 +
>  2 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 590b14304c..0b0e22a3d0 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -136,6 +136,7 @@ static int __init process_memory_node(const void *fdt, int node,
>      const __be32 *cell;
>      paddr_t start, size;
>      u32 reg_cells = address_cells + size_cells;
> +    struct meminfo *mem = data;
>
>      if ( address_cells < 1 || size_cells < 1 )
>          return -ENOENT;
> @@ -147,21 +148,46 @@ static int __init process_memory_node(const void *fdt, int node,
>      cell = (const __be32 *)prop->data;
>      banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>
> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
> +    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
What is logic behind the second part of the loop condition?

You know that if (banks > NR_MEM_BANKS) then you will exit with error. Do
you really need to iterate over loop in this case?

>      {
>          device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>          if ( !size )
>              continue;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> -        bootinfo.mem.nr_banks++;
> +        mem->bank[mem->nr_banks].start = start;
> +        mem->bank[mem->nr_banks].size = size;
> +        mem->nr_banks++;
>      }
>
> -    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> +    if ( mem->nr_banks == NR_MEM_BANKS )
Looks like you have the same off-by-one error, as in previous patch.
I can see that it was there earlier. But it is good time to fix it.

>          return -ENOSPC;
>      return 0;
>  }
>
> +static int __init process_reserved_memory_node(const void *fdt, int node,
> +                                               const char *name, int depth,
> +                                               u32 address_cells,
> +                                               u32 size_cells,
> +                                               void *data)
> +{
> +    int rc = process_memory_node(fdt, node, name, depth, address_cells,
> +                                 size_cells, data);
> +
> +    if ( rc == -ENOSPC )
> +        panic("Max number of supported reserved-memory regions reached.");
> +    else if ( rc != -ENOENT )
> +        return rc;
> +    return 0;
> +}
> +
> +static int __init process_reserved_memory(const void *fdt, int node,
> +                                          const char *name, int depth,
> +                                          u32 address_cells, u32 size_cells)
> +{
> +    return device_tree_for_each_node(fdt, node,
> +                                     process_reserved_memory_node,
> +                                     &bootinfo.reserved_mem);
> +}
> +
>  static void __init process_multiboot_node(const void *fdt, int node,
>                                            const char *name,
>                                            u32 address_cells, u32 size_cells)
> @@ -295,7 +321,10 @@ static int __init early_scan_node(const void *fdt,
>
>      if ( device_tree_node_matches(fdt, node, "memory") )
>          rc = process_memory_node(fdt, node, name, depth,
> -                                 address_cells, size_cells, NULL);
> +                                 address_cells, size_cells, &bootinfo.mem);
> +    else if ( depth == 1 && !strcmp(name, "reserved-memory") )
I believe you want to use dt_node_cmp() there.

> +        rc = process_reserved_memory(fdt, node, name, depth,
> +                                     address_cells, size_cells);
>      else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
>                device_tree_node_compatible(fdt, node, "multiboot,module" )))
>          process_multiboot_node(fdt, node, name, address_cells, size_cells);
> diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
> index 8bf3d5910a..efcba545c2 100644
> --- a/xen/include/asm-arm/setup.h
> +++ b/xen/include/asm-arm/setup.h
> @@ -66,6 +66,7 @@ struct bootcmdlines {
>
>  struct bootinfo {
>      struct meminfo mem;
> +    struct meminfo reserved_mem;
>      struct bootmodules modules;
>      struct bootcmdlines cmdlines;
>  #ifdef CONFIG_ACPI


--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 4/7] xen/arm: early_print_info print reserved_mem

[Volodymyr Babchuk]

Stefano Stabellini writes:

> Improve early_print_info to also print the banks saved in
> bootinfo.reserved_mem. Print them right after RESVD, increasing the same
> index.
>
> Since we are at it, also switch the existing RESVD print to use unsigned
> int.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

But, please see NIT below.

> ---
> Changes in v5:
> - switch to unsigned
>
> Changes in v4:
> - new patch
> ---
>  xen/arch/arm/bootfdt.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0b0e22a3d0..32153e6207 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -337,9 +337,10 @@ static int __init early_scan_node(const void *fdt,
>  static void __init early_print_info(void)
>  {
>      struct meminfo *mi = &bootinfo.mem;
> +    struct meminfo *mem_resv = &bootinfo.reserved_mem;
>      struct bootmodules *mods = &bootinfo.modules;
>      struct bootcmdlines *cmds = &bootinfo.cmdlines;
> -    int i, nr_rsvd;
> +    unsigned int i, j, nr_rsvd;
>  
>      for ( i = 0; i < mi->nr_banks; i++ )
>          printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> @@ -361,9 +362,15 @@ static void __init early_print_info(void)
>              continue;
>          /* fdt_get_mem_rsv returns length */
>          e += s;
> -        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
> +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n",
>                       i, s, e);
NIT: I see no reason, why this printk is split into two lines, as nicely fits
into one line.

>      }
> +    for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
> +    {
> +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
> +                     mem_resv->bank[j].start,
> +                     mem_resv->bank[j].start + mem_resv->bank[j].size - 1);
> +    }
>      printk("\n");
>      for ( i = 0 ; i < cmds->nr_mods; i++ )
>          printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions

[Volodymyr Babchuk]

Stefano Stabellini writes:

> Don't allow reserved-memory regions to be remapped into any unprivileged
> guests, until reserved-memory regions are properly supported in Xen. For
> now, do not call iomem_permit_access on them, because giving
> iomem_permit_access to dom0 means that the toolstack will be able to
> assign the region to a domU.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>
> Changes in v5:
> - fix check condition
> - use strnicmp
> - return error
> - improve commit message
>
> Changes in v4:
> - compare the parent name with reserved-memory
> - use dt_node_cmp
>
> Changes in v3:
> - new patch
> ---
>  xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4c8404155a..e0c0c01c88 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
>      bool need_mapping = !dt_device_for_passthrough(dev);
>      int res;
>  
> -    res = iomem_permit_access(d, paddr_to_pfn(addr),
> -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> -    if ( res )
> +    /*
> +     * Don't give iomem permissions for reserved-memory ranges to domUs
> +     * until reserved-memory support is complete.
> +     */
> +    if ( strnicmp(dt_node_full_name(dev), "/reserved-memory",
> +         strlen("/reserved-memory")) != 0 )
Why are you using strnicmp there? With such usage it is the same as
strcasecmp(). But, if you want to find "/reserved-memory" anywhere in
dt_node_full_name(dev), then you probably want to use strcasestr()


>      {
> -        printk(XENLOG_ERR "Unable to permit to dom%d access to"
> -               " 0x%"PRIx64" - 0x%"PRIx64"\n",
> -               d->domain_id,
> -               addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> -        return res;
> +        res = iomem_permit_access(d, paddr_to_pfn(addr),
> +                paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> +        if ( res )
> +        {
> +            printk(XENLOG_ERR "Unable to permit to dom%d access to"
> +                    " 0x%"PRIx64" - 0x%"PRIx64"\n",
> +                    d->domain_id,
> +                    addr & PAGE_MASK, PAGE_ALIGN(addr + len) - 1);
> +            return res;
> +        }
>      }
>  
>      if ( need_mapping )


-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Julien Grall]

Hi,

On 8/13/19 3:23 PM, Volodymyr Babchuk wrote:
> 
> Stefano Stabellini writes:
> 
>> As we parse the device tree in Xen, keep track of the reserved-memory
>> regions as they need special treatment (follow-up patches will make use
>> of the stored information.)
>>
>> Reuse process_memory_node to add reserved-memory regions to the
>> bootinfo.reserved_mem array.
>>
>> Refuse to continue once we reach the max number of reserved memory
>> regions to avoid accidentally mapping any portions of them into a VM.
>>
>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>
>> ---
>> Changes in v5:
>> - remove unneeded cast
>> - remove unneeded strlen check
>> - don't pass address_cells, size_cells, depth to device_tree_for_each_node
>>
>> Changes in v4:
>> - depth + 1 in process_reserved_memory_node
>> - pass address_cells and size_cells to device_tree_for_each_node
>> - pass struct meminfo * instead of a boolean to process_memory_node
>> - improve in-code comment
>> - use a separate process_reserved_memory_node (separate from
>>    process_memory_node) function wrapper to have different error handling
>>
>> Changes in v3:
>> - match only /reserved-memory
>> - put the warning back in place for reg not present on a normal memory
>>    region
>> - refuse to continue once we reach the max number of reserved memory
>>    regions
>>
>> Changes in v2:
>> - call process_memory_node from process_reserved_memory_node to avoid
>>    duplication
>> ---
>>   xen/arch/arm/bootfdt.c      | 41 +++++++++++++++++++++++++++++++------
>>   xen/include/asm-arm/setup.h |  1 +
>>   2 files changed, 36 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index 590b14304c..0b0e22a3d0 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -136,6 +136,7 @@ static int __init process_memory_node(const void *fdt, int node,
>>       const __be32 *cell;
>>       paddr_t start, size;
>>       u32 reg_cells = address_cells + size_cells;
>> +    struct meminfo *mem = data;
>>
>>       if ( address_cells < 1 || size_cells < 1 )
>>           return -ENOENT;
>> @@ -147,21 +148,46 @@ static int __init process_memory_node(const void *fdt, int node,
>>       cell = (const __be32 *)prop->data;
>>       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>>
>> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>> +    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
> What is logic behind the second part of the loop condition?
> 
> You know that if (banks > NR_MEM_BANKS) then you will exit with error. Do
> you really need to iterate over loop in this case?

Well, the error is ignored in the case of memory banks. So iterating 
over the first banks allows you to fill up bootinfo with as much as 
possible as RAM. The rest of the RAM will not be used by Xen.

> 
>>       {
>>           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>           if ( !size )
>>               continue;
>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>> -        bootinfo.mem.nr_banks++;
>> +        mem->bank[mem->nr_banks].start = start;
>> +        mem->bank[mem->nr_banks].size = size;
>> +        mem->nr_banks++;
>>       }
>>
>> -    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>> +    if ( mem->nr_banks == NR_MEM_BANKS )
> Looks like you have the same off-by-one error, as in previous patch.
> I can see that it was there earlier. But it is good time to fix it.

I don't think there was an off-by-one error before this series. So what 
do you mean?

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 4/7] xen/arm: early_print_info print reserved_mem

[Julien Grall]

Hi,

On 8/13/19 3:28 PM, Volodymyr Babchuk wrote:
> 
> Stefano Stabellini writes:
> 
>> Improve early_print_info to also print the banks saved in
>> bootinfo.reserved_mem. Print them right after RESVD, increasing the same
>> index.
>>
>> Since we are at it, also switch the existing RESVD print to use unsigned
>> int.
>>
>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>
> 
> But, please see NIT below.
> 
>> ---
>> Changes in v5:
>> - switch to unsigned
>>
>> Changes in v4:
>> - new patch
>> ---
>>   xen/arch/arm/bootfdt.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index 0b0e22a3d0..32153e6207 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -337,9 +337,10 @@ static int __init early_scan_node(const void *fdt,
>>   static void __init early_print_info(void)
>>   {
>>       struct meminfo *mi = &bootinfo.mem;
>> +    struct meminfo *mem_resv = &bootinfo.reserved_mem;
>>       struct bootmodules *mods = &bootinfo.modules;
>>       struct bootcmdlines *cmds = &bootinfo.cmdlines;
>> -    int i, nr_rsvd;
>> +    unsigned int i, j, nr_rsvd;
>>   
>>       for ( i = 0; i < mi->nr_banks; i++ )
>>           printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
>> @@ -361,9 +362,15 @@ static void __init early_print_info(void)
>>               continue;
>>           /* fdt_get_mem_rsv returns length */
>>           e += s;
>> -        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
>> +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n",
>>                        i, s, e);
> NIT: I see no reason, why this printk is split into two lines, as nicely fits
> into one line.

Not mentioning the wrong indentation in pretty much all this function 
;). I would prefer if we take care of the indentation issues in a patch 
before this one.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions

[Julien Grall]

Hi,

On 8/13/19 3:34 PM, Volodymyr Babchuk wrote:
> 
> Stefano Stabellini writes:
> 
>> Don't allow reserved-memory regions to be remapped into any unprivileged
>> guests, until reserved-memory regions are properly supported in Xen. For
>> now, do not call iomem_permit_access on them, because giving
>> iomem_permit_access to dom0 means that the toolstack will be able to
>> assign the region to a domU.
>>
>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>> ---
>>
>> Changes in v5:
>> - fix check condition
>> - use strnicmp
>> - return error
>> - improve commit message
>>
>> Changes in v4:
>> - compare the parent name with reserved-memory
>> - use dt_node_cmp
>>
>> Changes in v3:
>> - new patch
>> ---
>>   xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
>>   1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 4c8404155a..e0c0c01c88 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
>>       bool need_mapping = !dt_device_for_passthrough(dev);
>>       int res;
>>   
>> -    res = iomem_permit_access(d, paddr_to_pfn(addr),
>> -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
>> -    if ( res )
>> +    /*
>> +     * Don't give iomem permissions for reserved-memory ranges to domUs
>> +     * until reserved-memory support is complete.
>> +     */
>> +    if ( strnicmp(dt_node_full_name(dev), "/reserved-memory",
>> +         strlen("/reserved-memory")) != 0 )
> Why are you using strnicmp there? With such usage it is the same as
> strcasecmp().

Definitely not, strcasecmp() will compare that the two strings exactly 
match (ignoring the case of the characters). Here we only want to check 
the first part of the patch, so we need to use the length-limited version.

> But, if you want to find "/reserved-memory" anywhere in
> dt_node_full_name(dev), then you probably want to use strcasestr()

For a first strcasestr() is not provided by the string lib in Xen. So 
you would need to implement it.

But then all the reserved-memory regions are under the node 
/reserved-memory. A path /a/b/reserved-memory is not a valid memory region.

On a side note, the check is still incorrect here because you would 
allow /reserved-memory@... or /reserved-memory-test to pass.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> Hi,
>
> On 8/13/19 3:23 PM, Volodymyr Babchuk wrote:
>>
>> Stefano Stabellini writes:
>>
>>> As we parse the device tree in Xen, keep track of the reserved-memory
>>> regions as they need special treatment (follow-up patches will make use
>>> of the stored information.)
>>>
>>> Reuse process_memory_node to add reserved-memory regions to the
>>> bootinfo.reserved_mem array.
>>>
>>> Refuse to continue once we reach the max number of reserved memory
>>> regions to avoid accidentally mapping any portions of them into a VM.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>>
>>> ---
>>> Changes in v5:
>>> - remove unneeded cast
>>> - remove unneeded strlen check
>>> - don't pass address_cells, size_cells, depth to device_tree_for_each_node
>>>
>>> Changes in v4:
>>> - depth + 1 in process_reserved_memory_node
>>> - pass address_cells and size_cells to device_tree_for_each_node
>>> - pass struct meminfo * instead of a boolean to process_memory_node
>>> - improve in-code comment
>>> - use a separate process_reserved_memory_node (separate from
>>>    process_memory_node) function wrapper to have different error handling
>>>
>>> Changes in v3:
>>> - match only /reserved-memory
>>> - put the warning back in place for reg not present on a normal memory
>>>    region
>>> - refuse to continue once we reach the max number of reserved memory
>>>    regions
>>>
>>> Changes in v2:
>>> - call process_memory_node from process_reserved_memory_node to avoid
>>>    duplication
>>> ---
>>>   xen/arch/arm/bootfdt.c      | 41 +++++++++++++++++++++++++++++++------
>>>   xen/include/asm-arm/setup.h |  1 +
>>>   2 files changed, 36 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index 590b14304c..0b0e22a3d0 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -136,6 +136,7 @@ static int __init process_memory_node(const void *fdt, int node,
>>>       const __be32 *cell;
>>>       paddr_t start, size;
>>>       u32 reg_cells = address_cells + size_cells;
>>> +    struct meminfo *mem = data;
>>>
>>>       if ( address_cells < 1 || size_cells < 1 )
>>>           return -ENOENT;
>>> @@ -147,21 +148,46 @@ static int __init process_memory_node(const void *fdt, int node,
>>>       cell = (const __be32 *)prop->data;
>>>       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>>>
>>> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>>> +    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
>> What is logic behind the second part of the loop condition?
>>
>> You know that if (banks > NR_MEM_BANKS) then you will exit with error. Do
>> you really need to iterate over loop in this case?
>
> Well, the error is ignored in the case of memory banks. So iterating
> over the first banks allows you to fill up bootinfo with as much as
> possible as RAM. The rest of the RAM will not be used by Xen.
Fair enough.

>>
>>>       {
>>>           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>>           if ( !size )
>>>               continue;
>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>> -        bootinfo.mem.nr_banks++;
>>> +        mem->bank[mem->nr_banks].start = start;
>>> +        mem->bank[mem->nr_banks].size = size;
>>> +        mem->nr_banks++;
>>>       }
>>>
>>> -    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>> +    if ( mem->nr_banks == NR_MEM_BANKS )
>> Looks like you have the same off-by-one error, as in previous patch.
>> I can see that it was there earlier. But it is good time to fix it.
>
> I don't think there was an off-by-one error before this series. So
> what do you mean?
I explained this in patch #2. Imagine that NR_MEM_BANKS = 1 and you have
one memory node in the dtb. You'll fill the first element of the array
and mem->nr_banks will become 1. This is absolutely normal. But check
above will fail, which is not right.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Julien Grall]

Hi,

On 8/13/19 4:14 PM, Volodymyr Babchuk wrote:
> Julien Grall writes:
>> On 8/13/19 3:23 PM, Volodymyr Babchuk wrote:
>>> Stefano Stabellini writes:
>>>
>>>>        {
>>>>            device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>>>            if ( !size )
>>>>                continue;
>>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>> -        bootinfo.mem.nr_banks++;
>>>> +        mem->bank[mem->nr_banks].start = start;
>>>> +        mem->bank[mem->nr_banks].size = size;
>>>> +        mem->nr_banks++;
>>>>        }
>>>>
>>>> -    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>> +    if ( mem->nr_banks == NR_MEM_BANKS )
>>> Looks like you have the same off-by-one error, as in previous patch.
>>> I can see that it was there earlier. But it is good time to fix it.
>>
>> I don't think there was an off-by-one error before this series. So
>> what do you mean?
> I explained this in patch #2. Imagine that NR_MEM_BANKS = 1 and you have
> one memory node in the dtb. You'll fill the first element of the array
> and mem->nr_banks will become 1. This is absolutely normal. But check
> above will fail, which is not right.

Ok. So the off-by-one error has been introduced by this series. So this 
should be fixed in patch #2 not here.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Volodymyr Babchuk]

Julien Grall writes:

> Hi,
>
> On 8/13/19 4:14 PM, Volodymyr Babchuk wrote:
>> Julien Grall writes:
>>> On 8/13/19 3:23 PM, Volodymyr Babchuk wrote:
>>>> Stefano Stabellini writes:
>>>>
>>>>>        {
>>>>>            device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>>>>>            if ( !size )
>>>>>                continue;
>>>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>>>> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>> -        bootinfo.mem.nr_banks++;
>>>>> +        mem->bank[mem->nr_banks].start = start;
>>>>> +        mem->bank[mem->nr_banks].size = size;
>>>>> +        mem->nr_banks++;
>>>>>        }
>>>>>
>>>>> -    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>> +    if ( mem->nr_banks == NR_MEM_BANKS )
>>>> Looks like you have the same off-by-one error, as in previous patch.
>>>> I can see that it was there earlier. But it is good time to fix it.
>>>
>>> I don't think there was an off-by-one error before this series. So
>>> what do you mean?
>> I explained this in patch #2. Imagine that NR_MEM_BANKS = 1 and you have
>> one memory node in the dtb. You'll fill the first element of the array
>> and mem->nr_banks will become 1. This is absolutely normal. But check
>> above will fail, which is not right.
>
> Ok. So the off-by-one error has been introduced by this series. So
> this should be fixed in patch #2 not here.
Yes, sorry. I got lost in the code.

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 1/7] xen/arm: pass node to device_tree_for_each_node

[Julien Grall]

Hi,

On 8/12/19 11:28 PM, Stefano Stabellini wrote:
> Add a new parameter to device_tree_for_each_node: node, the node to
> start the search from. Passing 0 triggers the old behavior.
> 
> Set min_depth to depth of the current node + 1 and replace the for
> loop with a do/while loop to avoid scanning siblings of the initial node
> passed as an argument.
> 
> We need this change because in follow-up patches we want to be able to
> use reuse device_tree_for_each_node to call a function for each children
> nodes of a provided node and the node itself.

I have to say this would be fairly confusing for reserved-memory because 
you are only expecting to parse the subnode.

Furthermore, in the unlikely event to first node does have a property 
"regs", then #address-cells and #size-cells is going to be incorrect (we 
don't look up for its parent...).

So I think it would be best to consider to ignore the first node. This 
should not be an issue as none of the user care about the root node (i.e 
/). It would also makes the interface more straightforward.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall]

Hi,

On 8/12/19 11:28 PM, Stefano Stabellini wrote:
> Change the signature of process_memory_node to match
> device_tree_node_func. Thanks to this change, the next patch will be
> able to use device_tree_for_each_node to call process_memory_node on all
> the children of a provided node.
> 
> Return error if there is no reg property or if nr_banks is reached. Let
> the caller deal with the error.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v5:
> - return -ENOENT if address_cells or size_cells are not properly set
> 
> Changes in v4:
> - return error if there is no reg propery, remove printk
> - return error if nr_banks is reached
> 
> Changes in v3:
> - improve commit message
> - check return value of process_memory_node
> 
> Changes in v2:
> - new
> ---
>   xen/arch/arm/bootfdt.c | 29 +++++++++++++++--------------
>   1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index a872ea57d6..590b14304c 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt, int node,
>       return 0;
>   }
>   
> -static void __init process_memory_node(const void *fdt, int node,
> -                                       const char *name,
> -                                       u32 address_cells, u32 size_cells)
> +static int __init process_memory_node(const void *fdt, int node,
> +                                      const char *name, int depth,
> +                                      u32 address_cells, u32 size_cells,
> +                                      void *data)
>   {
>       const struct fdt_property *prop;
>       int i;
> @@ -137,18 +138,11 @@ static void __init process_memory_node(const void *fdt, int node,
>       u32 reg_cells = address_cells + size_cells;
>   
>       if ( address_cells < 1 || size_cells < 1 )
> -    {
> -        printk("fdt: node `%s': invalid #address-cells or #size-cells",
> -               name);
> -        return;
> -    }
> +        return -ENOENT;

I saw your answer on the previous version and didn't get the chance. 
Missing the "regs" property and wrong {address,size}-cells values are 
two different things.

In the former case, it just means the reserved-region will be allocated 
dynamically.

In the latter case, this is an error in the device-tree configuration. 
If you ignore it, you are at risk to not take into account a 
reserved-region.

I agree this is a bug in the Device-Tree, but doing basic sanity check 
for the users is always helpful.

With that in mind, I would keep the printk and return a different error 
here.


>   
>       prop = fdt_get_property(fdt, node, "reg", NULL);
>       if ( !prop )
> -    {
> -        printk("fdt: node `%s': missing `reg' property\n", name);
> -        return;
> -    }
> +        return -ENOENT;
>   
>       cell = (const __be32 *)prop->data;
>       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
>           bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>           bootinfo.mem.nr_banks++;
>       }
> +
> +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> +        return -ENOSPC;
> +    return 0;
>   }
>   
>   static void __init process_multiboot_node(const void *fdt, int node,
> @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt,
>                                     u32 address_cells, u32 size_cells,
>                                     void *data)
>   {
> +    int rc = 0;
> +
>       if ( device_tree_node_matches(fdt, node, "memory") )
> -        process_memory_node(fdt, node, name, address_cells, size_cells);
> +        rc = process_memory_node(fdt, node, name, depth,
> +                                 address_cells, size_cells, NULL);

As a small NIT there are no way for the user to know that the parsing 
failed because of the reg property were missing. Could you print an 
error message either here or maybe in device_tree_for_each() to say 
parsing as failed for node N?

>       else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
>                 device_tree_node_compatible(fdt, node, "multiboot,module" )))
>           process_multiboot_node(fdt, node, name, address_cells, size_cells);
>       else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
>           process_chosen_node(fdt, node, name, address_cells, size_cells);
>   
> -    return 0;
> +    return rc;
>   }
>   
>   static void __init early_print_info(void)
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 3/7] xen/arm: keep track of reserved-memory regions

[Julien Grall]

Hi Stefano,

On 12/08/2019 23:28, Stefano Stabellini wrote:
> As we parse the device tree in Xen, keep track of the reserved-memory
> regions as they need special treatment (follow-up patches will make use
> of the stored information.)
> 
> Reuse process_memory_node to add reserved-memory regions to the
> bootinfo.reserved_mem array.
> 
> Refuse to continue once we reach the max number of reserved memory
> regions to avoid accidentally mapping any portions of them into a VM.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> ---
> Changes in v5:
> - remove unneeded cast
> - remove unneeded strlen check
> - don't pass address_cells, size_cells, depth to device_tree_for_each_node
> 
> Changes in v4:
> - depth + 1 in process_reserved_memory_node
> - pass address_cells and size_cells to device_tree_for_each_node
> - pass struct meminfo * instead of a boolean to process_memory_node
> - improve in-code comment
> - use a separate process_reserved_memory_node (separate from
>    process_memory_node) function wrapper to have different error handling
> 
> Changes in v3:
> - match only /reserved-memory
> - put the warning back in place for reg not present on a normal memory
>    region
> - refuse to continue once we reach the max number of reserved memory
>    regions
> 
> Changes in v2:
> - call process_memory_node from process_reserved_memory_node to avoid
>    duplication
> ---
>   xen/arch/arm/bootfdt.c      | 41 +++++++++++++++++++++++++++++++------
>   xen/include/asm-arm/setup.h |  1 +
>   2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 590b14304c..0b0e22a3d0 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -136,6 +136,7 @@ static int __init process_memory_node(const void *fdt, int node,
>       const __be32 *cell;
>       paddr_t start, size;
>       u32 reg_cells = address_cells + size_cells;
> +    struct meminfo *mem = data;
>   
>       if ( address_cells < 1 || size_cells < 1 )
>           return -ENOENT;
> @@ -147,21 +148,46 @@ static int __init process_memory_node(const void *fdt, int node,
>       cell = (const __be32 *)prop->data;
>       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
>   
> -    for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
> +    for ( i = 0; i < banks && mem->nr_banks < NR_MEM_BANKS; i++ )
>       {
>           device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>           if ( !size )
>               continue;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
> -        bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> -        bootinfo.mem.nr_banks++;
> +        mem->bank[mem->nr_banks].start = start;
> +        mem->bank[mem->nr_banks].size = size;
> +        mem->nr_banks++;
>       }
>   
> -    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> +    if ( mem->nr_banks == NR_MEM_BANKS )

Assuming the off-by-one is going to be fixed in patch #2:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 4/7] xen/arm: early_print_info print reserved_mem

[Julien Grall]

Hi Stefano,

On 12/08/2019 23:28, Stefano Stabellini wrote:
> Improve early_print_info to also print the banks saved in
> bootinfo.reserved_mem. Print them right after RESVD, increasing the same
> index.
> 
> Since we are at it, also switch the existing RESVD print to use unsigned
> int.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v5:
> - switch to unsigned
> 
> Changes in v4:
> - new patch
> ---
>   xen/arch/arm/bootfdt.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 0b0e22a3d0..32153e6207 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -337,9 +337,10 @@ static int __init early_scan_node(const void *fdt,
>   static void __init early_print_info(void)
>   {
>       struct meminfo *mi = &bootinfo.mem;
> +    struct meminfo *mem_resv = &bootinfo.reserved_mem;
>       struct bootmodules *mods = &bootinfo.modules;
>       struct bootcmdlines *cmds = &bootinfo.cmdlines;
> -    int i, nr_rsvd;
> +    unsigned int i, j, nr_rsvd;
>   
>       for ( i = 0; i < mi->nr_banks; i++ )
>           printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> @@ -361,9 +362,15 @@ static void __init early_print_info(void)
>               continue;
>           /* fdt_get_mem_rsv returns length */
>           e += s;
> -        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
> +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n",
>                        i, s, e);

Can you add a patch before to fix the indentation within this function?

>       }
> +    for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
> +    {
> +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
> +                     mem_resv->bank[j].start,
> +                     mem_resv->bank[j].start + mem_resv->bank[j].size - 1);

Even if the most of the function is not correctly indented, new code should at 
least be.

Assuming the two are taken into account:

Acked-by: Julien Grall <julien.grall@arm.com>

> +    }
>       printk("\n");
>       for ( i = 0 ; i < cmds->nr_mods; i++ )
>           printk("CMDLINE[%"PRIpaddr"]:%s %s\n", cmds->cmdline[i].start,
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 1/7] xen/arm: pass node to device_tree_for_each_node

[Stefano Stabellini]

On Tue, 13 Aug 2019, Julien Grall wrote:
> Hi,
> 
> On 8/12/19 11:28 PM, Stefano Stabellini wrote:
> > Add a new parameter to device_tree_for_each_node: node, the node to
> > start the search from. Passing 0 triggers the old behavior.
> > 
> > Set min_depth to depth of the current node + 1 and replace the for
> > loop with a do/while loop to avoid scanning siblings of the initial node
> > passed as an argument.
> > 
> > We need this change because in follow-up patches we want to be able to
> > use reuse device_tree_for_each_node to call a function for each children
> > nodes of a provided node and the node itself.
> 
> I have to say this would be fairly confusing for reserved-memory because you
> are only expecting to parse the subnode.
> 
> Furthermore, in the unlikely event to first node does have a property "regs",
> then #address-cells and #size-cells is going to be incorrect (we don't look up
> for its parent...).
> 
> So I think it would be best to consider to ignore the first node. This should
> not be an issue as none of the user care about the root node (i.e /). It would
> also makes the interface more straightforward.

Yes, I can do that. It is a good idea.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 1/7] xen/arm: pass node to device_tree_for_each_node

[Stefano Stabellini]

On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
> > Add a new parameter to device_tree_for_each_node: node, the node to
> > start the search from. Passing 0 triggers the old behavior.
> >
> > Set min_depth to depth of the current node + 1 and replace the for
> > loop with a do/while loop to avoid scanning siblings of the initial node
> > passed as an argument.
> >
> > We need this change because in follow-up patches we want to be able to
> > use reuse device_tree_for_each_node to call a function for each children
> > nodes of a provided node and the node itself.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> 
> You can have my
> 
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>
> 
> providing that you'll fix formatting issue below.

Thank you! I'll fix the formatting issue, but won't add reviewed-by for
now as I'll change the function a bit to skip calling func() on the
first node.


> > ---
> > Changes in v5:
> > - go back to v3
> > - code style improvement in acpi/boot.c
> > - improve comments and commit message
> > - increase min_depth to avoid parsing siblings
> > - replace for with do/while loop and increase min_depth to avoid
> >   scanning siblings of the initial node
> > - pass only node, calculate depth
> >
> > Changes in v3:
> > - improve commit message
> > - improve in-code comments
> > - improve code style
> >
> > Changes in v2:
> > - new
> > ---
> >  xen/arch/arm/acpi/boot.c      |  8 +++++---
> >  xen/arch/arm/bootfdt.c        | 19 ++++++++++---------
> >  xen/include/xen/device_tree.h |  6 +++---
> >  3 files changed, 18 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > index 9b29769a10..d4957cca06 100644
> > --- a/xen/arch/arm/acpi/boot.c
> > +++ b/xen/arch/arm/acpi/boot.c
> > @@ -246,9 +246,11 @@ int __init acpi_boot_table_init(void)
> >       * - the device tree is not empty (it has more than just a /chosen node)
> >       *   and ACPI has not been force enabled (acpi=force)
> >       */
> > -    if ( param_acpi_off || ( !param_acpi_force
> > -                             && device_tree_for_each_node(device_tree_flattened,
> > -                                                   dt_scan_depth1_nodes, NULL)))
> > +    if ( param_acpi_off)
> > +        goto disable;
> > +	if ( !param_acpi_force &&
> > +		 device_tree_for_each_node(device_tree_flattened, 0,
> > +			                       dt_scan_depth1_nodes, NULL) )
> There is 3 tabs, followed by spaces.
> 
> This file missed emacs magic at the end. I think, this is cause for this
> formatting issue.
> 
> >          goto disable;
> >  
> >      /*
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 891b4b66ff..a872ea57d6 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -77,6 +77,7 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
> >  /**
> >   * device_tree_for_each_node - iterate over all device tree nodes
> >   * @fdt: flat device tree.
> > + * @node: node to start the search from
> >   * @func: function to call for each node.
> >   * @data: data to pass to @func.
> >   *
> > @@ -85,20 +86,17 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
> >   * Returns 0 if all nodes were iterated over successfully.  If @func
> >   * returns a value different from 0, that value is returned immediately.
> >   */
> > -int __init device_tree_for_each_node(const void *fdt,
> > +int __init device_tree_for_each_node(const void *fdt, int node,
> >                                       device_tree_node_func func,
> >                                       void *data)
> >  {
> > -    int node;
> > -    int depth;
> > +    int depth = fdt_node_depth(fdt, node);
> > +    int min_depth = depth + 1;
> >      u32 address_cells[DEVICE_TREE_MAX_DEPTH];
> >      u32 size_cells[DEVICE_TREE_MAX_DEPTH];
> >      int ret;
> >  
> > -    for ( node = 0, depth = 0;
> > -          node >=0 && depth >= 0;
> > -          node = fdt_next_node(fdt, node, &depth) )
> > -    {
> > +    do {
> >          const char *name = fdt_get_name(fdt, node, NULL);
> >          u32 as, ss;
> >  
> > @@ -120,7 +118,10 @@ int __init device_tree_for_each_node(const void *fdt,
> >          ret = func(fdt, node, name, depth, as, ss, data);
> >          if ( ret != 0 )
> >              return ret;
> > -    }
> > +
> > +        node = fdt_next_node(fdt, node, &depth);
> > +    } while ( node >= 0 && depth >= min_depth );
> > +
> >      return 0;
> >  }
> >  
> > @@ -357,7 +358,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr)
> >  
> >      add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false);
> >  
> > -    device_tree_for_each_node((void *)fdt, early_scan_node, NULL);
> > +    device_tree_for_each_node((void *)fdt, 0, early_scan_node, NULL);
> >      early_print_info();
> >  
> >      return fdt_totalsize(fdt);
> > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> > index 83156297e2..9a7a8f2dab 100644
> > --- a/xen/include/xen/device_tree.h
> > +++ b/xen/include/xen/device_tree.h
> > @@ -158,9 +158,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
> >  
> >  extern const void *device_tree_flattened;
> >  
> > -int device_tree_for_each_node(const void *fdt,
> > -                                     device_tree_node_func func,
> > -                                     void *data);
> > +int device_tree_for_each_node(const void *fdt, int node,
> > +                              device_tree_node_func func,
> > +                              void *data);
> >  
> >  /**
> >   * dt_unflatten_host_device_tree - Unflatten the host device tree
> 
> 
> -- 
> Volodymyr Babchuk at EPAM

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 4/7] xen/arm: early_print_info print reserved_mem

[Stefano Stabellini]

On Tue, 13 Aug 2019, Julien Grall wrote:
> Hi,
> 
> On 8/13/19 3:28 PM, Volodymyr Babchuk wrote:
> > 
> > Stefano Stabellini writes:
> > 
> > > Improve early_print_info to also print the banks saved in
> > > bootinfo.reserved_mem. Print them right after RESVD, increasing the same
> > > index.
> > > 
> > > Since we are at it, also switch the existing RESVD print to use unsigned
> > > int.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

Thanks


> > But, please see NIT below.
> > 
> > > ---
> > > Changes in v5:
> > > - switch to unsigned
> > > 
> > > Changes in v4:
> > > - new patch
> > > ---
> > >   xen/arch/arm/bootfdt.c | 11 +++++++++--
> > >   1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > index 0b0e22a3d0..32153e6207 100644
> > > --- a/xen/arch/arm/bootfdt.c
> > > +++ b/xen/arch/arm/bootfdt.c
> > > @@ -337,9 +337,10 @@ static int __init early_scan_node(const void *fdt,
> > >   static void __init early_print_info(void)
> > >   {
> > >       struct meminfo *mi = &bootinfo.mem;
> > > +    struct meminfo *mem_resv = &bootinfo.reserved_mem;
> > >       struct bootmodules *mods = &bootinfo.modules;
> > >       struct bootcmdlines *cmds = &bootinfo.cmdlines;
> > > -    int i, nr_rsvd;
> > > +    unsigned int i, j, nr_rsvd;
> > >         for ( i = 0; i < mi->nr_banks; i++ )
> > >           printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> > > @@ -361,9 +362,15 @@ static void __init early_print_info(void)
> > >               continue;
> > >           /* fdt_get_mem_rsv returns length */
> > >           e += s;
> > > -        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
> > > +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n",
> > >                        i, s, e);
> > NIT: I see no reason, why this printk is split into two lines, as nicely
> > fits
> > into one line.
> 
> Not mentioning the wrong indentation in pretty much all this function ;). I
> would prefer if we take care of the indentation issues in a patch before this
> one.

I'll add a patch

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Stefano Stabellini]

On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
> > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
> >          bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> >          bootinfo.mem.nr_banks++;
> >      }
> > +
> > +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> > +        return -ENOSPC;
> Are you sure that this logic is correct?
> 
> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
> in device tree, this function will fail. But it should not. I think you
> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS

You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
tree the code would return an error while actually it is normal.

I think the right check would be:

  if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
      return -ENOSPC;

(That's because it is impossible to get to nr_banks > NR_MEM_BANKS.)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions

[Stefano Stabellini]

On Tue, 13 Aug 2019, Julien Grall wrote:
> On 8/13/19 3:34 PM, Volodymyr Babchuk wrote:
> > 
> > Stefano Stabellini writes:
> > 
> > > Don't allow reserved-memory regions to be remapped into any unprivileged
> > > guests, until reserved-memory regions are properly supported in Xen. For
> > > now, do not call iomem_permit_access on them, because giving
> > > iomem_permit_access to dom0 means that the toolstack will be able to
> > > assign the region to a domU.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > ---
> > > 
> > > Changes in v5:
> > > - fix check condition
> > > - use strnicmp
> > > - return error
> > > - improve commit message
> > > 
> > > Changes in v4:
> > > - compare the parent name with reserved-memory
> > > - use dt_node_cmp
> > > 
> > > Changes in v3:
> > > - new patch
> > > ---
> > >   xen/arch/arm/domain_build.c | 24 ++++++++++++++++--------
> > >   1 file changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > > index 4c8404155a..e0c0c01c88 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -1155,15 +1155,23 @@ static int __init map_range_to_domain(const struct
> > > dt_device_node *dev,
> > >       bool need_mapping = !dt_device_for_passthrough(dev);
> > >       int res;
> > >   -    res = iomem_permit_access(d, paddr_to_pfn(addr),
> > > -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > > -    if ( res )
> > > +    /*
> > > +     * Don't give iomem permissions for reserved-memory ranges to domUs
> > > +     * until reserved-memory support is complete.
> > > +     */
> > > +    if ( strnicmp(dt_node_full_name(dev), "/reserved-memory",
> > > +         strlen("/reserved-memory")) != 0 )
> > Why are you using strnicmp there? With such usage it is the same as
> > strcasecmp().
> 
> Definitely not, strcasecmp() will compare that the two strings exactly match
> (ignoring the case of the characters). Here we only want to check the first
> part of the patch, so we need to use the length-limited version.
> 
> > But, if you want to find "/reserved-memory" anywhere in
> > dt_node_full_name(dev), then you probably want to use strcasestr()
> 
> For a first strcasestr() is not provided by the string lib in Xen. So you
> would need to implement it.
> 
> But then all the reserved-memory regions are under the node /reserved-memory.
> A path /a/b/reserved-memory is not a valid memory region.

Yes, I think strnicmp is the right one but...


> On a side note, the check is still incorrect here because you would allow
> /reserved-memory@... or /reserved-memory-test to pass.

...but you are right that we should deal with things like
"/reserved-memory-test" properly. It looks like we should compare
against "/reserved-memory/", note the '/' at the end.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Stefano Stabellini]

On Tue, 13 Aug 2019, Julien Grall wrote:
> On 8/12/19 11:28 PM, Stefano Stabellini wrote:
> > Change the signature of process_memory_node to match
> > device_tree_node_func. Thanks to this change, the next patch will be
> > able to use device_tree_for_each_node to call process_memory_node on all
> > the children of a provided node.
> > 
> > Return error if there is no reg property or if nr_banks is reached. Let
> > the caller deal with the error.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v5:
> > - return -ENOENT if address_cells or size_cells are not properly set
> > 
> > Changes in v4:
> > - return error if there is no reg propery, remove printk
> > - return error if nr_banks is reached
> > 
> > Changes in v3:
> > - improve commit message
> > - check return value of process_memory_node
> > 
> > Changes in v2:
> > - new
> > ---
> >   xen/arch/arm/bootfdt.c | 29 +++++++++++++++--------------
> >   1 file changed, 15 insertions(+), 14 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index a872ea57d6..590b14304c 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -125,9 +125,10 @@ int __init device_tree_for_each_node(const void *fdt,
> > int node,
> >       return 0;
> >   }
> >   -static void __init process_memory_node(const void *fdt, int node,
> > -                                       const char *name,
> > -                                       u32 address_cells, u32 size_cells)
> > +static int __init process_memory_node(const void *fdt, int node,
> > +                                      const char *name, int depth,
> > +                                      u32 address_cells, u32 size_cells,
> > +                                      void *data)
> >   {
> >       const struct fdt_property *prop;
> >       int i;
> > @@ -137,18 +138,11 @@ static void __init process_memory_node(const void
> > *fdt, int node,
> >       u32 reg_cells = address_cells + size_cells;
> >         if ( address_cells < 1 || size_cells < 1 )
> > -    {
> > -        printk("fdt: node `%s': invalid #address-cells or #size-cells",
> > -               name);
> > -        return;
> > -    }
> > +        return -ENOENT;
> 
> I saw your answer on the previous version and didn't get the chance. Missing
> the "regs" property and wrong {address,size}-cells values are two different
> things.
> 
> In the former case, it just means the reserved-region will be allocated
> dynamically.
> 
> In the latter case, this is an error in the device-tree configuration. If you
> ignore it, you are at risk to not take into account a reserved-region.
> 
> I agree this is a bug in the Device-Tree, but doing basic sanity check for the
> users is always helpful.
> 
> With that in mind, I would keep the printk and return a different error here.

OK. I'll use -EINVAL for this case, and keep the printk.

 
> >         prop = fdt_get_property(fdt, node, "reg", NULL);
> >       if ( !prop )
> > -    {
> > -        printk("fdt: node `%s': missing `reg' property\n", name);
> > -        return;
> > -    }
> > +        return -ENOENT;
> >         cell = (const __be32 *)prop->data;
> >       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt,
> > int node,
> >           bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
> >           bootinfo.mem.nr_banks++;
> >       }
> > +
> > +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
> > +        return -ENOSPC;
> > +    return 0;
> >   }
> >     static void __init process_multiboot_node(const void *fdt, int node,
> > @@ -293,15 +291,18 @@ static int __init early_scan_node(const void *fdt,
> >                                     u32 address_cells, u32 size_cells,
> >                                     void *data)
> >   {
> > +    int rc = 0;
> > +
> >       if ( device_tree_node_matches(fdt, node, "memory") )
> > -        process_memory_node(fdt, node, name, address_cells, size_cells);
> > +        rc = process_memory_node(fdt, node, name, depth,
> > +                                 address_cells, size_cells, NULL);
> 
> As a small NIT there are no way for the user to know that the parsing failed
> because of the reg property were missing. Could you print an error message
> either here or maybe in device_tree_for_each() to say parsing as failed for
> node N?

I'll add a new printk at the end of early_scan_node


> >       else if ( depth <= 3 && (device_tree_node_compatible(fdt, node,
> > "xen,multiboot-module" ) ||
> >                 device_tree_node_compatible(fdt, node, "multiboot,module"
> > )))
> >           process_multiboot_node(fdt, node, name, address_cells,
> > size_cells);
> >       else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen")
> > )
> >           process_chosen_node(fdt, node, name, address_cells, size_cells);
> >   -    return 0;
> > +    return rc;
> >   }
> >     static void __init early_print_info(void)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall]

Hi Stefano,

On 14/08/2019 23:35, Stefano Stabellini wrote:
> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
>>>           bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>           bootinfo.mem.nr_banks++;
>>>       }
>>> +
>>> +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>> +        return -ENOSPC;
>> Are you sure that this logic is correct?
>>
>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>> in device tree, this function will fail. But it should not. I think you
>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
> 
> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
> tree the code would return an error while actually it is normal.
> 
> I think the right check would be:
> 
>    if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>        return -ENOSPC;

FWIW, this check looks good to me.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions

[Julien Grall]

Hi Stefano,

On 14/08/2019 23:40, Stefano Stabellini wrote:
> On Tue, 13 Aug 2019, Julien Grall wrote:
>> On 8/13/19 3:34 PM, Volodymyr Babchuk wrote:
>>> Stefano Stabellini writes:
>> On a side note, the check is still incorrect here because you would allow
>> /reserved-memory@... or /reserved-memory-test to pass.
> 
> ...but you are right that we should deal with things like
> "/reserved-memory-test" properly. It looks like we should compare
> against "/reserved-memory/", note the '/' at the end.

You are right, the trailing / should ensure you only match anything under the 
reserved-memory node.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Volodymyr Babchuk]

Hi Stefano,

Stefano Stabellini writes:

> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>> > @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
>> >          bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>> >          bootinfo.mem.nr_banks++;
>> >      }
>> > +
>> > +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>> > +        return -ENOSPC;
>> Are you sure that this logic is correct?
>>
>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>> in device tree, this function will fail. But it should not. I think you
>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>
> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
> tree the code would return an error while actually it is normal.
>
> I think the right check would be:
>
>   if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>       return -ENOSPC;

Actually, this does not cover all corner cases. Here is the resulting
code:

 150     for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
 151     {
 152         device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
 153         if ( !size )
 154             continue;
 155         bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
 156         bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
 157         bootinfo.mem.nr_banks++;
 158     }
 159
 160     if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
 161         return -ENOSPC;

Lines 153-154 cause the issue.

Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
tree with. Nodes have sizes 0 and 1024. Your code will work as
intended. At the end of loop we will have banks = 2, i = 2 and
bootinfo.mem.nr_banks = 1.

But if we switch order of memory nodes, so first one will be with size
1024 and second one with size 0, your code will return -ENOSPC, because
we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.

I think, right solution will be to scan all nodes to count nodes
with size > 0. And then - either return an error or do second loop to
fill bootinfo.mem.bank[].

>
> (That's because it is impossible to get to nr_banks > NR_MEM_BANKS.)
Yes, this is my mistake.

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall]

Hi Volodymyr,

On 15/08/2019 12:20, Volodymyr Babchuk wrote:
> 
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>>> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void *fdt, int node,
>>>>           bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>           bootinfo.mem.nr_banks++;
>>>>       }
>>>> +
>>>> +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>> +        return -ENOSPC;
>>> Are you sure that this logic is correct?
>>>
>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>>> in device tree, this function will fail. But it should not. I think you
>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>>
>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
>> tree the code would return an error while actually it is normal.
>>
>> I think the right check would be:
>>
>>    if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>        return -ENOSPC;
> 
> Actually, this does not cover all corner cases. Here is the resulting
> code:
> 
>   150     for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>   151     {
>   152         device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>   153         if ( !size )
>   154             continue;
>   155         bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>   156         bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>   157         bootinfo.mem.nr_banks++;
>   158     }
>   159
>   160     if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>   161         return -ENOSPC;
> 
> Lines 153-154 cause the issue.
> 
> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
> tree with. Nodes have sizes 0 and 1024. Your code will work as
> intended. At the end of loop we will have banks = 2, i = 2 and
> bootinfo.mem.nr_banks = 1.
> 
> But if we switch order of memory nodes, so first one will be with size
> 1024 and second one with size 0, your code will return -ENOSPC, because
> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
> 
> I think, right solution will be to scan all nodes to count nodes
> with size > 0. And then - either return an error or do second loop to
> fill bootinfo.mem.bank[].

To be honest, a memory with size 0 is an error in the DT provided. So I would 
not care too much if Xen is not working as intended.

If we want to fix this, then we should bail out as we do for missing 'regs' and 
invalid 'address-cells'/'size-cells'.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall]

On 15/08/2019 12:24, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 15/08/2019 12:20, Volodymyr Babchuk wrote:
>>
>> Hi Stefano,
>>
>> Stefano Stabellini writes:
>>
>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>>>> @@ -162,6 +156,10 @@ static void __init process_memory_node(const void 
>>>>> *fdt, int node,
>>>>>           bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>           bootinfo.mem.nr_banks++;
>>>>>       }
>>>>> +
>>>>> +    if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>> +        return -ENOSPC;
>>>> Are you sure that this logic is correct?
>>>>
>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>>>> in device tree, this function will fail. But it should not. I think you
>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>>>
>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
>>> tree the code would return an error while actually it is normal.
>>>
>>> I think the right check would be:
>>>
>>>    if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>        return -ENOSPC;
>>
>> Actually, this does not cover all corner cases. Here is the resulting
>> code:
>>
>>   150     for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>>   151     {
>>   152         device_tree_get_reg(&cell, address_cells, size_cells, &start, 
>> &size);
>>   153         if ( !size )
>>   154             continue;
>>   155         bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>   156         bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>   157         bootinfo.mem.nr_banks++;
>>   158     }
>>   159
>>   160     if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>   161         return -ENOSPC;
>>
>> Lines 153-154 cause the issue.
>>
>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
>> tree with. Nodes have sizes 0 and 1024. Your code will work as
>> intended. At the end of loop we will have banks = 2, i = 2 and
>> bootinfo.mem.nr_banks = 1.
>>
>> But if we switch order of memory nodes, so first one will be with size
>> 1024 and second one with size 0, your code will return -ENOSPC, because
>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
>>
>> I think, right solution will be to scan all nodes to count nodes
>> with size > 0. And then - either return an error or do second loop to
>> fill bootinfo.mem.bank[].
> 
> To be honest, a memory with size 0 is an error in the DT provided. So I would 
> not care too much if Xen is not working as intended.
> 
> If we want to fix this, then we should bail out as we do for missing 'regs' and 
> invalid 'address-cells'/'size-cells'.

I send this too early. I forgot to mention that I would not be happy with 
parsing the Device-Tree twice just for benefits of wrong DT. If a DT is wrong 
then we should treat as such and shout at the user.

Repairing any wrong inputs should be done on best efforts.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Volodymyr Babchuk]

Hi Julien,

Julien Grall writes:

> On 15/08/2019 12:24, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 15/08/2019 12:20, Volodymyr Babchuk wrote:
>>>
>>> Hi Stefano,
>>>
>>> Stefano Stabellini writes:
>>>
>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>>>>> @@ -162,6 +156,10 @@ static void __init
>>>>>> process_memory_node(const void *fdt, int node,
>>>>>>  bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>>  bootinfo.mem.nr_banks++;
>>>>>>  }
>>>>>> +
>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>> + return -ENOSPC;
>>>>> Are you sure that this logic is correct?
>>>>>
>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>>>>> in device tree, this function will fail. But it should not. I think you
>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>>>>
>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
>>>> tree the code would return an error while actually it is normal.
>>>>
>>>> I think the right check would be:
>>>>
>>>>  if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>  return -ENOSPC;
>>>
>>> Actually, this does not cover all corner cases. Here is the resulting
>>> code:
>>>
>>>  150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>>>  151 {
>>>  152 device_tree_get_reg(&cell, address_cells, size_cells,
>>> &start, &size);
>>>  153 if ( !size )
>>>  154 continue;
>>>  155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>>  156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>  157 bootinfo.mem.nr_banks++;
>>>  158 }
>>>  159
>>>  160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>  161 return -ENOSPC;
>>>
>>> Lines 153-154 cause the issue.
>>>
>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
>>> tree with. Nodes have sizes 0 and 1024. Your code will work as
>>> intended. At the end of loop we will have banks = 2, i = 2 and
>>> bootinfo.mem.nr_banks = 1.
>>>
>>> But if we switch order of memory nodes, so first one will be with size
>>> 1024 and second one with size 0, your code will return -ENOSPC, because
>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
>>>
>>> I think, right solution will be to scan all nodes to count nodes
>>> with size > 0. And then - either return an error or do second loop to
>>> fill bootinfo.mem.bank[].
>>
>> To be honest, a memory with size 0 is an error in the DT
>> provided. So I would not care too much if Xen is not working as
>> intended.
>>
>> If we want to fix this, then we should bail out as we do for missing
>> 'regs' and invalid 'address-cells'/'size-cells'.
>
> I send this too early. I forgot to mention that I would not be happy
> with parsing the Device-Tree twice just for benefits of wrong DT. If a
> DT is wrong then we should treat as such and shout at the user.
Fair enough. But then at line 154 we need to return an error, instead of
continuing the iterations. And in this case we can simplify the error
check to (banks > NR_MEM_BANKS).

-- 
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall]

Hi Volodymyr,

On 15/08/2019 13:14, Volodymyr Babchuk wrote:
> Julien Grall writes:
> 
>> On 15/08/2019 12:24, Julien Grall wrote:
>>> Hi Volodymyr,
>>>
>>> On 15/08/2019 12:20, Volodymyr Babchuk wrote:
>>>>
>>>> Hi Stefano,
>>>>
>>>> Stefano Stabellini writes:
>>>>
>>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>>>>>> @@ -162,6 +156,10 @@ static void __init
>>>>>>> process_memory_node(const void *fdt, int node,
>>>>>>>   bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>>>   bootinfo.mem.nr_banks++;
>>>>>>>   }
>>>>>>> +
>>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>>> + return -ENOSPC;
>>>>>> Are you sure that this logic is correct?
>>>>>>
>>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>>>>>> in device tree, this function will fail. But it should not. I think you
>>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>>>>>
>>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
>>>>> tree the code would return an error while actually it is normal.
>>>>>
>>>>> I think the right check would be:
>>>>>
>>>>>   if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>   return -ENOSPC;
>>>>
>>>> Actually, this does not cover all corner cases. Here is the resulting
>>>> code:
>>>>
>>>>   150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>>>>   151 {
>>>>   152 device_tree_get_reg(&cell, address_cells, size_cells,
>>>> &start, &size);
>>>>   153 if ( !size )
>>>>   154 continue;
>>>>   155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>>>   156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>   157 bootinfo.mem.nr_banks++;
>>>>   158 }
>>>>   159
>>>>   160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>   161 return -ENOSPC;
>>>>
>>>> Lines 153-154 cause the issue.
>>>>
>>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
>>>> tree with. Nodes have sizes 0 and 1024. Your code will work as
>>>> intended. At the end of loop we will have banks = 2, i = 2 and
>>>> bootinfo.mem.nr_banks = 1.
>>>>
>>>> But if we switch order of memory nodes, so first one will be with size
>>>> 1024 and second one with size 0, your code will return -ENOSPC, because
>>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
>>>>
>>>> I think, right solution will be to scan all nodes to count nodes
>>>> with size > 0. And then - either return an error or do second loop to
>>>> fill bootinfo.mem.bank[].
>>>
>>> To be honest, a memory with size 0 is an error in the DT
>>> provided. So I would not care too much if Xen is not working as
>>> intended.
>>>
>>> If we want to fix this, then we should bail out as we do for missing
>>> 'regs' and invalid 'address-cells'/'size-cells'.
>>
>> I send this too early. I forgot to mention that I would not be happy
>> with parsing the Device-Tree twice just for benefits of wrong DT. If a
>> DT is wrong then we should treat as such and shout at the user.
> Fair enough. But then at line 154 we need to return an error, instead of
> continuing the iterations. And in this case we can simplify the error
> check to (banks > NR_MEM_BANKS).

I am afraid this would not be correct. It is allowed to have multiple memory 
nodes in the device-tree. This function only deal with one node at the times.

In particular banks is the number of regions described in the node. With the 
check you suggest, you would only catch the case where a node contain more banks 
than supported. It does not tell you whether there are enough space left in 
mem.bank[...] to cater the regions described by the node.

So we need the check suggested by Stefano.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Volodymyr Babchuk]

Julien Grall writes:

> Hi Volodymyr,
>
> On 15/08/2019 13:14, Volodymyr Babchuk wrote:
>> Julien Grall writes:
>>
>>> On 15/08/2019 12:24, Julien Grall wrote:
>>>> Hi Volodymyr,
>>>>
>>>> On 15/08/2019 12:20, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>> Stefano Stabellini writes:
>>>>>
>>>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>>>>>>> @@ -162,6 +156,10 @@ static void __init
>>>>>>>> process_memory_node(const void *fdt, int node,
>>>>>>>>   bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>>>>   bootinfo.mem.nr_banks++;
>>>>>>>>   }
>>>>>>>> +
>>>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>>>> + return -ENOSPC;
>>>>>>> Are you sure that this logic is correct?
>>>>>>>
>>>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>>>>>>> in device tree, this function will fail. But it should not. I think you
>>>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>>>>>>
>>>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
>>>>>> tree the code would return an error while actually it is normal.
>>>>>>
>>>>>> I think the right check would be:
>>>>>>
>>>>>>   if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>>   return -ENOSPC;
>>>>>
>>>>> Actually, this does not cover all corner cases. Here is the resulting
>>>>> code:
>>>>>
>>>>>   150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>>>>>   151 {
>>>>>   152 device_tree_get_reg(&cell, address_cells, size_cells,
>>>>> &start, &size);
>>>>>   153 if ( !size )
>>>>>   154 continue;
>>>>>   155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>>>>   156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>   157 bootinfo.mem.nr_banks++;
>>>>>   158 }
>>>>>   159
>>>>>   160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>   161 return -ENOSPC;
>>>>>
>>>>> Lines 153-154 cause the issue.
>>>>>
>>>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
>>>>> tree with. Nodes have sizes 0 and 1024. Your code will work as
>>>>> intended. At the end of loop we will have banks = 2, i = 2 and
>>>>> bootinfo.mem.nr_banks = 1.
>>>>>
>>>>> But if we switch order of memory nodes, so first one will be with size
>>>>> 1024 and second one with size 0, your code will return -ENOSPC, because
>>>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
>>>>>
>>>>> I think, right solution will be to scan all nodes to count nodes
>>>>> with size > 0. And then - either return an error or do second loop to
>>>>> fill bootinfo.mem.bank[].
>>>>
>>>> To be honest, a memory with size 0 is an error in the DT
>>>> provided. So I would not care too much if Xen is not working as
>>>> intended.
>>>>
>>>> If we want to fix this, then we should bail out as we do for missing
>>>> 'regs' and invalid 'address-cells'/'size-cells'.
>>>
>>> I send this too early. I forgot to mention that I would not be happy
>>> with parsing the Device-Tree twice just for benefits of wrong DT. If a
>>> DT is wrong then we should treat as such and shout at the user.
>> Fair enough. But then at line 154 we need to return an error, instead of
>> continuing the iterations. And in this case we can simplify the error
>> check to (banks > NR_MEM_BANKS).
>
> I am afraid this would not be correct. It is allowed to have multiple
> memory nodes in the device-tree. This function only deal with one node
> at the times.
Okay, I see the point there.

> In particular banks is the number of regions described in the
> node. With the check you suggest, you would only catch the case where
> a node contain more banks than supported. It does not tell you whether
> there are enough space left in mem.bank[...] to cater the regions
> described by the node.
Yes, right. But, we can free space:

(banks + bootinfo.mem.nr_banks > NR_MEM_BANKS)

> So we need the check suggested by Stefano.
As I said earlier, it does not cover all corner cases. It will behave
differently, depending on ordering of entries in "reg" property (if we
allow zero-length regions). Yes, this is the user's problem, but I think
it is better to have consistent behavior even in case of user's fault.

But were saying, that it is error to have region with zero length. So,
instead of

 device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
 if ( !size )
     continue;

we need

 device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
 if ( !size )
     return -ENOENT;

In this case, check suggested by Stefano will work fine, but it will be
redundant, because we can either do early check for free space in the
array, or just write

 if ( i < banks )
     return -ENOSPC;

If we want array to be filled no mater what.


Anyways, I don't want to press on this anymore. I just wanted to share
my concerns.

--
Volodymyr Babchuk at EPAM
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v5 2/7] xen/arm: make process_memory_node a device_tree_node_func

[Julien Grall]

On 15/08/2019 14:51, Volodymyr Babchuk wrote:
> 
> Julien Grall writes:
> 
>> Hi Volodymyr,
>>
>> On 15/08/2019 13:14, Volodymyr Babchuk wrote:
>>> Julien Grall writes:
>>>
>>>> On 15/08/2019 12:24, Julien Grall wrote:
>>>>> Hi Volodymyr,
>>>>>
>>>>> On 15/08/2019 12:20, Volodymyr Babchuk wrote:
>>>>>>
>>>>>> Hi Stefano,
>>>>>>
>>>>>> Stefano Stabellini writes:
>>>>>>
>>>>>>> On Tue, 13 Aug 2019, Volodymyr Babchuk wrote:
>>>>>>>>> @@ -162,6 +156,10 @@ static void __init
>>>>>>>>> process_memory_node(const void *fdt, int node,
>>>>>>>>>    bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>>>>>    bootinfo.mem.nr_banks++;
>>>>>>>>>    }
>>>>>>>>> +
>>>>>>>>> + if ( bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>>>>> + return -ENOSPC;
>>>>>>>> Are you sure that this logic is correct?
>>>>>>>>
>>>>>>>> For example, if NR_MEM_BANKS is 1, and we have exactly one memory node
>>>>>>>> in device tree, this function will fail. But it should not. I think you
>>>>>>>> want this condition: bootinfo.mem.nr_banks > NR_MEM_BANKS
>>>>>>>
>>>>>>> You are right, if NR_MEM_BANKS is 1 and we have 1 memory node in device
>>>>>>> tree the code would return an error while actually it is normal.
>>>>>>>
>>>>>>> I think the right check would be:
>>>>>>>
>>>>>>>    if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>>>    return -ENOSPC;
>>>>>>
>>>>>> Actually, this does not cover all corner cases. Here is the resulting
>>>>>> code:
>>>>>>
>>>>>>    150 for ( i = 0; i < banks && bootinfo.mem.nr_banks < NR_MEM_BANKS; i++ )
>>>>>>    151 {
>>>>>>    152 device_tree_get_reg(&cell, address_cells, size_cells,
>>>>>> &start, &size);
>>>>>>    153 if ( !size )
>>>>>>    154 continue;
>>>>>>    155 bootinfo.mem.bank[bootinfo.mem.nr_banks].start = start;
>>>>>>    156 bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
>>>>>>    157 bootinfo.mem.nr_banks++;
>>>>>>    158 }
>>>>>>    159
>>>>>>    160 if ( i < banks && bootinfo.mem.nr_banks == NR_MEM_BANKS )
>>>>>>    161 return -ENOSPC;
>>>>>>
>>>>>> Lines 153-154 cause the issue.
>>>>>>
>>>>>> Imagine that NR_MEM_BANKS = 1 and we have two memory nodes in device
>>>>>> tree with. Nodes have sizes 0 and 1024. Your code will work as
>>>>>> intended. At the end of loop we will have banks = 2, i = 2 and
>>>>>> bootinfo.mem.nr_banks = 1.
>>>>>>
>>>>>> But if we switch order of memory nodes, so first one will be with size
>>>>>> 1024 and second one with size 0, your code will return -ENOSPC, because
>>>>>> we'll have banks = 2, i = 1, bootinfo.mem.nr_banks = 1.
>>>>>>
>>>>>> I think, right solution will be to scan all nodes to count nodes
>>>>>> with size > 0. And then - either return an error or do second loop to
>>>>>> fill bootinfo.mem.bank[].
>>>>>
>>>>> To be honest, a memory with size 0 is an error in the DT
>>>>> provided. So I would not care too much if Xen is not working as
>>>>> intended.
>>>>>
>>>>> If we want to fix this, then we should bail out as we do for missing
>>>>> 'regs' and invalid 'address-cells'/'size-cells'.
>>>>
>>>> I send this too early. I forgot to mention that I would not be happy
>>>> with parsing the Device-Tree twice just for benefits of wrong DT. If a
>>>> DT is wrong then we should treat as such and shout at the user.
>>> Fair enough. But then at line 154 we need to return an error, instead of
>>> continuing the iterations. And in this case we can simplify the error
>>> check to (banks > NR_MEM_BANKS).
>>
>> I am afraid this would not be correct. It is allowed to have multiple
>> memory nodes in the device-tree. This function only deal with one node
>> at the times.
> Okay, I see the point there.
> 
>> In particular banks is the number of regions described in the
>> node. With the check you suggest, you would only catch the case where
>> a node contain more banks than supported. It does not tell you whether
>> there are enough space left in mem.bank[...] to cater the regions
>> described by the node.
> Yes, right. But, we can free space:
> 
> (banks + bootinfo.mem.nr_banks > NR_MEM_BANKS)

I guess you mean before the loop? If so, this is possible but then you will 
ignore the full node rather than trying to add as much regions as possible.

To give an exagerated example, imagine a the DT has a single node with 
NR_MEM_BANKS + 1. You will end up to not add any banks, so Xen will see no 
memory. This is not very ideal.

> 
>> So we need the check suggested by Stefano.
> As I said earlier, it does not cover all corner cases. It will behave
> differently, depending on ordering of entries in "reg" property (if we
> allow zero-length regions). Yes, this is the user's problem, but I think
> it is better to have consistent behavior even in case of user's fault.

Where did I say it cover all corner cases? As I said "If a DT is wrong then we 
should treat as such and shout at the user."


> 
> But were saying, that it is error to have region with zero length. So,
> instead of
> 
>   device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>   if ( !size )
>       continue;
> 
> we need
> 
>   device_tree_get_reg(&cell, address_cells, size_cells, &start, &size);
>   if ( !size )
>       return -ENOENT; >
> In this case, check suggested by Stefano will work fine, but it will be
> redundant, because we can either do early check for free space in the
> array, or just write

See above for the early check.

>   if ( i < banks )
>       return -ENOSPC;

This is another option for Stefano check. I don't particularly care on the check 
as long as it is correct.

> 
> If we want array to be filled no mater what.
> 
> Anyways, I don't want to press on this anymore. I just wanted to share
> my concerns.

You are preaching the converted. However, I have already pointed multiple times 
that we need to fill the array as much as possible. This is not a user fault but 
a Xen limitation. So I am not sure why you are pushing for an early check.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel