|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 03/11] xen/arm: re-define a set of data structures for static shared memory region
Hi Penny,
On 06/12/2023 10:06, Penny Zheng wrote:
>
>
> This commit introduces a set of separate data structures to deal with
> static shared memory at different stages.
>
> In boot-time host device tree parsing, we introduce a new structure
> "struct shm_node" and a new field "shminfo" in bootinfo to describe and
> store parsed shm info.
>
> In acquire_nr_borrower_domain, it is better to use SHMID as unique identifier
> to iterate "shminfo", other than address and size.
>
> In the last, a new anonymized structure "shminfo", which is a array of
> compound structure that contains SHMID and a "struct membank membank"
> describing shared memory regions in guest address space, is created in "kinfo"
> when dealing with domain information.
This commit msg describes what the patch does but not why.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
> v1 -> v2:
> - As the original "struct shm_membank" was making reserving memory more
> complex and actually memory information could be still got from host Device\
> Tree when dealing with domain construction, we introduce a new simple
> structure
> "struct shm_node" in bootinfo to only store SHMID and "nr_borrowers"
> - Further restrict the scope of the local variable
> "struct meminfo *mem = &bootinfo.reserved_mem"
> - Introduce a new local global data "shm_data" in bootfdt.c. In which,
> reserved
> memory bank is recorded together with the shm node, to assist doing shm node
> verification.
> - Define a set of local variables that point to
> "shm_data.shm_nodes[i].membank->start", etc, to make the code more readable.
> - Use SHMID to iterate "shminfo" to find requested shm node, as we no
> longer store host memory bank info in shm node.
> - A new anonymized structure, which is a array of compound structure that
> contains SHMID and a "struct membank membank", describing shared memory region
> in guest, is introduced in "kinfo".
> ---
> v2 -> v3:
> - rebase and no changes
> ---
> v3 -> v4:
> rebase and no change
> ---
> v4 -> v5:
> - With all shm-related functions classified into static-shmem.c, there
> is no need to import local global data "shm_data".
> ---
> xen/arch/arm/dom0less-build.c | 3 +-
> xen/arch/arm/domain_build.c | 3 +-
> xen/arch/arm/include/asm/kernel.h | 9 +-
> xen/arch/arm/include/asm/setup.h | 24 +++++-
> xen/arch/arm/include/asm/static-shmem.h | 4 +-
> xen/arch/arm/static-shmem.c | 104 ++++++++++++++----------
> 6 files changed, 92 insertions(+), 55 deletions(-)
>
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index fb63ec6fd1..ac096fa3fa 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -645,8 +645,7 @@ static int __init prepare_dtb_domU(struct domain *d,
> struct kernel_info *kinfo)
> if ( ret )
> goto err;
>
> - ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
> - &kinfo->shm_mem);
> + ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, kinfo);
> if ( ret )
> goto err;
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 613b2885ce..64ae944431 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1767,8 +1767,7 @@ static int __init handle_node(struct domain *d, struct
> kernel_info *kinfo,
> return res;
> }
>
> - res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
> - &kinfo->shm_mem);
> + res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells,
> kinfo);
> if ( res )
> return res;
> }
> diff --git a/xen/arch/arm/include/asm/kernel.h
> b/xen/arch/arm/include/asm/kernel.h
> index 0a23e86c2d..db3d8232fa 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -39,7 +39,14 @@ struct kernel_info {
> void *fdt; /* flat device tree */
> paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */
> struct meminfo mem;
> - struct meminfo shm_mem;
> + /* Static shared memory banks */
> + struct {
> + unsigned int nr_banks;
> + struct {
> + char shm_id[MAX_SHM_ID_LENGTH];
> + struct membank membank;
> + } bank[NR_MEM_BANKS];
> + } shminfo;
AFAICT, the only user of this structure is static-shmem.c so why not protecting
it with #ifdef?
>
> /* kernel entry point */
> paddr_t entry;
> diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> index d15a88d2e0..3a2b35ea46 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -50,10 +50,6 @@ struct membank {
> paddr_t start;
> paddr_t size;
> enum membank_type type;
> -#ifdef CONFIG_STATIC_SHM
> - char shm_id[MAX_SHM_ID_LENGTH];
> - unsigned int nr_shm_borrowers;
> -#endif
> };
>
> struct meminfo {
> @@ -95,6 +91,17 @@ struct bootcmdlines {
> struct bootcmdline cmdline[MAX_MODULES];
> };
>
> +#ifdef CONFIG_STATIC_SHM
> +/*
> + * struct shm_node represents a static shared memory node shared between
> + * multiple domains, identified by the unique SHMID("xen,shm-id").
> + */
> +struct shm_node {
> + char shm_id[MAX_SHM_ID_LENGTH];
> + unsigned int nr_shm_borrowers;
> +};
> +#endif
> +
> struct bootinfo {
> struct meminfo mem;
> /* The reserved regions are only used when booting using Device-Tree */
> @@ -105,6 +112,15 @@ struct bootinfo {
> struct meminfo acpi;
> #endif
> bool static_heap;
> +#ifdef CONFIG_STATIC_SHM
> + struct {
> + unsigned int nr_nodes;
> + struct {
> + struct shm_node node;
> + const struct membank *membank;
> + } shm_nodes[NR_MEM_BANKS];
> + } shminfo;
I find it a bit confusing to have 2 structures named exactly the same (here and
in kinfo).
Something like shminfo_nodes would be better.
Also, correct me if I'm wrong. The reason for this structure is to avoid
keeping membank growing?
> +#endif
> };
>
> struct map_range_data
> diff --git a/xen/arch/arm/include/asm/static-shmem.h
> b/xen/arch/arm/include/asm/static-shmem.h
> index 1536ff18b8..66a3f4c146 100644
> --- a/xen/arch/arm/include/asm/static-shmem.h
> +++ b/xen/arch/arm/include/asm/static-shmem.h
> @@ -8,7 +8,7 @@
> #ifdef CONFIG_STATIC_SHM
>
> int make_resv_memory_node(const struct domain *d, void *fdt, int addrcells,
> - int sizecells, const struct meminfo *mem);
> + int sizecells, const struct kernel_info *kinfo);
>
> int process_shm(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node);
> @@ -28,7 +28,7 @@ int process_shm_node(const void *fdt, int node, uint32_t
> address_cells,
>
> static inline int make_resv_memory_node(const struct domain *d, void *fdt,
> int addrcells, int sizecells,
> - const struct meminfo *mem)
> + const struct kernel_info *kinfo)
> {
> return 0;
> }
> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
> index 1a1a9386e4..6a3d8a54bd 100644
> --- a/xen/arch/arm/static-shmem.c
> +++ b/xen/arch/arm/static-shmem.c
> @@ -6,28 +6,25 @@
> #include <asm/domain_build.h>
> #include <asm/static-shmem.h>
>
> -static int __init acquire_nr_borrower_domain(struct domain *d,
> - paddr_t pbase, paddr_t psize,
> +static int __init acquire_nr_borrower_domain(const char *shm_id,
> unsigned long *nr_borrowers)
Why is nr_borrowers unsigned long but nr_shm_borrowers is unsigned int?
> {
> - unsigned int bank;
> + struct shm_node *shm_node;
Can be const
> + unsigned int i;
>
> - /* Iterate reserved memory to find requested shm bank. */
> - for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ )
> + /* Iterate to find requested static shared memory node. */
> + for ( i = 0; i < bootinfo.shminfo.nr_nodes; i++ )
> {
> - paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start;
> - paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size;
> + shm_node = &bootinfo.shminfo.shm_nodes[i].node;
>
> - if ( (pbase == bank_start) && (psize == bank_size) )
> - break;
> + if ( strcmp(shm_id, shm_node->shm_id) == 0 )
> + {
> + *nr_borrowers = shm_node->nr_shm_borrowers;
> + return 0;
> + }
> }
>
> - if ( bank == bootinfo.reserved_mem.nr_banks )
> - return -ENOENT;
> -
> - *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers;
> -
> - return 0;
> + return -ENOENT;
> }
>
> /*
> @@ -91,7 +88,7 @@ static mfn_t __init acquire_shared_memory_bank(struct
> domain *d,
>
> static int __init assign_shared_memory(struct domain *d,
> paddr_t pbase, paddr_t psize,
> - paddr_t gbase)
> + paddr_t gbase, const char *shm_id)
> {
> mfn_t smfn;
> int ret = 0;
> @@ -125,7 +122,7 @@ static int __init assign_shared_memory(struct domain *d,
> * Get the right amount of references per page, which is the number of
> * borrower domains.
> */
> - ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers);
> + ret = acquire_nr_borrower_domain(shm_id, &nr_borrowers);
> if ( ret )
> return ret;
>
> @@ -161,13 +158,16 @@ static int __init append_shm_bank_to_domain(struct
> kernel_info *kinfo,
> paddr_t start, paddr_t size,
> const char *shm_id)
> {
> - if ( kinfo->shm_mem.nr_banks >= NR_MEM_BANKS )
> + unsigned int nr_banks = kinfo->shminfo.nr_banks;
> + struct membank *membank = &kinfo->shminfo.bank[nr_banks].membank;
> +
> + if ( nr_banks >= 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;
> - safe_strcpy(kinfo->shm_mem.bank[kinfo->shm_mem.nr_banks].shm_id, shm_id);
> - kinfo->shm_mem.nr_banks++;
> + membank->start = start;
> + membank->size = size;
> + safe_strcpy(kinfo->shminfo.bank[nr_banks].shm_id, shm_id);
> + kinfo->shminfo.nr_banks++;
>
> return 0;
> }
> @@ -251,7 +251,7 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
> * specified, so they should be assigned to dom_io.
> */
> ret = assign_shared_memory(owner_dom_io ? dom_io : d,
> - pbase, psize, gbase);
> + pbase, psize, gbase, shm_id);
> if ( ret )
> return ret;
> }
> @@ -279,12 +279,12 @@ int __init process_shm(struct domain *d, struct
> kernel_info *kinfo,
>
> static int __init make_shm_memory_node(const struct domain *d, void *fdt,
> int addrcells, int sizecells,
> - const struct meminfo *mem)
> + const struct kernel_info *kinfo)
> {
> unsigned int i = 0;
> int res = 0;
>
> - if ( mem->nr_banks == 0 )
> + if ( kinfo->shminfo.nr_banks == 0 )
> return -ENOENT;
>
> /*
> @@ -294,17 +294,17 @@ static int __init make_shm_memory_node(const struct
> domain *d, void *fdt,
> */
> dt_dprintk("Create xen-shmem node\n");
>
> - for ( ; i < mem->nr_banks; i++ )
> + for ( ; i < kinfo->shminfo.nr_banks; i++ )
> {
> - uint64_t start = mem->bank[i].start;
> - uint64_t size = mem->bank[i].size;
> + uint64_t start = kinfo->shminfo.bank[i].membank.start;
> + uint64_t size = kinfo->shminfo.bank[i].membank.size;
> const char compat[] = "xen,shared-memory-v1";
> /* Worst case addrcells + sizecells */
> __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
> __be32 *cells;
> unsigned int len = (addrcells + sizecells) * sizeof(__be32);
>
> - res = domain_fdt_begin_node(fdt, "xen-shmem", mem->bank[i].start);
> + res = domain_fdt_begin_node(fdt, "xen-shmem", start);
> if ( res )
> return res;
>
> @@ -322,7 +322,7 @@ static int __init make_shm_memory_node(const struct
> domain *d, void *fdt,
> dt_dprintk("Shared memory bank %u: %#"PRIx64"->%#"PRIx64"\n",
> i, start, start + size);
>
> - res = fdt_property_string(fdt, "xen,id", mem->bank[i].shm_id);
> + res = fdt_property_string(fdt, "xen,id",
> kinfo->shminfo.bank[i].shm_id);
> if ( res )
> return res;
>
> @@ -350,7 +350,6 @@ int __init process_shm_node(const void *fdt, int node,
> uint32_t address_cells,
> const struct fdt_property *prop, *prop_id, *prop_role;
> const __be32 *cell;
> paddr_t paddr, gaddr, size, end;
> - struct meminfo *mem = &bootinfo.reserved_mem;
> unsigned int i;
> int len;
> bool owner = false;
> @@ -429,17 +428,21 @@ int __init process_shm_node(const void *fdt, int node,
> uint32_t address_cells,
> return -EINVAL;
> }
>
> - for ( i = 0; i < mem->nr_banks; i++ )
> + for ( i = 0; i < bootinfo.shminfo.nr_nodes; i++ )
> {
> + paddr_t bank_start = bootinfo.shminfo.shm_nodes[i].membank->start;
> + paddr_t bank_size = bootinfo.shminfo.shm_nodes[i].membank->size;
> + const char *bank_id = bootinfo.shminfo.shm_nodes[i].node.shm_id;
> +
> /*
> * Meet the following check:
> * 1) The shm ID matches and the region exactly match
> * 2) The shm ID doesn't match and the region doesn't overlap
> * with an existing one
> */
> - if ( paddr == mem->bank[i].start && size == mem->bank[i].size )
> + if ( paddr == bank_start && size == bank_size )
> {
> - if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) ==
> 0 )
> + if ( strncmp(shm_id, bank_id, MAX_SHM_ID_LENGTH) == 0 )
In following else if you still use mem->bank[i].shm_id which results in a build
failure.
You should use bank_id there as well.
~Michal
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |