Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0
@ 2019-08-06 21:49 Stefano Stabellini
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node Stefano Stabellini
                   ` (7 more replies)
  0 siblings, 8 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: julien.grall, sstabellini

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 45ce5b8749a220ad7c4ce5d5eba7c201a9418078:

  mm: Safe to clear PGC_allocated on xenheap pages without an extra reference (2019-08-06 12:19:55 +0100)

are available in the Git repository at:

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

for you to fetch changes up to b3b20c987fe531f706fe50dba5e749c3cb306b3f:

  xen/arm: add reserved-memory regions to the dom0 memory node (2019-08-06 14:36:14 -0700)

----------------------------------------------------------------
Stefano Stabellini (7):
      xen/arm: extend 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      |  2 +-
 xen/arch/arm/bootfdt.c        | 94 +++++++++++++++++++++++++++++++++----------
 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, 160 insertions(+), 42 deletions(-)

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-07 16:08   ` Julien Grall
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

Add new parameters to device_tree_for_each_node: node, depth,
address_cells, size_cells.
Node is the parent node to start the search from;
depth is the min depth of the search (the depth of the parent node);
address_cells and size_cells are the respective parameters at the parent
node. Passing 0, 0, 0, 0 triggers the old behavior.

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.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v4:
- add address_cells and size_cells parameters

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

Changes in v2:
- new
---
 xen/arch/arm/acpi/boot.c      |  2 +-
 xen/arch/arm/bootfdt.c        | 21 +++++++++++++++------
 xen/include/xen/device_tree.h |  6 ++++--
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 9b29769a10..d275f8c535 100644
--- a/xen/arch/arm/acpi/boot.c
+++ b/xen/arch/arm/acpi/boot.c
@@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void)
      */
     if ( param_acpi_off || ( !param_acpi_force
                              && device_tree_for_each_node(device_tree_flattened,
-                                                   dt_scan_depth1_nodes, NULL)))
+                                 0, 0, 0, 0, dt_scan_depth1_nodes, NULL)))
         goto disable;
 
     /*
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 891b4b66ff..dfce8c2bfe 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -77,6 +77,10 @@ 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: parent node to start the search from
+ * @depth: depth of the parent node
+ * @address_cells: address_cells at the parent node
+ * @size_cells: size_cells at the parent node
  * @func: function to call for each node.
  * @data: data to pass to @func.
  *
@@ -86,17 +90,22 @@ static u32 __init device_tree_get_u32(const void *fdt, int node,
  * returns a value different from 0, that value is returned immediately.
  */
 int __init device_tree_for_each_node(const void *fdt,
+                                     int node, int depth,
+                                     u32 address_cells_p, u32 size_cells_p,
                                      device_tree_node_func func,
                                      void *data)
 {
-    int node;
-    int depth;
     u32 address_cells[DEVICE_TREE_MAX_DEPTH];
     u32 size_cells[DEVICE_TREE_MAX_DEPTH];
-    int ret;
+    int ret, min_depth = depth;
+
+    if ( depth > 0 )
+    {
+        address_cells[depth - 1] = address_cells_p;
+        size_cells[depth - 1] = size_cells_p;
+    }
 
-    for ( node = 0, depth = 0;
-          node >=0 && depth >= 0;
+    for ( ; node >= 0 && depth >= min_depth;
           node = fdt_next_node(fdt, node, &depth) )
     {
         const char *name = fdt_get_name(fdt, node, NULL);
@@ -357,7 +366,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, 0, 0, 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..358c67c912 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -159,8 +159,10 @@ 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 node, int depth,
+                              u32 address_cells, u32 size_cells,
+                              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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-07 16:19   ` Julien Grall
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions Stefano Stabellini
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

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, remove printk.
Return error if nr_banks is reached.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
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 | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index dfce8c2bfe..c22d57cd72 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -133,9 +133,10 @@ int __init device_tree_for_each_node(const void *fdt,
     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;
@@ -148,15 +149,12 @@ static void __init process_memory_node(const void *fdt, int node,
     {
         printk("fdt: node `%s': invalid #address-cells or #size-cells",
                name);
-        return;
+        return 0;
     }
 
     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));
