| Message ID | 20240423082532.776623-7-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt2 | expand |
Hi Luca, On 23/04/2024 10:25, Luca Fancellu wrote: > > > This commit implements the logic to have the static shared memory banks > from the Xen heap instead of having the host physical address passed from > the user. > > When the host physical address is not supplied, the physical memory is > taken from the Xen heap using allocate_domheap_memory, the allocation > needs to occur at the first handled DT node and the allocated banks > need to be saved somewhere, so introduce the 'shm_heap_banks' static > global variable of type 'struct meminfo' that will hold the banks > allocated from the heap, its field .shmem_extra will be used to point > to the bootinfo shared memory banks .shmem_extra space, so that there > is not further allocation of memory and every bank in shm_heap_banks > can be safely identified by the shm_id to reconstruct its traceability > and if it was allocated or not. > > A search into 'shm_heap_banks' will reveal if the banks were allocated > or not, in case the host address is not passed, and the callback given > to allocate_domheap_memory will store the banks in the structure and > map them to the current domain, to do that, some changes to > acquire_shared_memory_bank are made to let it differentiate if the bank > is from the heap and if it is, then assign_pages is called for every > bank. > > When the bank is already allocated, for every bank allocated with the > corresponding shm_id, handle_shared_mem_bank is called and the mapping > are done. > > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> I tested this patch and it resulted in assertion: Assertion 's <= e' failed at common/rangeset.c:189 I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided), start + size would overflow. Did you test this patch? ~Michal
> On 10 May 2024, at 10:17, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > > On 23/04/2024 10:25, Luca Fancellu wrote: >> >> >> This commit implements the logic to have the static shared memory banks >> from the Xen heap instead of having the host physical address passed from >> the user. >> >> When the host physical address is not supplied, the physical memory is >> taken from the Xen heap using allocate_domheap_memory, the allocation >> needs to occur at the first handled DT node and the allocated banks >> need to be saved somewhere, so introduce the 'shm_heap_banks' static >> global variable of type 'struct meminfo' that will hold the banks >> allocated from the heap, its field .shmem_extra will be used to point >> to the bootinfo shared memory banks .shmem_extra space, so that there >> is not further allocation of memory and every bank in shm_heap_banks >> can be safely identified by the shm_id to reconstruct its traceability >> and if it was allocated or not. >> >> A search into 'shm_heap_banks' will reveal if the banks were allocated >> or not, in case the host address is not passed, and the callback given >> to allocate_domheap_memory will store the banks in the structure and >> map them to the current domain, to do that, some changes to >> acquire_shared_memory_bank are made to let it differentiate if the bank >> is from the heap and if it is, then assign_pages is called for every >> bank. >> >> When the bank is already allocated, for every bank allocated with the >> corresponding shm_id, handle_shared_mem_bank is called and the mapping >> are done. >> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > > I tested this patch and it resulted in assertion: > Assertion 's <= e' failed at common/rangeset.c:189 > > I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided), > start + size would overflow. Did you test this patch? Hi Michal, Mmm I’m testing with a dom0less setup, without dom0, I think I missed that part because my guests doesn’t have the hypervisor node (enhanced), sorry about that, I’ll test using your setup, can you confirm what are you using? > > ~Michal
Hi Luca, On 10/05/2024 11:25, Luca Fancellu wrote: > > >> On 10 May 2024, at 10:17, Michal Orzel <michal.orzel@amd.com> wrote: >> >> Hi Luca, >> >> On 23/04/2024 10:25, Luca Fancellu wrote: >>> >>> >>> This commit implements the logic to have the static shared memory banks >>> from the Xen heap instead of having the host physical address passed from >>> the user. >>> >>> When the host physical address is not supplied, the physical memory is >>> taken from the Xen heap using allocate_domheap_memory, the allocation >>> needs to occur at the first handled DT node and the allocated banks >>> need to be saved somewhere, so introduce the 'shm_heap_banks' static >>> global variable of type 'struct meminfo' that will hold the banks >>> allocated from the heap, its field .shmem_extra will be used to point >>> to the bootinfo shared memory banks .shmem_extra space, so that there >>> is not further allocation of memory and every bank in shm_heap_banks >>> can be safely identified by the shm_id to reconstruct its traceability >>> and if it was allocated or not. >>> >>> A search into 'shm_heap_banks' will reveal if the banks were allocated >>> or not, in case the host address is not passed, and the callback given >>> to allocate_domheap_memory will store the banks in the structure and >>> map them to the current domain, to do that, some changes to >>> acquire_shared_memory_bank are made to let it differentiate if the bank >>> is from the heap and if it is, then assign_pages is called for every >>> bank. >>> >>> When the bank is already allocated, for every bank allocated with the >>> corresponding shm_id, handle_shared_mem_bank is called and the mapping >>> are done. >>> >>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> >> I tested this patch and it resulted in assertion: >> Assertion 's <= e' failed at common/rangeset.c:189 >> >> I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided), >> start + size would overflow. Did you test this patch? > > Hi Michal, > > Mmm I’m testing with a dom0less setup, without dom0, I think I missed that part because my guests doesn’t have > the hypervisor node (enhanced), sorry about that, I’ll test using your setup, can you confirm what are you using? I have these Qemu tests (with and without SMMU): - shmem between domU1 and domU2 with/without host address provided (owner domIO or domU1) - shmem between dom0 and domU1 with/without host address provided (owner domIO or dom0) BTW. What was the conclusion about the issue if shmem region clashes with RAM allocated for 1:1 domU e.g. dom0. Accidentally, I end up with a configuration where Xen allocated for dom0 a region of RAM clashing with my shmem region. ~Michal
> On 10 May 2024, at 10:32, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > > On 10/05/2024 11:25, Luca Fancellu wrote: >> >> >>> On 10 May 2024, at 10:17, Michal Orzel <michal.orzel@amd.com> wrote: >>> >>> Hi Luca, >>> >>> On 23/04/2024 10:25, Luca Fancellu wrote: >>>> >>>> >>>> This commit implements the logic to have the static shared memory banks >>>> from the Xen heap instead of having the host physical address passed from >>>> the user. >>>> >>>> When the host physical address is not supplied, the physical memory is >>>> taken from the Xen heap using allocate_domheap_memory, the allocation >>>> needs to occur at the first handled DT node and the allocated banks >>>> need to be saved somewhere, so introduce the 'shm_heap_banks' static >>>> global variable of type 'struct meminfo' that will hold the banks >>>> allocated from the heap, its field .shmem_extra will be used to point >>>> to the bootinfo shared memory banks .shmem_extra space, so that there >>>> is not further allocation of memory and every bank in shm_heap_banks >>>> can be safely identified by the shm_id to reconstruct its traceability >>>> and if it was allocated or not. >>>> >>>> A search into 'shm_heap_banks' will reveal if the banks were allocated >>>> or not, in case the host address is not passed, and the callback given >>>> to allocate_domheap_memory will store the banks in the structure and >>>> map them to the current domain, to do that, some changes to >>>> acquire_shared_memory_bank are made to let it differentiate if the bank >>>> is from the heap and if it is, then assign_pages is called for every >>>> bank. >>>> >>>> When the bank is already allocated, for every bank allocated with the >>>> corresponding shm_id, handle_shared_mem_bank is called and the mapping >>>> are done. >>>> >>>> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >>> >>> I tested this patch and it resulted in assertion: >>> Assertion 's <= e' failed at common/rangeset.c:189 >>> >>> I checked and in find_unallocated_memory(), given that start is ~0UL (host address not provided), >>> start + size would overflow. Did you test this patch? >> >> Hi Michal, >> >> Mmm I’m testing with a dom0less setup, without dom0, I think I missed that part because my guests doesn’t have >> the hypervisor node (enhanced), sorry about that, I’ll test using your setup, can you confirm what are you using? > I have these Qemu tests (with and without SMMU): > - shmem between domU1 and domU2 with/without host address provided (owner domIO or domU1) Ok, I tested this one, but without enhanced domUs > - shmem between dom0 and domU1 with/without host address provided (owner domIO or dom0) I’m missing this one, I’ll check everywhere where bootinfo_get_shmem() is used, sorry for the oversight > > BTW. What was the conclusion about the issue if shmem region clashes with RAM allocated for 1:1 domU e.g. dom0. > Accidentally, I end up with a configuration where Xen allocated for dom0 a region of RAM clashing with my shmem region. So the conclusion is that we should not have this configuration, but at the moment there is a tech debt because to enforce the check we should do some work around the p2m as Julien suggested, but it’s not trivial because seems that some hyper calls are currently relaying on overwriting the mappings. > > ~Michal
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 1c03bb7f1882..10396ed52ff1 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -9,6 +9,18 @@ #include <asm/static-memory.h> #include <asm/static-shmem.h> +typedef struct { + struct domain *d; + paddr_t gbase; + bool owner_dom_io; + const char *role_str; + struct shmem_membank_extra *bank_extra_info; +} alloc_heap_pages_cb_extra; + +static struct meminfo __initdata shm_heap_banks = { + .common.max_banks = NR_MEM_BANKS +}; + static void __init __maybe_unused build_assertions(void) { /* @@ -66,7 +78,8 @@ static bool __init is_shm_allocated_to_domio(paddr_t pbase) } static mfn_t __init acquire_shared_memory_bank(struct domain *d, - paddr_t pbase, paddr_t psize) + paddr_t pbase, paddr_t psize, + bool bank_from_heap) { mfn_t smfn; unsigned long nr_pfns; @@ -86,19 +99,31 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, d->max_pages += nr_pfns; smfn = maddr_to_mfn(pbase); - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0); + if ( bank_from_heap ) + /* + * When host address is not provided, static shared memory is + * allocated from heap and shall be assigned to owner domain. + */ + res = assign_pages(maddr_to_page(pbase), nr_pfns, d, 0); + else + res = acquire_domstatic_pages(d, smfn, nr_pfns, 0); + if ( res ) { - printk(XENLOG_ERR - "%pd: failed to acquire static memory: %d.\n", d, res); - d->max_pages -= nr_pfns; - return INVALID_MFN; + printk(XENLOG_ERR "%pd: failed to %s static memory: %d.\n", d, + bank_from_heap ? "assign" : "acquire", res); + goto fail; } return smfn; + + fail: + d->max_pages -= nr_pfns; + return INVALID_MFN; } static int __init assign_shared_memory(struct domain *d, paddr_t gbase, + bool bank_from_heap, const struct membank *shm_bank) { mfn_t smfn; @@ -113,10 +138,7 @@ static int __init assign_shared_memory(struct domain *d, paddr_t gbase, psize = shm_bank->size; nr_borrowers = shm_bank->shmem_extra->nr_shm_borrowers; - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", - d, pbase, pbase + psize); - - smfn = acquire_shared_memory_bank(d, pbase, psize); + smfn = acquire_shared_memory_bank(d, pbase, psize, bank_from_heap); if ( mfn_eq(smfn, INVALID_MFN) ) return -EINVAL; @@ -188,6 +210,7 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start, static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, bool owner_dom_io, const char *role_str, + bool bank_from_heap, const struct membank *shm_bank) { paddr_t pbase, psize; @@ -197,6 +220,10 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, pbase = shm_bank->start; psize = shm_bank->size; + + printk("%pd: SHMEM map from %s: mphys 0x%"PRIpaddr" -> gphys 0x%"PRIpaddr", size 0x%"PRIpaddr"\n", + d, bank_from_heap ? "Xen heap" : "Host", pbase, gbase, psize); + /* * 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 @@ -209,7 +236,8 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, * We found the first borrower of the region, the owner was not * specified, so they should be assigned to dom_io. */ - ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, shm_bank); + ret = assign_shared_memory(owner_dom_io ? dom_io : d, gbase, + bank_from_heap, shm_bank); if ( ret ) return ret; } @@ -226,6 +254,40 @@ static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, return 0; } +static int __init save_map_heap_pages(struct domain *d, struct page_info *pg, + unsigned int order, void *extra) +{ + alloc_heap_pages_cb_extra *b_extra = (alloc_heap_pages_cb_extra *)extra; + int idx = shm_heap_banks.common.nr_banks; + int ret = -ENOSPC; + + BUG_ON(!b_extra); + + if ( idx < shm_heap_banks.common.max_banks ) + { + shm_heap_banks.bank[idx].start = page_to_maddr(pg); + shm_heap_banks.bank[idx].size = (1ULL << (PAGE_SHIFT + order)); + shm_heap_banks.bank[idx].shmem_extra = b_extra->bank_extra_info; + shm_heap_banks.common.nr_banks++; + + ret = handle_shared_mem_bank(b_extra->d, b_extra->gbase, + b_extra->owner_dom_io, b_extra->role_str, + true, &shm_heap_banks.bank[idx]); + if ( !ret ) + { + /* Increment guest physical address for next mapping */ + b_extra->gbase += shm_heap_banks.bank[idx].size; + ret = 0; + } + } + + if ( ret ) + printk("Failed to allocate static shared memory from Xen heap: (%d)\n", + ret); + + return ret; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -264,42 +326,101 @@ 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; - } + /* + * "role" property is optional and if it is defined explicitly, + * then the owner domain is not the default "dom_io" domain. + */ + if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) + owner_dom_io = false; /* - * xen,shared-mem = <pbase, gbase, size>; - * TODO: pbase is optional. + * xen,shared-mem = <[pbase,] gbase, size>; + * 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; - gbase = dt_read_paddr(cells + addr_cells, addr_cells); - 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; - } + if ( pbase != INVALID_PADDR ) + { + /* guest phys address is after host phys address */ + gbase = dt_read_paddr(cells + addr_cells, addr_cells); + + 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; + } + + /* The host physical address is supplied by the user */ + ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str, + false, boot_shm_bank); + if ( ret ) + return ret; + } + else + { + /* + * The host physical address is not supplied by the user, so it + * means that the banks needs to be allocated from the Xen heap, + * look into the already allocated banks from the heap. + */ + const struct membank *alloc_bank = find_shm(&shm_heap_banks.common, + shm_id); - /* - * "role" property is optional and if it is defined explicitly, - * then the owner domain is not the default "dom_io" domain. - */ - if ( dt_property_read_string(shm_node, "role", &role_str) == 0 ) - owner_dom_io = false; + /* guest phys address is right at the beginning */ + gbase = dt_read_paddr(cells, addr_cells); - ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str, - boot_shm_bank); - if ( ret ) - return ret; + if ( !alloc_bank ) + { + alloc_heap_pages_cb_extra cb_arg = { d, gbase, owner_dom_io, + role_str, boot_shm_bank->shmem_extra }; + + /* shm_id identified bank is not yet allocated */ + if ( !allocate_domheap_memory(NULL, psize, save_map_heap_pages, + &cb_arg) ) + { + printk(XENLOG_ERR + "Failed to allocate (%"PRIpaddr"MB) pages as static shared memory from heap\n", + psize >> 20); + return -EINVAL; + } + } + else + { + /* shm_id identified bank is already allocated */ + const struct membank *end_bank = + &shm_heap_banks.bank[shm_heap_banks.common.nr_banks]; + paddr_t gbase_bank = gbase; + + /* + * Static shared memory banks that are taken from the Xen heap + * are allocated sequentially in shm_heap_banks, so starting + * from the first bank found identified by shm_id, the code can + * just advance by one bank at the time until it reaches the end + * of the array or it finds another bank NOT identified by + * shm_id + */ + for ( ; alloc_bank < end_bank; alloc_bank++ ) + { + if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id, + MAX_SHM_ID_LENGTH) != 0 ) + break; + + ret = handle_shared_mem_bank(d, gbase_bank, owner_dom_io, + role_str, true, alloc_bank); + if ( ret ) + return ret; + + /* Increment guest physical address for next mapping */ + gbase_bank += alloc_bank->size; + } + } + } /* * Record static shared memory region info for later setting
This commit implements the logic to have the static shared memory banks from the Xen heap instead of having the host physical address passed from the user. When the host physical address is not supplied, the physical memory is taken from the Xen heap using allocate_domheap_memory, the allocation needs to occur at the first handled DT node and the allocated banks need to be saved somewhere, so introduce the 'shm_heap_banks' static global variable of type 'struct meminfo' that will hold the banks allocated from the heap, its field .shmem_extra will be used to point to the bootinfo shared memory banks .shmem_extra space, so that there is not further allocation of memory and every bank in shm_heap_banks can be safely identified by the shm_id to reconstruct its traceability and if it was allocated or not. A search into 'shm_heap_banks' will reveal if the banks were allocated or not, in case the host address is not passed, and the callback given to allocate_domheap_memory will store the banks in the structure and map them to the current domain, to do that, some changes to acquire_shared_memory_bank are made to let it differentiate if the bank is from the heap and if it is, then assign_pages is called for every bank. When the bank is already allocated, for every bank allocated with the corresponding shm_id, handle_shared_mem_bank is called and the mapping are done. Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> --- xen/arch/arm/static-shmem.c | 193 +++++++++++++++++++++++++++++------- 1 file changed, 157 insertions(+), 36 deletions(-)