[Xen-devel] [PATCH v3 0/6] reserved-memory in dom0

[Xen-devel] [PATCH v3 0/6] 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.)

One change is still outstanding: cleaning up process_memory_node so that
it is not completely shared between the normal memory case and the
reserved-memory case.


The following changes since commit 11911563610786615c2b3a01cdcaaf09a6f9e38d:

  xen/arm: fix mask calculation in pdx_init_mask (2019-06-21 14:07:47 -0700)

are available in the Git repository at:

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

for you to fetch changes up to da50a3f6d37aa42fd73dab4a50c2de7133d9b162:

  xen/arm: add reserved-memory regions to the dom0 memory node (2019-06-21 16:52:38 -0700)

----------------------------------------------------------------
Stefano Stabellini (6):
      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: 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        | 68 +++++++++++++++++++++++++++++++------------
 xen/arch/arm/domain_build.c   | 40 +++++++++++++++++++------
 xen/arch/arm/setup.c          | 53 +++++++++++++++++++++++++++++++--
 xen/include/asm-arm/setup.h   |  1 +
 xen/include/xen/device_tree.h |  5 ++--
 6 files changed, 136 insertions(+), 33 deletions(-)

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

[Xen-devel] [PATCH v3 1/6] xen/arm: extend device_tree_for_each_node

[Stefano Stabellini]

Add two new parameters to device_tree_for_each_node: node and depth.
Node is the parent node to start the search from and depth is the min
depth of the search (the depth of the parent node). Passing 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 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        | 12 ++++++------
 xen/include/xen/device_tree.h |  5 +++--
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c
index 9b29769a10..cfc85c2b61 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, dt_scan_depth1_nodes, NULL)))
         goto disable;
 
     /*
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 891b4b66ff..2f9cb8878d 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -77,6 +77,8 @@ 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
  * @func: function to call for each node.
  * @data: data to pass to @func.
  *
@@ -86,17 +88,15 @@ 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,
                                      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;
 
-    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 +357,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, 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..e1ec6cb88d 100644
--- a/xen/include/xen/device_tree.h
+++ b/xen/include/xen/device_tree.h
@@ -159,8 +159,9 @@ typedef int (*device_tree_node_func)(const void *fdt,
 extern const void *device_tree_flattened;
 
 int device_tree_for_each_node(const void *fdt,
-                                     device_tree_node_func func,
-                                     void *data);
+                              int node, int depth,
+                              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 v3 1/6] xen/arm: extend device_tree_for_each_node

[Julien Grall]

Hi Stefano,

On 6/22/19 12:56 AM, Stefano Stabellini wrote:
> Add two new parameters to device_tree_for_each_node: node and depth.
> Node is the parent node to start the search from and depth is the min
> depth of the search (the depth of the parent node). Passing 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 v3:
> - improve commit message
> - improve in-code comments
> - improve code style

You haven't addressed my questions and potential errors I pointed out on 
v2. See <885fc62e-dbe2-bb8f-1476-de5b5d7df2c8@arm.com>

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH v3 2/6] 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.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
Changes in v3:
- improve commit message
- check return value of process_memory_node

Changes in v2:
- new
---
 xen/arch/arm/bootfdt.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 2f9cb8878d..611724433b 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -124,9 +124,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;
@@ -139,14 +140,14 @@ 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 0;
     }
 
     cell = (const __be32 *)prop->data;
@@ -161,6 +162,8 @@ static void __init process_memory_node(const void *fdt, int node,
         bootinfo.mem.bank[bootinfo.mem.nr_banks].size = size;
         bootinfo.mem.nr_banks++;
     }
+
+    return 0;
 }
 
 static void __init process_multiboot_node(const void *fdt, int node,
@@ -292,15 +295,18 @@ static int __init early_scan_node(const void *fdt,
                                   u32 address_cells, u32 size_cells,
                                   void *data)
 {
+    int rc = 0;
+
     if ( device_tree_node_matches(fdt, node, "memory") )
-        process_memory_node(fdt, node, name, address_cells, size_cells);
+        rc = process_memory_node(fdt, node, name, depth,
+                                 address_cells, size_cells, NULL);
     else if ( depth <= 3 && (device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ||
               device_tree_node_compatible(fdt, node, "multiboot,module" )))
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
     else if ( depth == 1 && device_tree_node_matches(fdt, node, "chosen") )
         process_chosen_node(fdt, node, name, address_cells, size_cells);
 
-    return 0;
+    return rc;
 }
 
 static void __init early_print_info(void)
-- 
2.17.1


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

[Xen-devel] [PATCH v3 3/6] 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>

---
It is cleaner to avoid sharing the whole function process_memory_node
between the normal memory case and the reserved-memory case. I'll do it
in the next version once I understand the best way do to it.

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

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index 611724433b..b24ab10cb9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -135,6 +135,8 @@ 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;
+    bool reserved = (bool)data;
 
     if ( address_cells < 1 || size_cells < 1 )
     {
@@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, int node,
         return 0;
     }
 
+    if ( reserved )
+        mem = &bootinfo.reserved_mem;
+    else
+        mem = &bootinfo.mem;
+
     prop = fdt_get_property(fdt, node, "reg", NULL);
     if ( !prop )
     {
-        printk("fdt: node `%s': missing `reg' property\n", name);
+        if ( !reserved )
+            printk("fdt: node `%s': missing `reg' property\n", name);
         return 0;
     }
 
     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++;
     }
+    /*
+     * We reached the max number of supported reserved-memory regions.
+     * Stop and refuse to continue. We don't want to risk Xen allocating
+     * those regions as normal memory to a VM.
+     */
+    BUG_ON(reserved && mem->nr_banks == NR_MEM_BANKS);
 
     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)