@@ -170,6 +168,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,
@@ -301,15 +303,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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node Stefano Stabellini
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-07 16:33   ` Julien Grall
  2019-08-07 16:46   ` Julien Grall
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem Stefano Stabellini
                   ` (4 subsequent siblings)
  7 siblings, 2 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

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 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      | 43 +++++++++++++++++++++++++++++++------
 xen/include/asm-arm/setup.h |  1 +
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index c22d57cd72..3e6fd63b16 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -144,6 +144,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 = (struct meminfo *)data;
 
     if ( address_cells < 1 || size_cells < 1 )
     {
@@ -159,21 +160,47 @@ 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, depth + 1,
+                                     address_cells, size_cells,
+                                     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)
@@ -307,7 +334,11 @@ 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") &&
+              strlen(name) == strlen("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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
                   ` (2 preceding siblings ...)
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-07 16:36   ` Julien Grall
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 5/7] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions Stefano Stabellini
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

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

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v4:
- new patch
---
 xen/arch/arm/bootfdt.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 3e6fd63b16..b8f2e53122 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -351,9 +351,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;
+    int i, j, nr_rsvd;
 
     for ( i = 0; i < mi->nr_banks; i++ )
         printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
@@ -378,6 +379,12 @@ static void __init early_print_info(void)
         printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
                      i, s, e);
     }
+    for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
+    {
+        printk(" RESVD[%d]: %"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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 5/7] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
                   ` (3 preceding siblings ...)
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
                   ` (4 preceding siblings ...)
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 5/7] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-08 19:19   ` Volodymyr Babchuk
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 7/7] xen/arm: add reserved-memory regions to the dom0 memory node Stefano Stabellini
  2019-08-08 19:11 ` [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Volodymyr Babchuk
  7 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

Don't allow reserved-memory regions to be remapped into any guests,
until reserved-memory regions are properly supported in Xen. For now,
do not call iomem_permit_access for them.

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

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..267e0549e2 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
     struct map_range_data *mr_data = data;
     struct domain *d = mr_data->d;
     bool need_mapping = !dt_device_for_passthrough(dev);
+    const struct dt_device_node *parent = dt_get_parent(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 until
+     * reserved-memory support is complete.
+     */
+    if ( dt_node_cmp(dt_node_name(parent), "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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [Xen-devel] [PATCH v4 7/7] xen/arm: add reserved-memory regions to the dom0 memory node
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
                   ` (5 preceding siblings ...)
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
@ 2019-08-06 21:49 ` Stefano Stabellini
  2019-08-07 18:29   ` Julien Grall
  2019-08-08 19:11 ` [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Volodymyr Babchuk
  7 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-06 21:49 UTC (permalink / raw)
  To: xen-devel; +Cc: Stefano Stabellini, julien.grall, sstabellini

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>
---
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 267e0549e2..d0a96d62b0 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node Stefano Stabellini
@ 2019-08-07 16:08   ` Julien Grall
  2019-08-07 16:16     ` Julien Grall
  2019-08-09 22:01     ` Stefano Stabellini
  0 siblings, 2 replies; 28+ messages in thread
From: Julien Grall @ 2019-08-07 16:08 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini

Hi Stefano,

On 06/08/2019 22:49, Stefano Stabellini wrote:
> Add new parameters to device_tree_for_each_node: node, depth,
> address_cells, size_cells.

address_cells (resp. size_cells) are named address_cells_p (resp. size_cells_p) 
in the code.

But I am not convinced you need them. Per the DT spec (v0.2 section 2.3.5), the 
parent should either contain the #address-cells and #size-cells or default 
values (resp. 2 and 1) will be used. It is clearly stated that values are not 
inherited from the ancestors.

So technically the implementation of device_tree_for_each_node() is incorrect. 
If you follow the spec here, then the address_cells_p and size_cells_p would 
become unnecessary.

> Node is the parent node to start the search from;
> depth is the min depth of the search (the depth of the parent node);
> address_cells and size_cells are the respective parameters at the parent
> node. Passing 0, 0, 0, 0 triggers the old behavior. >
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v4:
> - add address_cells and size_cells parameters
> 
> Changes in v3:
> - improve commit message
> - improve in-code comments
> - improve code style
> 
> Changes in v2:
> - new
> ---
>   xen/arch/arm/acpi/boot.c      |  2 +-
>   xen/arch/arm/bootfdt.c        | 21 +++++++++++++++------
>   xen/include/xen/device_tree.h |  6 ++++--
>   3 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> index 9b29769a10..d275f8c535 100644
> --- a/xen/arch/arm/acpi/boot.c
> +++ b/xen/arch/arm/acpi/boot.c
> @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void)
>        */
>       if ( param_acpi_off || ( !param_acpi_force
>                                && device_tree_for_each_node(device_tree_flattened,
> -                                                   dt_scan_depth1_nodes, NULL)))
> +                                 0, 0, 0, 0, dt_scan_depth1_nodes, NULL)))

NIT: Can we split the if?

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node
  2019-08-07 16:08   ` Julien Grall
@ 2019-08-07 16:16     ` Julien Grall
  2019-08-09 22:01     ` Stefano Stabellini
  1 sibling, 0 replies; 28+ messages in thread
From: Julien Grall @ 2019-08-07 16:16 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini



On 07/08/2019 17:08, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/08/2019 22:49, Stefano Stabellini wrote:
>> Add new parameters to device_tree_for_each_node: node, depth,
>> address_cells, size_cells.
> 
> address_cells (resp. size_cells) are named address_cells_p (resp. size_cells_p) 
> in the code.
> 
> But I am not convinced you need them. Per the DT spec (v0.2 section 2.3.5), the 
> parent should either contain the #address-cells and #size-cells or default 
> values (resp. 2 and 1) will be used. It is clearly stated that values are not 
> inherited from the ancestors.
> 
> So technically the implementation of device_tree_for_each_node() is incorrect. 
> If you follow the spec here, then the address_cells_p and size_cells_p would 
> become unnecessary.
> 
>> Node is the parent node to start the search from;
>> depth is the min depth of the search (the depth of the parent node);

Actually, on the previous version [1] you agreed that this should be the 
children depth node and not the parent node. Why hasn't it been changed?

>> address_cells and size_cells are the respective parameters at the parent
>> node. Passing 0, 0, 0, 0 triggers the old behavior.
>> 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.
>>
>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>> ---
>> Changes in v4:
>> - add address_cells and size_cells parameters
>>
>> Changes in v3:
>> - improve commit message
>> - improve in-code comments
>> - improve code style
>>
>> Changes in v2:
>> - new
>> ---
>>   xen/arch/arm/acpi/boot.c      |  2 +-
>>   xen/arch/arm/bootfdt.c        | 21 +++++++++++++++------
>>   xen/include/xen/device_tree.h |  6 ++++--
>>   3 files changed, 20 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
>> index 9b29769a10..d275f8c535 100644
>> --- a/xen/arch/arm/acpi/boot.c
>> +++ b/xen/arch/arm/acpi/boot.c
>> @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void)
>>        */
>>       if ( param_acpi_off || ( !param_acpi_force
>>                                && 
>> device_tree_for_each_node(device_tree_flattened,
>> -                                                   dt_scan_depth1_nodes, NULL)))
>> +                                 0, 0, 0, 0, dt_scan_depth1_nodes, NULL)))
> 
> NIT: Can we split the if?
> 
> Cheers,
> 

[1] <alpine.DEB.2.21.1908061206000.2451@sstabellini-ThinkPad-T480s>

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
@ 2019-08-07 16:19   ` Julien Grall
  2019-08-09 21:08     ` Stefano Stabellini
  0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2019-08-07 16:19 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini

Hi Stefano,

On 06/08/2019 22:49, 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, remove printk.
> Return error if nr_banks is reached.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> 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 | 25 +++++++++++++++----------
>   1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index dfce8c2bfe..c22d57cd72 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -133,9 +133,10 @@ int __init device_tree_for_each_node(const void *fdt,
>       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;
> @@ -148,15 +149,12 @@ static void __init process_memory_node(const void *fdt, int node,
>       {
>           printk("fdt: node `%s': invalid #address-cells or #size-cells",
>                  name);
> -        return;
> +        return 0;

