Hi Stefano and Julien
> -----Original Message-----
> From: Stefano Stabellini <sstabell...@kernel.org>
> Sent: Saturday, June 25, 2022 5:57 AM
> To: Julien Grall <jul...@xen.org>
> Cc: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org;
> Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>
> Subject: Re: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest
> device tree
>
> On Fri, 24 Jun 2022, Julien Grall wrote:
> > On 20/06/2022 06:11, Penny Zheng wrote:
> > > 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@<address> 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 <penny.zh...@arm.com>
> > > Reviewed-by: Stefano Stabellini <sstabell...@kernel.org>
> > > ---
> > > 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 | 143
> +++++++++++++++++++++++++++++-
> > > xen/arch/arm/include/asm/kernel.h | 1 +
> > > xen/arch/arm/include/asm/setup.h | 1 +
> > > 3 files changed, 143 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/domain_build.c
> > > b/xen/arch/arm/domain_build.c index 1584e6c2ce..4d62440a0e 100644
> > > --- a/xen/arch/arm/domain_build.c
> > > +++ b/xen/arch/arm/domain_build.c
> > > @@ -900,7 +900,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,
> > > + u32 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;
> > > + 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; @@ -971,6 +986,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;
> > > @@ -1301,6 +1324,117 @@ 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,
> > > + struct meminfo *mem)
> >
> > NIT: AFAICT mem is not changed, so it should be const.
> >
> > > +{
> > > + unsigned long i = 0;
> >
> > NIT: This should be "unsigned int" to match the type of nr_banks.
> >
> > > + 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@<address>.
> > > + */
> > > + 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;
> > > + uint8_t shm_id = mem->bank[i].shm_id;
> > > + /* Placeholder for xen-shmem@ + a 64-bit number + \0 */
> > > + char buf[27];
> > > + const char compat[] = "xen,shared-memory-v1";
> > > + __be32 reg[4];
> > > + __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, buf);
> > > + if ( res )
> > > + return res;
> > > +
> > > + res = fdt_property(fdt, "compatible", compat, sizeof(compat));
> > > + if ( res )
> > > + return res;
> > > +
> > > + cells = reg;
> > > + dt_child_set_range(&cells, addrcells, sizecells, start,
> > > + size);
> > > +
> > > + res = fdt_property(fdt, "reg", reg, len);
> > > + if ( res )
> > > + return res;
> > > +
> > > + dt_dprintk("Shared memory bank %lu: %#"PRIx64"->%#"PRIx64"\n",
> > > + i, start, start + size);
> > > +
> > > + res = fdt_property_cell(fdt, "xen,id", shm_id);
> >
> > Looking at the Linux binding, "xen,id" is meant to be a string. But
> > here you are writing it as an integer.
>
> Good catch!
>
>
> > Given that the Linux binding is already merged, I think the Xen
> > binding should be changed.
>
> We would be compliant with both bindings (xen and linux) just by writing
> shm_id as string here, but if it is not too difficult we might as well
> harmonize
> the two bindings and also define xen,shm-id as a string.
>
> On the Xen side, I would suggest to put a clear size limit so that the string
> is
> easier to handle.
I've already made the xen,shm-id parsed as a string too, seeing the below code:
"
prop_id = fdt_get_property(fdt, node, "xen,shm-id", NULL);
if ( !prop_id )
return -ENOENT;
shm_id = simple_strtoul(prop_id->data, NULL, 10);
if ( shm_id >= NR_MEM_BANKS )
{
printk("fdt: invalid `xen,shm-id` %lu for static shared memory node.\n",
shm_id);
return -EINVAL;
}
"
The size limit is smaller than 256, just as stated in doc:
"
- xen,shm-id
A string that represents the unique identifier of the shared memory
region. The maximum identifier shall be "xen,shm-id = 255".
"
Hope this fits what both of you suggested~~~
