[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>, "julien@xxxxxxx" <julien@xxxxxxx>
  • From: Penny Zheng <Penny.Zheng@xxxxxxx>
  • Date: Thu, 7 Jul 2022 04:01:18 +0000
  • Accept-language: en-US
  • Arc-authentication-results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=lists.xenproject.org smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com])
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none
  • Arc-message-signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Cw1yN525yKkhsVbRuy1UxSu0d3aa02fU2gVk7Lvz1Ws=; b=lg1oyb2u3pLrG7XA3Wj+5W/NlHjoSS6b1RIFAQEYF5ciN32uvfMlUCmHNBDTuimUfWQP53K/lHCtr9zsjglotMUvmmwxwaCn7GGnO4Gv2MhhPV5SQqZgIW5m772YsdW87NIa+BuHmTIEe/cyQj6kx+pkiuG9xfyJBggUc5U1K+zBEdcT+KktI+9Dh1RQlzb6eTqE1MS6EDA1jIPuf6KbcUhOaZY7xgGSkDcei9Lpx43c4OWjkM22O6D/B9fJD/2R1zypoY9Eh0XykOCr2xcxecklhDPdAJeHOgpyfDKdOKZoHj8vTjO+NJMYuh1604VYmRCT7REBArzjXMdPiRuqCw==
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Cw1yN525yKkhsVbRuy1UxSu0d3aa02fU2gVk7Lvz1Ws=; b=ASOQ4acl+4Xz7zh0ZK2BVFQaT74Jo8tT3cuznptW3NMq6LPvJ8iPZ6WLd22HkLAF5+/5iQOgkMt2vUtUxPdgnuPxQJG2wVVTCsm3+ZSgZ4lW+INoJRWkDW/ru5PZARqYMsjetFIS0DEIF1sGCpn2urBZdvDxydClads9IBRo6ww8IyY8l/TpBF+i+wEbpXEwg0P4TjtlaS5wGLkdyGOKaa3zmUqOtRI/VLoE+tiQN3o7lvHOJokG/+RHrzFzMGVpj0xFXF5zjGjj2fPLv/6G1GnbRrvvoSd2AwzVNyucYPVKvi6Askui7TzjbUcFjC+Mwcvk/2pAr7y7LUX/GHXubg==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=QGmKY5H+mdZFoWCA5CFlF4zOnUGsFzBxU1Vhj+edtAsjr0EMsJ58ZqfMkcDiF+bZb3Gv2UqG3Xem/g5LUdT8la+M4NxPwl2rsyTtDb7oy1OBuivfc3YuT6gohmxlz2TixzmKABDRRYTYnoirfln6J4BmVyU65c0GumsUi1dPWC2xbU2ZnpXmo2IxN990+58+d8YuKgstADiWF3/rKV/p0L23gGlHQdeCRyY8gCMKqSFfZ9I9/+EVE3SgCHbLCcgEtJGNPZy+5OsAORFSmi83s6wGbl13/MCgf9AdbshpaA+4w6kl8PwggibGyIiCpMZNlKkBAQm8yGQEVOkZWi2Y1g==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YaLvW58dp72jygXd1CLvGKGAiAVrjqqIUBZREhqR+SpjlRs1oIBA+pSHsmnIB4OVIMrQoyi7mG3y9vpUg4h3zhjFdGlVonMEHG/bn5Dj6TlC7KI8VGtOMcnQYVIrPACROgFt8lyx2oyQJGUFCJ+AtAnhkCmhg0/rSMdNY624uqZB+M8KVJL+jH7UOO1iiIIQVb0s7bti07EIOIzs/bUT9EsuUMX2zUvRZiVPEsrqkLQuMLu6XPEFo7IjMl0ornk5UbfOJ2t/ajFsHSABlz+bdUMnyPBu/jYjLqGuXskn+F91xGHzR5uf+XhYFbEwcK+WEibo6B5gZjfoQAK/l+78Mg==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Julien Grall <julien@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Chen <Wei.Chen@xxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Thu, 07 Jul 2022 04:01:49 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
  • Nodisclaimer: true
  • Original-authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Thread-index: AQHYhGRGiM2+sN9ON0uL+hMDdx46E61e+NuAgAApAYCADsZBcIAENhcAgAAuPSA=
  • Thread-topic: [PATCH v5 7/8] xen/arm: create shared memory nodes in guest device tree

Hi Stefano and julien

> -----Original Message-----
> From: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Sent: Thursday, July 7, 2022 7:53 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>; Julien Grall 
> <julien@xxxxxxx>;
> xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Chen <Wei.Chen@xxxxxxx>; Bertrand
> Marquis <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> <Volodymyr_Babchuk@xxxxxxxx>
> 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@xxxxxxxxxx>
> > > Sent: Saturday, June 25, 2022 5:57 AM
> > > To: Julien Grall <julien@xxxxxxx>
> > > Cc: Penny Zheng <Penny.Zheng@xxxxxxx>;
> > > xen-devel@xxxxxxxxxxxxxxxxxxxx; Wei Chen <Wei.Chen@xxxxxxx>;
> Stefano
> > > Stabellini <sstabellini@xxxxxxxxxx>; Bertrand Marquis
> > > <Bertrand.Marquis@xxxxxxx>; Volodymyr Babchuk
> > > <Volodymyr_Babchuk@xxxxxxxx>
> > > 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@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.
> >
> > 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...



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.