| Message ID | 20220620051114.210118-8-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | static shared memory on dom0less system | expand |
Hi Penny, 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.zheng@arm.com> > Reviewed-by: Stefano Stabellini <sstabellini@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. Given that the Linux binding is already merged, I think the Xen binding should be changed. > + if ( res ) > + return res; > + > + res = fdt_end_node(fdt); > + if ( res ) > + return res; > + } > + > + return res; > +} > +#else > +static int __init make_shm_memory_node(const struct domain *d, > + void *fdt, > + int addrcells, int sizecells, > + struct meminfo *mem) > +{ > + ASSERT_UNREACHABLE(); > +} > +#endif > + > +static int __init make_resv_memory_node(const struct domain *d, > + void *fdt, > + int addrcells, int sizecells, > + struct meminfo *mem) > +{ > + int res = 0; > + /* Placeholder for reserved-memory\0 */ > + char resvbuf[16] = "reserved-memory"; > + > + if ( mem->nr_banks == 0 ) > + /* No shared memory provided. */ > + return 0; > + > + dt_dprintk("Create reserved-memory node\n"); > + > + res = fdt_begin_node(fdt, resvbuf); > + if ( res ) > + return res; > + > + res = fdt_property(fdt, "ranges", NULL, 0); > + if ( res ) > + return res; > + > + res = fdt_property_cell(fdt, "#address-cells", addrcells); > + if ( res ) > + return res; > + > + res = fdt_property_cell(fdt, "#size-cells", sizecells); > + if ( res ) > + return res; > + > + res = make_shm_memory_node(d, fdt, addrcells, sizecells, mem); > + if ( res ) > + return res; > + > + res = fdt_end_node(fdt); > + > + return res; > +} > + > static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) > { > struct meminfo *ext_regions = data; > @@ -3078,6 +3212,11 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) > if ( ret ) > goto err; > > + ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, > + &kinfo->shm_mem); > + if ( ret ) > + goto err; > + > /* > * domain_handle_dtb_bootmodule has to be called before the rest of > * the device tree is generated because it depends on the value of > @@ -3454,7 +3593,7 @@ static int __init construct_domU(struct domain *d, > assign_static_memory_11(d, &kinfo, node); > > #ifdef CONFIG_STATIC_SHM > - rc = process_shm(d, node); > + rc = process_shm(d, &kinfo, node); > if ( rc < 0 ) > return rc; > #endif > diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h > index c4dc039b54..2cc506b100 100644 > --- a/xen/arch/arm/include/asm/kernel.h > +++ b/xen/arch/arm/include/asm/kernel.h > @@ -19,6 +19,7 @@ struct kernel_info { > void *fdt; /* flat device tree */ > paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ > struct meminfo mem; > + struct meminfo shm_mem; > > /* kernel entry point */ > paddr_t entry; > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index 5063e5d077..7497cc40aa 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -29,6 +29,7 @@ struct membank { > bool xen_domain; /* whether the memory bank is bound to a Xen domain. */ > #ifdef CONFIG_STATIC_SHM > unsigned int nr_shm_domain; > + uint8_t shm_id ; /* ID identifier of a static shared memory bank. */ I am not entirely happy that we are defining shm_id for everyone. We are at v5, so I am OK for now. But I would at least like "shm_id" to be defined before nr_shm_domain so we re-use the existing hole and avoid increasing the size of the structure. Cheers,
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.zheng@arm.com> > > Reviewed-by: Stefano Stabellini <sstabellini@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.
Hi Stefano and Julien > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: Saturday, June 25, 2022 5:57 AM > To: Julien Grall <julien@xen.org> > Cc: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@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.zheng@arm.com> > > > Reviewed-by: Stefano Stabellini <sstabellini@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~~~
On 04/07/2022 08:45, Penny Zheng wrote: > Hi Stefano and Julien Hi Penny, >> -----Original Message----- >> From: Stefano Stabellini <sstabellini@kernel.org> >>>> + 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); Why do you want to convert the string to a number? > if ( shm_id >= NR_MEM_BANKS ) IIRC, you are not using "shm_id" to index any bank. So why do you want to check against 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". The first sentence reads as the xen,shm-id can a free form string. But then the second sentence suggests a number (not a string). In any case, it is still unclear why you want to convert the string to an ID. From my understanding, Stefano was suggested a limit on the characters rather than a limit on the number. If the latter is desirable, then the documentation should be a bit clearer and you need to validate the input provided by the user. Cheers,
On Tue, 5 Jul 2022, Julien Grall wrote: > On 04/07/2022 08:45, Penny Zheng wrote: > > Hi Stefano and Julien > > Hi Penny, > > > > -----Original Message----- > > > From: Stefano Stabellini <sstabellini@kernel.org> > > > > > + 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); > > Why do you want to convert the string to a number? > > > > if ( shm_id >= NR_MEM_BANKS ) > > IIRC, you are not using "shm_id" to index any bank. So why do you want to > check against 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". > > The first sentence reads as the xen,shm-id can a free form string. But then > the second sentence suggests a number (not a string). > > In any case, it is still unclear why you want to convert the string to an ID. > From my understanding, Stefano was suggested a limit on the characters rather > than a limit on the number. Just confirming that yes, I was suggesting a strict limit on the number of characters, assuming we accept a freeform string. I think a freeform string is more convenient and flexible for the user. But it is not required: our only requirement is that the Linux device tree Xen generates has "xen,id" in the form of a string, but that could be a string representing a number, e.g. "255". > If the latter is desirable, then the documentation should be a bit clearer and > you need to validate the input provided by the user.
On Mon, 4 Jul 2022, Penny Zheng wrote: > Hi Stefano and Julien > > > -----Original Message----- > > From: Stefano Stabellini <sstabellini@kernel.org> > > Sent: Saturday, June 25, 2022 5:57 AM > > To: Julien Grall <julien@xen.org> > > Cc: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > > Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > > Volodymyr Babchuk <Volodymyr_Babchuk@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.zheng@arm.com> > > > > Reviewed-by: Stefano Stabellini <sstabellini@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~~~ Yes. I think supporting arbitrary strings like "my-shared-mem-1" would be nice-to-have but I wouldn't make it a hard requirement. "255" as a string would match Linux's requirements for xen,id.
Hi Stefano and julien > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: Thursday, July 7, 2022 7:53 AM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; Bertrand > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest > device tree > > On Mon, 4 Jul 2022, Penny Zheng wrote: > > Hi Stefano and Julien > > > > > -----Original Message----- > > > From: Stefano Stabellini <sstabellini@kernel.org> > > > Sent: Saturday, June 25, 2022 5:57 AM > > > To: Julien Grall <julien@xen.org> > > > Cc: Penny Zheng <Penny.Zheng@arm.com>; > > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; > Stefano > > > Stabellini <sstabellini@kernel.org>; Bertrand Marquis > > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > > > <Volodymyr_Babchuk@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.zheng@arm.com> > > > > > Reviewed-by: Stefano Stabellini <sstabellini@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~~~ > > Yes. I think supporting arbitrary strings like "my-shared-mem-1" would be > nice-to-have but I wouldn't make it a hard requirement. > Oh, the example "my-shared-mem-1" really expands my mind, I think I understand what you and Julien referred as free form string, which shall not be limited to only numeric number... thanks!!! You were suggesting a strict limit on the number of characters, TBH, I have no clue What the standard is here. Any suggestions? If considering padding, maybe 19? " struct membank { paddr_t start; paddr_t size; bool xen_domain; /* whether the memory bank is bound to a Xen domain. */ #ifdef CONFIG_STATIC_SHM char shm_id[19]; unsigned int nr_shm_borrowers; #endif }; " > "255" as a string would match Linux's requirements for xen,id. I will use your example "my-shm-mem-1", I think its better for users to understand, at least for me...
On Thu, 7 Jul 2022, Penny Zheng wrote: > Hi Stefano and julien > > > -----Original Message----- > > From: Stefano Stabellini <sstabellini@kernel.org> > > Sent: Thursday, July 7, 2022 7:53 AM > > To: Penny Zheng <Penny.Zheng@arm.com> > > Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall <julien@xen.org>; > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; Bertrand > > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > > <Volodymyr_Babchuk@epam.com> > > Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest > > device tree > > > > On Mon, 4 Jul 2022, Penny Zheng wrote: > > > Hi Stefano and Julien > > > > > > > -----Original Message----- > > > > From: Stefano Stabellini <sstabellini@kernel.org> > > > > Sent: Saturday, June 25, 2022 5:57 AM > > > > To: Julien Grall <julien@xen.org> > > > > Cc: Penny Zheng <Penny.Zheng@arm.com>; > > > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; > > Stefano > > > > Stabellini <sstabellini@kernel.org>; Bertrand Marquis > > > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > > > > <Volodymyr_Babchuk@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.zheng@arm.com> > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@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~~~ > > > > Yes. I think supporting arbitrary strings like "my-shared-mem-1" would be > > nice-to-have but I wouldn't make it a hard requirement. > > > > Oh, the example "my-shared-mem-1" really expands my mind, I think I understand > what you and Julien referred as free form string, which shall not be limited to only > numeric number... thanks!!! > > You were suggesting a strict limit on the number of characters, TBH, I have no clue > What the standard is here. Any suggestions? > > If considering padding, maybe 19? > " > struct membank { > paddr_t start; > paddr_t size; > bool xen_domain; /* whether the memory bank is bound to a Xen domain. */ > #ifdef CONFIG_STATIC_SHM > char shm_id[19]; > unsigned int nr_shm_borrowers; > #endif > }; > " Yeah I suggested a strict limit on the number of chars so that we could embed the string in struct membank. I would pick 16 characters which is equivalent to two uint64_t in terms of memory usage. > > "255" as a string would match Linux's requirements for xen,id. > > I will use your example "my-shm-mem-1", I think its better for users > to understand, at least for me... +1
Hi Stefano > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: Saturday, July 9, 2022 12:41 AM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Stefano Stabellini <sstabellini@kernel.org>; julien@xen.org; xen- > devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; Bertrand > Marquis <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com> > Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest > device tree > > On Thu, 7 Jul 2022, Penny Zheng wrote: > > Hi Stefano and julien > > > > > -----Original Message----- > > > From: Stefano Stabellini <sstabellini@kernel.org> > > > Sent: Thursday, July 7, 2022 7:53 AM > > > To: Penny Zheng <Penny.Zheng@arm.com> > > > Cc: Stefano Stabellini <sstabellini@kernel.org>; Julien Grall > > > <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Chen > > > <Wei.Chen@arm.com>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; > > > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > > Subject: RE: [PATCH v5 7/8] xen/arm: create shared memory nodes in > > > guest device tree > > > > > > On Mon, 4 Jul 2022, Penny Zheng wrote: > > > > Hi Stefano and Julien > > > > > > > > > -----Original Message----- > > > > > From: Stefano Stabellini <sstabellini@kernel.org> > > > > > Sent: Saturday, June 25, 2022 5:57 AM > > > > > To: Julien Grall <julien@xen.org> > > > > > Cc: Penny Zheng <Penny.Zheng@arm.com>; > > > > > xen-devel@lists.xenproject.org; Wei Chen <Wei.Chen@arm.com>; > > > Stefano > > > > > Stabellini <sstabellini@kernel.org>; Bertrand Marquis > > > > > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > > > > > <Volodymyr_Babchuk@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.zheng@arm.com> > > > > > > > Reviewed-by: Stefano Stabellini <sstabellini@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~~~ > > > > > > Yes. I think supporting arbitrary strings like "my-shared-mem-1" > > > would be nice-to-have but I wouldn't make it a hard requirement. > > > > > > > Oh, the example "my-shared-mem-1" really expands my mind, I think I > > understand what you and Julien referred as free form string, which > > shall not be limited to only numeric number... thanks!!! > > > > You were suggesting a strict limit on the number of characters, TBH, I > > have no clue What the standard is here. Any suggestions? > > > > If considering padding, maybe 19? > > " > > struct membank { > > paddr_t start; > > paddr_t size; > > bool xen_domain; /* whether the memory bank is bound to a Xen > > domain. */ #ifdef CONFIG_STATIC_SHM > > char shm_id[19]; > > unsigned int nr_shm_borrowers; > > #endif > > }; > > " > > Yeah I suggested a strict limit on the number of chars so that we could > embed the string in struct membank. I would pick 16 characters which is > equivalent to two uint64_t in terms of memory usage. > > Hmm, Am I calculating wrongly? When it reaches to nr_shm_borrowers, it requires 4 bytes-aligned, and if it is 16 characters, it will ask extra 3 bytes to do the padding(8 + 8 + 1 + 16 + "3"). This is the reason why I chose 19, to make use of every byte. Or maybe 16 characters is applied to be the multiple orders of 2, which has more flexibility for newly added field? Just out of curiosity why you choose 16 over 19, hope it doesn't bother too much~ > > > "255" as a string would match Linux's requirements for xen,id. > > > > I will use your example "my-shm-mem-1", I think its better for users > > to understand, at least for me... > > +1
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) +{ + unsigned long 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@<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); + if ( res ) + return res; + + res = fdt_end_node(fdt); + if ( res ) + return res; + } + + return res; +} +#else +static int __init make_shm_memory_node(const struct domain *d, + void *fdt, + int addrcells, int sizecells, + struct meminfo *mem) +{ + ASSERT_UNREACHABLE(); +} +#endif + +static int __init make_resv_memory_node(const struct domain *d, + void *fdt, + int addrcells, int sizecells, + struct meminfo *mem) +{ + int res = 0; + /* Placeholder for reserved-memory\0 */ + char resvbuf[16] = "reserved-memory"; + + if ( mem->nr_banks == 0 ) + /* No shared memory provided. */ + return 0; + + dt_dprintk("Create reserved-memory node\n"); + + res = fdt_begin_node(fdt, resvbuf); + if ( res ) + return res; + + res = fdt_property(fdt, "ranges", NULL, 0); + if ( res ) + return res; + + res = fdt_property_cell(fdt, "#address-cells", addrcells); + if ( res ) + return res; + + res = fdt_property_cell(fdt, "#size-cells", sizecells); + if ( res ) + return res; + + res = make_shm_memory_node(d, fdt, addrcells, sizecells, mem); + if ( res ) + return res; + + res = fdt_end_node(fdt); + + return res; +} + static int __init add_ext_regions(unsigned long s, unsigned long e, void *data) { struct meminfo *ext_regions = data; @@ -3078,6 +3212,11 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; + ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, + &kinfo->shm_mem); + if ( ret ) + goto err; + /* * domain_handle_dtb_bootmodule has to be called before the rest of * the device tree is generated because it depends on the value of @@ -3454,7 +3593,7 @@ static int __init construct_domU(struct domain *d, assign_static_memory_11(d, &kinfo, node); #ifdef CONFIG_STATIC_SHM - rc = process_shm(d, node); + rc = process_shm(d, &kinfo, node); if ( rc < 0 ) return rc; #endif diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index c4dc039b54..2cc506b100 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -19,6 +19,7 @@ struct kernel_info { void *fdt; /* flat device tree */ paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ struct meminfo mem; + struct meminfo shm_mem; /* kernel entry point */ paddr_t entry; diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index 5063e5d077..7497cc40aa 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -29,6 +29,7 @@ struct membank { bool xen_domain; /* whether the memory bank is bound to a Xen domain. */ #ifdef CONFIG_STATIC_SHM unsigned int nr_shm_domain; + uint8_t shm_id ; /* ID identifier of a static shared memory bank. */ #endif };
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.zheng@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@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(-)