Why does the lack of valid #address-cells and #size-cells is a success when...


>       }
>   
>       prop = fdt_get_property(fdt, node, "reg", NULL);
>       if ( !prop )
> -    {
> -        printk("fdt: node `%s': missing `reg' property\n", name);
> -        return;
> -    }
> +        return -ENOENT;

... this is an error?

>   
>       cell = (const __be32 *)prop->data;
>       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> @@ -170,6 +168,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,
> @@ -301,15 +303,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)
> 

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions Stefano Stabellini
@ 2019-08-07 16:33   ` Julien Grall
  2019-08-09 22:19     ` Stefano Stabellini
  2019-08-07 16:46   ` Julien Grall
  1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2019-08-07 16:33 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini

Hi Stefano,

On 06/08/2019 22:49, 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 v4:
> - depth + 1 in process_reserved_memory_node

Ah, you fixed it in this patch. But then, this does not match the documentation 
in patch #1.

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

I can't see any comment, is that an improvement? :)

> - 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      | 43 +++++++++++++++++++++++++++++++------
>   xen/include/asm-arm/setup.h |  1 +
>   2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index c22d57cd72..3e6fd63b16 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -144,6 +144,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 = (struct meminfo *)data;

The cast is unnecessary.

The rest of the code looks good. Pending the discussion about 
device_tree_for_each_node:

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

>   
>       if ( address_cells < 1 || size_cells < 1 )
>       {
> @@ -159,21 +160,47 @@ 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, depth + 1,
> +                                     address_cells, size_cells,
> +                                     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)
> @@ -307,7 +334,11 @@ 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") &&
> +              strlen(name) == strlen("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
> 

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem Stefano Stabellini
@ 2019-08-07 16:36   ` Julien Grall
  2019-08-09 20:29     ` Stefano Stabellini
  0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2019-08-07 16:36 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini

Hi Stefano,

On 06/08/2019 22:49, 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.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
> Changes in v4:
> - new patch
> ---
>   xen/arch/arm/bootfdt.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 3e6fd63b16..b8f2e53122 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -351,9 +351,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;
> +    int i, j, nr_rsvd;

May I ask you to switch them to unsigned int?

>   
>       for ( i = 0; i < mi->nr_banks; i++ )
>           printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> @@ -378,6 +379,12 @@ static void __init early_print_info(void)
>           printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
>                        i, s, e);
>       }
> +    for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
> +    {
> +        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n", i,

With that the request above: s/%d/%u/

> +                     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,
> 

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions Stefano Stabellini
  2019-08-07 16:33   ` Julien Grall
@ 2019-08-07 16:46   ` Julien Grall
  2019-08-09 20:37     ` Stefano Stabellini
  1 sibling, 1 reply; 28+ messages in thread
From: Julien Grall @ 2019-08-07 16:46 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini



On 06/08/2019 22:49, Stefano Stabellini wrote:
>   static void __init process_multiboot_node(const void *fdt, int node,
>                                             const char *name,
>                                             u32 address_cells, u32 size_cells)
> @@ -307,7 +334,11 @@ 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") &&
> +              strlen(name) == strlen("reserved-memory") )

Unless my stdlib knowledge is rusty, strcmp() will only return 0 if the two 
string exactly matches. This implies the two strings are exactly the same length.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 7/7] xen/arm: add reserved-memory regions to the dom0 memory node
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 7/7] xen/arm: add reserved-memory regions to the dom0 memory node Stefano Stabellini
@ 2019-08-07 18:29   ` Julien Grall
  0 siblings, 0 replies; 28+ messages in thread
From: Julien Grall @ 2019-08-07 18:29 UTC (permalink / raw)
  To: Stefano Stabellini, xen-devel; +Cc: Stefano Stabellini

Hi Stefano,

On 8/6/19 10:49 PM, Stefano Stabellini wrote:
> 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>

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0
  2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
                   ` (6 preceding siblings ...)
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 7/7] xen/arm: add reserved-memory regions to the dom0 memory node Stefano Stabellini
@ 2019-08-08 19:11 ` Volodymyr Babchuk
  2019-08-08 19:16   ` Stefano Stabellini
  7 siblings, 1 reply; 28+ messages in thread
From: Volodymyr Babchuk @ 2019-08-08 19:11 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, julien.grall


