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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> > 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.