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

RE: [PATCH v5 1/8] xen/arm: introduce static shared memory


  • To: Julien Grall <julien@xxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Penny Zheng <Penny.Zheng@xxxxxxx>
  • Date: Wed, 29 Jun 2022 08:39:42 +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=ThyY9lgDddYrBpL/j+Izlw3fl/9L0beLfZ8UUZAG4+s=; b=F6n3rpZxj+ZeibgnYD5ZZACtQFMrHCmRAJQ4DtjJ62gLuwUxsC66FejSwoigNKpJ8KNz3W4j6UBWk5XV5IzG3vICEB8wdPguWAe4CY8cW3BdHODjsAl25dGL1eJgYuAOyb1H+yLWKYHZmLZxF/B52ULr8g7UTgmpUbgBGNKMU+4X3FI0A5c8WUy4cCZRs7OVQST8rfWC1PaGFk6QBlujZCR43XDH8rCs86thU+2Jca799YDaKRI1V6pRKD9XP8WN35mUxqcrc/iUrLusXc+EOMYjHCqrLXQ2cD2hwpMAPk1rl8xQy5MUh5ZL2sWjcHIyAy6/OKA8Yxum0sOpInbHPQ==
  • 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=ThyY9lgDddYrBpL/j+Izlw3fl/9L0beLfZ8UUZAG4+s=; b=HRkWV39I74WGOe0WFgibqjBwtaao0S+FeaUghnte2bxds5u6x3fvNGmjcYRiuMoFCbpALcmNkFTywwIsd1lr8vZjfY+pN4SJDLFZDfSIjfglEulXaKPcmOjDOyAWR25BOtNGn1yKJCgmOArXyvw5RHrM/mkP2LjN8c1XmowjCXT17iryXMLbS6gRTYriBICPyQy6JwvDVktgqNeQLossF7WH4Yk+3chIDZcflYJJWfZGPgYqD5oeYsBEh3tOP1PkstP6alprGtD5Ec2GZEQGSXajt/o+RzY3JunnR6rAXXtWJfV9nV1W50/B0jEsnCoaatlQBRoyecrroC8/lFVXRA==
  • Arc-seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=XaShdd57gP/hjaE+l3f+H7CcnXHEbcRWfUhrFRBSoe3eR4S/3zrxlFL0RkrpLPymHSwK42XerXrHYFr3NT1Z5NzidwiUJNpExgCklDRWk6BV+kfc1i3geyCqaZgY/vGIYrQc7m1lPmizqxVxKu2PggYQgO5TCicGwc8XuT4yNDmLC25dOtFymXBhGeXVzg62/x31e/i0ernlVo/DZbukOw1E2X0hoW9U83VT06jQPW0fB+6k6CpMt20LwWBEwaSxms9ZT6HosREuJsS+sibeKIDcCtB0P6iMkmx8/oIm7on4tXJyt3r0cNmonKxZOzQ1KYDKLlI5/T/Xt/frSBYG2A==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=fcE0XaGRs+CkJjC1Ec3HjYTFVDU+1x7+b9GgTDyxtRiLXcRWb9CFNRqZEVa+LRWS5sC+W4qkXStaNQi3+YMX3gEWeBqikrRmiXihtkAckZY81n77Osc11kf4Z9XdIC1+x19mKmpXeIbISmsgLJQ8vT2eDY86UWVZ1SvEe1a/oQ3IQao+4PMQU4K8Vbw40w2EajJPi4likPwfA0DBi6eC+G0YXoSJSOV+It2j2Sm9U0yAQFpSdZiJI+r4uA1YOUCgm3U8jLP1mfDbM78EMVKNyp/x1OWjWv0/nLE3pjoaXEPdphwQ+NzI9hLr3zOrG1cSp8mNVBQg/7ylsT2PqrGxyA==
  • Authentication-results-original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com;
  • Cc: Wei Chen <Wei.Chen@xxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, Bertrand Marquis <Bertrand.Marquis@xxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
  • Delivery-date: Wed, 29 Jun 2022 08:40:04 +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: AQHYhGRAIMWAAZoglEyYz3pT8Ubq/a1e3kuAgAc4ucA=
  • Thread-topic: [PATCH v5 1/8] xen/arm: introduce static shared memory

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, June 25, 2022 1:55 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> Subject: Re: [PATCH v5 1/8] xen/arm: introduce static shared memory
> 
> Hi Penny,
> 
> On 20/06/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@xxxxxxx>
> >
> > This patch serie introduces a new feature: setting up static
> 
> Typo: s/serie/series/
> 
> > shared memory on a dom0less system, through device tree configuration.
> >
> > This commit parses shared memory node at boot-time, and reserve it in
> > bootinfo.reserved_mem to avoid other use.
> >
> > This commits proposes a new Kconfig CONFIG_STATIC_SHM to wrap
> > static-shm-related codes, and this option depends on static memory(
> > CONFIG_STATIC_MEMORY). That's because that later we want to reuse a
> > few helpers, guarded with CONFIG_STATIC_MEMORY, like
> > acquire_staticmem_pages, etc, on static shared memory.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> > v5 change:
> > - no change
> > ---
> > v4 change:
> > - nit fix on doc
> > ---
> > v3 change:
> > - make nr_shm_domain unsigned int
> > ---
> > v2 change:
> > - document refinement
> > - remove bitmap and use the iteration to check
> > - add a new field nr_shm_domain to keep the number of shared domain
> > ---
> >   docs/misc/arm/device-tree/booting.txt | 120
> ++++++++++++++++++++++++++
> >   xen/arch/arm/Kconfig                  |   6 ++
> >   xen/arch/arm/bootfdt.c                |  68 +++++++++++++++
> >   xen/arch/arm/include/asm/setup.h      |   3 +
> >   4 files changed, 197 insertions(+)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 98253414b8..6467bc5a28 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -378,3 +378,123 @@ device-tree:
> >
> >   This will reserve a 512MB region starting at the host physical address
> >   0x30000000 to be exclusively used by DomU1.
> > +
> > +Static Shared Memory
> > +====================
> > +
> > +The static shared memory device tree nodes allow users to statically
> > +set up shared memory on dom0less system, enabling domains to do
> > +shm-based communication.
> > +
> > +- compatible
> > +
> > +    "xen,domain-shared-memory-v1"
> > +
> > +- xen,shm-id
> > +
> > +    An 8-bit integer that represents the unique identifier of the shared
> memory
> > +    region. The maximum identifier shall be "xen,shm-id = <0xff>".
> > +
> > +- xen,shared-mem
> > +
> > +    An array takes a physical address, which is the base address of the
> > +    shared memory region in host physical address space, a size, and a
> guest
> > +    physical address, as the target address of the mapping. The number of
> cells
> > +    for the host address (and size) is the same as the guest 
> > pseudo-physical
> > +    address and they are inherited from the parent node.
> 
> Sorry for jump in the discussion late. But as this is going to be a stable 
> ABI, I
> would to make sure the interface is going to be easily extendable.
> 
> AFAIU, with your proposal the host physical address is mandatory. I would
> expect that some user may want to share memory but don't care about the
> exact location in memory. So I think it would be good to make it optional in
> the binding.
> 
> I think this wants to be done now because it would be difficult to change the
> binding afterwards (the host physical address is the first set of cells).
> 
> The Xen doesn't need to handle the optional case.
> 
> [...]
> 
> > diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig index
> > be9eff0141..7321f47c0f 100644
> > --- a/xen/arch/arm/Kconfig
> > +++ b/xen/arch/arm/Kconfig
> > @@ -139,6 +139,12 @@ config TEE
> >
> >   source "arch/arm/tee/Kconfig"
> >
> > +config STATIC_SHM
> > +   bool "Statically shared memory on a dom0less system" if
> UNSUPPORTED
> 
> You also want to update SUPPORT.md.
> 
> > +   depends on STATIC_MEMORY
> > +   help
> > +     This option enables statically shared memory on a dom0less system.
> > +
> >   endmenu
> >
> >   menu "ARM errata workaround via the alternative framework"
> > diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index
> > ec81a45de9..38dcb05d5d 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -361,6 +361,70 @@ static int __init process_domain_node(const void
> *fdt, int node,
> >                                      size_cells, &bootinfo.reserved_mem, 
> > true);
> >   }
> >
> > +#ifdef CONFIG_STATIC_SHM
> > +static int __init process_shm_node(const void *fdt, int node,
> > +                                   u32 address_cells, u32 size_cells)
> > +{
> > +    const struct fdt_property *prop;
> > +    const __be32 *cell;
> > +    paddr_t paddr, size;
> > +    struct meminfo *mem = &bootinfo.reserved_mem;
> > +    unsigned long i;
> 
> nr_banks is "unsigned int" so I think this should be "unsigned int" as well.
> 
> > +
> > +    if ( address_cells < 1 || size_cells < 1 )
> > +    {
> > +        printk("fdt: invalid #address-cells or #size-cells for static 
> > shared
> memory node.\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    prop = fdt_get_property(fdt, node, "xen,shared-mem", NULL);
> > +    if ( !prop )
> > +        return -ENOENT;
> > +
> > +    /*
> > +     * xen,shared-mem = <paddr, size, gaddr>;
> > +     * Memory region starting from physical address #paddr of #size shall
> > +     * be mapped to guest physical address #gaddr as static shared memory
> > +     * region.
> > +     */
> > +    cell = (const __be32 *)prop->data;
> > +    device_tree_get_reg(&cell, address_cells, size_cells, &paddr,
> > + &size);
> 
> Please check the len of the property to confirm is it big enough to contain
> "paddr", "size", and "gaddr".
> 
> > +    for ( i = 0; i < mem->nr_banks; i++ )
> > +    {
> > +        /*
> > +         * A static shared memory region could be shared between multiple
> > +         * domains.
> > +         */
> > +        if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> > +            break;

Maybe I need to add a check on shm-id:
"
        /*
         * A static shared memory region could be shared between multiple
         * domains.
         */
        if ( strcmp(shm_id, mem->bank[i].shm_id) == 0 )
        {
            if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
                break;
            else
            {
                printk("Warning: xen,shm-id %s does not match for all the nodes 
using the same region.\n",
                       shm_id);
                return -EINVAL;
            }
        }
"
Wdyt?

> > +    }
> > +
> > +    if ( i == mem->nr_banks )
> > +    {
> > +        if ( i < NRshm_MEM_BANKS )
> > +        {
> > +            /* Static shared memory shall be reserved from any other use. 
> > */
> > +            mem->bank[mem->nr_banks].start = paddr;
> > +            mem->bank[mem->nr_banks].size = size;
> > +            mem->bank[mem->nr_banks].xen_domain = true;
> > +            mem->nr_banks++;
> > +        }
> > +        else
> > +        {
> > +            printk("Warning: Max number of supported memory regions
> reached.\n");
> > +            return -ENOSPC;
> > +        }
> > +    }
> > +    /*
> > +     * keep a count of the number of domains, which later may be used to
> > +     * calculate the number of the reference count.
> > +     */
> > +    mem->bank[i].nr_shm_domain++;
> > +
> > +    return 0;
> > +}
> > +#endif
> > +
> >   static int __init early_scan_node(const void *fdt,
> >                                     int node, const char *name, int depth,
> >                                     u32 address_cells, u32 size_cells,
> > @@ -386,6 +450,10 @@ static int __init early_scan_node(const void *fdt,
> >           process_chosen_node(fdt, node, name, address_cells, size_cells);
> >       else if ( depth == 2 && device_tree_node_compatible(fdt, node,
> "xen,domain") )
> >           rc = process_domain_node(fdt, node, name, address_cells,
> > size_cells);
> > +#ifdef CONFIG_STATIC_SHM
> > +    else if ( depth <= 3 && device_tree_node_compatible(fdt, node,
> "xen,domain-shared-memory-v1") )
> > +        rc = process_shm_node(fdt, node, address_cells, size_cells);
> > +#endif
> >
> >       if ( rc < 0 )
> >           printk("fdt: node `%s': parsing failed\n", name); diff --git
> > a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> > index 2bb01ecfa8..5063e5d077 100644
> > --- a/xen/arch/arm/include/asm/setup.h
> > +++ b/xen/arch/arm/include/asm/setup.h
> > @@ -27,6 +27,9 @@ 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
> > +    unsigned int nr_shm_domain;
> > +#endif
> >   };
> >
> >   struct meminfo {
> 
> Cheers,
> 
> --
> Julien Grall

 


Rackspace

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