Hi Stefano,

Stefano Stabellini writes:

> Hi all,
>

Looks like you are not using add_maintainer.pl or your MAINTAINERS file
is out of date. In any case I'm not getting CC'ed and I'm missing your
e-mails.

So it would be cool if you'll CC me if you want me to review your
patches.

> This patch series introduces partial reserved-memory support for dom0
> only (no domU support for reserved-memory yet.)
>
>
> The following changes since commit 45ce5b8749a220ad7c4ce5d5eba7c201a9418078:
>
>   mm: Safe to clear PGC_allocated on xenheap pages without an extra reference (2019-08-06 12:19:55 +0100)
>
> are available in the Git repository at:
>
>   http://xenbits.xenproject.org/git-http/people/sstabellini/xen-unstable.git reserved-mem-2
>
> for you to fetch changes up to b3b20c987fe531f706fe50dba5e749c3cb306b3f:
>
>   xen/arm: add reserved-memory regions to the dom0 memory node (2019-08-06 14:36:14 -0700)
>
> ----------------------------------------------------------------
> Stefano Stabellini (7):
>       xen/arm: extend 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      |  2 +-
>  xen/arch/arm/bootfdt.c        | 94 +++++++++++++++++++++++++++++++++----------
>  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, 160 insertions(+), 42 deletions(-)
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel


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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0
  2019-08-08 19:11 ` [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Volodymyr Babchuk
@ 2019-08-08 19:16   ` Stefano Stabellini
  0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-08 19:16 UTC (permalink / raw)
  To: Volodymyr Babchuk; +Cc: xen-devel, julien.grall, Stefano Stabellini

On Thu, 8 Aug 2019, Volodymyr Babchuk wrote:
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
> > Hi all,
> >
> 
> Looks like you are not using add_maintainer.pl or your MAINTAINERS file
> is out of date. In any case I'm not getting CC'ed and I'm missing your
> e-mails.
> 
> So it would be cool if you'll CC me if you want me to review your
> patches.

Yes, sorry. I noticed the issue after sending this series. I'll make
sure to add you to the next ones.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions
  2019-08-06 21:49 ` [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
@ 2019-08-08 19:19   ` Volodymyr Babchuk
  2019-08-09 22:56     ` Stefano Stabellini
  0 siblings, 1 reply; 28+ messages in thread
From: Volodymyr Babchuk @ 2019-08-08 19:19 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, julien.grall, Stefano Stabellini


Hi Stefano,

Stefano Stabellini writes:

> Don't allow reserved-memory regions to be remapped into any guests,
> until reserved-memory regions are properly supported in Xen. For now,
> do not call iomem_permit_access for them.
>
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>
> 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..267e0549e2 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
>      struct map_range_data *mr_data = data;
>      struct domain *d = mr_data->d;
>      bool need_mapping = !dt_device_for_passthrough(dev);
> +    const struct dt_device_node *parent = dt_get_parent(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 until
> +     * reserved-memory support is complete.
> +     */
> +    if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 )
Am I missing something, or you are permitting access only if it from a
"reserved-memory" node? This contradicts with patch description.

>      {
> -        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 )
So, this region cold be mapped, but without the access?


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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem
  2019-08-07 16:36   ` Julien Grall
@ 2019-08-09 20:29     ` Stefano Stabellini
  0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-09 20:29 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini

On Wed, 7 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/08/2019 22:49, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v4:
> > - new patch
> > ---
> >   xen/arch/arm/bootfdt.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 3e6fd63b16..b8f2e53122 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -351,9 +351,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;
> > +    int i, j, nr_rsvd;
> 
> May I ask you to switch them to unsigned int?
> 
> >         for ( i = 0; i < mi->nr_banks; i++ )
> >           printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> > @@ -378,6 +379,12 @@ static void __init early_print_info(void)
> >           printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n",
> >                        i, s, e);
> >       }
> > +    for ( j = 0; j < mem_resv->nr_banks; j++, i++ )
> > +    {
> > +        printk(" RESVD[%d]: %"PRIpaddr" - %"PRIpaddr"\n", i,
> 
> With that the request above: s/%d/%u/

Good idea. I'll take the opportunity to also switch the RESVD[%d] to
RESVD[%u] just above


> > +                     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,
> > 


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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-07 16:46   ` Julien Grall
@ 2019-08-09 20:37     ` Stefano Stabellini
  0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-09 20:37 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini

On Wed, 7 Aug 2019, Julien Grall wrote:
> On 06/08/2019 22:49, Stefano Stabellini wrote:
> >   static void __init process_multiboot_node(const void *fdt, int node,
> >                                             const char *name,
> >                                             u32 address_cells, u32
> > size_cells)
> > @@ -307,7 +334,11 @@ 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") &&
> > +              strlen(name) == strlen("reserved-memory") )
> 
> Unless my stdlib knowledge is rusty, strcmp() will only return 0 if the two
> string exactly matches. This implies the two strings are exactly the same
> length.

Good memory :-)
You are right, I double-checked to confirm it. I'll remove the strlen
check.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func
  2019-08-07 16:19   ` Julien Grall
