diff mbox series

[v5,10/11] xen/arm: fix duplicate /reserved-memory node in Dom0

Message ID 20231206090623.1932275-11-Penny.Zheng@arm.com (mailing list archive)
State New, archived
Headers show
Series Follow-up static shared memory PART I | expand

Commit Message

Penny Zheng Dec. 6, 2023, 9:06 a.m. UTC
In case there is a /reserved-memory node already present in the host dtb,
current Xen codes would create yet another /reserved-memory node specially
for the static shm in Dom0 Device Tree.

Xen will use write_properties() to copy the reserved memory nodes from host dtb
to Dom0 FDT, so we want to insert the shm node along with the copying.
And avoiding duplication, we add a checking before make_resv_memory_node().

Signed-off-by: Penny Zheng <penny.zheng@arm.com>

---
v3 -> v4:
new commit
---
v4 -> v5:
rebase and no change
---
 xen/arch/arm/domain_build.c             | 27 ++++++++++++++++++++++---
 xen/arch/arm/include/asm/kernel.h       |  2 ++
 xen/arch/arm/include/asm/static-shmem.h | 14 +++++++++++++
 xen/arch/arm/static-shmem.c             |  6 +++---
 4 files changed, 43 insertions(+), 6 deletions(-)

Comments

Michal Orzel Dec. 7, 2023, 11:14 a.m. UTC | #1
Hi Penny,

On 06/12/2023 10:06, Penny Zheng wrote:
> 
> 
> In case there is a /reserved-memory node already present in the host dtb,
> current Xen codes would create yet another /reserved-memory node specially
> for the static shm in Dom0 Device Tree.
Rational missing:
This would result in an incorrect device tree generation and guest would not be able
to detect the static shared memory region.

> 
> Xen will use write_properties() to copy the reserved memory nodes from host dtb
> to Dom0 FDT, so we want to insert the shm node along with the copying.
> And avoiding duplication, we add a checking before make_resv_memory_node().
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> 
> ---
> v3 -> v4:
> new commit
> ---
> v4 -> v5:
> rebase and no change
> ---
>  xen/arch/arm/domain_build.c             | 27 ++++++++++++++++++++++---
>  xen/arch/arm/include/asm/kernel.h       |  2 ++
>  xen/arch/arm/include/asm/static-shmem.h | 14 +++++++++++++
>  xen/arch/arm/static-shmem.c             |  6 +++---
>  4 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index e040f8a6d9..f098678ea3 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -752,6 +752,23 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>          }
>      }
> 
> +    if ( dt_node_path_is_equal(node, "/reserved-memory") )
> +    {
> +        kinfo->resv_mem = true;
I think I raised this concern last time. kinfo is used to store per-domain configuration.
Information whether the reserved memory is present in host dtb or not does not fit there.
Therefore, I would move this flag to bootinfo.

> +
> +        /* shared memory provided. */
> +        if ( kinfo->shminfo.nr_banks != 0 )
> +        {
> +            uint32_t addrcells = dt_n_addr_cells(node),
> +                     sizecells = dt_n_size_cells(node);
I find this always a bit odd. Just uint32_t sizecells on the next line, just like you did in all the other patches.

~Michal
diff mbox series

Patch

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index e040f8a6d9..f098678ea3 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -752,6 +752,23 @@  static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
         }
     }
 
+    if ( dt_node_path_is_equal(node, "/reserved-memory") )
+    {
+        kinfo->resv_mem = true;
+
+        /* shared memory provided. */
+        if ( kinfo->shminfo.nr_banks != 0 )
+        {
+            uint32_t addrcells = dt_n_addr_cells(node),
+                     sizecells = dt_n_size_cells(node);
+
+            res = make_shm_memory_node(d, kinfo->fdt,
+                                       addrcells, sizecells, kinfo);
+            if ( res )
+                return res;
+        }
+    }
+
     return 0;
 }
 
@@ -1856,9 +1873,13 @@  static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
                 return res;
         }
 
-        res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
-        if ( res )
-            return res;
+        /* Avoid duplicate /reserved-memory nodes in Device Tree */
+        if ( !kinfo->resv_mem )
+        {
+            res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
+            if ( res )
+                return res;
+        }
     }
 
     res = fdt_end_node(kinfo->fdt);
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index db3d8232fa..8fe2105a91 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -39,6 +39,8 @@  struct kernel_info {
     void *fdt; /* flat device tree */
     paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
     struct meminfo mem;
+    /* Whether we have /reserved-memory node in host Device Tree */
+    bool resv_mem;
     /* Static shared memory banks */
     struct {
         unsigned int nr_banks;
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index d149985291..6cb4ef9646 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -33,6 +33,11 @@  int remove_shm_from_rangeset(const struct kernel_info *kinfo,
 
 int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
                               struct meminfo *orig_ext);
+
+int make_shm_memory_node(const struct domain *d,
+                         void *fdt,
+                         int addrcells, int sizecells,
+                         const struct kernel_info *kinfo);
 #else /* !CONFIG_STATIC_SHM */
 
 static inline int make_resv_memory_node(const struct domain *d, void *fdt,
@@ -72,6 +77,15 @@  static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
 {
     return 0;
 }
+
+static inline int make_shm_memory_node(const struct domain *d,
+                                       void *fdt,
+                                       int addrcells, int sizecells,
+                                       const struct kernel_info *kinfo)
+{
+    return 0;
+}
+
 #endif /* CONFIG_STATIC_SHM */
 
 #endif /* __ASM_STATIC_SHMEM_H_ */
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index a06949abaf..bfce5bbad0 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -505,9 +505,9 @@  int __init process_shm(struct domain *d, struct kernel_info *kinfo,
     return 0;
 }
 
-static int __init make_shm_memory_node(const struct domain *d, void *fdt,
-                                       int addrcells, int sizecells,
-                                       const struct kernel_info *kinfo)
+int __init make_shm_memory_node(const struct domain *d, void *fdt,
+                                int addrcells, int sizecells,
+                                const struct kernel_info *kinfo)
 {
     unsigned int i = 0;
     int res = 0;