+{
+    device_tree_for_each_node(fdt, node, depth, process_memory_node, (void*)true);
+    return 0;
+}
+
 static void __init process_multiboot_node(const void *fdt, int node,
                                           const char *name,
                                           u32 address_cells, u32 size_cells)
@@ -299,7 +321,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, (void*)false);
+    else if ( depth == 1 && !strcmp(name, "reserved-memory") &&
+              strlen(name) == strlen("reserved-memory") )
+        rc = process_reserved_memory_node(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 v3 3/6] xen/arm: keep track of reserved-memory regions

[Oleksandr]

On 22.06.19 02:56, Stefano Stabellini wrote:

Hi Stefano

> 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>
>
> ---
> It is cleaner to avoid sharing the whole function process_memory_node
> between the normal memory case and the reserved-memory case. I'll do it
> in the next version once I understand the best way do to it.
>
> ---
> 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      | 38 +++++++++++++++++++++++++++++++------
>   xen/include/asm-arm/setup.h |  1 +
>   2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 611724433b..b24ab10cb9 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -135,6 +135,8 @@ 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;
> +    bool reserved = (bool)data;
>   
>       if ( address_cells < 1 || size_cells < 1 )
>       {
> @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, int node,
>           return 0;
>       }
>   
> +    if ( reserved )
> +        mem = &bootinfo.reserved_mem;
> +    else
> +        mem = &bootinfo.mem;
> +
>       prop = fdt_get_property(fdt, node, "reg", NULL);
>       if ( !prop )
>       {
> -        printk("fdt: node `%s': missing `reg' property\n", name);
> +        if ( !reserved )
> +            printk("fdt: node `%s': missing `reg' property\n", name);
>           return 0;
>       }
>   
>       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++;
>       }
> +    /*
> +     * We reached the max number of supported reserved-memory regions.
> +     * Stop and refuse to continue. We don't want to risk Xen allocating
> +     * those regions as normal memory to a VM.
> +     */
> +    BUG_ON(reserved && mem->nr_banks == NR_MEM_BANKS);
>   
>       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)
> +{
> +    device_tree_for_each_node(fdt, node, depth, process_memory_node, (void*)true);
> +    return 0;
> +}

I have tested this series and got the same behavior as with V2 [1].

