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

[Julien Grall <julien@xxxxxxx>]

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;
> +    }
> +
> +    if ( i == mem->nr_banks )
> +    {
> +        if ( i < NR_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