Re: [PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree

2022-08-26 Thread Julien Grall

Hi Penny,

On 21/07/2022 14:21, Penny Zheng wrote:

+#ifdef CONFIG_STATIC_SHM
+static int __init make_shm_memory_node(const struct domain *d,
+   void *fdt,
+   int addrcells, int sizecells,
+   const struct meminfo *mem)
+{
+unsigned int i = 0;
+int res = 0;
+
+if ( mem->nr_banks == 0 )
+return -ENOENT;
+
+/*
+ * For each shared memory region, a range is exposed under
+ * the /reserved-memory node as a child node. Each range sub-node is
+ * named xen-shmem@.
+ */
+dt_dprintk("Create xen-shmem node\n");
+
+for ( ; i < mem->nr_banks; i++ )
+{
+uint64_t start = mem->bank[i].start;
+uint64_t size = mem->bank[i].size;
+/* Placeholder for xen-shmem@ + a 64-bit number + \0 */
+char buf[27];
+const char compat[] = "xen,shared-memory-v1";
+__be32 reg[addrcells + sizecells];


This doesn't build for me:

arch/arm/domain_build.c: In function ‘make_shm_memory_node’:
arch/arm/domain_build.c:1380:9: error: ISO C90 forbids variable length 
array ‘reg’ [-Werror=vla]

 __be32 reg[addrcells + sizecells];
 ^~

I haven't yet review the patch. But I think we would want to dynamically 
allocate 'reg' like we do in other places unless it is possible to know 
the maximum size of the array.


Cheers,

--
Julien Grall



[PATCH v6 7/9] xen/arm: create shared memory nodes in guest device tree

2022-07-21 Thread Penny Zheng
We expose the shared memory to the domU using the "xen,shared-memory-v1"
reserved-memory binding. See
Documentation/devicetree/bindings/reserved-memory/xen,shared-memory.txt
in Linux for the corresponding device tree binding.

To save the cost of re-parsing shared memory device tree configuration when
creating shared memory nodes in guest device tree, this commit adds new field
"shm_mem" to store shm-info per domain.

For each shared memory region, a range is exposed under
the /reserved-memory node as a child node. Each range sub-node is
named xen-shmem@ and has the following properties:
- compatible:
compatible = "xen,shared-memory-v1"
- reg:
the base guest physical address and size of the shared memory region
- xen,id:
a string that identifies the shared memory region.

Signed-off-by: Penny Zheng 
Reviewed-by: Stefano Stabellini 
---
v6 change:
- change "struct meminfo *mem" to "const struct meminfo *mem"
- change "unsigned long i" to "unsigned int i" to match the type of nr_banks.
- accroding to the Linux binding, "xen,id" is meant to be a string, not
an integer
---
v5 change:
- no change
---
v4 change:
- no change
---
v3 change:
- move field "shm_mem" to kernel_info
---
v2 change:
- using xzalloc
- shm_id should be uint8_t
- make reg a local variable
- add #address-cells and #size-cells properties
- fix alignment
---
 xen/arch/arm/domain_build.c   | 146 +-
 xen/arch/arm/include/asm/kernel.h |   1 +
 2 files changed, 145 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index f76fbbc644..8d2c465c99 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -910,7 +910,22 @@ static int __init allocate_shared_memory(struct domain *d,
 return ret;
 }
 
-static int __init process_shm(struct domain *d,
+static int __init append_shm_bank_to_domain(struct kernel_info *kinfo,
+paddr_t start, paddr_t size,
+const char *shm_id)
+{
+if ( (kinfo->shm_mem.nr_banks + 1) > NR_MEM_BANKS )
+return -ENOMEM;
+
+kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].start = start;
+kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].size = size;
+safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
+kinfo->shm_mem.nr_banks++;
+
+return 0;
+}
+
+static int __init process_shm(struct domain *d, struct kernel_info *kinfo,
   const struct dt_device_node *node)
 {
 struct dt_device_node *shm_node;
@@ -924,6 +939,7 @@ static int __init process_shm(struct domain *d,
 int ret = 0;
 unsigned int i;
 const char *role_str;
+const char *shm_id;
 bool owner_dom_io = true;
 
 if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") 
)
@@ -968,6 +984,9 @@ static int __init process_shm(struct domain *d,
 if ( dt_property_read_string(shm_node, "role", &role_str) == 0 )
 owner_dom_io = false;
 
+dt_property_read_string(shm_node, "xen,shm-id", &shm_id);
+ASSERT((strlen(shm_id) > 0) && (strlen(shm_id) < MAX_SHM_ID_LENGTH));
+
 /*
  * Per static shared memory region could be shared between multiple
  * domains.
@@ -994,6 +1013,14 @@ static int __init process_shm(struct domain *d,
 if ( ret )
 return ret;
 }
+
+/*
+ * Record static shared memory region info for later setting
+ * up shm-node in guest device tree.
+ */
+ret = append_shm_bank_to_domain(kinfo, gbase, psize, shm_id);
+if ( ret )
+return ret;
 }
 
 return 0;
@@ -1324,6 +1351,116 @@ static int __init make_memory_node(const struct domain 
*d,
 return res;
 }
 
+#ifdef CONFIG_STATIC_SHM
+static int __init make_shm_memory_node(const struct domain *d,
+   void *fdt,
+   int addrcells, int sizecells,
+   const struct meminfo *mem)
+{
+unsigned int i = 0;
+int res = 0;
+
+if ( mem->nr_banks == 0 )
+return -ENOENT;
+
+/*
+ * For each shared memory region, a range is exposed under
+ * the /reserved-memory node as a child node. Each range sub-node is
+ * named xen-shmem@.
+ */
+dt_dprintk("Create xen-shmem node\n");
+
+for ( ; i < mem->nr_banks; i++ )
+{
+uint64_t start = mem->bank[i].start;
+uint64_t size = mem->bank[i].size;
+/* Placeholder for xen-shmem@ + a 64-bit number + \0 */
+char buf[27];
+const char compat[] = "xen,shared-memory-v1";
+__be32 reg[addrcells + sizecells];
+__be32 *cells;
+unsigned int len = (addrcells + sizecells) * sizeof(__be32);
+
+snprintf(buf, sizeof(buf), "xen-shmem@%"PRIx64, mem->bank[i].start);
+res = fdt_begin_node(fdt,