The "non-reserved-memory" node in my device-tree 
(sram@47FFF000->scp_shmem@0) is still interpreted as a "reserved-memory".
I think, this takes place because current implementation iterates over 
all device tree nodes starting
from real "reserved-memory" node up to the end of the device tree. And 
if there is at least one "non-reserved-memory" node (with a suitable 
depth and valid "reg" property)
located down the device tree, it will be mistakenly inserted to 
bootinfo.reserved_mem (as in my case).

----------

Unpacked device tree:
{
         ...

         memory@48000000 {
                 device_type = "memory";
                 reg = <0x00000000 0x48000000 0x00000000 0x78000000 
0x00000005 0x00000000 0x00000000 0x80000000 0x00000006 0x00000000 
0x00000000 0x80000000 0x00000007 0x00000000 0x00000000 0x80000000>;
         };
         reserved-memory {
                 #address-cells = <0x00000002>;
                 #size-cells = <0x00000002>;
                 ranges;
                 linux,lossy_decompress@54000000 {
                         no-map;
                         reg = <0x00000000 0x54000000 0x00000000 
0x03000000>;
                         linux,phandle = <0x000000b0>;
                         phandle = <0x000000b0>;
                 };
                 linux,adsp@57000000 {
                         compatible = "shared-dma-pool";
                         reusable;
                         reg = <0x00000000 0x57000000 0x00000000 
0x01000000>;
                 };
                 linux,cma@58000000 {
                         compatible = "shared-dma-pool";
                         reusable;
                         reg = <0x00000000 0x58000000 0x00000000 
0x18000000>;
                         linux,cma-default;
                 };
                 linux,multimedia@70000000 {
                         compatible = "shared-dma-pool";
                         reusable;
                         reg = <0x00000000 0x70000000 0x00000000 
0x10000000>;
                         linux,phandle = <0x000000af>;
                         phandle = <0x000000af>;
                 };
         };

         ...

         sram@47FFF000 {
                 compatible = "mmio-sram";
                 reg = <0x00000000 0x47fff000 0x00000000 0x00001000>;
                 #address-cells = <0x00000001>;
                 #size-cells = <0x00000001>;
                 ranges = <0x00000000 0x00000000 0x47fff000 0x00001000>;
                 scp_shmem@0 {
                         compatible = "mmio-sram";
                         reg = <0x00000000 0x00000200>;
                         linux,phandle = <0x000000b2>;
                         phandle = <0x000000b2>;
                 };
         };

         ...
};

----------

Xen log:

Starting kernel ...

- UART enabled -
- CPU 00000000 booting -
- Current EL 00000008 -
- Xen starting at EL2 -
- Zero BSS -
- Setting up control registers -
- Turning on paging -
- Ready -
(XEN) Checking for initrd in /chosen
(XEN) Initrd 0000000074000040-0000000076252c54
(XEN) node memory@48000000: insert bank 0: 0x48000000->0xc0000000 type: 
normal
(XEN) node memory@48000000: insert bank 1: 0x500000000->0x580000000 
type: normal
(XEN) node memory@48000000: insert bank 2: 0x600000000->0x680000000 
type: normal
(XEN) node memory@48000000: insert bank 3: 0x700000000->0x780000000 
type: normal
(XEN) node linux,lossy_decompress@54000000: insert bank 0: 
0x54000000->0x57000000 type: reserved
(XEN) node linux,adsp@57000000: insert bank 0: 0x57000000->0x58000000 
type: reserved
(XEN) node linux,cma@58000000: insert bank 0: 0x58000000->0x70000000 
type: reserved
(XEN) node linux,multimedia@70000000: insert bank 0: 
0x70000000->0x80000000 type: reserved
(XEN) node scp_shmem@0: insert bank 0: 0->0x200 type: reserved 
<--------------- Not a reserved memory
(XEN) RAM: 0000000048000000 - 00000000bfffffff
(XEN) RAM: 0000000500000000 - 000000057fffffff
(XEN) RAM: 0000000600000000 - 000000067fffffff
(XEN) RAM: 0000000700000000 - 000000077fffffff
(XEN)
(XEN) MODULE[0]: 0000000048000000 - 0000000048014080 Device Tree
(XEN) MODULE[1]: 0000000074000040 - 0000000076252c54 Ramdisk
(XEN) MODULE[2]: 000000007a000000 - 000000007c000000 Kernel
(XEN) MODULE[3]: 000000007c000000 - 000000007c010000 XSM
(XEN)  RESVD[0]: 0000000048000000 - 0000000048014000
(XEN)  RESVD[1]: 0000000074000040 - 0000000076252c54

