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

[Penny Zheng <Penny.Zheng@arm.com>]

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