@ 2019-08-09 21:08     ` Stefano Stabellini
  0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-09 21:08 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Stefano Stabellini

On Wed, 7 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/08/2019 22:49, 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, remove printk.
> > Return error if nr_banks is reached.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > 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 | 25 +++++++++++++++----------
> >   1 file changed, 15 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index dfce8c2bfe..c22d57cd72 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -133,9 +133,10 @@ int __init device_tree_for_each_node(const void *fdt,
> >       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;
> > @@ -148,15 +149,12 @@ static void __init process_memory_node(const void
> > *fdt, int node,
> >       {
> >           printk("fdt: node `%s': invalid #address-cells or #size-cells",
> >                  name);
> > -        return;
> > +        return 0;
> 
> Why does the lack of valid #address-cells and #size-cells is a success when...
> 
> 
> >       }
> >         prop = fdt_get_property(fdt, node, "reg", NULL);
> >       if ( !prop )
> > -    {
> > -        printk("fdt: node `%s': missing `reg' property\n", name);
> > -        return;
> > -    }
> > +        return -ENOENT;
> 
> ... this is an error?

Yes, you have a good point. Both should be returning -ENOENT,
conceptually they are the same kind of issue. Also, I confirmed that it
works properly by returning -ENOENT in both cases.

I'll do that.


> >         cell = (const __be32 *)prop->data;
> >       banks = fdt32_to_cpu(prop->len) / (reg_cells * sizeof (u32));
> > @@ -170,6 +168,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,
> > @@ -301,15 +303,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)
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node
  2019-08-07 16:08   ` Julien Grall
  2019-08-07 16:16     ` Julien Grall
@ 2019-08-09 22:01     ` Stefano Stabellini
  1 sibling, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-09 22:01 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini

On Wed, 7 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/08/2019 22:49, Stefano Stabellini wrote:
> > Add new parameters to device_tree_for_each_node: node, depth,
> > address_cells, size_cells.
> 
> address_cells (resp. size_cells) are named address_cells_p (resp.
> size_cells_p) in the code.
> 
> But I am not convinced you need them. Per the DT spec (v0.2 section 2.3.5),
> the parent should either contain the #address-cells and #size-cells or default
> values (resp. 2 and 1) will be used. It is clearly stated that values are not
> inherited from the ancestors.

You are right, also on page 24 of the ePAPR.


> So technically the implementation of device_tree_for_each_node() is incorrect.
> If you follow the spec here, then the address_cells_p and size_cells_p would
> become unnecessary.

Indeed.


> > Node is the parent node to start the search from;
> > depth is the min depth of the search (the depth of the parent node);
> > address_cells and size_cells are the respective parameters at the parent
> > node. Passing 0, 0, 0, 0 triggers the old behavior. >
> > 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> > Changes in v4:
> > - add address_cells and size_cells parameters
> > 
> > Changes in v3:
> > - improve commit message
> > - improve in-code comments
> > - improve code style
> > 
> > Changes in v2:
> > - new
> > ---
> >   xen/arch/arm/acpi/boot.c      |  2 +-
> >   xen/arch/arm/bootfdt.c        | 21 +++++++++++++++------
> >   xen/include/xen/device_tree.h |  6 ++++--
> >   3 files changed, 20 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
> > index 9b29769a10..d275f8c535 100644
> > --- a/xen/arch/arm/acpi/boot.c
> > +++ b/xen/arch/arm/acpi/boot.c
> > @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void)
> >        */
> >       if ( param_acpi_off || ( !param_acpi_force
> >                                &&
> > device_tree_for_each_node(device_tree_flattened,
> > -                                                   dt_scan_depth1_nodes,
> > NULL)))
> > +                                 0, 0, 0, 0, dt_scan_depth1_nodes, NULL)))
> 
> NIT: Can we split the if?
 
Sure

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-07 16:33   ` Julien Grall
@ 2019-08-09 22:19     ` Stefano Stabellini
  2019-08-09 23:57       ` Julien Grall
  0 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-09 22:19 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Stefano Stabellini

On Wed, 7 Aug 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/08/2019 22:49, 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 v4:
> > - depth + 1 in process_reserved_memory_node
> 
> Ah, you fixed it in this patch. But then, this does not match the
> documentation in patch #1.

Yes good point, see below


> > - 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
> 
> I can't see any comment, is that an improvement? :)

It got lost with the refactoring of the code, but I don't think we need
it anymore


> > - 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      | 43 +++++++++++++++++++++++++++++++------
> >   xen/include/asm-arm/setup.h |  1 +
> >   2 files changed, 38 insertions(+), 6 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index c22d57cd72..3e6fd63b16 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -144,6 +144,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 = (struct meminfo *)data;
> 
> The cast is unnecessary.
> 
> The rest of the code looks good. Pending the discussion about
> device_tree_for_each_node:
> 
> Acked-by: Julien Grall <julien.grall@arm.com>

Thank you. I removed the cast. Also, I think that it makes more sense to
do the depth increase (depth + 1) inside the implementation of
device_tree_for_each_node instead of at the caller site, like it is done
in this patch. This would match the documentation better and is cleaner
from an interface point of view. So I'll remove the depth increase from
this patch and move it to the first patch (min_depth = depth + 1).