----------

[1] 
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg45632.html


-- 
Regards,

Oleksandr Tyshchenko


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

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

[Julien Grall]

Hi,

On 7/8/19 8:02 PM, Oleksandr wrote:
> On 22.06.19 02:56, Stefano Stabellini wrote:
> I have tested this series and got the same behavior as with V2 [1].
> 
> The "non-reserved-memory" node in my device-tree 
> (sram@47FFF000->scp_shmem@0) is still interpreted as a "reserved-memory".
> I think, this takes place because current implementation iterates over 
> all device tree nodes starting
> from real "reserved-memory" node up to the end of the device tree. And 
> if there is at least one "non-reserved-memory" node (with a suitable 
> depth and valid "reg" property)
> located down the device tree, it will be mistakenly inserted to 
> bootinfo.reserved_mem (as in my case).

The problem is because we are passing the depth of the parent. Because 
of that, it will look for anything at the same depth (see the check 
depth >= min_depth in device_tree_for_each_node).

The correct solution here, would be to use the depth of the child (i.e 
depth + 1) when calling device_tree_for_each_node in 
process_reserved_memory_node.

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Wed, 10 Jul 2019, Julien Grall wrote:
> Hi,
> 
> On 7/8/19 8:02 PM, Oleksandr wrote:
> > On 22.06.19 02:56, Stefano Stabellini wrote:
> > I have tested this series and got the same behavior as with V2 [1].
> > 
> > The "non-reserved-memory" node in my device-tree
> > (sram@47FFF000->scp_shmem@0) is still interpreted as a "reserved-memory".
> > I think, this takes place because current implementation iterates over all
> > device tree nodes starting
> > from real "reserved-memory" node up to the end of the device tree. And if
> > there is at least one "non-reserved-memory" node (with a suitable depth and
> > valid "reg" property)
> > located down the device tree, it will be mistakenly inserted to
> > bootinfo.reserved_mem (as in my case).
> 
> The problem is because we are passing the depth of the parent. Because of
> that, it will look for anything at the same depth (see the check depth >=
> min_depth in device_tree_for_each_node).
> 
> The correct solution here, would be to use the depth of the child (i.e depth +
> 1) when calling device_tree_for_each_node in process_reserved_memory_node.

Yes, that is the right thing to do, together with also passing
address_cells and size_cells of the parent to device_tree_for_each_node,
so that it can be calculate appropriately at all times, regardless of
depth.

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

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

[Julien Grall]

Hi,

On 6/22/19 12:56 AM, 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>
> 
> ---
> It is cleaner to avoid sharing the whole function process_memory_node
> between the normal memory case and the reserved-memory case. I'll do it
> in the next version once I understand the best way do to it.

parse_reg(....)
{

if (reg not present)
   return -ENOPRESENT

/* parse regs */

return (full) ? -EFULL : 0;
}

process_memory_node(....)
{
    return parse_reg(...);
}

process_reserved_region()
{
     ret = parse_reg(...);
     if ( ret == -EFULL )
       panic(....);
     else if ( ret != -ENOPRESENT )
       return ret;
     return 0;
}

> 
> ---
> 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      | 38 +++++++++++++++++++++++++++++++------
>   xen/include/asm-arm/setup.h |  1 +
>   2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 611724433b..b24ab10cb9 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -135,6 +135,8 @@ 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;
> +    bool reserved = (bool)data;
>   
>       if ( address_cells < 1 || size_cells < 1 )
>       {
> @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt, int node,
>           return 0;
>       }
>   
> +    if ( reserved )
> +        mem = &bootinfo.reserved_mem;
> +    else
> +        mem = &bootinfo.mem;

