Hi Michal

On 2023/9/11 17:40, Michal Orzel wrote:
Hi Penny,

On 11/09/2023 06:04, 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
s/codes/code/

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.zh...@arm.com>

---
v3 -> v4:
new commit
---
  xen/arch/arm/domain_build.c       | 31 ++++++++++++++++++++++++++++---
  xen/arch/arm/include/asm/kernel.h |  2 ++
  2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 02d903be78..dad234e4b5 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1401,6 +1401,10 @@ static int __init handle_linux_pci_domain(struct 
kernel_info *kinfo,
      return fdt_property_cell(kinfo->fdt, "linux,pci-domain", segment);
  }

+static int __init make_shm_memory_node(const struct domain *d,
+                                       void *fdt,
+                                       int addrcells, int sizecells,
+                                       const struct kernel_info *kinfo);
  static int __init write_properties(struct domain *d, struct kernel_info 
*kinfo,
                                     const struct dt_device_node *node)
  {
@@ -1549,6 +1553,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;
kinfo structure is used to store per-domain parameters and presence of reserved 
memory in host dtb
does not fit into this. Moreover, there is no need to introduce yet another 
flag for that given
that in other parts of the code in Xen we use bootinfo.reserved_mem.nr_banks to 
check if there are
reserved regions.

+
+        /* 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);
I haven't yet looked at previous patches but does it make sense to request passing 
both kinfo->fdt and kinfo?
IMO, the latter would be sufficient. This would apply to other functions you 
modified.


yes, the latter would be sufficient. I'll fix it in next version.


+            if ( res )
+                return res;
+        }
+    }
+
      return 0;
  }

@@ -2856,9 +2877,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 )
No need for a new flag as mentioned above. Just before this line of code there 
is a check:
if ( bootinfo.reserved_mem.nr_banks > 0 )
{
     res = make_memory_node(d, kinfo->fdt, addrcells, sizecells,
                             &bootinfo.reserved_mem);
     if ( res )
         return res;
}
so you can just append the following:
else
{
     res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
     if ( res )
         return res;
}
to achieve the same without a need for a new flag.


Right now, bootinfo.reserved_mem is not only containing reserved regions described in host FDT /reserved-memory, but also the reserved ones for domain only, like in xen,static-mem = <xxx>.
So simply with non-zero bootinfo.reserved_mem.nr_banks, we couldn't tell
whether we had a /reserved-memory node in host FDT.

I agree that kinfo is not a good place to store whether host FDT has a
/reserved-memory node. Maybe bootinfo.has_resv_memory_node?


~Michal

Reply via email to