[PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
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 --- v2 changes: - add static inline get_shmem_heap_banks(), given the changes to the struct membanks interface. Rebase changes due to removal of owner_dom_io arg from handle_shared_mem_bank. Change save_map_heap_pages return type given the changes to the allocate_domheap_memory callback type. --- xen/arch/arm/static-shmem.c | 186 ++-- 1 file changed, 155 insertions(+), 31 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index ddaacbc77740..9c3a83042d8b 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -9,6 +9,22 @@ #include #include +typedef struct { +struct domain *d; +paddr_t gbase; +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 inline struct membanks *get_shmem_heap_banks(void) +{ +return container_of(&shm_heap_banks.common, struct membanks, common); +} + static void __init __maybe_unused build_assertions(void) { /* @@ -64,7 +80,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; @@ -84,19 +101,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; @@ -109,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; @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, const char *role_str, + bool bank_from_heap,
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
Hi Luca, On 15/05/2024 16:26, 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. NIT for the future: it's better to split 10 lines long sentence into multiple ones. Otherwise it reads difficult. > > 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 > --- > v2 changes: > - add static inline get_shmem_heap_banks(), given the changes to the >struct membanks interface. Rebase changes due to removal of >owner_dom_io arg from handle_shared_mem_bank. >Change save_map_heap_pages return type given the changes to the >allocate_domheap_memory callback type. > --- > xen/arch/arm/static-shmem.c | 186 ++-- > 1 file changed, 155 insertions(+), 31 deletions(-) > > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index ddaacbc77740..9c3a83042d8b 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -9,6 +9,22 @@ > #include > #include > > +typedef struct { > +struct domain *d; > +paddr_t gbase; > +const char *role_str; You could swap role_str and gbase to avoid a 4B hole on arm32 > +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 Do we expect that many banks? > +}; > + > +static inline struct membanks *get_shmem_heap_banks(void) > +{ > +return container_of(&shm_heap_banks.common, struct membanks, common); > +} > + > static void __init __maybe_unused build_assertions(void) > { > /* > @@ -64,7 +80,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; > @@ -84,19 +101,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; > @@ -109,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); > - > -
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
Hi Michal, > On 20 May 2024, at 12:16, Michal Orzel wrote: > > Hi Luca, > > On 15/05/2024 16:26, 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. > NIT for the future: it's better to split 10 lines long sentence into multiple > ones. > Otherwise it reads difficult. I’ll do, >> >> xen/arch/arm/static-shmem.c | 186 ++-- >> 1 file changed, 155 insertions(+), 31 deletions(-) >> >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index ddaacbc77740..9c3a83042d8b 100644 >> --- a/xen/arch/arm/static-shmem.c >> +++ b/xen/arch/arm/static-shmem.c >> @@ -9,6 +9,22 @@ >> #include >> #include >> >> +typedef struct { >> +struct domain *d; >> +paddr_t gbase; >> +const char *role_str; > You could swap role_str and gbase to avoid a 4B hole on arm32 Sure I will, > >> +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 > Do we expect that many banks? Not really, but I was trying to don’t introduce another type, do you think it’s better instead to introduce a new type only here, with a lower amount of banks? Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ member. >> >> static int __init assign_shared_memory(struct domain *d, paddr_t gbase, >> + bool bank_from_heap, >>const struct membank *shm_bank) >> { >> mfn_t smfn; >> @@ -109,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; >> >> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, >> paddr_t start, >> >> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, >> const char *role_str, >> + bool bank_from_heap, >> const struct membank *shm_bank) >> { >> bool owner_dom_io = true; >> @@ -192,6 +219,9 @@ 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); > This looks more like a debug print since I don't expect user to want to see a > machine address. printk(XENLOG_DEBUG ? >> >> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>const struct dt_device_node *node) >> { >> @@ -265,37 +329,97 @@ 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 */ >> +dt_property_read_string(shm_node, "role", &role_str); > This function returns a value but you seem to ignore it Sure, I’ll handle that >> >> -ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank); >> -if ( ret ) >> -return ret; >> +if ( !alloc_bank ) >> +{ >> +alloc_heap_pages_cb_extra cb_arg = { d, gbase, role_str, >> +boot_shm_bank->shmem_extra }; >> + >> +/* shm_id identified bank is not yet allocated */ >> +if ( !allocate_domheap_memory(NULL,
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
On 20/05/2024 14:44, Luca Fancellu wrote: > > > Hi Michal, > >> On 20 May 2024, at 12:16, Michal Orzel wrote: >> >> Hi Luca, >> >> On 15/05/2024 16:26, 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. >> NIT for the future: it's better to split 10 lines long sentence into >> multiple ones. >> Otherwise it reads difficult. > > I’ll do, > >>> >>> xen/arch/arm/static-shmem.c | 186 ++-- >>> 1 file changed, 155 insertions(+), 31 deletions(-) >>> >>> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >>> index ddaacbc77740..9c3a83042d8b 100644 >>> --- a/xen/arch/arm/static-shmem.c >>> +++ b/xen/arch/arm/static-shmem.c >>> @@ -9,6 +9,22 @@ >>> #include >>> #include >>> >>> +typedef struct { >>> +struct domain *d; >>> +paddr_t gbase; >>> +const char *role_str; >> You could swap role_str and gbase to avoid a 4B hole on arm32 > > Sure I will, > >> >>> +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 >> Do we expect that many banks? > > Not really, but I was trying to don’t introduce another type, do you think > it’s better instead to > introduce a new type only here, with a lower amount of banks? I'd be ok with meminfo provided you add a reasoning behind this being meminfo and not shared_meminfo. > > Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ > member. Would it result in a smaller footprint overall? > >>> >>> static int __init assign_shared_memory(struct domain *d, paddr_t gbase, >>> + bool bank_from_heap, >>>const struct membank *shm_bank) >>> { >>> mfn_t smfn; >>> @@ -109,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; >>> >>> @@ -183,6 +209,7 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, >>> paddr_t start, >>> >>> static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, >>> const char *role_str, >>> + bool bank_from_heap, >>> const struct membank *shm_bank) >>> { >>> bool owner_dom_io = true; >>> @@ -192,6 +219,9 @@ 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); >> This looks more like a debug print since I don't expect user to want to see >> a machine address. > > printk(XENLOG_DEBUG ? Why would you want user to know the machine physical address? It's a heap address. > > >>> >>> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>>const struct dt_device_node *node) >>> { >>> @@ -265,37 +329,97 @@ 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 */ >>> +dt_property_read_string(shm_node, "role", &role_str); >> This function returns a value but you seem to ignore it > > Sure, I’ll handle that > >>> >>> -ret =
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
>> >>> +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 >>> Do we expect that many banks? >> >> Not really, but I was trying to don’t introduce another type, do you think >> it’s better instead to >> introduce a new type only here, with a lower amount of banks? > I'd be ok with meminfo provided you add a reasoning behind this being meminfo > and not shared_meminfo. > >> >> Because if we take struct shared_meminfo, we would waste mem for its ‘extra’ >> member. > Would it result in a smaller footprint overall? Well overall yes, meminfo now is 255 banks, shared_meminfo is 64 in total, even if we use 32 of them and 32 are wasted. Otherwise, as I said, I could do something like this in this module: static struct shared_heap_meminfo { struct membanks_hdr common; struct membank bank[NR_SHMEM_BANKS]; } __initdata shm_heap_banks = { .common.max_banks = NR_SHMEM_BANKS }; bool owner_dom_io = true; @@ -192,6 +219,9 @@ 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); >>> This looks more like a debug print since I don't expect user to want to see >>> a machine address. >> >> printk(XENLOG_DEBUG ? > Why would you want user to know the machine physical address? It's a heap > address. Oh ok your comment was about removing it, ok I don’t have strong objections to that. >> >> -ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank); -if ( ret ) -return ret; +if ( !alloc_bank ) +{ +alloc_heap_pages_cb_extra cb_arg = { d, gbase, 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", >>> Why limiting to MB? >> >> I think I used it from domain_build.c, do you think it’s better to limit it >> on KB instead? > User can request static shmem region of as little as 1 page. Ok I’ll change to KB > >> +for ( ; alloc_bank < end_bank; alloc_bank++ ) +{ +if ( strncmp(shm_id, alloc_bank->shmem_extra->shm_id, + MAX_SHM_ID_LENGTH) != 0 ) >>> shm_id has been already validated above, hence no need for a safe version >>> of strcmp >>> >> >> I always try to use the safe version, even when redundant, I feel that if >> someone is copying part of the code, >> at least it would copy a safe version. Anyway I will change it if it’s not >> desirable. >> >> Cheers, >> Luca >> >> > > ~Michal Cheers, Luca
Re: [PATCH v2 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
On 20/05/2024 15:11, Luca Fancellu wrote: > > >>> > +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 Do we expect that many banks? >>> >>> Not really, but I was trying to don’t introduce another type, do you think >>> it’s better instead to >>> introduce a new type only here, with a lower amount of banks? >> I'd be ok with meminfo provided you add a reasoning behind this being >> meminfo and not shared_meminfo. >> >>> >>> Because if we take struct shared_meminfo, we would waste mem for its >>> ‘extra’ member. >> Would it result in a smaller footprint overall? > > Well overall yes, meminfo now is 255 banks, shared_meminfo is 64 in total, > even if we use 32 of them and > 32 are wasted. > > Otherwise, as I said, I could do something like this in this module: > > static struct shared_heap_meminfo { > struct membanks_hdr common; > struct membank bank[NR_SHMEM_BANKS]; > } __initdata shm_heap_banks = { > .common.max_banks = NR_SHMEM_BANKS > }; If that's all it takes to avoid defining unnecessarily big variable, then I'd be ok with it. ~Michal