Rather than passing a bool, you could pass bootinfo.{mem, reserved_mem} 
in parameter.

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

I would just get rid of this print and return an error than allow the 
caller to decide what to do.

>           return 0;
>       }
>   
>       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++;
>       }
> +    /*
> +     * We reached the max number of supported reserved-memory regions.
> +     * Stop and refuse to continue. We don't want to risk Xen allocating
> +     * those regions as normal memory to a VM.

The last sentence is confusing because reserved-region are normal memory 
that have been carved out for a specific usage. Also, the problem is not 
only with VM but any memory allocation.

So a better sentence would be: "We don't want to give the pages to the 
allocator".

> +     */
> +    BUG_ON(reserved && mem->nr_banks == NR_MEM_BANKS);
>   
>       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)
> +{
> +    device_tree_for_each_node(fdt, node, depth, process_memory_node, (void*)true);
> +    return 0;
> +}
> +
>   static void __init process_multiboot_node(const void *fdt, int node,
>                                             const char *name,
>                                             u32 address_cells, u32 size_cells)
> @@ -299,7 +321,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, (void*)false);
> +    else if ( depth == 1 && !strcmp(name, "reserved-memory") &&
> +              strlen(name) == strlen("reserved-memory") )
> +        rc = process_reserved_memory_node(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 v3 3/6] xen/arm: keep track of reserved-memory regions

[Stefano Stabellini]

On Wed, 10 Jul 2019, Julien Grall wrote:
> Hi,
> 
> On 6/22/19 12:56 AM, 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>
> > 
> > ---
> > It is cleaner to avoid sharing the whole function process_memory_node
> > between the normal memory case and the reserved-memory case. I'll do it
> > in the next version once I understand the best way do to it.
> 
> parse_reg(....)
> {
> 
> if (reg not present)
>   return -ENOPRESENT
> 
> /* parse regs */
> 
> return (full) ? -EFULL : 0;
> }
> 
> process_memory_node(....)
> {
>    return parse_reg(...);
> }
> 
> process_reserved_region()
> {
>     ret = parse_reg(...);
>     if ( ret == -EFULL )
>       panic(....);
>     else if ( ret != -ENOPRESENT )
>       return ret;
>     return 0;
> }

Thank you, that clarified things a lot!


> > ---
> > 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      | 38 +++++++++++++++++++++++++++++++------
> >   xen/include/asm-arm/setup.h |  1 +
> >   2 files changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> > index 611724433b..b24ab10cb9 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -135,6 +135,8 @@ 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;
> > +    bool reserved = (bool)data;
> >         if ( address_cells < 1 || size_cells < 1 )
> >       {
> > @@ -143,29 +145,49 @@ static int __init process_memory_node(const void *fdt,
> > int node,
> >           return 0;
> >       }
> >   +    if ( reserved )
> > +        mem = &bootinfo.reserved_mem;
> > +    else
> > +        mem = &bootinfo.mem;
> 
> Rather than passing a bool, you could pass bootinfo.{mem, reserved_mem} in
> parameter.

I'll do that


> > +
> >       prop = fdt_get_property(fdt, node, "reg", NULL);
> >       if ( !prop )
> >       {
> > -        printk("fdt: node `%s': missing `reg' property\n", name);
> > +        if ( !reserved )
> > +            printk("fdt: node `%s': missing `reg' property\n", name);
> 
> I would just get rid of this print and return an error than allow the caller
> to decide what to do.

Yep


> >           return 0;
> >       }
> >         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++;
> >       }
> > +    /*
> > +     * We reached the max number of supported reserved-memory regions.
> > +     * Stop and refuse to continue. We don't want to risk Xen allocating
> > +     * those regions as normal memory to a VM.
> 
> The last sentence is confusing because reserved-region are normal memory that
> have been carved out for a specific usage. Also, the problem is not only with
> VM but any memory allocation.
> 
> So a better sentence would be: "We don't want to give the pages to the
> allocator".

Thanks, I'll make the change

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

[Xen-devel] [PATCH v3 4/6] 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>
---
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..d9cfb1aa2f 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

Re: [Xen-devel] [PATCH v3 4/6] xen/arm: handle reserved-memory in consider_modules and dt_unreserved_regions

[Julien Grall]

Hi Stefano,

On 6/22/19 12:56 AM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 215746a5c3..d9cfb1aa2f 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);

Coding style: space before and after the operator. Ideally, the rest of

> +            dt_unreserved_regions(s, r_s, cb, i+1);

Ditto.

With that:

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

[Xen-devel] [PATCH v3 5/6] 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:
- new patch
---
 xen/arch/arm/domain_build.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index d9836779d1..76dd4bf6f9 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1158,15 +1158,22 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
     bool need_mapping = !dt_device_for_passthrough(dev);
     int res;
 
-    res = iomem_permit_access(d, paddr_to_pfn(addr),
-                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
-    if ( res )
+    /*
+     * Don't give iomem permissions for reserved-memory ranges until
+     * reserved-memory support is complete.
+     */
+    if ( strcmp(dt_node_name(dev), "reserved-memory") )
     {
-        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 v3 5/6] xen/arm: don't iomem_permit_access for reserved-memory regions

