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

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

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, June 25, 2022 1:55 AM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org
> Cc: 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 1/8] xen/arm: introduce static shared memory
> 
> Hi Penny,
> 
> On 20/06/2022 06:11, Penny Zheng wrote:
> > From: Penny Zheng <penny.zheng@arm.com>
> >
> > 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@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > 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