[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |