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

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

[Stefano Stabellini]

Hi all,

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


The following changes since commit 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

[Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node

[Stefano Stabellini]

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

Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node

[Julien Grall]

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

Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node

[Julien Grall]

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

Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node

[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

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

[Stefano Stabellini]

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

Return error if there is no reg property, 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

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

[Julien Grall]

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

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

[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

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

[Stefano Stabellini]

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

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

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

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

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

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

[Julien Grall]

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

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

[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

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

[Julien Grall]

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

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

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

[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

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

[Julien Grall]

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

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

[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

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

[Stefano Stabellini]

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

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

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

[Julien Grall]

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

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

[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

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

[Stefano Stabellini]

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

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

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

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

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

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


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

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

[Stefano Stabellini]

Don't allow reserved-memory regions to be remapped into any 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

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

[Volodymyr Babchuk]

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

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

[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

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

[Julien Grall]

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

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

[Stefano Stabellini]

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

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

[Stefano Stabellini]

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

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

Also, make a small code style fix in make_memory_node.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
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

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

[Julien Grall]

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

Re: [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0

[Volodymyr Babchuk]

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

Re: [Xen-devel] [PATCH v4 0/7] reserved-memory in dom0

[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