Given the change, I won't add the acked-by to give you the chance to
give it another look.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions
  2019-08-08 19:19   ` Volodymyr Babchuk
@ 2019-08-09 22:56     ` Stefano Stabellini
  2019-08-12 10:43       ` Julien Grall
  0 siblings, 1 reply; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-09 22:56 UTC (permalink / raw)
  To: Volodymyr Babchuk; +Cc: xen-devel, julien.grall, Stefano Stabellini

On Thu, 8 Aug 2019, Volodymyr Babchuk wrote:
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
> > Don't allow reserved-memory regions to be remapped into any guests,
> > until reserved-memory regions are properly supported in Xen. For now,
> > do not call iomem_permit_access for them.
> >
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >
> > 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..267e0549e2 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
> >      struct map_range_data *mr_data = data;
> >      struct domain *d = mr_data->d;
> >      bool need_mapping = !dt_device_for_passthrough(dev);
> > +    const struct dt_device_node *parent = dt_get_parent(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 until
> > +     * reserved-memory support is complete.
> > +     */
> > +    if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 )
> Am I missing something, or you are permitting access only if it from a
> "reserved-memory" node? This contradicts with patch description.

Well spotted! I inverted the condition by mistake.


> >      {
> > -        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 )
> So, this region cold be mapped, but without the access?

I'll change it to return early from the function for reserved-memory
regions.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-09 22:19     ` Stefano Stabellini
@ 2019-08-09 23:57       ` Julien Grall
  2019-08-12 18:10         ` Stefano Stabellini
  0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2019-08-09 23:57 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel, Julien Grall, Stefano Stabellini

