| Message ID | 20231206090623.1932275-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory PART I | expand |
Hi Penny, On 06/12/2023 10:06, Penny Zheng wrote: > > > We use paddr_assigned to indicate whether host address is provided, by > checking the length of "xen,shared-mem" property. > > The shm matching criteria shall also be adapt to cover the new scenario, by > adding when host address is not provided, if SHMID matches, the region size > must exactly match too. > > During domain creation, right now, a static shared memory node could be > banked with a statically configured host memory bank, or arbitrary > host memory of dedicated size, which will later be allocated from heap by Xen. > To cover both scenarios, we create a new structure shm_meminfo. It is very > alike meminfo, but with the maximum array size being a smaller number > NR_SHM_BANKS(16). > As "shm_meminfo" is also a new member of "enum meminfo_type", we shall implement > its own callback "retrieve_shm_meminfo" to have access to all MACRO > helpers, e.g. GET_MEMBANK(...). Previous comments apply here as well and this patch depends on the decision of others \wrt previous patch. I'll just add generic remarks. > > Also, to make codes tidy and clear, we extract codes about parsing > "xen,shared-mem" property from function "process_shm" and move them into > a new helper "parse_shm_property". > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > v1 -> v2 > - In order to get allocated banked host memory info during domain creat ion, > we create a new structure shm_meminfo. It is very alike meminfo, with > the maximum array size being NR_SHM_BANKS. As shm_meminfo is a new > member of type meminfo_type, we shall implement its own callback > retrieve_shm_meminfo to have access to all MACRO helpers, e.g. > GET_MEMBANK(...) > - rename "acquire_shm_memnode" to "find_shm_memnode" > --- > v2 -> v3 > - rebase and no changes > --- > v3 -> v4: > - rebase and no change > --- > v4 -> v5: > - fix bugs of that tot_size and membank shall not be member of union, > but struct, to differentiate two types of static shared memory node. > --- > xen/arch/arm/domain_build.c | 3 + > xen/arch/arm/include/asm/setup.h | 14 +- > xen/arch/arm/include/asm/static-shmem.h | 3 + > xen/arch/arm/static-shmem.c | 360 ++++++++++++++++++------ > 4 files changed, 293 insertions(+), 87 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index a8bc78baa5..c69d481d34 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -70,6 +70,9 @@ static void __init retrieve_meminfo(void *mem, unsigned int *max_mem_banks, > > retrieve_fn __initdata retrievers[MAX_MEMINFO_TYPE] = { > [NORMAL_MEMINFO] = retrieve_meminfo, > +#ifdef CONFIG_STATIC_SHM > + [SHM_MEMINFO] = retrieve_shm_meminfo, > +#endif > }; > #endif > > diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h > index bc5f08be97..043588cd2d 100644 > --- a/xen/arch/arm/include/asm/setup.h > +++ b/xen/arch/arm/include/asm/setup.h > @@ -59,6 +59,9 @@ struct meminfo { > > enum meminfo_type { > NORMAL_MEMINFO, > +#ifdef CONFIG_STATIC_SHM > + SHM_MEMINFO, > +#endif > MAX_MEMINFO_TYPE, > }; > > @@ -150,7 +153,16 @@ struct bootinfo { > unsigned int nr_nodes; > struct { > struct shm_node node; > - const struct membank *membank; > + /* > + * For a static shared memory node, it is either banked with > + * a statically configured host memory bank, or arbitrary host > + * memory which will be allocated by Xen with a specified total > + * size(tot_size). > + */ > + struct { > + const struct membank *membank; > + paddr_t tot_size; > + }; > } shm_nodes[NR_MEM_BANKS]; > } shminfo; > #endif > diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h > index 66a3f4c146..a67445cec8 100644 > --- a/xen/arch/arm/include/asm/static-shmem.h > +++ b/xen/arch/arm/include/asm/static-shmem.h > @@ -24,6 +24,9 @@ static inline int process_shm_chosen(struct domain *d, > int process_shm_node(const void *fdt, int node, uint32_t address_cells, > uint32_t size_cells); > > +void retrieve_shm_meminfo(void *mem, unsigned int *max_mem_banks, > + struct membank **bank, > + unsigned int **nr_banks); > #else /* !CONFIG_STATIC_SHM */ > > static inline int make_resv_memory_node(const struct domain *d, void *fdt, > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index 6a3d8a54bd..a9eb26d543 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -6,6 +6,50 @@ > #include <asm/domain_build.h> > #include <asm/static-shmem.h> > > +#define NR_SHM_BANKS 16 > + > +/* > + * There are two types of static shared memory node: > + * A static shared memory node could be banked with a statically > + * configured host memory bank, or a set of arbitrary host memory > + * banks allocated from heap by Xen on runtime. > + */ > +struct shm_meminfo { > + unsigned int nr_banks; > + struct membank bank[NR_SHM_BANKS]; > + paddr_t tot_size; > +}; > + > +/* > + * struct shm_memnode holds banked host memory info for a static > + * shared memory node > + */ > +struct shm_memnode { With the number of structures introduced in this series, the chosen naming does not help > + char shm_id[MAX_SHM_ID_LENGTH]; > + struct shm_meminfo meminfo; > +}; > + > +static __initdata struct { > + unsigned int nr_nodes; > + struct shm_memnode node[NR_MEM_BANKS]; > +} shm_memdata; > + > +void __init retrieve_shm_meminfo(void *mem, unsigned int *max_mem_banks, > + struct membank **bank, > + unsigned int **nr_banks) > +{ > + struct shm_meminfo *meminfo = (struct shm_meminfo *)mem; > + > + if ( max_mem_banks ) > + *max_mem_banks = NR_SHM_BANKS; > + > + if ( nr_banks ) > + *nr_banks = &(meminfo->nr_banks); > + > + if ( bank ) > + *bank = meminfo->bank; > +} > + > static int __init acquire_nr_borrower_domain(const char *shm_id, > unsigned long *nr_borrowers) > { > @@ -172,6 +216,129 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo, > return 0; > } > > +static struct shm_memnode * __init find_shm_memnode(const char *shm_id) > +{ > + unsigned int i; > + struct shm_memnode *shm_memnode; > + > + for ( i = 0 ; i < shm_memdata.nr_nodes; i++ ) > + { > + shm_memnode = &shm_memdata.node[i]; > + > + if ( strcmp(shm_id, shm_memnode->shm_id) == 0 ) > + return shm_memnode; > + } > + > + if ( i == NR_MEM_BANKS ) > + return NULL; > + We can only be here if i == 0 i.e. no shm nodes? > + shm_memnode = &shm_memdata.node[i]; > + safe_strcpy(shm_memnode->shm_id, shm_id); > + shm_memdata.nr_nodes++; leave one empty line here, please > + return shm_memnode; > +} > + > +/* Parse "xen,shared-mem" property in static shared memory node */ > +static struct shm_memnode * __init parse_shm_property(struct domain *d, > + const struct dt_device_node *dt_node, > + bool *paddr_assigned, paddr_t *gbase, > + const char *shm_id) > +{ > + uint32_t addr_cells, size_cells; > + const struct dt_property *prop; > + const __be32 *cells; > + uint32_t len; > + unsigned int i; > + paddr_t pbase, psize; > + struct shm_memnode *shm_memnode; > + > + /* xen,shared-mem = <pbase, gbase, size>; And pbase could be optional. */ > + prop = dt_find_property(dt_node, "xen,shared-mem", &len); > + BUG_ON(!prop); > + cells = (const __be32 *)prop->value; > + > + addr_cells = dt_n_addr_cells(dt_node); > + size_cells = dt_n_size_cells(dt_node); > + if ( len != dt_cells_to_size(addr_cells + size_cells + addr_cells) ) > + { > + /* pbase is not provided in "xen,shared-mem" */ > + if ( len == dt_cells_to_size(size_cells + addr_cells) ) > + *paddr_assigned = false; what if paddr_assigned is NULL? Also, wouldn't pbase_provided be a better name? > + else > + { > + printk("fdt: invalid `xen,shared-mem` property.\n"); If you are modifying the code anyway, please drop '.' at the end of line. No need for that > + return NULL; > + } > + } > + > + /* > + * If we firstly process the shared memory node with unique "xen,shm-id", > + * we allocate a new member "shm_memnode" for it in shm_memdata. > + */ > + shm_memnode = find_shm_memnode(shm_id); > + BUG_ON(!shm_memnode); > + if ( !(*paddr_assigned) ) > + { > + device_tree_get_reg(&cells, addr_cells, size_cells, gbase, &psize); > + /* Whether it is a new shm node? */ > + if ( shm_memnode->meminfo.tot_size == 0 ) > + goto out_early1; > + else no need for this else > + goto out_early2; > + } > + else no need for this else. You can be here only if pbase was specified > + { > + device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, gbase); > + psize = dt_read_number(cells, size_cells); > + > + /* Whether it is a new shm node? */ > + if ( shm_memnode->meminfo.nr_banks != 0 ) In previous check with the same comment, you use tot_size to determine if it is a new shm mode > + goto out_early2; > + } > + > + /* > + * The static shared memory node #dt_node is banked with a > + * statically configured host memory bank. > + */ > + shm_memnode->meminfo.bank[0].start = pbase; > + shm_memnode->meminfo.bank[0].size = psize; > + shm_memnode->meminfo.nr_banks = 1; You should be first checking for below error conditions before doing assignment? > + > + if ( !IS_ALIGNED(pbase, PAGE_SIZE) ) > + { > + printk("%pd: physical address 0x%"PRIpaddr" is not suitably aligned.\n", > + d, pbase); > + return NULL; > + } > + > + for ( i = 0; i < PFN_DOWN(psize); i++ ) > + if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) > + { > + printk("%pd: invalid physical MFN 0x%"PRI_mfn"\n for static shared memory node\n", > + d, mfn_x(mfn_add(maddr_to_mfn(pbase), i))); > + return NULL; > + } > + > + out_early1: not really informative. How about new_shm_node:? > + if ( !IS_ALIGNED(psize, PAGE_SIZE) ) > + { > + printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n", > + d, psize); > + return NULL; > + } > + shm_memnode->meminfo.tot_size = psize; > + > + out_early2: > + if ( !IS_ALIGNED(*gbase, PAGE_SIZE) ) > + { > + printk("%pd: guest address 0x%"PRIpaddr" is not suitably aligned.\n", > + d, *gbase); > + return NULL; > + } > + > + return shm_memnode; > +} > + > int __init process_shm(struct domain *d, struct kernel_info *kinfo, > const struct dt_device_node *node) > { > @@ -179,51 +346,17 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > > dt_for_each_child_node(node, shm_node) > { > - const struct dt_property *prop; > - const __be32 *cells; > - uint32_t addr_cells, size_cells; > paddr_t gbase, pbase, psize; > int ret = 0; > - unsigned int i; > const char *role_str; > const char *shm_id; > bool owner_dom_io = true; > + bool paddr_assigned = true; wouldn't it be better if parse_shm_property was responsible for setting this to either false or true. At the moment it only sets it to false. > + struct shm_memnode *shm_memnode; > > if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) > continue; > > - /* > - * xen,shared-mem = <pbase, gbase, size>; > - * TODO: pbase is optional. > - */ > - addr_cells = dt_n_addr_cells(shm_node); > - size_cells = dt_n_size_cells(shm_node); > - prop = dt_find_property(shm_node, "xen,shared-mem", NULL); > - BUG_ON(!prop); > - cells = (const __be32 *)prop->value; > - device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase); > - psize = dt_read_paddr(cells, size_cells); > - if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) ) > - { > - printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n", > - d, pbase, gbase); > - return -EINVAL; > - } > - if ( !IS_ALIGNED(psize, PAGE_SIZE) ) > - { > - printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n", > - d, psize); > - return -EINVAL; > - } > - > - for ( i = 0; i < PFN_DOWN(psize); i++ ) > - if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) > - { > - printk("%pd: invalid physical address 0x%"PRI_mfn"\n", > - d, mfn_x(mfn_add(maddr_to_mfn(pbase), i))); > - return -EINVAL; > - } > - > /* > * "role" property is optional and if it is defined explicitly, > * then the owner domain is not the default "dom_io" domain. > @@ -238,6 +371,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > } > BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH)); > > + shm_memnode = parse_shm_property(d, shm_node, &paddr_assigned, &gbase, > + shm_id); > + if ( !shm_memnode ) > + return -EINVAL; empty line here please > + pbase = shm_memnode->meminfo.bank[0].start; > + psize = shm_memnode->meminfo.bank[0].size; > + > /* > * DOMID_IO is a fake domain and is not described in the Device-Tree. > * Therefore when the owner of the shared region is DOMID_IO, we will > @@ -349,10 +489,10 @@ 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; > + paddr_t paddr, gaddr, size; > unsigned int i; > int len; > - bool owner = false; > + bool owner = false, paddr_assigned = true; > const char *shm_id; > > if ( address_cells < 1 || size_cells < 1 ) > @@ -404,96 +544,140 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > > if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) ) > { > + /* paddr is not provided in "xen,shared-mem" */ > if ( len == dt_cells_to_size(size_cells + address_cells) ) > - printk("fdt: host physical address must be chosen by users at the moment.\n"); > - > - printk("fdt: invalid `xen,shared-mem` property.\n"); > - return -EINVAL; > + paddr_assigned = false; > + else > + { > + printk("fdt: invalid `xen,shared-mem` property.\n"); > + return -EINVAL; > + } > } > > cell = (const __be32 *)prop->data; > - device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr); > - size = dt_next_cell(size_cells, &cell); > - > - if ( !size ) > + if ( !paddr_assigned ) > + device_tree_get_reg(&cell, address_cells, size_cells, &gaddr, &size); > + else > { > - printk("fdt: the size for static shared memory region can not be zero\n"); > - return -EINVAL; > - } > + paddr_t end; > + > + device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr); > + size = dt_next_cell(size_cells, &cell); > + > + if ( !size ) > + { > + printk("fdt: the size for static shared memory region can not be zero\n"); > + return -EINVAL; > + } > + > + end = paddr + size; > + if ( end <= paddr ) > + { > + printk("fdt: static shared memory region %s overflow\n", shm_id); > + return -EINVAL; > + } > > - end = paddr + size; > - if ( end <= paddr ) > - { > - printk("fdt: static shared memory region %s overflow\n", shm_id); > - return -EINVAL; > } > > 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; > + bool is_shmid_equal = strncmp(shm_id, bank_id, MAX_SHM_ID_LENGTH) == 0 ? > + true : false; > > /* > * Meet the following check: > + * when host address is provided: > * 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 > + * when host address is not provided: > + * 1) The shm ID matches and the region size exactly match > + */ > + /* > + * For a static shared memory node, it is either banked with > + * a statically configured host memory bank(membank != NULL), or > + * arbitrary host memory which will later be allocated by Xen( > + * tot_size != 0). > */ > - if ( paddr == bank_start && size == bank_size ) > + if ( bootinfo.shminfo.shm_nodes[i].membank != NULL ) > { > - if ( strncmp(shm_id, bank_id, MAX_SHM_ID_LENGTH) == 0 ) > + paddr_t bank_start = bootinfo.shminfo.shm_nodes[i].membank->start; > + paddr_t bank_size = bootinfo.shminfo.shm_nodes[i].membank->size; both lines > 80 chars > + bool is_same_region = (paddr == bank_start) && (size == bank_size) ? > + true : false; > + > + if ( is_same_region && is_shmid_equal ) > break; > - else > + else if ( is_same_region || is_shmid_equal ) > { > printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n", > shm_id); > return -EINVAL; > } continue here and ... > } > - else if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) > - continue; > else no need for this else > { > - printk("fdt: different shared memory region could not share the same shm ID %s\n", > - shm_id); > - return -EINVAL; > + paddr_t tot_size = bootinfo.shminfo.shm_nodes[i].tot_size; > + bool is_same_region = tot_size == size ? true : false; > + > + if ( !paddr_assigned && is_same_region && is_shmid_equal ) > + break; > + else if ( is_shmid_equal ) > + { > + if ( paddr_assigned ) > + { > + printk("fdt: two different types of static shared memory region could not share the same shm-id %s\n", > + shm_id); > + return -EINVAL; > + } > + > + printk("fdt: when host address is not provided, if xen,shm-id matches, size must stay the same too(%"PRIpaddr" -> %"PRIpaddr")\n", > + size, tot_size); > + return -EINVAL; > + } > } > } > > if ( i == bootinfo.shminfo.nr_nodes ) > { > - struct meminfo *mem = &bootinfo.reserved_mem; > - > - if ( (i < NR_MEM_BANKS) && (mem->nr_banks < NR_MEM_BANKS) ) > + if ( i < NR_MEM_BANKS ) if ( i == NR_MEM_BANKS ) goto fail; this would reduce the number of if/else blocks > { > - struct membank *membank = &mem->bank[mem->nr_banks]; > struct shm_node *shm_node = &bootinfo.shminfo.shm_nodes[i].node; > - > - if ( check_reserved_regions_overlap(paddr, size) ) > - return -EINVAL; > - > - /* Static shared memory shall be reserved from any other use. */ > - membank->start = paddr; > - membank->size = size; > - membank->type = MEMBANK_STATIC_DOMAIN; > - mem->nr_banks++; > + struct meminfo *mem = &bootinfo.reserved_mem; > > /* Record static shared memory node info in bootinfo.shminfo */ > safe_strcpy(shm_node->shm_id, shm_id); > - /* > - * Reserved memory bank is recorded together to assist > - * doing shm node verification. > - */ > - bootinfo.shminfo.shm_nodes[i].membank = membank; > bootinfo.shminfo.nr_nodes++; > + > + if ( !paddr_assigned ) > + bootinfo.shminfo.shm_nodes[i].tot_size = size; > + else if ( mem->nr_banks < NR_MEM_BANKS ) > + { > + struct membank *membank = &mem->bank[mem->nr_banks]; > + > + if ( check_reserved_regions_overlap(paddr, size) ) > + return -EINVAL; > + > + /* Static shared memory shall be reserved from any other use. */ > + membank->start = paddr; > + membank->size = size; > + membank->type = MEMBANK_STATIC_DOMAIN; > + mem->nr_banks++; > + > + /* > + * Reserved memory bank is recorded together to assist > + * doing shm node verification. > + */ > + bootinfo.shminfo.shm_nodes[i].membank = membank; > + } > + else > + goto fail; > } > else > - { > - printk("Warning: Max number of supported memory regions reached.\n"); > - return -ENOSPC; > - } > + goto fail; > } > + > /* > * keep a count of the number of borrowers, which later may be used > * to calculate the reference count. > @@ -502,6 +686,10 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > bootinfo.shminfo.shm_nodes[i].node.nr_shm_borrowers++; > > return 0; > + > + fail: > + printk("Warning: Max number of supported memory regions reached.\n"); > + return -ENOSPC; > } > > int __init make_resv_memory_node(const struct domain *d, void *fdt, > -- > 2.25.1 > ~Michal
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index a8bc78baa5..c69d481d34 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -70,6 +70,9 @@ static void __init retrieve_meminfo(void *mem, unsigned int *max_mem_banks, retrieve_fn __initdata retrievers[MAX_MEMINFO_TYPE] = { [NORMAL_MEMINFO] = retrieve_meminfo, +#ifdef CONFIG_STATIC_SHM + [SHM_MEMINFO] = retrieve_shm_meminfo, +#endif }; #endif diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index bc5f08be97..043588cd2d 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -59,6 +59,9 @@ struct meminfo { enum meminfo_type { NORMAL_MEMINFO, +#ifdef CONFIG_STATIC_SHM + SHM_MEMINFO, +#endif MAX_MEMINFO_TYPE, }; @@ -150,7 +153,16 @@ struct bootinfo { unsigned int nr_nodes; struct { struct shm_node node; - const struct membank *membank; + /* + * For a static shared memory node, it is either banked with + * a statically configured host memory bank, or arbitrary host + * memory which will be allocated by Xen with a specified total + * size(tot_size). + */ + struct { + const struct membank *membank; + paddr_t tot_size; + }; } shm_nodes[NR_MEM_BANKS]; } shminfo; #endif diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index 66a3f4c146..a67445cec8 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -24,6 +24,9 @@ static inline int process_shm_chosen(struct domain *d, int process_shm_node(const void *fdt, int node, uint32_t address_cells, uint32_t size_cells); +void retrieve_shm_meminfo(void *mem, unsigned int *max_mem_banks, + struct membank **bank, + unsigned int **nr_banks); #else /* !CONFIG_STATIC_SHM */ static inline int make_resv_memory_node(const struct domain *d, void *fdt, diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 6a3d8a54bd..a9eb26d543 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -6,6 +6,50 @@ #include <asm/domain_build.h> #include <asm/static-shmem.h> +#define NR_SHM_BANKS 16 + +/* + * There are two types of static shared memory node: + * A static shared memory node could be banked with a statically + * configured host memory bank, or a set of arbitrary host memory + * banks allocated from heap by Xen on runtime. + */ +struct shm_meminfo { + unsigned int nr_banks; + struct membank bank[NR_SHM_BANKS]; + paddr_t tot_size; +}; + +/* + * struct shm_memnode holds banked host memory info for a static + * shared memory node + */ +struct shm_memnode { + char shm_id[MAX_SHM_ID_LENGTH]; + struct shm_meminfo meminfo; +}; + +static __initdata struct { + unsigned int nr_nodes; + struct shm_memnode node[NR_MEM_BANKS]; +} shm_memdata; + +void __init retrieve_shm_meminfo(void *mem, unsigned int *max_mem_banks, + struct membank **bank, + unsigned int **nr_banks) +{ + struct shm_meminfo *meminfo = (struct shm_meminfo *)mem; + + if ( max_mem_banks ) + *max_mem_banks = NR_SHM_BANKS; + + if ( nr_banks ) + *nr_banks = &(meminfo->nr_banks); + + if ( bank ) + *bank = meminfo->bank; +} + static int __init acquire_nr_borrower_domain(const char *shm_id, unsigned long *nr_borrowers) { @@ -172,6 +216,129 @@ static int __init append_shm_bank_to_domain(struct kernel_info *kinfo, return 0; } +static struct shm_memnode * __init find_shm_memnode(const char *shm_id) +{ + unsigned int i; + struct shm_memnode *shm_memnode; + + for ( i = 0 ; i < shm_memdata.nr_nodes; i++ ) + { + shm_memnode = &shm_memdata.node[i]; + + if ( strcmp(shm_id, shm_memnode->shm_id) == 0 ) + return shm_memnode; + } + + if ( i == NR_MEM_BANKS ) + return NULL; + + shm_memnode = &shm_memdata.node[i]; + safe_strcpy(shm_memnode->shm_id, shm_id); + shm_memdata.nr_nodes++; + return shm_memnode; +} + +/* Parse "xen,shared-mem" property in static shared memory node */ +static struct shm_memnode * __init parse_shm_property(struct domain *d, + const struct dt_device_node *dt_node, + bool *paddr_assigned, paddr_t *gbase, + const char *shm_id) +{ + uint32_t addr_cells, size_cells; + const struct dt_property *prop; + const __be32 *cells; + uint32_t len; + unsigned int i; + paddr_t pbase, psize; + struct shm_memnode *shm_memnode; + + /* xen,shared-mem = <pbase, gbase, size>; And pbase could be optional. */ + prop = dt_find_property(dt_node, "xen,shared-mem", &len); + BUG_ON(!prop); + cells = (const __be32 *)prop->value; + + addr_cells = dt_n_addr_cells(dt_node); + size_cells = dt_n_size_cells(dt_node); + if ( len != dt_cells_to_size(addr_cells + size_cells + addr_cells) ) + { + /* pbase is not provided in "xen,shared-mem" */ + if ( len == dt_cells_to_size(size_cells + addr_cells) ) + *paddr_assigned = false; + else + { + printk("fdt: invalid `xen,shared-mem` property.\n"); + return NULL; + } + } + + /* + * If we firstly process the shared memory node with unique "xen,shm-id", + * we allocate a new member "shm_memnode" for it in shm_memdata. + */ + shm_memnode = find_shm_memnode(shm_id); + BUG_ON(!shm_memnode); + if ( !(*paddr_assigned) ) + { + device_tree_get_reg(&cells, addr_cells, size_cells, gbase, &psize); + /* Whether it is a new shm node? */ + if ( shm_memnode->meminfo.tot_size == 0 ) + goto out_early1; + else + goto out_early2; + } + else + { + device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, gbase); + psize = dt_read_number(cells, size_cells); + + /* Whether it is a new shm node? */ + if ( shm_memnode->meminfo.nr_banks != 0 ) + goto out_early2; + } + + /* + * The static shared memory node #dt_node is banked with a + * statically configured host memory bank. + */ + shm_memnode->meminfo.bank[0].start = pbase; + shm_memnode->meminfo.bank[0].size = psize; + shm_memnode->meminfo.nr_banks = 1; + + if ( !IS_ALIGNED(pbase, PAGE_SIZE) ) + { + printk("%pd: physical address 0x%"PRIpaddr" is not suitably aligned.\n", + d, pbase); + return NULL; + } + + for ( i = 0; i < PFN_DOWN(psize); i++ ) + if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) + { + printk("%pd: invalid physical MFN 0x%"PRI_mfn"\n for static shared memory node\n", + d, mfn_x(mfn_add(maddr_to_mfn(pbase), i))); + return NULL; + } + + out_early1: + if ( !IS_ALIGNED(psize, PAGE_SIZE) ) + { + printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n", + d, psize); + return NULL; + } + shm_memnode->meminfo.tot_size = psize; + + out_early2: + if ( !IS_ALIGNED(*gbase, PAGE_SIZE) ) + { + printk("%pd: guest address 0x%"PRIpaddr" is not suitably aligned.\n", + d, *gbase); + return NULL; + } + + return shm_memnode; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -179,51 +346,17 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, dt_for_each_child_node(node, shm_node) { - const struct dt_property *prop; - const __be32 *cells; - uint32_t addr_cells, size_cells; paddr_t gbase, pbase, psize; int ret = 0; - unsigned int i; const char *role_str; const char *shm_id; bool owner_dom_io = true; + bool paddr_assigned = true; + struct shm_memnode *shm_memnode; if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) continue; - /* - * xen,shared-mem = <pbase, gbase, size>; - * TODO: pbase is optional. - */ - addr_cells = dt_n_addr_cells(shm_node); - size_cells = dt_n_size_cells(shm_node); - prop = dt_find_property(shm_node, "xen,shared-mem", NULL); - BUG_ON(!prop); - cells = (const __be32 *)prop->value; - device_tree_get_reg(&cells, addr_cells, addr_cells, &pbase, &gbase); - psize = dt_read_paddr(cells, size_cells); - if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(gbase, PAGE_SIZE) ) - { - printk("%pd: physical address 0x%"PRIpaddr", or guest address 0x%"PRIpaddr" is not suitably aligned.\n", - d, pbase, gbase); - return -EINVAL; - } - if ( !IS_ALIGNED(psize, PAGE_SIZE) ) - { - printk("%pd: size 0x%"PRIpaddr" is not suitably aligned\n", - d, psize); - return -EINVAL; - } - - for ( i = 0; i < PFN_DOWN(psize); i++ ) - if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) ) - { - printk("%pd: invalid physical address 0x%"PRI_mfn"\n", - d, mfn_x(mfn_add(maddr_to_mfn(pbase), i))); - return -EINVAL; - } - /* * "role" property is optional and if it is defined explicitly, * then the owner domain is not the default "dom_io" domain. @@ -238,6 +371,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, } BUG_ON((strlen(shm_id) <= 0) || (strlen(shm_id) >= MAX_SHM_ID_LENGTH)); + shm_memnode = parse_shm_property(d, shm_node, &paddr_assigned, &gbase, + shm_id); + if ( !shm_memnode ) + return -EINVAL; + pbase = shm_memnode->meminfo.bank[0].start; + psize = shm_memnode->meminfo.bank[0].size; + /* * DOMID_IO is a fake domain and is not described in the Device-Tree. * Therefore when the owner of the shared region is DOMID_IO, we will @@ -349,10 +489,10 @@ 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; + paddr_t paddr, gaddr, size; unsigned int i; int len; - bool owner = false; + bool owner = false, paddr_assigned = true; const char *shm_id; if ( address_cells < 1 || size_cells < 1 ) @@ -404,96 +544,140 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) ) { + /* paddr is not provided in "xen,shared-mem" */ if ( len == dt_cells_to_size(size_cells + address_cells) ) - printk("fdt: host physical address must be chosen by users at the moment.\n"); - - printk("fdt: invalid `xen,shared-mem` property.\n"); - return -EINVAL; + paddr_assigned = false; + else + { + printk("fdt: invalid `xen,shared-mem` property.\n"); + return -EINVAL; + } } cell = (const __be32 *)prop->data; - device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr); - size = dt_next_cell(size_cells, &cell); - - if ( !size ) + if ( !paddr_assigned ) + device_tree_get_reg(&cell, address_cells, size_cells, &gaddr, &size); + else { - printk("fdt: the size for static shared memory region can not be zero\n"); - return -EINVAL; - } + paddr_t end; + + device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr); + size = dt_next_cell(size_cells, &cell); + + if ( !size ) + { + printk("fdt: the size for static shared memory region can not be zero\n"); + return -EINVAL; + } + + end = paddr + size; + if ( end <= paddr ) + { + printk("fdt: static shared memory region %s overflow\n", shm_id); + return -EINVAL; + } - end = paddr + size; - if ( end <= paddr ) - { - printk("fdt: static shared memory region %s overflow\n", shm_id); - return -EINVAL; } 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; + bool is_shmid_equal = strncmp(shm_id, bank_id, MAX_SHM_ID_LENGTH) == 0 ? + true : false; /* * Meet the following check: + * when host address is provided: * 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 + * when host address is not provided: + * 1) The shm ID matches and the region size exactly match + */ + /* + * For a static shared memory node, it is either banked with + * a statically configured host memory bank(membank != NULL), or + * arbitrary host memory which will later be allocated by Xen( + * tot_size != 0). */ - if ( paddr == bank_start && size == bank_size ) + if ( bootinfo.shminfo.shm_nodes[i].membank != NULL ) { - if ( strncmp(shm_id, bank_id, MAX_SHM_ID_LENGTH) == 0 ) + paddr_t bank_start = bootinfo.shminfo.shm_nodes[i].membank->start; + paddr_t bank_size = bootinfo.shminfo.shm_nodes[i].membank->size; + bool is_same_region = (paddr == bank_start) && (size == bank_size) ? + true : false; + + if ( is_same_region && is_shmid_equal ) break; - else + else if ( is_same_region || is_shmid_equal ) { printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n", shm_id); return -EINVAL; } } - else if ( strcmp(shm_id, mem->bank[i].shm_id) != 0 ) - continue; else { - printk("fdt: different shared memory region could not share the same shm ID %s\n", - shm_id); - return -EINVAL; + paddr_t tot_size = bootinfo.shminfo.shm_nodes[i].tot_size; + bool is_same_region = tot_size == size ? true : false; + + if ( !paddr_assigned && is_same_region && is_shmid_equal ) + break; + else if ( is_shmid_equal ) + { + if ( paddr_assigned ) + { + printk("fdt: two different types of static shared memory region could not share the same shm-id %s\n", + shm_id); + return -EINVAL; + } + + printk("fdt: when host address is not provided, if xen,shm-id matches, size must stay the same too(%"PRIpaddr" -> %"PRIpaddr")\n", + size, tot_size); + return -EINVAL; + } } } if ( i == bootinfo.shminfo.nr_nodes ) { - struct meminfo *mem = &bootinfo.reserved_mem; - - if ( (i < NR_MEM_BANKS) && (mem->nr_banks < NR_MEM_BANKS) ) + if ( i < NR_MEM_BANKS ) { - struct membank *membank = &mem->bank[mem->nr_banks]; struct shm_node *shm_node = &bootinfo.shminfo.shm_nodes[i].node; - - if ( check_reserved_regions_overlap(paddr, size) ) - return -EINVAL; - - /* Static shared memory shall be reserved from any other use. */ - membank->start = paddr; - membank->size = size; - membank->type = MEMBANK_STATIC_DOMAIN; - mem->nr_banks++; + struct meminfo *mem = &bootinfo.reserved_mem; /* Record static shared memory node info in bootinfo.shminfo */ safe_strcpy(shm_node->shm_id, shm_id); - /* - * Reserved memory bank is recorded together to assist - * doing shm node verification. - */ - bootinfo.shminfo.shm_nodes[i].membank = membank; bootinfo.shminfo.nr_nodes++; + + if ( !paddr_assigned ) + bootinfo.shminfo.shm_nodes[i].tot_size = size; + else if ( mem->nr_banks < NR_MEM_BANKS ) + { + struct membank *membank = &mem->bank[mem->nr_banks]; + + if ( check_reserved_regions_overlap(paddr, size) ) + return -EINVAL; + + /* Static shared memory shall be reserved from any other use. */ + membank->start = paddr; + membank->size = size; + membank->type = MEMBANK_STATIC_DOMAIN; + mem->nr_banks++; + + /* + * Reserved memory bank is recorded together to assist + * doing shm node verification. + */ + bootinfo.shminfo.shm_nodes[i].membank = membank; + } + else + goto fail; } else - { - printk("Warning: Max number of supported memory regions reached.\n"); - return -ENOSPC; - } + goto fail; } + /* * keep a count of the number of borrowers, which later may be used * to calculate the reference count. @@ -502,6 +686,10 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, bootinfo.shminfo.shm_nodes[i].node.nr_shm_borrowers++; return 0; + + fail: + printk("Warning: Max number of supported memory regions reached.\n"); + return -ENOSPC; } int __init make_resv_memory_node(const struct domain *d, void *fdt,
We use paddr_assigned to indicate whether host address is provided, by checking the length of "xen,shared-mem" property. The shm matching criteria shall also be adapt to cover the new scenario, by adding when host address is not provided, if SHMID matches, the region size must exactly match too. During domain creation, right now, a static shared memory node could be banked with a statically configured host memory bank, or arbitrary host memory of dedicated size, which will later be allocated from heap by Xen. To cover both scenarios, we create a new structure shm_meminfo. It is very alike meminfo, but with the maximum array size being a smaller number NR_SHM_BANKS(16). As "shm_meminfo" is also a new member of "enum meminfo_type", we shall implement its own callback "retrieve_shm_meminfo" to have access to all MACRO helpers, e.g. GET_MEMBANK(...). Also, to make codes tidy and clear, we extract codes about parsing "xen,shared-mem" property from function "process_shm" and move them into a new helper "parse_shm_property". Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v1 -> v2 - In order to get allocated banked host memory info during domain creat ion, we create a new structure shm_meminfo. It is very alike meminfo, with the maximum array size being NR_SHM_BANKS. As shm_meminfo is a new member of type meminfo_type, we shall implement its own callback retrieve_shm_meminfo to have access to all MACRO helpers, e.g. GET_MEMBANK(...) - rename "acquire_shm_memnode" to "find_shm_memnode" --- v2 -> v3 - rebase and no changes --- v3 -> v4: - rebase and no change --- v4 -> v5: - fix bugs of that tot_size and membank shall not be member of union, but struct, to differentiate two types of static shared memory node. --- xen/arch/arm/domain_build.c | 3 + xen/arch/arm/include/asm/setup.h | 14 +- xen/arch/arm/include/asm/static-shmem.h | 3 + xen/arch/arm/static-shmem.c | 360 ++++++++++++++++++------ 4 files changed, 293 insertions(+), 87 deletions(-)