[Oleksandr]

On 22.06.19 02:56, Stefano Stabellini wrote:

Hi, Stefano

> 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:
> - new patch
> ---
>   xen/arch/arm/domain_build.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d9836779d1..76dd4bf6f9 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1158,15 +1158,22 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
>       bool need_mapping = !dt_device_for_passthrough(dev);
>       int res;
>   
> -    res = iomem_permit_access(d, paddr_to_pfn(addr),
> -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> -    if ( res )
> +    /*
> +     * Don't give iomem permissions for reserved-memory ranges until
> +     * reserved-memory support is complete.
> +     */
> +    if ( strcmp(dt_node_name(dev), "reserved-memory") )

This filter doesn't catch child nodes if the "reserved-memory" node has 
ones.

Here is my "reserved-memory" node:

reserved-memory {
         #address-cells = <2>;
         #size-cells = <2>;
         ranges;

         /* device specific region for Lossy Decompression */
         lossy_decompress: linux,lossy_decompress@54000000 {
             no-map;
             reg = <0x00000000 0x54000000 0x0 0x03000000>;
         };

         /* For Audio DSP */
         adsp_reserved: linux,adsp@57000000 {
             compatible = "shared-dma-pool";
             reusable;
             reg = <0x00000000 0x57000000 0x0 0x01000000>;
         };

         /* global autoconfigured region for contiguous allocations */
         linux,cma@58000000 {
             compatible = "shared-dma-pool";
             reusable;
             reg = <0x00000000 0x58000000 0x0 0x18000000>;
             linux,cma-default;
         };

         /* device specific region for contiguous allocations */
         mmp_reserved: linux,multimedia@70000000 {
             compatible = "shared-dma-pool";
             reusable;
             reg = <0x00000000 0x70000000 0x0 0x10000000>;
         };
     };


So, the dt_node_name(dev) for child nodes are:

- linux,lossy_decompress
- linux,adsp
- linux,cma
- linux,multimedia


Probably, we should check whether the parent node is "reserved-memory" 
as well.


-- 
Regards,

Oleksandr Tyshchenko


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

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

[Stefano Stabellini]

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

On Tue, 9 Jul 2019, Oleksandr wrote:
> 
> On 22.06.19 02:56, Stefano Stabellini wrote:
> 
> Hi, Stefano
> 
> > 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:
> > - new patch
> > ---
> >   xen/arch/arm/domain_build.c | 23 +++++++++++++++--------
> >   1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index d9836779d1..76dd4bf6f9 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1158,15 +1158,22 @@ static int __init map_range_to_domain(const struct
> > dt_device_node *dev,
> >       bool need_mapping = !dt_device_for_passthrough(dev);
> >       int res;
> >   -    res = iomem_permit_access(d, paddr_to_pfn(addr),
> > -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> > -    if ( res )
> > +    /*
> > +     * Don't give iomem permissions for reserved-memory ranges until
> > +     * reserved-memory support is complete.
> > +     */
> > +    if ( strcmp(dt_node_name(dev), "reserved-memory") )
> 
> This filter doesn't catch child nodes if the "reserved-memory" node has ones.
> 
> Here is my "reserved-memory" node:
> 
> reserved-memory {
>         #address-cells = <2>;
>         #size-cells = <2>;
>         ranges;
> 
>         /* device specific region for Lossy Decompression */
>         lossy_decompress: linux,lossy_decompress@54000000 {
>             no-map;
>             reg = <0x00000000 0x54000000 0x0 0x03000000>;
>         };
> 
>         /* For Audio DSP */
>         adsp_reserved: linux,adsp@57000000 {
>             compatible = "shared-dma-pool";
>             reusable;
>             reg = <0x00000000 0x57000000 0x0 0x01000000>;
>         };
> 
>         /* global autoconfigured region for contiguous allocations */
>         linux,cma@58000000 {
>             compatible = "shared-dma-pool";
>             reusable;
>             reg = <0x00000000 0x58000000 0x0 0x18000000>;
>             linux,cma-default;
>         };
> 
>         /* device specific region for contiguous allocations */
>         mmp_reserved: linux,multimedia@70000000 {
>             compatible = "shared-dma-pool";
>             reusable;
>             reg = <0x00000000 0x70000000 0x0 0x10000000>;
>         };
>     };
> 
> 
> So, the dt_node_name(dev) for child nodes are:
> 
> - linux,lossy_decompress
> - linux,adsp
> - linux,cma
> - linux,multimedia
> 
> 
> Probably, we should check whether the parent node is "reserved-memory" as
> well.

Yes, that is a mistake, thanks for spotting it. I should check the
parent node name instead.

[-- 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 v3 5/6] xen/arm: don't iomem_permit_access for reserved-memory regions

[Julien Grall]

Hi Stefano,

On 6/22/19 12:56 AM, Stefano Stabellini wrote:
> 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:
> - new patch
> ---
>   xen/arch/arm/domain_build.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d9836779d1..76dd4bf6f9 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1158,15 +1158,22 @@ static int __init map_range_to_domain(const struct dt_device_node *dev,
>       bool need_mapping = !dt_device_for_passthrough(dev);
>       int res;
>   
> -    res = iomem_permit_access(d, paddr_to_pfn(addr),
> -                              paddr_to_pfn(PAGE_ALIGN(addr + len - 1)));
> -    if ( res )
> +    /*
> +     * Don't give iomem permissions for reserved-memory ranges until
> +     * reserved-memory support is complete.
> +     */
> +    if ( strcmp(dt_node_name(dev), "reserved-memory") )

Aside Oleksandr's comment, you should technically use dt_node_cmp(...) 
for comparing node name.

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

Cheers,

-- 
Julien Grall

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

[Xen-devel] [PATCH v3 6/6] 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.

Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
---
 xen/arch/arm/domain_build.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 76dd4bf6f9..5047eb4c28 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -643,7 +643,8 @@ static int __init make_memory_node(const struct domain *d,
 {
     int res, i;
     int reg_size = addrcells + sizecells;
-    int nr_cells = reg_size*kinfo->mem.nr_banks;
+    int nr_cells = reg_size * (kinfo->mem.nr_banks + (is_hardware_domain(d) ?
+                               bootinfo.reserved_mem.nr_banks : 0));
     __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
     __be32 *cells;
 
@@ -673,6 +674,20 @@ static int __init make_memory_node(const struct domain *d,
         dt_child_set_range(&cells, addrcells, sizecells, start, size);
     }
 
+    if ( is_hardware_domain(d) )
+    {
+        for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
+        {
+            u64 start = bootinfo.reserved_mem.bank[i].start;
+            u64 size = bootinfo.reserved_mem.bank[i].size;
+
+            dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
+                    i, start, start + size);
+
+            dt_child_set_range(&cells, addrcells, sizecells, start, size);
+        }
+    }
+
     res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
     if ( res )
         return res;
-- 
2.17.1


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

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

[Julien Grall]

Hi Stefano,

On 6/22/19 12:56 AM, 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.
> 
> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> ---
>   xen/arch/arm/domain_build.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 76dd4bf6f9..5047eb4c28 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -643,7 +643,8 @@ static int __init make_memory_node(const struct domain *d,
>   {
>       int res, i;
>       int reg_size = addrcells + sizecells;
> -    int nr_cells = reg_size*kinfo->mem.nr_banks;
> +    int nr_cells = reg_size * (kinfo->mem.nr_banks + (is_hardware_domain(d) ?
> +                               bootinfo.reserved_mem.nr_banks : 0));

A device-tree is allowed to have multiple memory node. So I would 
actually prefer if we create a new node for the reserved-memory regions.

You could replace the last parameter with struct meminfo *. Note that 
you would need to specify a unit in the node-name.

>       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];

Note that you will overrun reg here if the two arrays (reserved_mem and 
mem) are full.

>       __be32 *cells;
>   
> @@ -673,6 +674,20 @@ static int __init make_memory_node(const struct domain *d,
>           dt_child_set_range(&cells, addrcells, sizecells, start, size);
>       }
>   
> +    if ( is_hardware_domain(d) )
> +    {
> +        for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
> +        {
> +            u64 start = bootinfo.reserved_mem.bank[i].start;
> +            u64 size = bootinfo.reserved_mem.bank[i].size;
> +
> +            dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> +                    i, start, start + size);
> +
> +            dt_child_set_range(&cells, addrcells, sizecells, start, size);
> +        }
> +    }
> +
>       res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
>       if ( res )
>           return res;
> 

Cheers,

-- 
Julien Grall

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

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

[Stefano Stabellini]

On Wed, 10 Jul 2019, Julien Grall wrote:
> Hi Stefano,
> 
> On 6/22/19 12:56 AM, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > ---
> >   xen/arch/arm/domain_build.c | 17 ++++++++++++++++-
> >   1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 76dd4bf6f9..5047eb4c28 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -643,7 +643,8 @@ static int __init make_memory_node(const struct domain
> > *d,
> >   {
> >       int res, i;
> >       int reg_size = addrcells + sizecells;
> > -    int nr_cells = reg_size*kinfo->mem.nr_banks;
> > +    int nr_cells = reg_size * (kinfo->mem.nr_banks + (is_hardware_domain(d)
> > ?
> > +                               bootinfo.reserved_mem.nr_banks : 0));
> 
> A device-tree is allowed to have multiple memory node. So I would actually
> prefer if we create a new node for the reserved-memory regions.
> 
> You could replace the last parameter with struct meminfo *. Note that you
> would need to specify a unit in the node-name.

Yes, makes sense, good suggestion.


> >       __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */];
> >
> Note that you will overrun reg here if the two arrays (reserved_mem and mem)
> are full.

Won't be a problem anymore after making the suggested changes.


> >       __be32 *cells;
> >   @@ -673,6 +674,20 @@ static int __init make_memory_node(const struct
> > domain *d,
> >           dt_child_set_range(&cells, addrcells, sizecells, start, size);
> >       }
> >   +    if ( is_hardware_domain(d) )
> > +    {
> > +        for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
> > +        {
> > +            u64 start = bootinfo.reserved_mem.bank[i].start;
> > +            u64 size = bootinfo.reserved_mem.bank[i].size;
> > +
> > +            dt_dprintk("  Bank %d: %#"PRIx64"->%#"PRIx64"\n",
> > +                    i, start, start + size);
> > +
> > +            dt_child_set_range(&cells, addrcells, sizecells, start, size);
> > +        }
> > +    }
> > +
> >       res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg));
> >       if ( res )
> >           return res;
> > 
> 
> Cheers,
> 
> -- 
> Julien Grall
> 

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