[-- Attachment #1.1: Type: text/plain, Size: 3543 bytes --]

On Fri, 9 Aug 2019, 23:21 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Wed, 7 Aug 2019, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 06/08/2019 22:49, 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 v4:
> > > - depth + 1 in process_reserved_memory_node
> >
> > Ah, you fixed it in this patch. But then, this does not match the
> > documentation in patch #1.
>
> Yes good point, see below
>
>
> > > - 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
> >
> > I can't see any comment, is that an improvement? :)
>
> It got lost with the refactoring of the code, but I don't think we need
> it anymore
>
>
> > > - 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      | 43
> +++++++++++++++++++++++++++++++------
> > >   xen/include/asm-arm/setup.h |  1 +
> > >   2 files changed, 38 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > > index c22d57cd72..3e6fd63b16 100644
> > > --- a/xen/arch/arm/bootfdt.c
> > > +++ b/xen/arch/arm/bootfdt.c
> > > @@ -144,6 +144,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 = (struct meminfo *)data;
> >
> > The cast is unnecessary.
> >
> > The rest of the code looks good. Pending the discussion about
> > device_tree_for_each_node:
> >
> > Acked-by: Julien Grall <julien.grall@arm.com>
>
> Thank you. I removed the cast. Also, I think that it makes more sense to
> do the depth increase (depth + 1) inside the implementation of
> device_tree_for_each_node instead of at the caller site, like it is done
> in this patch. This would match the documentation better and is cleaner
> from an interface point of view. So I'll remove the depth increase from
> this patch and move it to the first patch (min_depth = depth + 1).
>

Well, you don't need to pass the depth at all. It is just an artificial
number for libfdt to know were to stop.

We also don't need the absolute depth in any of the early FDT. The relative
one is sufficient.


> Given the change, I won't add the acked-by to give you the chance to
> give it another look.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

[-- Attachment #1.2: Type: text/html, Size: 5113 bytes --]

<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 9 Aug 2019, 23:21 Stefano Stabellini, &lt;<a href="mailto:sstabellini@kernel.org">sstabellini@kernel.org</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, 7 Aug 2019, Julien Grall wrote:<br>
&gt; Hi Stefano,<br>
&gt; <br>
&gt; On 06/08/2019 22:49, Stefano Stabellini wrote:<br>
&gt; &gt; As we parse the device tree in Xen, keep track of the reserved-memory<br>
&gt; &gt; regions as they need special treatment (follow-up patches will make use<br>
&gt; &gt; of the stored information.)<br>
&gt; &gt; <br>
&gt; &gt; Reuse process_memory_node to add reserved-memory regions to the<br>
&gt; &gt; bootinfo.reserved_mem array.<br>
&gt; &gt; <br>
&gt; &gt; Refuse to continue once we reach the max number of reserved memory<br>
&gt; &gt; regions to avoid accidentally mapping any portions of them into a VM.<br>
&gt; &gt; <br>
&gt; &gt; Signed-off-by: Stefano Stabellini &lt;<a href="mailto:stefanos@xilinx.com" target="_blank" rel="noreferrer">stefanos@xilinx.com</a>&gt;<br>
&gt; &gt; <br>
&gt; &gt; ---<br>
&gt; &gt; Changes in v4:<br>
&gt; &gt; - depth + 1 in process_reserved_memory_node<br>
&gt; <br>
&gt; Ah, you fixed it in this patch. But then, this does not match the<br>
&gt; documentation in patch #1.<br>
<br>
Yes good point, see below<br>
<br>
<br>
&gt; &gt; - pass address_cells and size_cells to device_tree_for_each_node<br>
&gt; &gt; - pass struct meminfo * instead of a boolean to process_memory_node<br>
&gt; &gt; - improve in-code comment<br>
&gt; <br>
&gt; I can&#39;t see any comment, is that an improvement? :)<br>
<br>
It got lost with the refactoring of the code, but I don&#39;t think we need<br>
it anymore<br>
<br>
<br>
&gt; &gt; - use a separate process_reserved_memory_node (separate from<br>
&gt; &gt;    process_memory_node) function wrapper to have different error handling<br>
&gt; &gt; <br>
&gt; &gt; Changes in v3:<br>
&gt; &gt; - match only /reserved-memory<br>
&gt; &gt; - put the warning back in place for reg not present on a normal memory<br>
&gt; &gt;    region<br>
&gt; &gt; - refuse to continue once we reach the max number of reserved memory<br>
&gt; &gt;    regions<br>
&gt; &gt; <br>
&gt; &gt; Changes in v2:<br>
&gt; &gt; - call process_memory_node from process_reserved_memory_node to avoid<br>
&gt; &gt;    duplication<br>
&gt; &gt; ---<br>
&gt; &gt;   xen/arch/arm/bootfdt.c      | 43 +++++++++++++++++++++++++++++++------<br>
&gt; &gt;   xen/include/asm-arm/setup.h |  1 +<br>
&gt; &gt;   2 files changed, 38 insertions(+), 6 deletions(-)<br>
&gt; &gt; <br>
&gt; &gt; diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c<br>
&gt; &gt; index c22d57cd72..3e6fd63b16 100644<br>
&gt; &gt; --- a/xen/arch/arm/bootfdt.c<br>
&gt; &gt; +++ b/xen/arch/arm/bootfdt.c<br>
&gt; &gt; @@ -144,6 +144,7 @@ static int __init process_memory_node(const void *fdt,<br>
&gt; &gt; int node,<br>
&gt; &gt;       const __be32 *cell;<br>
&gt; &gt;       paddr_t start, size;<br>
&gt; &gt;       u32 reg_cells = address_cells + size_cells;<br>
&gt; &gt; +    struct meminfo *mem = (struct meminfo *)data;<br>
&gt; <br>
&gt; The cast is unnecessary.<br>
&gt; <br>
&gt; The rest of the code looks good. Pending the discussion about<br>
&gt; device_tree_for_each_node:<br>
&gt; <br>
&gt; Acked-by: Julien Grall &lt;<a href="mailto:julien.grall@arm.com" target="_blank" rel="noreferrer">julien.grall@arm.com</a>&gt;<br>
<br>
Thank you. I removed the cast. Also, I think that it makes more sense to<br>
do the depth increase (depth + 1) inside the implementation of<br>
device_tree_for_each_node instead of at the caller site, like it is done<br>
in this patch. This would match the documentation better and is cleaner<br>
from an interface point of view. So I&#39;ll remove the depth increase from<br>
this patch and move it to the first patch (min_depth = depth + 1).<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Well, you don&#39;t need to pass the depth at all. It is just an artificial number for libfdt to know were to stop.</div><div dir="auto"><br></div><div dir="auto">We also don&#39;t need the absolute depth in any of the early FDT. The relative one is sufficient.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Given the change, I won&#39;t add the acked-by to give you the chance to<br>
give it another look.<br>
<br>
_______________________________________________<br>
Xen-devel mailing list<br>
<a href="mailto:Xen-devel@lists.xenproject.org" target="_blank" rel="noreferrer">Xen-devel@lists.xenproject.org</a><br>
<a href="https://lists.xenproject.org/mailman/listinfo/xen-devel" rel="noreferrer noreferrer" target="_blank">https://lists.xenproject.org/mailman/listinfo/xen-devel</a></blockquote></div></div></div>

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions
  2019-08-09 22:56     ` Stefano Stabellini
@ 2019-08-12 10:43       ` Julien Grall
  2019-08-12 17:45         ` Stefano Stabellini
  0 siblings, 1 reply; 28+ messages in thread
From: Julien Grall @ 2019-08-12 10:43 UTC (permalink / raw)
  To: Stefano Stabellini, Volodymyr Babchuk; +Cc: xen-devel

Hi,

On 09/08/2019 23:56, Stefano Stabellini wrote:
> On Thu, 8 Aug 2019, Volodymyr Babchuk wrote:
>> Hi Stefano,
>>
>> Stefano Stabellini writes:
>>
>>> Don't allow reserved-memory regions to be remapped into any guests,
>>> until reserved-memory regions are properly supported in Xen. For now,
>>> do not call iomem_permit_access for them.
>>>
>>> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
>>> ---
>>>
>>> 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..267e0549e2 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
>>>       struct map_range_data *mr_data = data;
>>>       struct domain *d = mr_data->d;
>>>       bool need_mapping = !dt_device_for_passthrough(dev);
>>> +    const struct dt_device_node *parent = dt_get_parent(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 until
>>> +     * reserved-memory support is complete.
>>> +     */
>>> +    if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 )
>> Am I missing something, or you are permitting access only if it from a
>> "reserved-memory" node? This contradicts with patch description.
> 
> Well spotted! I inverted the condition by mistake.
> 
> 
>>>       {
>>> -        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 )
>> So, this region cold be mapped, but without the access?

IOMEM access and mapping are two different things. The former gives a domain 
control over managing the region (i.e mapping, unmapping, giving access to 
another domain). The latter will map the region in the P2M so the domain can 
read/write into it.

> 
> I'll change it to return early from the function for reserved-memory
> regions.

I am not sure to understand you suggestion here... You still need to have 
reserved-regions mapped into the hardware domain. The only thing we want to 
prevent is the domain to manage the region.

Cheers,

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions
  2019-08-12 10:43       ` Julien Grall
@ 2019-08-12 17:45         ` Stefano Stabellini
  0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-12 17:45 UTC (permalink / raw)
  To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk

On Mon, 12 Aug 2019, Julien Grall wrote:
> On 09/08/2019 23:56, Stefano Stabellini wrote:
> > On Thu, 8 Aug 2019, Volodymyr Babchuk wrote:
> > > Hi Stefano,
> > > 
> > > Stefano Stabellini writes:
> > > 
> > > > Don't allow reserved-memory regions to be remapped into any guests,
> > > > until reserved-memory regions are properly supported in Xen. For now,
> > > > do not call iomem_permit_access for them.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > ---
> > > > 
> > > > 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..267e0549e2 100644
> > > > --- a/xen/arch/arm/domain_build.c
> > > > +++ b/xen/arch/arm/domain_build.c
> > > > @@ -1153,17 +1153,25 @@ static int __init map_range_to_domain(const
> > > > struct dt_device_node *dev,
> > > >       struct map_range_data *mr_data = data;
> > > >       struct domain *d = mr_data->d;
> > > >       bool need_mapping = !dt_device_for_passthrough(dev);
> > > > +    const struct dt_device_node *parent = dt_get_parent(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 until
> > > > +     * reserved-memory support is complete.
> > > > +     */
> > > > +    if ( dt_node_cmp(dt_node_name(parent), "reserved-memory") == 0 )
> > > Am I missing something, or you are permitting access only if it from a
> > > "reserved-memory" node? This contradicts with patch description.
> > 
> > Well spotted! I inverted the condition by mistake.
> > 
> > 
> > > >       {
> > > > -        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 )
> > > So, this region cold be mapped, but without the access?
> 
> IOMEM access and mapping are two different things. The former gives a domain
> control over managing the region (i.e mapping, unmapping, giving access to
> another domain). The latter will map the region in the P2M so the domain can
> read/write into it.
> 
> > 
> > I'll change it to return early from the function for reserved-memory
> > regions.
> 
> I am not sure to understand you suggestion here... You still need to have
> reserved-regions mapped into the hardware domain. The only thing we want to
> prevent is the domain to manage the region.

I forgot that giving iomem permission to dom0 automatically means that
the toolstack can give iomem permission to a domU for the same region.

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions
  2019-08-09 23:57       ` Julien Grall
@ 2019-08-12 18:10         ` Stefano Stabellini
  0 siblings, 0 replies; 28+ messages in thread
From: Stefano Stabellini @ 2019-08-12 18:10 UTC (permalink / raw)
  To: Julien Grall
  Cc: xen-devel, Julien Grall, Stefano Stabellini, Stefano Stabellini

[-- Attachment #1: Type: text/plain, Size: 3824 bytes --]

On Sat, 10 Aug 2019, Julien Grall wrote:
> On Fri, 9 Aug 2019, 23:21 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Wed, 7 Aug 2019, Julien Grall wrote:
>       > Hi Stefano,
>       >
>       > On 06/08/2019 22:49, 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 v4:
>       > > - depth + 1 in process_reserved_memory_node
>       >
>       > Ah, you fixed it in this patch. But then, this does not match the
>       > documentation in patch #1.
> 
>       Yes good point, see below
> 
> 
>       > > - 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
>       >
>       > I can't see any comment, is that an improvement? :)
> 
>       It got lost with the refactoring of the code, but I don't think we need
>       it anymore
> 
> 
>       > > - 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      | 43 +++++++++++++++++++++++++++++++------
>       > >   xen/include/asm-arm/setup.h |  1 +
>       > >   2 files changed, 38 insertions(+), 6 deletions(-)
>       > >
>       > > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>       > > index c22d57cd72..3e6fd63b16 100644
>       > > --- a/xen/arch/arm/bootfdt.c
>       > > +++ b/xen/arch/arm/bootfdt.c
>       > > @@ -144,6 +144,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 = (struct meminfo *)data;
>       >
>       > The cast is unnecessary.
>       >
>       > The rest of the code looks good. Pending the discussion about
>       > device_tree_for_each_node:
>       >
>       > Acked-by: Julien Grall <julien.grall@arm.com>
> 
>       Thank you. I removed the cast. Also, I think that it makes more sense to
>       do the depth increase (depth + 1) inside the implementation of
>       device_tree_for_each_node instead of at the caller site, like it is done
>       in this patch. This would match the documentation better and is cleaner
>       from an interface point of view. So I'll remove the depth increase from
>       this patch and move it to the first patch (min_depth = depth + 1).
> 
> 
> Well, you don't need to pass the depth at all. It is just an artificial number for libfdt to know were to stop.
> 
> We also don't need the absolute depth in any of the early FDT. The relative one is sufficient.

Yes, you are right, good suggestion

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

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

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, back to index

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 21:49 [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node Stefano Stabellini
2019-08-07 16:08   ` Julien Grall
2019-08-07 16:16     ` Julien Grall
2019-08-09 22:01     ` Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 2/7] xen/arm: make process_memory_node a device_tree_node_func Stefano Stabellini
2019-08-07 16:19   ` Julien Grall
2019-08-09 21:08     ` Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 3/7] xen/arm: keep track of reserved-memory regions Stefano Stabellini
2019-08-07 16:33   ` Julien Grall
2019-08-09 22:19     ` Stefano Stabellini
2019-08-09 23:57       ` Julien Grall
2019-08-12 18:10         ` Stefano Stabellini
2019-08-07 16:46   ` Julien Grall
2019-08-09 20:37     ` Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 4/7] xen/arm: early_print_info print reserved_mem Stefano Stabellini
2019-08-07 16:36   ` Julien Grall
2019-08-09 20:29     ` Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 5/7] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 6/7] xen/arm: don't iomem_permit_access for reserved-memory regions Stefano Stabellini
2019-08-08 19:19   ` Volodymyr Babchuk
2019-08-09 22:56     ` Stefano Stabellini
2019-08-12 10:43       ` Julien Grall
2019-08-12 17:45         ` Stefano Stabellini
2019-08-06 21:49 ` [Xen-devel] [PATCH v4 7/7] xen/arm: add reserved-memory regions to the dom0 memory node Stefano Stabellini
2019-08-07 18:29   ` Julien Grall
2019-08-08 19:11 ` [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0 Volodymyr Babchuk
2019-08-08 19:16   ` Stefano Stabellini

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@archiver.kernel.org
	public-inbox-index xen-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox