| Message ID | 20240522075151.3373899-5-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt2 | expand |
Hi Luca, On 22/05/2024 09:51, Luca Fancellu wrote: > > > Handle the parsing of the 'xen,shared-mem' property when the host physical > address is not provided, this commit is introducing the logic to parse it, > but the functionality is still not implemented and will be part of future > commits. > > Rework the logic inside process_shm_node to check the shm_id before doing > the other checks, because it ease the logic itself, add more comment on > the logic. > Now when the host physical address is not provided, the value > INVALID_PADDR is chosen to signal this condition and it is stored as > start of the bank, due to that change also early_print_info_shmem and > init_sharedmem_pages are changed, to not handle banks with start equal > to INVALID_PADDR. > > Another change is done inside meminfo_overlap_check, to skip banks that > are starting with the start address INVALID_PADDR, that function is used > to check banks from reserved memory, shared memory and ACPI and since > the comment above the function states that wrapping around is not handled, > it's unlikely for these bank to have the start address as INVALID_PADDR. > Same change is done inside consider_modules, find_unallocated_memory and > dt_unreserved_regions functions, in order to skip banks that starts with > INVALID_PADDR from any computation. > The changes above holds because of this consideration. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > Reviewed-by: Michal Orzel <michal.orzel@amd.com> > --- > v3 changes: > - fix typo in commit msg, add R-by Michal > v2 changes: > - fix comments, add parenthesis to some conditions, remove unneeded > variables, remove else branch, increment counter in the for loop, > skip INVALID_PADDR start banks from also consider_modules, > find_unallocated_memory and dt_unreserved_regions. (Michal) > --- > xen/arch/arm/arm32/mmu/mm.c | 11 +++- > xen/arch/arm/domain_build.c | 5 ++ > xen/arch/arm/setup.c | 14 +++- > xen/arch/arm/static-shmem.c | 125 +++++++++++++++++++++++++----------- > 4 files changed, 111 insertions(+), 44 deletions(-) > > diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c > index be480c31ea05..30a7aa1e8e51 100644 > --- a/xen/arch/arm/arm32/mmu/mm.c > +++ b/xen/arch/arm/arm32/mmu/mm.c > @@ -101,8 +101,15 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, > nr += reserved_mem->nr_banks; > for ( ; i - nr < shmem->nr_banks; i++ ) > { > - paddr_t r_s = shmem->bank[i - nr].start; > - paddr_t r_e = r_s + shmem->bank[i - nr].size; > + paddr_t r_s, r_e; > + > + r_s = shmem->bank[i - nr].start; > + > + /* Shared memory banks can contain INVALID_PADDR as start */ > + if ( INVALID_PADDR == r_s ) > + continue; > + > + r_e = r_s + shmem->bank[i - nr].size; > > if ( s < r_e && r_s < e ) > { > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 968c497efc78..02e741685102 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -927,6 +927,11 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, > for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) > { > start = mem_banks[i]->bank[j].start; > + > + /* Shared memory banks can contain INVALID_PADDR as start */ > + if ( INVALID_PADDR == start ) > + continue; > + > end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size; > res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), > PFN_DOWN(end - 1)); > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index c4e5c19b11d6..0c2fdaceaf21 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -240,8 +240,15 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, > offset = reserved_mem->nr_banks; > for ( ; i - offset < shmem->nr_banks; i++ ) > { > - paddr_t r_s = shmem->bank[i - offset].start; > - paddr_t r_e = r_s + shmem->bank[i - offset].size; > + paddr_t r_s, r_e; > + > + r_s = shmem->bank[i - offset].start; > + > + /* Shared memory banks can contain INVALID_PADDR as start */ > + if ( INVALID_PADDR == r_s ) > + continue; > + > + r_e = r_s + shmem->bank[i - offset].size; > > if ( s < r_e && r_s < e ) > { > @@ -272,7 +279,8 @@ static bool __init meminfo_overlap_check(const struct membanks *mem, > bank_start = mem->bank[i].start; > bank_end = bank_start + mem->bank[i].size; > > - if ( region_end <= bank_start || region_start >= bank_end ) > + if ( INVALID_PADDR == bank_start || region_end <= bank_start || > + region_start >= bank_end ) > continue; > else > { > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index c15a65130659..74c81904b8a4 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -264,6 +264,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, > pbase = boot_shm_bank->start; > psize = boot_shm_bank->size; > > + if ( INVALID_PADDR == pbase ) > + { > + printk("%pd: host physical address must be chosen by users at the moment", d); > + return -EINVAL; > + } > + > /* > * xen,shared-mem = <pbase, gbase, size>; > * TODO: pbase is optional. > @@ -377,7 +383,8 @@ 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 = INVALID_PADDR; > + paddr_t gaddr, size, end; > struct membanks *mem = bootinfo_get_shmem(); > struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra(); > unsigned int i; > @@ -432,24 +439,37 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > if ( !prop ) > return -ENOENT; > > + cell = (const __be32 *)prop->data; > if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) ) > { > - 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; > + if ( len == dt_cells_to_size(address_cells + size_cells) ) > + device_tree_get_reg(&cell, address_cells, size_cells, &gaddr, > + &size); > + else > + { > + printk("fdt: invalid `xen,shared-mem` property.\n"); > + return -EINVAL; > + } > } > + else > + { > + device_tree_get_reg(&cell, address_cells, address_cells, &paddr, > + &gaddr); > + size = dt_next_cell(size_cells, &cell); > > - cell = (const __be32 *)prop->data; > - device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr); > - size = dt_next_cell(size_cells, &cell); > + if ( !IS_ALIGNED(paddr, PAGE_SIZE) ) > + { > + printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n", > + paddr); > + return -EINVAL; > + } > > - if ( !IS_ALIGNED(paddr, PAGE_SIZE) ) > - { > - printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n", > - paddr); > - return -EINVAL; > + end = paddr + size; > + if ( end <= paddr ) > + { > + printk("fdt: static shared memory region %s overflow\n", shm_id); > + return -EINVAL; > + } > } > > if ( !IS_ALIGNED(gaddr, PAGE_SIZE) ) > @@ -471,39 +491,64 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, > 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 < mem->nr_banks; i++ ) > { > /* > * 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 > + * - 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 > */ > - if ( paddr == mem->bank[i].start && size == mem->bank[i].size ) > + bool paddr_assigned = (INVALID_PADDR == paddr); Shouldn't it be INVALID_PADDR != paddr to indicate that paddr was assigned? Otherwise, looking at the code belowe you would allow a configuration where the shm_id matches but the phys addresses don't. ~Michal
Hi Michal, >> for ( i = 0; i < mem->nr_banks; i++ ) >> { >> /* >> * 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 >> + * - 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 >> */ >> - if ( paddr == mem->bank[i].start && size == mem->bank[i].size ) >> + bool paddr_assigned = (INVALID_PADDR == paddr); > Shouldn't it be INVALID_PADDR != paddr to indicate that paddr was assigned? Otherwise, looking at the > code belowe you would allow a configuration where the shm_id matches but the phys addresses don't. You are right, good catch, somehow it escaped testing, I’ll fix in the next push > > ~Michal
diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c index be480c31ea05..30a7aa1e8e51 100644 --- a/xen/arch/arm/arm32/mmu/mm.c +++ b/xen/arch/arm/arm32/mmu/mm.c @@ -101,8 +101,15 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, nr += reserved_mem->nr_banks; for ( ; i - nr < shmem->nr_banks; i++ ) { - paddr_t r_s = shmem->bank[i - nr].start; - paddr_t r_e = r_s + shmem->bank[i - nr].size; + paddr_t r_s, r_e; + + r_s = shmem->bank[i - nr].start; + + /* Shared memory banks can contain INVALID_PADDR as start */ + if ( INVALID_PADDR == r_s ) + continue; + + r_e = r_s + shmem->bank[i - nr].size; if ( s < r_e && r_s < e ) { diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 968c497efc78..02e741685102 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -927,6 +927,11 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) { start = mem_banks[i]->bank[j].start; + + /* Shared memory banks can contain INVALID_PADDR as start */ + if ( INVALID_PADDR == start ) + continue; + end = mem_banks[i]->bank[j].start + mem_banks[i]->bank[j].size; res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), PFN_DOWN(end - 1)); diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index c4e5c19b11d6..0c2fdaceaf21 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -240,8 +240,15 @@ static void __init dt_unreserved_regions(paddr_t s, paddr_t e, offset = reserved_mem->nr_banks; for ( ; i - offset < shmem->nr_banks; i++ ) { - paddr_t r_s = shmem->bank[i - offset].start; - paddr_t r_e = r_s + shmem->bank[i - offset].size; + paddr_t r_s, r_e; + + r_s = shmem->bank[i - offset].start; + + /* Shared memory banks can contain INVALID_PADDR as start */ + if ( INVALID_PADDR == r_s ) + continue; + + r_e = r_s + shmem->bank[i - offset].size; if ( s < r_e && r_s < e ) { @@ -272,7 +279,8 @@ static bool __init meminfo_overlap_check(const struct membanks *mem, bank_start = mem->bank[i].start; bank_end = bank_start + mem->bank[i].size; - if ( region_end <= bank_start || region_start >= bank_end ) + if ( INVALID_PADDR == bank_start || region_end <= bank_start || + region_start >= bank_end ) continue; else { diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index c15a65130659..74c81904b8a4 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -264,6 +264,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, pbase = boot_shm_bank->start; psize = boot_shm_bank->size; + if ( INVALID_PADDR == pbase ) + { + printk("%pd: host physical address must be chosen by users at the moment", d); + return -EINVAL; + } + /* * xen,shared-mem = <pbase, gbase, size>; * TODO: pbase is optional. @@ -377,7 +383,8 @@ 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 = INVALID_PADDR; + paddr_t gaddr, size, end; struct membanks *mem = bootinfo_get_shmem(); struct shmem_membank_extra *shmem_extra = bootinfo_get_shmem_extra(); unsigned int i; @@ -432,24 +439,37 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, if ( !prop ) return -ENOENT; + cell = (const __be32 *)prop->data; if ( len != dt_cells_to_size(address_cells + size_cells + address_cells) ) { - 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; + if ( len == dt_cells_to_size(address_cells + size_cells) ) + device_tree_get_reg(&cell, address_cells, size_cells, &gaddr, + &size); + else + { + printk("fdt: invalid `xen,shared-mem` property.\n"); + return -EINVAL; + } } + else + { + device_tree_get_reg(&cell, address_cells, address_cells, &paddr, + &gaddr); + size = dt_next_cell(size_cells, &cell); - cell = (const __be32 *)prop->data; - device_tree_get_reg(&cell, address_cells, address_cells, &paddr, &gaddr); - size = dt_next_cell(size_cells, &cell); + if ( !IS_ALIGNED(paddr, PAGE_SIZE) ) + { + printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n", + paddr); + return -EINVAL; + } - if ( !IS_ALIGNED(paddr, PAGE_SIZE) ) - { - printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n", - paddr); - return -EINVAL; + end = paddr + size; + if ( end <= paddr ) + { + printk("fdt: static shared memory region %s overflow\n", shm_id); + return -EINVAL; + } } if ( !IS_ALIGNED(gaddr, PAGE_SIZE) ) @@ -471,39 +491,64 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, 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 < mem->nr_banks; i++ ) { /* * 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 + * - 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 */ - if ( paddr == mem->bank[i].start && size == mem->bank[i].size ) + bool paddr_assigned = (INVALID_PADDR == paddr); + + if ( strncmp(shm_id, shmem_extra[i].shm_id, MAX_SHM_ID_LENGTH) == 0 ) { - if ( strncmp(shm_id, shmem_extra[i].shm_id, - MAX_SHM_ID_LENGTH) == 0 ) + /* + * Regions have same shm_id (cases): + * 1) physical host address is supplied: + * - OK: paddr is equal and size is equal (same region) + * - Fail: paddr doesn't match or size doesn't match (there + * cannot exists two shmem regions with same shm_id) + * 2) physical host address is NOT supplied: + * - OK: size is equal (same region) + * - Fail: size is not equal (same shm_id must identify only one + * region, there can't be two different regions with same + * shm_id) + */ + bool start_match = paddr_assigned ? (paddr == mem->bank[i].start) : + true; + + if ( start_match && (size == mem->bank[i].size) ) break; else { - printk("fdt: xen,shm-id %s does not match for all the nodes using the same region.\n", + printk("fdt: different shared memory region could not share the same shm ID %s\n", shm_id); return -EINVAL; } } - else if ( strncmp(shm_id, shmem_extra[i].shm_id, - MAX_SHM_ID_LENGTH) != 0 ) + + /* + * Regions have different shm_id (cases): + * 1) physical host address is supplied: + * - OK: paddr different, or size different (case where paddr + * is equal but psize is different are wrong, but they + * are handled later when checking for overlapping) + * - Fail: paddr equal and size equal (the same region can't be + * identified with different shm_id) + * 2) physical host address is NOT supplied: + * - OK: Both have different shm_id so even with same size they + * can exists + */ + if ( !paddr_assigned || (paddr != mem->bank[i].start) || + (size != mem->bank[i].size) ) continue; else { - printk("fdt: different shared memory region could not share the same shm ID %s\n", + printk("fdt: xen,shm-id %s does not match for all the nodes using the same region\n", shm_id); return -EINVAL; } @@ -513,7 +558,8 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, { if (i < mem->max_banks) { - if ( check_reserved_regions_overlap(paddr, size) ) + if ( (paddr != INVALID_PADDR) && + check_reserved_regions_overlap(paddr, size) ) return -EINVAL; /* Static shared memory shall be reserved from any other use. */ @@ -583,13 +629,13 @@ void __init early_print_info_shmem(void) { const struct membanks *shmem = bootinfo_get_shmem(); unsigned int bank; + unsigned int printed = 0; - for ( bank = 0; bank < shmem->nr_banks; bank++ ) - { - printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", bank, - shmem->bank[bank].start, - shmem->bank[bank].start + shmem->bank[bank].size - 1); - } + for ( bank = 0; bank < shmem->nr_banks; bank++, printed++ ) + if ( shmem->bank[bank].start != INVALID_PADDR ) + printk(" SHMEM[%u]: %"PRIpaddr" - %"PRIpaddr"\n", printed, + shmem->bank[bank].start, + shmem->bank[bank].start + shmem->bank[bank].size - 1); } void __init init_sharedmem_pages(void) @@ -598,7 +644,8 @@ void __init init_sharedmem_pages(void) unsigned int bank; for ( bank = 0 ; bank < shmem->nr_banks; bank++ ) - init_staticmem_bank(&shmem->bank[bank]); + if ( shmem->bank[bank].start != INVALID_PADDR ) + init_staticmem_bank(&shmem->bank[bank]); } int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,