Re: [PATCH v4 0/7] Static shared memory followup v2 - pt2
+ Oleksii > On 24 May 2024, at 13:40, Luca Fancellu wrote: > > This serie is a partial rework of this other serie: > https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ > > The original serie is addressing an issue of the static shared memory feature > that impacts the memory footprint of other component when the feature is > enabled, another issue impacts the device tree generation for the guests when > the feature is enabled and used and the last one is a missing feature that is > the option to have a static shared memory region that is not from the host > address space. > > This serie is handling some comment on the original serie and it is splitting > the rework in two part, this first part is addressing the memory footprint > issue > and the device tree generation and currently is fully merged > (https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/), > this serie is addressing the static shared memory allocation from the Xen > heap. > > Luca Fancellu (5): > xen/arm: Lookup bootinfo shm bank during the mapping > xen/arm: Wrap shared memory mapping code in one function > xen/arm: Parse xen,shared-mem when host phys address is not provided > xen/arm: Rework heap page allocation outside allocate_bank_memory > xen/arm: Implement the logic for static shared memory from Xen heap > > Penny Zheng (2): > xen/p2m: put reference for level 2 superpage > xen/docs: Describe static shared memory when host address is not >provided > > docs/misc/arm/device-tree/booting.txt | 52 ++- > xen/arch/arm/arm32/mmu/mm.c | 11 +- > xen/arch/arm/dom0less-build.c | 4 +- > xen/arch/arm/domain_build.c | 84 +++-- > xen/arch/arm/include/asm/domain_build.h | 9 +- > xen/arch/arm/mmu/p2m.c | 82 +++-- > xen/arch/arm/setup.c| 14 +- > xen/arch/arm/static-shmem.c | 432 +--- > 8 files changed, 502 insertions(+), 186 deletions(-) > > -- > 2.34.1 > > Hi, We would like this serie to be in Xen 4.19, there was a misunderstanding on our side because we thought that since the serie was sent before the last posting date, it could have been a candidate for merging in the new release, instead after speaking with Julien and Oleksii we are now aware that we need to provide a justification for its presence. Pros to this serie is that we are closing the circle for static shared memory, allowing it to use memory from the host or from Xen, it is also a feature that is not enabled by default, so it should not cause too much disruption in case of any bugs that escaped the review, however we’ve tested many configurations for that with/without enabling the feature if that can be an additional value. Cons: we are touching some common code related to p2m, but also there the impact should be minimal because the new code is subject to l2 foreign mapping (to be confirmed maybe from a p2m expert like Julien). The comments on patch 3 of this serie are addressed by this patch: https://patchwork.kernel.org/project/xen-devel/patch/20240528125603.2467640-1-luca.fance...@arm.com/ And the serie is fully reviewed. So our request is to allow this serie in 4.19, Oleksii, ARM maintainers, do you agree on that? Cheers, Luca
[PATCH v4.2] xen/p2m: put reference for level 2 superpage
From: Penny Zheng We are doing foreign memory mapping for static shared memory, and there is a great possibility that it could be super mapped. But today, p2m_put_l3_page could not handle superpages. This commits implements a new function p2m_put_l2_superpage to handle level 2 superpages, specifically for helping put extra references for foreign superpages. Modify relinquish_p2m_mapping as well to take into account preemption when we have a level-2 foreign mapping. Currently level 1 superpages are not handled because Xen is not preemptible and therefore some work is needed to handle such superpages, for which at some point Xen might end up freeing memory and therefore for such a big mapping it could end up in a very long operation. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v4.2 changes: - rework commit message to don't explicit say the size of the mapping but only say the level, remove unnecessary p2m_is_superpage() check in p2m_put_page, remove (level >= 2) condition to call p2m_put_page inside p2m_free_entry because the code in that function can already handle the levels, move TODO comment inside p2m_put_page, avoid mentioning 2MB in the comments. (Julien) - This patch is meant to superseed the homonymous patch in the serie: https://patchwork.kernel.org/project/xen-devel/cover/20240524124055.3871399-1-luca.fance...@arm.com/ v4 changes: - optimised the path to call put_page() on the foreign mapping as Julien suggested. Add a comment in p2m_put_l2_superpage to state that any changes needs to take into account some change in the relinquish code. (Julien) v3 changes: - Add reasoning why we don't support now 1GB superpage, remove level_order variable from p2m_put_l2_superpage, update TODO comment inside p2m_free_entry, use XEN_PT_LEVEL_ORDER(2) instead of value 9 inside relinquish_p2m_mapping. (Michal) v2: - Do not handle 1GB super page as there might be some issue where a lot of calls to put_page(...) might be issued which could lead to free memory that is a long operation. v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/ --- xen/arch/arm/mmu/p2m.c | 81 +++--- 1 file changed, 61 insertions(+), 20 deletions(-) diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index 41fcca011cf4..1725cca649b5 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -753,34 +753,72 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, return rc; } -/* - * Put any references on the single 4K page referenced by pte. - * TODO: Handle superpages, for now we only take special references for leaf - * pages (specifically foreign ones, which can't be super mapped today). - */ -static void p2m_put_l3_page(const lpae_t pte) +static void p2m_put_foreign_page(struct page_info *pg) { -mfn_t mfn = lpae_get_mfn(pte); - -ASSERT(p2m_is_valid(pte)); - /* - * TODO: Handle other p2m types - * * It's safe to do the put_page here because page_alloc will * flush the TLBs if the page is reallocated before the end of * this loop. */ -if ( p2m_is_foreign(pte.p2m.type) ) +put_page(pg); +} + +/* Put any references on the single 4K page referenced by mfn. */ +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type) +{ +/* TODO: Handle other p2m types */ +if ( p2m_is_foreign(type) ) { ASSERT(mfn_valid(mfn)); -put_page(mfn_to_page(mfn)); +p2m_put_foreign_page(mfn_to_page(mfn)); } /* Detect the xenheap page and mark the stored GFN as invalid. */ -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); } +/* Put any references on the superpage referenced by mfn. */ +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type) +{ +struct page_info *pg; +unsigned int i; + +/* + * TODO: Handle other p2m types, but be aware that any changes to handle + * different types should require an update on the relinquish code to handle + * preemption. + */ +if ( !p2m_is_foreign(type) ) +return; + +ASSERT(mfn_valid(mfn)); + +pg = mfn_to_page(mfn); + +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++, pg++ ) +p2m_put_foreign_page(pg); +} + +/* Put any references on the page referenced by pte. */ +static void p2m_put_page(const lpae_t pte, unsigned int level) +{ +mfn_t mfn = lpae_get_mfn(pte); + +ASSERT(p2m_is_valid(pte)); + +/* + * TODO: Currently we don't handle level 1 super-page, Xen is not + * preemptible and therefore some work is needed to handle such + * superpages, for which at some point Xen might end up freeing memory + * and therefore for such a big mapping it could end up in a very
Re: [PATCH v4 3/7] xen/p2m: put reference for level 2 superpage
Hi Julien, > On 24 May 2024, at 13:56, Julien Grall wrote: > > Hi Luca, > > On 24/05/2024 13:40, Luca Fancellu wrote: >> From: Penny Zheng >> We are doing foreign memory mapping for static shared memory, and >> there is a great possibility that it could be super mapped. >> But today, p2m_put_l3_page could not handle superpages. >> This commits implements a new function p2m_put_l2_superpage to handle >> 2MB superpages, specifically for helping put extra references for >> foreign superpages. >> Modify relinquish_p2m_mapping as well to take into account preemption >> when type is foreign memory and order is above 9 (2MB). >> Currently 1GB superpages are not handled because Xen is not preemptible >> and therefore some work is needed to handle such superpages, for which >> at some point Xen might end up freeing memory and therefore for such a >> big mapping it could end up in a very long operation. >> Signed-off-by: Penny Zheng >> Signed-off-by: Luca Fancellu >> --- >> v4 changes: >> - optimised the path to call put_page() on the foreign mapping as >>Julien suggested. Add a comment in p2m_put_l2_superpage to state >>that any changes needs to take into account some change in the >>relinquish code. (Julien) >> v3 changes: >> - Add reasoning why we don't support now 1GB superpage, remove level_order >>variable from p2m_put_l2_superpage, update TODO comment inside >>p2m_free_entry, use XEN_PT_LEVEL_ORDER(2) instead of value 9 inside >>relinquish_p2m_mapping. (Michal) >> v2: >> - Do not handle 1GB super page as there might be some issue where >>a lot of calls to put_page(...) might be issued which could lead >>to free memory that is a long operation. >> v1: >> - patch from >> https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/ >> --- >> xen/arch/arm/mmu/p2m.c | 82 +++--- >> 1 file changed, 62 insertions(+), 20 deletions(-) >> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c >> index 41fcca011cf4..986c5a03c54b 100644 >> --- a/xen/arch/arm/mmu/p2m.c >> +++ b/xen/arch/arm/mmu/p2m.c >> @@ -753,34 +753,66 @@ static int p2m_mem_access_radix_set(struct p2m_domain >> *p2m, gfn_t gfn, >> return rc; >> } >> -/* >> - * Put any references on the single 4K page referenced by pte. >> - * TODO: Handle superpages, for now we only take special references for leaf >> - * pages (specifically foreign ones, which can't be super mapped today). >> - */ >> -static void p2m_put_l3_page(const lpae_t pte) >> +static void p2m_put_foreign_page(struct page_info *pg) >> { >> -mfn_t mfn = lpae_get_mfn(pte); >> - >> -ASSERT(p2m_is_valid(pte)); >> - >> /* >> - * TODO: Handle other p2m types >> - * >> * It's safe to do the put_page here because page_alloc will >> * flush the TLBs if the page is reallocated before the end of >> * this loop. >> */ >> -if ( p2m_is_foreign(pte.p2m.type) ) >> +put_page(pg); >> +} >> + >> +/* Put any references on the single 4K page referenced by mfn. */ >> +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type) >> +{ >> +/* TODO: Handle other p2m types */ >> +if ( p2m_is_foreign(type) ) >> { >> ASSERT(mfn_valid(mfn)); >> -put_page(mfn_to_page(mfn)); >> +p2m_put_foreign_page(mfn_to_page(mfn)); >> } >> /* Detect the xenheap page and mark the stored GFN as invalid. */ >> -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) >> +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) >> page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); >> } >> +/* Put any references on the superpage referenced by mfn. */ >> +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type) >> +{ >> +struct page_info *pg; >> +unsigned int i; >> + >> +/* >> + * TODO: Handle other p2m types, but be aware that any changes to handle >> + * different types should require an update on the relinquish code to >> handle >> + * preemption. >> + */ >> +if ( !p2m_is_foreign(type) ) >> +return; >> + >> +ASSERT(mfn_valid(mfn)); >> + >> +pg = mfn_to_page(mfn); >> + >> +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++, pg++ ) >> +p2m_put_foreign_page(pg); >> +} >> + >> +/* Put
[PATCH v4 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. Introduce the 'shm_heap_banks' for that reason, a struct that will hold the banks allocated from the heap, its field bank[].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 Reviewed-by: Michal Orzel --- v4 changes: - Add R-by Michal v3 changes: - reworded commit msg section, swap role_str and gbase in alloc_heap_pages_cb_extra to avoid padding hole in arm32, remove not needed printk, modify printk to print KB instead of KB, swap strncmp for strcmp, reduced memory footprint for shm_heap_banks. (Michal) 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 | 187 ++-- 1 file changed, 155 insertions(+), 32 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index bc093b9da9ea..dbb017c7d76e 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -9,6 +9,25 @@ #include #include +typedef struct { +struct domain *d; +const char *role_str; +paddr_t gbase; +struct shmem_membank_extra *bank_extra_info; +} alloc_heap_pages_cb_extra; + +static struct { +struct membanks_hdr common; +struct membank bank[NR_SHMEM_BANKS]; +} shm_heap_banks __initdata = { +.common.max_banks = NR_SHMEM_BANKS +}; + +static inline struct membanks *get_shmem_heap_banks(void) +{ +return container_of(_heap_banks.common, struct membanks, common); +} + static void __init __maybe_unused build_assertions(void) { /* @@ -63,7 +82,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; @@ -83,19 +103,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; @@ -108,10 +140,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_sha
[PATCH v4 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
The current static shared memory code is using bootinfo banks when it needs to find the number of borrowers, so every time assign_shared_memory is called, the bank is searched in the bootinfo.shmem structure. There is nothing wrong with it, however the bank can be used also to retrieve the start address and size and also to pass less argument to assign_shared_memory. When retrieving the information from the bootinfo bank, it's also possible to move the checks on alignment to process_shm_node in the early stages. So create a new function find_shm_bank_by_id() which takes a 'struct shared_meminfo' structure and the shared memory ID, to look for a bank with a matching ID, take the physical host address and size from the bank, pass the bank to assign_shared_memory() removing the now unnecessary arguments and finally remove the acquire_nr_borrower_domain() function since now the information can be extracted from the passed bank. Move the "xen,shm-id" parsing early in process_shm to bail out quickly in case of errors (unlikely), as said above, move the checks on alignment to process_shm_node. Drawback of this change is that now the bootinfo are used also when the bank doesn't need to be allocated, however it will be convenient later to use it as an argument for assign_shared_memory when dealing with the use case where the Host physical address is not supplied by the user. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3 changes: - switch strncmp with strcmp in find_shm_bank_by_id, fix commit msg typo, add R-by Michal. v2 changes: - fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(), swap region size check different from zero and size alignment, remove not necessary BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 100 +++- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 78881dd1d3f7..0a1c327e90ea 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -19,29 +19,21 @@ static void __init __maybe_unused build_assertions(void) offsetof(struct shared_meminfo, bank))); } -static int __init acquire_nr_borrower_domain(struct domain *d, - paddr_t pbase, paddr_t psize, - unsigned long *nr_borrowers) +static const struct membank __init * +find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id) { -const struct membanks *shmem = bootinfo_get_shmem(); unsigned int bank; -/* Iterate reserved memory to find requested shm bank. */ for ( bank = 0 ; bank < shmem->nr_banks; bank++ ) { -paddr_t bank_start = shmem->bank[bank].start; -paddr_t bank_size = shmem->bank[bank].size; - -if ( (pbase == bank_start) && (psize == bank_size) ) +if ( strcmp(shm_id, shmem->bank[bank].shmem_extra->shm_id) == 0 ) break; } if ( bank == shmem->nr_banks ) -return -ENOENT; +return NULL; -*nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers; - -return 0; +return >bank[bank]; } /* @@ -103,14 +95,18 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, return smfn; } -static int __init assign_shared_memory(struct domain *d, - paddr_t pbase, paddr_t psize, - paddr_t gbase) +static int __init assign_shared_memory(struct domain *d, paddr_t gbase, + const struct membank *shm_bank) { mfn_t smfn; int ret = 0; unsigned long nr_pages, nr_borrowers, i; struct page_info *page; +paddr_t pbase, psize; + +pbase = shm_bank->start; +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); @@ -135,14 +131,6 @@ static int __init assign_shared_memory(struct domain *d, } } -/* - * Get the right amount of references per page, which is the number of - * borrower domains. - */ -ret = acquire_nr_borrower_domain(d, pbase, psize, _borrowers); -if ( ret ) -return ret; - /* * Instead of letting borrower domain get a page ref, we add as many * additional reference as the number of borrowers when the owner @@ -199,6 +187,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, dt_for_each_child_node(node, shm_node) { +const struct membank *boot_shm_bank; const struct dt_property *prop; const __be32 *cells; uint32_t addr_cells, size_cells; @@ -212,6 +201,23 @@ int __init process_shm(struct domain *d, struct kernel_
[PATCH v4 0/7] Static shared memory followup v2 - pt2
This serie is a partial rework of this other serie: https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ The original serie is addressing an issue of the static shared memory feature that impacts the memory footprint of other component when the feature is enabled, another issue impacts the device tree generation for the guests when the feature is enabled and used and the last one is a missing feature that is the option to have a static shared memory region that is not from the host address space. This serie is handling some comment on the original serie and it is splitting the rework in two part, this first part is addressing the memory footprint issue and the device tree generation and currently is fully merged (https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/), this serie is addressing the static shared memory allocation from the Xen heap. Luca Fancellu (5): xen/arm: Lookup bootinfo shm bank during the mapping xen/arm: Wrap shared memory mapping code in one function xen/arm: Parse xen,shared-mem when host phys address is not provided xen/arm: Rework heap page allocation outside allocate_bank_memory xen/arm: Implement the logic for static shared memory from Xen heap Penny Zheng (2): xen/p2m: put reference for level 2 superpage xen/docs: Describe static shared memory when host address is not provided docs/misc/arm/device-tree/booting.txt | 52 ++- xen/arch/arm/arm32/mmu/mm.c | 11 +- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 84 +++-- xen/arch/arm/include/asm/domain_build.h | 9 +- xen/arch/arm/mmu/p2m.c | 82 +++-- xen/arch/arm/setup.c| 14 +- xen/arch/arm/static-shmem.c | 432 +--- 8 files changed, 502 insertions(+), 186 deletions(-) -- 2.34.1
[PATCH v4 2/7] xen/arm: Wrap shared memory mapping code in one function
Wrap the code and logic that is calling assign_shared_memory and map_regions_p2mt into a new function 'handle_shared_mem_bank', it will become useful later when the code will allow the user to don't pass the host physical address. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3 changes: - check return value of dt_property_read_string, add R-by Michal v2 changes: - add blank line, move owner_dom_io computation inside handle_shared_mem_bank in order to reduce args count, remove not needed BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 86 +++-- 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 0a1c327e90ea..c15a65130659 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -180,6 +180,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, return 0; } +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, + const char *role_str, + const struct membank *shm_bank) +{ +bool owner_dom_io = true; +paddr_t pbase, psize; +int ret; + +pbase = shm_bank->start; +psize = shm_bank->size; + +/* + * "role" property is optional and if it is defined explicitly, + * then the owner domain is not the default "dom_io" domain. + */ +if ( role_str != NULL ) +owner_dom_io = false; + +/* + * 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 + * only find the borrowers. + */ +if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) +{ +/* + * 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); +if ( ret ) +return ret; +} + +if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) +{ +/* Set up P2M foreign mapping for borrower domain. */ +ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); +if ( ret ) +return ret; +} + +return 0; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -196,7 +243,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, unsigned int i; const char *role_str; const char *shm_id; -bool owner_dom_io = true; if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) continue; @@ -237,39 +283,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, 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", _str) == 0 ) -owner_dom_io = false; +/* "role" property is optional */ +if ( dt_property_read_string(shm_node, "role", _str) != 0 ) +role_str = NULL; -/* - * 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 - * only find the borrowers. - */ -if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || - (!owner_dom_io && strcmp(role_str, "owner") == 0) ) -{ -/* - * 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, - boot_shm_bank); -if ( ret ) -return ret; -} - -if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) -{ -/* Set up P2M foreign mapping for borrower domain. */ -ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), - _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); -if ( ret ) -return ret; -} +ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank); +if ( ret ) +return ret; /* * Record static shared memory region info for later setting -- 2.34.1
[PATCH v4 7/7] xen/docs: Describe static shared memory when host address is not provided
From: Penny Zheng This commit describe the new scenario where host address is not provided in "xen,shared-mem" property and a new example is added to the page to explain in details. Take the occasion to fix some typos in the page. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - Add Michal R-by v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-10-penny.zh...@arm.com/ with some changes in the commit message. --- docs/misc/arm/device-tree/booting.txt | 52 --- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index bbd955e9c2f6..ac4bad6fe5e0 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -590,7 +590,7 @@ communication. An array takes a physical address, which is the base address of the shared memory region in host physical address space, a size, and a guest physical address, as the target address of the mapping. -e.g. xen,shared-mem = < [host physical address] [guest address] [size] > +e.g. xen,shared-mem = < [host physical address] [guest address] [size] >; It shall also meet the following criteria: 1) If the SHM ID matches with an existing region, the address range of the @@ -601,8 +601,8 @@ communication. The number of cells for the host address (and size) is the same as the guest pseudo-physical address and they are inherited from the parent node. -Host physical address is optional, when missing Xen decides the location -(currently unimplemented). +Host physical address is optional, when missing Xen decides the location. +e.g. xen,shared-mem = < [guest address] [size] >; - role (Optional) @@ -629,7 +629,7 @@ chosen { role = "owner"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x1000 0x1000>; -} +}; domU1 { compatible = "xen,domain"; @@ -640,25 +640,36 @@ chosen { vpl011; /* - * shared memory region identified as 0x0(xen,shm-id = <0x0>) - * is shared between Dom0 and DomU1. + * shared memory region "my-shared-mem-0" is shared + * between Dom0 and DomU1. */ domU1-shared-mem@1000 { compatible = "xen,domain-shared-memory-v1"; role = "borrower"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x5000 0x1000>; -} +}; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between DomU1 and DomU2. + * shared memory region "my-shared-mem-1" is shared between + * DomU1 and DomU2. */ domU1-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x6000 0x2000>; -} +}; + +/* + * shared memory region "my-shared-mem-2" is shared between + * DomU1 and DomU2. + */ +domU1-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "owner"; +xen,shared-mem = <0x8000 0x2000>; +}; .. @@ -672,14 +683,21 @@ chosen { cpus = <1>; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between domU1 and domU2. + * shared memory region "my-shared-mem-1" is shared between + * domU1 and domU2. */ domU2-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x7000 0x2000>; -} +}; + +domU2-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "borrower"; +xen,shared-mem = <0x9000 0x2000>; +}; .. }; @@ -699,3 +717,11 @@ shared between DomU1 and DomU2. It will get mapped at 0x6000 in DomU1 guest physical address space, and at 0x7000 in DomU2 guest physical address space. DomU1 and DomU2 are both the borrower domain, the owner domain is the default owner domain DOMID_IO. + +For the static shared memory region "my-shared-mem-2", since host physical +address is not provided by user, Xen will automatically allocate 512MB +from heap a
[PATCH v4 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
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 Reviewed-by: Michal Orzel --- v4 changes: - Fix wrong condition for paddr_assigned (Michal) 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..bc093b9da9ea 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -264,6 +264,12 @@ int __init process_shm(struct domai
[PATCH v4 3/7] xen/p2m: put reference for level 2 superpage
From: Penny Zheng We are doing foreign memory mapping for static shared memory, and there is a great possibility that it could be super mapped. But today, p2m_put_l3_page could not handle superpages. This commits implements a new function p2m_put_l2_superpage to handle 2MB superpages, specifically for helping put extra references for foreign superpages. Modify relinquish_p2m_mapping as well to take into account preemption when type is foreign memory and order is above 9 (2MB). Currently 1GB superpages are not handled because Xen is not preemptible and therefore some work is needed to handle such superpages, for which at some point Xen might end up freeing memory and therefore for such a big mapping it could end up in a very long operation. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v4 changes: - optimised the path to call put_page() on the foreign mapping as Julien suggested. Add a comment in p2m_put_l2_superpage to state that any changes needs to take into account some change in the relinquish code. (Julien) v3 changes: - Add reasoning why we don't support now 1GB superpage, remove level_order variable from p2m_put_l2_superpage, update TODO comment inside p2m_free_entry, use XEN_PT_LEVEL_ORDER(2) instead of value 9 inside relinquish_p2m_mapping. (Michal) v2: - Do not handle 1GB super page as there might be some issue where a lot of calls to put_page(...) might be issued which could lead to free memory that is a long operation. v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/ --- xen/arch/arm/mmu/p2m.c | 82 +++--- 1 file changed, 62 insertions(+), 20 deletions(-) diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index 41fcca011cf4..986c5a03c54b 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -753,34 +753,66 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, return rc; } -/* - * Put any references on the single 4K page referenced by pte. - * TODO: Handle superpages, for now we only take special references for leaf - * pages (specifically foreign ones, which can't be super mapped today). - */ -static void p2m_put_l3_page(const lpae_t pte) +static void p2m_put_foreign_page(struct page_info *pg) { -mfn_t mfn = lpae_get_mfn(pte); - -ASSERT(p2m_is_valid(pte)); - /* - * TODO: Handle other p2m types - * * It's safe to do the put_page here because page_alloc will * flush the TLBs if the page is reallocated before the end of * this loop. */ -if ( p2m_is_foreign(pte.p2m.type) ) +put_page(pg); +} + +/* Put any references on the single 4K page referenced by mfn. */ +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type) +{ +/* TODO: Handle other p2m types */ +if ( p2m_is_foreign(type) ) { ASSERT(mfn_valid(mfn)); -put_page(mfn_to_page(mfn)); +p2m_put_foreign_page(mfn_to_page(mfn)); } /* Detect the xenheap page and mark the stored GFN as invalid. */ -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); } +/* Put any references on the superpage referenced by mfn. */ +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type) +{ +struct page_info *pg; +unsigned int i; + +/* + * TODO: Handle other p2m types, but be aware that any changes to handle + * different types should require an update on the relinquish code to handle + * preemption. + */ +if ( !p2m_is_foreign(type) ) +return; + +ASSERT(mfn_valid(mfn)); + +pg = mfn_to_page(mfn); + +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++, pg++ ) +p2m_put_foreign_page(pg); +} + +/* Put any references on the page referenced by pte. */ +static void p2m_put_page(const lpae_t pte, unsigned int level) +{ +mfn_t mfn = lpae_get_mfn(pte); + +ASSERT(p2m_is_valid(pte)); + +/* We have a second level 2M superpage */ +if ( p2m_is_superpage(pte, level) && (level == 2) ) +return p2m_put_l2_superpage(mfn, pte.p2m.type); +else if ( level == 3 ) +return p2m_put_l3_page(mfn, pte.p2m.type); +} + /* Free lpae sub-tree behind an entry */ static void p2m_free_entry(struct p2m_domain *p2m, lpae_t entry, unsigned int level) @@ -809,9 +841,16 @@ static void p2m_free_entry(struct p2m_domain *p2m, #endif p2m->stats.mappings[level]--; -/* Nothing to do if the entry is a super-page. */ -if ( level == 3 ) -p2m_put_l3_page(entry); +/* + * TODO: Currently we don't handle 1GB super-page, Xen is not + * preemptible and therefore some work is needed to handle such + * superpages, for which at some point Xen might end up freeing me
[PATCH v4 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
The function allocate_bank_memory allocates pages from the heap and maps them to the guest using guest_physmap_add_page. As a preparation work to support static shared memory bank when the host physical address is not provided, Xen needs to allocate memory from the heap, so rework allocate_bank_memory moving out the page allocation in a new function called allocate_domheap_memory. The function allocate_domheap_memory takes a callback function and a pointer to some extra information passed to the callback and this function will be called for every region, until a defined size is reached. In order to keep allocate_bank_memory functionality, the callback passed to allocate_domheap_memory is a wrapper for guest_physmap_add_page. Let allocate_domheap_memory be externally visible, in order to use it in the future from the static shared memory module. Take the opportunity to change the signature of allocate_bank_memory and remove the 'struct domain' parameter, which can be retrieved from 'struct kernel_info'. No functional changes is intended. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3 changes: - Add R-by Michal v2: - Reduced scope of pg var in allocate_domheap_memory, removed not necessary BUG_ON(), changed callback to return bool and fix comment. (Michal) --- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 79 + xen/arch/arm/include/asm/domain_build.h | 9 ++- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 74f053c242f4..20ddf6f8f250 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo) mem->nr_banks = 0; bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), bank_size) ) goto fail; bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), bank_size) ) goto fail; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 02e741685102..669970c86fd5 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -417,30 +417,15 @@ static void __init allocate_memory_11(struct domain *d, } #ifdef CONFIG_DOM0LESS_BOOT -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, - gfn_t sgfn, paddr_t tot_size) +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size, +alloc_domheap_mem_cb cb, void *extra) { -struct membanks *mem = kernel_info_get_mem(kinfo); -int res; -struct page_info *pg; -struct membank *bank; -unsigned int max_order = ~0; - -/* - * allocate_bank_memory can be called with a tot_size of zero for - * the second memory bank. It is not an error and we can safely - * avoid creating a zero-size memory bank. - */ -if ( tot_size == 0 ) -return true; - -bank = >bank[mem->nr_banks]; -bank->start = gfn_to_gaddr(sgfn); -bank->size = tot_size; +unsigned int max_order = UINT_MAX; while ( tot_size > 0 ) { unsigned int order = get_allocation_size(tot_size); +struct page_info *pg; order = min(max_order, order); @@ -463,17 +448,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, continue; } -res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order); -if ( res ) -{ -dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +if ( !cb(d, pg, order, extra) ) return false; -} -sgfn = gfn_add(sgfn, 1UL << order); tot_size -= (1ULL << (PAGE_SHIFT + order)); } +return true; +} + +static bool __init guest_map_pages(struct domain *d, struct page_info *pg, + unsigned int order, void *extra) +{ +gfn_t *sgfn = (gfn_t *)extra; +int res; + +BUG_ON(!sgfn); +res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order); +if ( res ) +{ +dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +return false; +} + +*sgfn = gfn_add(*sgfn, 1UL << order); + +return true; +} + +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn, + paddr_t tot_size) +{ +struct membanks *mem = kernel_info_get_mem(kinfo); +struct domain *d = kinfo->d; +
Re: [PATCH v3 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
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
Re: [PATCH v3 3/7] xen/p2m: put reference for level 2 superpage
Hi Julien, > On 22 May 2024, at 14:25, Julien Grall wrote: > >> diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c >> index 41fcca011cf4..b496266deef6 100644 >> --- a/xen/arch/arm/mmu/p2m.c >> +++ b/xen/arch/arm/mmu/p2m.c >> @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain >> *p2m, gfn_t gfn, >> return rc; >> } >> -/* >> - * Put any references on the single 4K page referenced by pte. >> - * TODO: Handle superpages, for now we only take special references for leaf >> - * pages (specifically foreign ones, which can't be super mapped today). >> - */ >> -static void p2m_put_l3_page(const lpae_t pte) >> +/* Put any references on the single 4K page referenced by mfn. */ >> +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type) >> { >> -mfn_t mfn = lpae_get_mfn(pte); >> - >> -ASSERT(p2m_is_valid(pte)); >> - >> /* >> * TODO: Handle other p2m types >> * >> @@ -771,16 +763,43 @@ static void p2m_put_l3_page(const lpae_t pte) >> * flush the TLBs if the page is reallocated before the end of >> * this loop. >> */ >> -if ( p2m_is_foreign(pte.p2m.type) ) >> +if ( p2m_is_foreign(type) ) >> { >> ASSERT(mfn_valid(mfn)); >> put_page(mfn_to_page(mfn)); >> } >> /* Detect the xenheap page and mark the stored GFN as invalid. */ >> -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) >> +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) >> page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); >> } > > All the pages within a 2MB mapping should be the same type. So... > >> +/* Put any references on the superpage referenced by mfn. */ >> +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type) >> +{ >> +unsigned int i; >> + >> +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ ) >> +{ >> +p2m_put_l3_page(mfn, type); >> + >> +mfn = mfn_add(mfn, 1); >> +} > > ... this solution is a bit wasteful as we will now call p2m_put_l3_page() 512 > times even though there is nothing to do. > > So instead can we move the checks outside to optimize the path a bit? You mean this? diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index b496266deef6..d40cddda48f3 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -794,7 +794,8 @@ static void p2m_put_page(const lpae_t pte, unsigned int level) ASSERT(p2m_is_valid(pte)); /* We have a second level 2M superpage */ -if ( p2m_is_superpage(pte, level) && (level == 2) ) +if ( p2m_is_superpage(pte, level) && (level == 2) && + p2m_is_foreign(pte.p2m.type) ) return p2m_put_l2_superpage(mfn, pte.p2m.type); else if ( level == 3 ) return p2m_put_l3_page(mfn, pte.p2m.type); > Otherwise... > >> +} >> + >> +/* Put any references on the page referenced by pte. */ >> +static void p2m_put_page(const lpae_t pte, unsigned int level) >> +{ >> +mfn_t mfn = lpae_get_mfn(pte); >> + >> +ASSERT(p2m_is_valid(pte)); >> + >> +/* We have a second level 2M superpage */ >> +if ( p2m_is_superpage(pte, level) && (level == 2) ) >> +return p2m_put_l2_superpage(mfn, pte.p2m.type); >> +else if ( level == 3 ) >> +return p2m_put_l3_page(mfn, pte.p2m.type); >> +} >> + >> /* Free lpae sub-tree behind an entry */ >> static void p2m_free_entry(struct p2m_domain *p2m, >> lpae_t entry, unsigned int level) >> @@ -809,9 +828,16 @@ static void p2m_free_entry(struct p2m_domain *p2m, >> #endif >>p2m->stats.mappings[level]--; >> -/* Nothing to do if the entry is a super-page. */ >> -if ( level == 3 ) >> -p2m_put_l3_page(entry); >> +/* >> + * TODO: Currently we don't handle 1GB super-page, Xen is not >> + * preemptible and therefore some work is needed to handle such >> + * superpages, for which at some point Xen might end up freeing >> memory >> + * and therefore for such a big mapping it could end up in a very >> long >> + * operation. >> + */ >> +if ( level >= 2 ) >> +p2m_put_page(entry, level); >> + >> return; >> } >> @@ -1558,9 +1584,12 @@ int relinquish_p2m_mapping(struct domain *d) >>count++; >> /* >> - * Arbitrarily preempt every 512 iterations. >> + * Arbitrarily preempt every 512 iterations or when type is foreign >> + * mapping and the order is above 9 (2MB). >> */ >> -if ( !(count % 512) && hypercall_preempt_check() ) >> +if ( (!(count % 512) || >> + (p2m_is_foreign(t) && (order > XEN_PT_LEVEL_ORDER(2 && > > ... we would need to preempt for every 2MB rather than just for the > p2m_is_foreign(). Ok otherwise you are suggesting that if we don’t go for the solution above we drop p2m_is_foreign(t) from the condition here, am I right? > > BTW, p2m_put_l3_page() has also another
[PATCH v3 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. Introduce the 'shm_heap_banks' for that reason, a struct that will hold the banks allocated from the heap, its field bank[].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 --- v3 changes: - reworded commit msg section, swap role_str and gbase in alloc_heap_pages_cb_extra to avoid padding hole in arm32, remove not needed printk, modify printk to print KB instead of MB, swap strncmp for strcmp, reduced memory footprint for shm_heap_banks. (Michal) 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 | 187 ++-- 1 file changed, 155 insertions(+), 32 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 74c81904b8a4..53e8d3ecf030 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -9,6 +9,25 @@ #include #include +typedef struct { +struct domain *d; +const char *role_str; +paddr_t gbase; +struct shmem_membank_extra *bank_extra_info; +} alloc_heap_pages_cb_extra; + +static struct { +struct membanks_hdr common; +struct membank bank[NR_SHMEM_BANKS]; +} shm_heap_banks __initdata = { +.common.max_banks = NR_SHMEM_BANKS +}; + +static inline struct membanks *get_shmem_heap_banks(void) +{ +return container_of(_heap_banks.common, struct membanks, common); +} + static void __init __maybe_unused build_assertions(void) { /* @@ -63,7 +82,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; @@ -83,19 +103,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; @@ -108,10 +140,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
[PATCH v3 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
The function allocate_bank_memory allocates pages from the heap and maps them to the guest using guest_physmap_add_page. As a preparation work to support static shared memory bank when the host physical address is not provided, Xen needs to allocate memory from the heap, so rework allocate_bank_memory moving out the page allocation in a new function called allocate_domheap_memory. The function allocate_domheap_memory takes a callback function and a pointer to some extra information passed to the callback and this function will be called for every region, until a defined size is reached. In order to keep allocate_bank_memory functionality, the callback passed to allocate_domheap_memory is a wrapper for guest_physmap_add_page. Let allocate_domheap_memory be externally visible, in order to use it in the future from the static shared memory module. Take the opportunity to change the signature of allocate_bank_memory and remove the 'struct domain' parameter, which can be retrieved from 'struct kernel_info'. No functional changes is intended. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3 changes: - Add R-by Michal v2: - Reduced scope of pg var in allocate_domheap_memory, removed not necessary BUG_ON(), changed callback to return bool and fix comment. (Michal) --- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 79 + xen/arch/arm/include/asm/domain_build.h | 9 ++- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 74f053c242f4..20ddf6f8f250 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo) mem->nr_banks = 0; bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), bank_size) ) goto fail; bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), bank_size) ) goto fail; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 02e741685102..669970c86fd5 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -417,30 +417,15 @@ static void __init allocate_memory_11(struct domain *d, } #ifdef CONFIG_DOM0LESS_BOOT -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, - gfn_t sgfn, paddr_t tot_size) +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size, +alloc_domheap_mem_cb cb, void *extra) { -struct membanks *mem = kernel_info_get_mem(kinfo); -int res; -struct page_info *pg; -struct membank *bank; -unsigned int max_order = ~0; - -/* - * allocate_bank_memory can be called with a tot_size of zero for - * the second memory bank. It is not an error and we can safely - * avoid creating a zero-size memory bank. - */ -if ( tot_size == 0 ) -return true; - -bank = >bank[mem->nr_banks]; -bank->start = gfn_to_gaddr(sgfn); -bank->size = tot_size; +unsigned int max_order = UINT_MAX; while ( tot_size > 0 ) { unsigned int order = get_allocation_size(tot_size); +struct page_info *pg; order = min(max_order, order); @@ -463,17 +448,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, continue; } -res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order); -if ( res ) -{ -dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +if ( !cb(d, pg, order, extra) ) return false; -} -sgfn = gfn_add(sgfn, 1UL << order); tot_size -= (1ULL << (PAGE_SHIFT + order)); } +return true; +} + +static bool __init guest_map_pages(struct domain *d, struct page_info *pg, + unsigned int order, void *extra) +{ +gfn_t *sgfn = (gfn_t *)extra; +int res; + +BUG_ON(!sgfn); +res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order); +if ( res ) +{ +dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +return false; +} + +*sgfn = gfn_add(*sgfn, 1UL << order); + +return true; +} + +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn, + paddr_t tot_size) +{ +struct membanks *mem = kernel_info_get_mem(kinfo); +struct domain *d = kinfo->d; +
[PATCH v3 7/7] xen/docs: Describe static shared memory when host address is not provided
From: Penny Zheng This commit describe the new scenario where host address is not provided in "xen,shared-mem" property and a new example is added to the page to explain in details. Take the occasion to fix some typos in the page. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - Add Michal R-by v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-10-penny.zh...@arm.com/ with some changes in the commit message. --- docs/misc/arm/device-tree/booting.txt | 52 --- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index bbd955e9c2f6..ac4bad6fe5e0 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -590,7 +590,7 @@ communication. An array takes a physical address, which is the base address of the shared memory region in host physical address space, a size, and a guest physical address, as the target address of the mapping. -e.g. xen,shared-mem = < [host physical address] [guest address] [size] > +e.g. xen,shared-mem = < [host physical address] [guest address] [size] >; It shall also meet the following criteria: 1) If the SHM ID matches with an existing region, the address range of the @@ -601,8 +601,8 @@ communication. The number of cells for the host address (and size) is the same as the guest pseudo-physical address and they are inherited from the parent node. -Host physical address is optional, when missing Xen decides the location -(currently unimplemented). +Host physical address is optional, when missing Xen decides the location. +e.g. xen,shared-mem = < [guest address] [size] >; - role (Optional) @@ -629,7 +629,7 @@ chosen { role = "owner"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x1000 0x1000>; -} +}; domU1 { compatible = "xen,domain"; @@ -640,25 +640,36 @@ chosen { vpl011; /* - * shared memory region identified as 0x0(xen,shm-id = <0x0>) - * is shared between Dom0 and DomU1. + * shared memory region "my-shared-mem-0" is shared + * between Dom0 and DomU1. */ domU1-shared-mem@1000 { compatible = "xen,domain-shared-memory-v1"; role = "borrower"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x5000 0x1000>; -} +}; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between DomU1 and DomU2. + * shared memory region "my-shared-mem-1" is shared between + * DomU1 and DomU2. */ domU1-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x6000 0x2000>; -} +}; + +/* + * shared memory region "my-shared-mem-2" is shared between + * DomU1 and DomU2. + */ +domU1-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "owner"; +xen,shared-mem = <0x8000 0x2000>; +}; .. @@ -672,14 +683,21 @@ chosen { cpus = <1>; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between domU1 and domU2. + * shared memory region "my-shared-mem-1" is shared between + * domU1 and domU2. */ domU2-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x7000 0x2000>; -} +}; + +domU2-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "borrower"; +xen,shared-mem = <0x9000 0x2000>; +}; .. }; @@ -699,3 +717,11 @@ shared between DomU1 and DomU2. It will get mapped at 0x6000 in DomU1 guest physical address space, and at 0x7000 in DomU2 guest physical address space. DomU1 and DomU2 are both the borrower domain, the owner domain is the default owner domain DOMID_IO. + +For the static shared memory region "my-shared-mem-2", since host physical +address is not provided by user, Xen will automatically allocate 512MB +from heap a
[PATCH v3 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
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 Reviewed-by: Michal Orzel --- 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, pba
[PATCH v3 0/7] Static shared memory followup v2 - pt2
This serie is a partial rework of this other serie: https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ The original serie is addressing an issue of the static shared memory feature that impacts the memory footprint of other component when the feature is enabled, another issue impacts the device tree generation for the guests when the feature is enabled and used and the last one is a missing feature that is the option to have a static shared memory region that is not from the host address space. This serie is handling some comment on the original serie and it is splitting the rework in two part, this first part is addressing the memory footprint issue and the device tree generation and currently is fully merged (https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/), this serie is addressing the static shared memory allocation from the Xen heap. Luca Fancellu (5): xen/arm: Lookup bootinfo shm bank during the mapping xen/arm: Wrap shared memory mapping code in one function xen/arm: Parse xen,shared-mem when host phys address is not provided xen/arm: Rework heap page allocation outside allocate_bank_memory xen/arm: Implement the logic for static shared memory from Xen heap Penny Zheng (2): xen/p2m: put reference for level 2 superpage xen/docs: Describe static shared memory when host address is not provided docs/misc/arm/device-tree/booting.txt | 52 ++- xen/arch/arm/arm32/mmu/mm.c | 11 +- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 84 +++-- xen/arch/arm/include/asm/domain_build.h | 9 +- xen/arch/arm/mmu/p2m.c | 63 +++- xen/arch/arm/setup.c| 14 +- xen/arch/arm/static-shmem.c | 432 +--- 8 files changed, 486 insertions(+), 183 deletions(-) -- 2.34.1
[PATCH v3 2/7] xen/arm: Wrap shared memory mapping code in one function
Wrap the code and logic that is calling assign_shared_memory and map_regions_p2mt into a new function 'handle_shared_mem_bank', it will become useful later when the code will allow the user to don't pass the host physical address. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3 changes: - check return value of dt_property_read_string, add R-by Michal v2 changes: - add blank line, move owner_dom_io computation inside handle_shared_mem_bank in order to reduce args count, remove not needed BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 86 +++-- 1 file changed, 53 insertions(+), 33 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 0a1c327e90ea..c15a65130659 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -180,6 +180,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, return 0; } +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, + const char *role_str, + const struct membank *shm_bank) +{ +bool owner_dom_io = true; +paddr_t pbase, psize; +int ret; + +pbase = shm_bank->start; +psize = shm_bank->size; + +/* + * "role" property is optional and if it is defined explicitly, + * then the owner domain is not the default "dom_io" domain. + */ +if ( role_str != NULL ) +owner_dom_io = false; + +/* + * 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 + * only find the borrowers. + */ +if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) +{ +/* + * 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); +if ( ret ) +return ret; +} + +if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) +{ +/* Set up P2M foreign mapping for borrower domain. */ +ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); +if ( ret ) +return ret; +} + +return 0; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -196,7 +243,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, unsigned int i; const char *role_str; const char *shm_id; -bool owner_dom_io = true; if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) continue; @@ -237,39 +283,13 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, 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", _str) == 0 ) -owner_dom_io = false; +/* "role" property is optional */ +if ( dt_property_read_string(shm_node, "role", _str) != 0 ) +role_str = NULL; -/* - * 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 - * only find the borrowers. - */ -if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || - (!owner_dom_io && strcmp(role_str, "owner") == 0) ) -{ -/* - * 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, - boot_shm_bank); -if ( ret ) -return ret; -} - -if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) -{ -/* Set up P2M foreign mapping for borrower domain. */ -ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), - _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); -if ( ret ) -return ret; -} +ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank); +if ( ret ) +return ret; /* * Record static shared memory region info for later setting -- 2.34.1
[PATCH v3 3/7] xen/p2m: put reference for level 2 superpage
From: Penny Zheng We are doing foreign memory mapping for static shared memory, and there is a great possibility that it could be super mapped. But today, p2m_put_l3_page could not handle superpages. This commits implements a new function p2m_put_l2_superpage to handle 2MB superpages, specifically for helping put extra references for foreign superpages. Modify relinquish_p2m_mapping as well to take into account preemption when type is foreign memory and order is above 9 (2MB). Currently 1GB superpages are not handled because Xen is not preemptible and therefore some work is needed to handle such superpages, for which at some point Xen might end up freeing memory and therefore for such a big mapping it could end up in a very long operation. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v3: - Add reasoning why we don't support now 1GB superpage, remove level_order variable from p2m_put_l2_superpage, update TODO comment inside p2m_free_entry, use XEN_PT_LEVEL_ORDER(2) instead of value 9 inside relinquish_p2m_mapping. (Michal) v2: - Do not handle 1GB super page as there might be some issue where a lot of calls to put_page(...) might be issued which could lead to free memory that is a long operation. v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/ --- xen/arch/arm/mmu/p2m.c | 63 ++ 1 file changed, 46 insertions(+), 17 deletions(-) diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index 41fcca011cf4..b496266deef6 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, return rc; } -/* - * Put any references on the single 4K page referenced by pte. - * TODO: Handle superpages, for now we only take special references for leaf - * pages (specifically foreign ones, which can't be super mapped today). - */ -static void p2m_put_l3_page(const lpae_t pte) +/* Put any references on the single 4K page referenced by mfn. */ +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type) { -mfn_t mfn = lpae_get_mfn(pte); - -ASSERT(p2m_is_valid(pte)); - /* * TODO: Handle other p2m types * @@ -771,16 +763,43 @@ static void p2m_put_l3_page(const lpae_t pte) * flush the TLBs if the page is reallocated before the end of * this loop. */ -if ( p2m_is_foreign(pte.p2m.type) ) +if ( p2m_is_foreign(type) ) { ASSERT(mfn_valid(mfn)); put_page(mfn_to_page(mfn)); } /* Detect the xenheap page and mark the stored GFN as invalid. */ -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); } +/* Put any references on the superpage referenced by mfn. */ +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type) +{ +unsigned int i; + +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ ) +{ +p2m_put_l3_page(mfn, type); + +mfn = mfn_add(mfn, 1); +} +} + +/* Put any references on the page referenced by pte. */ +static void p2m_put_page(const lpae_t pte, unsigned int level) +{ +mfn_t mfn = lpae_get_mfn(pte); + +ASSERT(p2m_is_valid(pte)); + +/* We have a second level 2M superpage */ +if ( p2m_is_superpage(pte, level) && (level == 2) ) +return p2m_put_l2_superpage(mfn, pte.p2m.type); +else if ( level == 3 ) +return p2m_put_l3_page(mfn, pte.p2m.type); +} + /* Free lpae sub-tree behind an entry */ static void p2m_free_entry(struct p2m_domain *p2m, lpae_t entry, unsigned int level) @@ -809,9 +828,16 @@ static void p2m_free_entry(struct p2m_domain *p2m, #endif p2m->stats.mappings[level]--; -/* Nothing to do if the entry is a super-page. */ -if ( level == 3 ) -p2m_put_l3_page(entry); +/* + * TODO: Currently we don't handle 1GB super-page, Xen is not + * preemptible and therefore some work is needed to handle such + * superpages, for which at some point Xen might end up freeing memory + * and therefore for such a big mapping it could end up in a very long + * operation. + */ +if ( level >= 2 ) +p2m_put_page(entry, level); + return; } @@ -1558,9 +1584,12 @@ int relinquish_p2m_mapping(struct domain *d) count++; /* - * Arbitrarily preempt every 512 iterations. + * Arbitrarily preempt every 512 iterations or when type is foreign + * mapping and the order is above 9 (2MB). */ -if ( !(count % 512) && hypercall_preempt_check() ) +if ( (!(count % 512) || + (p2m_is_foreign(t) && (order > XEN_PT_LEVEL_ORDER(2 && +
[PATCH v3 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
The current static shared memory code is using bootinfo banks when it needs to find the number of borrowers, so every time assign_shared_memory is called, the bank is searched in the bootinfo.shmem structure. There is nothing wrong with it, however the bank can be used also to retrieve the start address and size and also to pass less argument to assign_shared_memory. When retrieving the information from the bootinfo bank, it's also possible to move the checks on alignment to process_shm_node in the early stages. So create a new function find_shm_bank_by_id() which takes a 'struct shared_meminfo' structure and the shared memory ID, to look for a bank with a matching ID, take the physical host address and size from the bank, pass the bank to assign_shared_memory() removing the now unnecessary arguments and finally remove the acquire_nr_borrower_domain() function since now the information can be extracted from the passed bank. Move the "xen,shm-id" parsing early in process_shm to bail out quickly in case of errors (unlikely), as said above, move the checks on alignment to process_shm_node. Drawback of this change is that now the bootinfo are used also when the bank doesn't need to be allocated, however it will be convenient later to use it as an argument for assign_shared_memory when dealing with the use case where the Host physical address is not supplied by the user. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3 changes: - switch strncmp with strcmp in find_shm_bank_by_id, fix commit msg typo, add R-by Michal. v2 changes: - fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(), swap region size check different from zero and size alignment, remove not necessary BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 100 +++- 1 file changed, 53 insertions(+), 47 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 78881dd1d3f7..0a1c327e90ea 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -19,29 +19,21 @@ static void __init __maybe_unused build_assertions(void) offsetof(struct shared_meminfo, bank))); } -static int __init acquire_nr_borrower_domain(struct domain *d, - paddr_t pbase, paddr_t psize, - unsigned long *nr_borrowers) +static const struct membank __init * +find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id) { -const struct membanks *shmem = bootinfo_get_shmem(); unsigned int bank; -/* Iterate reserved memory to find requested shm bank. */ for ( bank = 0 ; bank < shmem->nr_banks; bank++ ) { -paddr_t bank_start = shmem->bank[bank].start; -paddr_t bank_size = shmem->bank[bank].size; - -if ( (pbase == bank_start) && (psize == bank_size) ) +if ( strcmp(shm_id, shmem->bank[bank].shmem_extra->shm_id) == 0 ) break; } if ( bank == shmem->nr_banks ) -return -ENOENT; +return NULL; -*nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers; - -return 0; +return >bank[bank]; } /* @@ -103,14 +95,18 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, return smfn; } -static int __init assign_shared_memory(struct domain *d, - paddr_t pbase, paddr_t psize, - paddr_t gbase) +static int __init assign_shared_memory(struct domain *d, paddr_t gbase, + const struct membank *shm_bank) { mfn_t smfn; int ret = 0; unsigned long nr_pages, nr_borrowers, i; struct page_info *page; +paddr_t pbase, psize; + +pbase = shm_bank->start; +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); @@ -135,14 +131,6 @@ static int __init assign_shared_memory(struct domain *d, } } -/* - * Get the right amount of references per page, which is the number of - * borrower domains. - */ -ret = acquire_nr_borrower_domain(d, pbase, psize, _borrowers); -if ( ret ) -return ret; - /* * Instead of letting borrower domain get a page ref, we add as many * additional reference as the number of borrowers when the owner @@ -199,6 +187,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, dt_for_each_child_node(node, shm_node) { +const struct membank *boot_shm_bank; const struct dt_property *prop; const __be32 *cells; uint32_t addr_cells, size_cells; @@ -212,6 +201,23 @@ int __init process_shm(struct domain *d, struct kernel_
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, + _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
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",
Re: [PATCH v3 1/4] xen/arm/static-shmem: Static-shmem should be direct-mapped for direct-mapped domains
> On 20 May 2024, at 12:24, Michal Orzel wrote: > > Hi Henry, > > +CC: Luca > > On 17/05/2024 05:21, Henry Wang wrote: >> >> >> Currently, users are allowed to map static shared memory in a >> non-direct-mapped way for direct-mapped domains. This can lead to >> clashing of guest memory spaces. Also, the current extended region >> finding logic only removes the host physical addresses of the >> static shared memory areas for direct-mapped domains, which may be >> inconsistent with the guest memory map if users map the static >> shared memory in a non-direct-mapped way. This will lead to incorrect >> extended region calculation results. >> >> To make things easier, add restriction that static shared memory >> should also be direct-mapped for direct-mapped domains. Check the >> host physical address to be matched with guest physical address when >> parsing the device tree. Document this restriction in the doc. > I'm ok with this restriction. > > @Luca, do you have any use case preventing us from making this restriction? Hi Michal, Henry, I think it’s sensible, I don’t think we have any use case for direct-mapped domains using non direct mapped static shared memory. Cheers, Luca
Re: [PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function
Hi Michal, >> >> -/* >> - * "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", _str) == 0 ) >> -owner_dom_io = false; >> +/* "role" property is optional */ >> +dt_property_read_string(shm_node, "role", _str); > This now violates a MISRA rule saying that if a function returns a value, > this value needs to be checked. > I think you should check if return value is non zero and if such, assign > role_str NULL (thus removing such > assignment from a definition). Sure, I’ll do it. > > Other than that: > Reviewed-by: Michal Orzel > > ~Michal Cheers, Luca
[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(_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,
[PATCH v2 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
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 don't 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 --- 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 23150122f7c4..3d46f32831bf 100644 --- a/xen/arch/arm/arm32/mmu/mm.c +++ b/xen/arch/arm/arm32/mmu/mm.c @@ -128,8 +128,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 f6550809cfdf..4febba69ef63 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -926,6 +926,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 d242674381d4..016859f6982a 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -265,8 +265,15 @@ static void __init dt_unreserved_regions(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 ) { @@ -297,7 +304,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 8a14d120690c..ddaacbc77740 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -265,6 +265,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 ==
[PATCH v2 0/7] Static shared memory followup v2 - pt2
This serie is a partial rework of this other serie: https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ The original serie is addressing an issue of the static shared memory feature that impacts the memory footprint of other component when the feature is enabled, another issue impacts the device tree generation for the guests when the feature is enabled and used and the last one is a missing feature that is the option to have a static shared memory region that is not from the host address space. This serie is handling some comment on the original serie and it is splitting the rework in two part, this first part is addressing the memory footprint issue and the device tree generation and currently is fully merged (https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/), this serie is addressing the static shared memory allocation from the Xen heap. Luca Fancellu (5): xen/arm: Lookup bootinfo shm bank during the mapping xen/arm: Wrap shared memory mapping code in one function xen/arm: Parse xen,shared-mem when host phys address is not provided xen/arm: Rework heap page allocation outside allocate_bank_memory xen/arm: Implement the logic for static shared memory from Xen heap Penny Zheng (2): xen/p2m: put reference for level 2 superpage xen/docs: Describe static shared memory when host address is not provided docs/misc/arm/device-tree/booting.txt | 52 ++- xen/arch/arm/arm32/mmu/mm.c | 11 +- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 84 +++-- xen/arch/arm/include/asm/domain_build.h | 9 +- xen/arch/arm/mmu/p2m.c | 57 +++- xen/arch/arm/setup.c| 14 +- xen/arch/arm/static-shmem.c | 435 +--- 8 files changed, 482 insertions(+), 184 deletions(-) -- 2.34.1
[PATCH v2 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
The function allocate_bank_memory allocates pages from the heap and maps them to the guest using guest_physmap_add_page. As a preparation work to support static shared memory bank when the host physical address is not provided, Xen needs to allocate memory from the heap, so rework allocate_bank_memory moving out the page allocation in a new function called allocate_domheap_memory. The function allocate_domheap_memory takes a callback function and a pointer to some extra information passed to the callback and this function will be called for every region, until a defined size is reached. In order to keep allocate_bank_memory functionality, the callback passed to allocate_domheap_memory is a wrapper for guest_physmap_add_page. Let allocate_domheap_memory be externally visible, in order to use it in the future from the static shared memory module. Take the opportunity to change the signature of allocate_bank_memory and remove the 'struct domain' parameter, which can be retrieved from 'struct kernel_info'. No functional changes is intended. Signed-off-by: Luca Fancellu --- v2: - Reduced scope of pg var in allocate_domheap_memory, removed not necessary BUG_ON(), changed callback to return bool and fix comment. (Michal) --- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 79 + xen/arch/arm/include/asm/domain_build.h | 9 ++- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 74f053c242f4..20ddf6f8f250 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo) mem->nr_banks = 0; bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), bank_size) ) goto fail; bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), bank_size) ) goto fail; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 4febba69ef63..fd8babacb9df 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -417,30 +417,15 @@ static void __init allocate_memory_11(struct domain *d, } #ifdef CONFIG_DOM0LESS_BOOT -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, - gfn_t sgfn, paddr_t tot_size) +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size, +alloc_domheap_mem_cb cb, void *extra) { -struct membanks *mem = kernel_info_get_mem(kinfo); -int res; -struct page_info *pg; -struct membank *bank; -unsigned int max_order = ~0; - -/* - * allocate_bank_memory can be called with a tot_size of zero for - * the second memory bank. It is not an error and we can safely - * avoid creating a zero-size memory bank. - */ -if ( tot_size == 0 ) -return true; - -bank = >bank[mem->nr_banks]; -bank->start = gfn_to_gaddr(sgfn); -bank->size = tot_size; +unsigned int max_order = UINT_MAX; while ( tot_size > 0 ) { unsigned int order = get_allocation_size(tot_size); +struct page_info *pg; order = min(max_order, order); @@ -463,17 +448,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, continue; } -res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order); -if ( res ) -{ -dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +if ( !cb(d, pg, order, extra) ) return false; -} -sgfn = gfn_add(sgfn, 1UL << order); tot_size -= (1ULL << (PAGE_SHIFT + order)); } +return true; +} + +static bool __init guest_map_pages(struct domain *d, struct page_info *pg, + unsigned int order, void *extra) +{ +gfn_t *sgfn = (gfn_t *)extra; +int res; + +BUG_ON(!sgfn); +res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order); +if ( res ) +{ +dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +return false; +} + +*sgfn = gfn_add(*sgfn, 1UL << order); + +return true; +} + +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn, + paddr_t tot_size) +{ +struct membanks *mem = kernel_info_get_mem(kinfo); +struct domain *d = kinfo->d; +struct membank *bank; + +/* + * allocate_bank_me
[PATCH v2 2/7] xen/arm: Wrap shared memory mapping code in one function
Wrap the code and logic that is calling assign_shared_memory and map_regions_p2mt into a new function 'handle_shared_mem_bank', it will become useful later when the code will allow the user to don't pass the host physical address. Signed-off-by: Luca Fancellu --- v2 changes: - add blank line, move owner_dom_io computation inside handle_shared_mem_bank in order to reduce args count, remove not needed BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 87 ++--- 1 file changed, 53 insertions(+), 34 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 0afc86c43f85..8a14d120690c 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -181,6 +181,53 @@ append_shm_bank_to_domain(struct kernel_info *kinfo, paddr_t start, return 0; } +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, + const char *role_str, + const struct membank *shm_bank) +{ +bool owner_dom_io = true; +paddr_t pbase, psize; +int ret; + +pbase = shm_bank->start; +psize = shm_bank->size; + +/* + * "role" property is optional and if it is defined explicitly, + * then the owner domain is not the default "dom_io" domain. + */ +if ( role_str != NULL ) +owner_dom_io = false; + +/* + * 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 + * only find the borrowers. + */ +if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) +{ +/* + * 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); +if ( ret ) +return ret; +} + +if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) +{ +/* Set up P2M foreign mapping for borrower domain. */ +ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); +if ( ret ) +return ret; +} + +return 0; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -195,9 +242,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, paddr_t gbase, pbase, psize; int ret = 0; unsigned int i; -const char *role_str; +const char *role_str = NULL; const char *shm_id; -bool owner_dom_io = true; if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) continue; @@ -238,39 +284,12 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, 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", _str) == 0 ) -owner_dom_io = false; +/* "role" property is optional */ +dt_property_read_string(shm_node, "role", _str); -/* - * 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 - * only find the borrowers. - */ -if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || - (!owner_dom_io && strcmp(role_str, "owner") == 0) ) -{ -/* - * 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, - boot_shm_bank); -if ( ret ) -return ret; -} - -if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) -{ -/* Set up P2M foreign mapping for borrower domain. */ -ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), - _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); -if ( ret ) -return ret; -} +ret = handle_shared_mem_bank(d, gbase, role_str, boot_shm_bank); +if ( ret ) +return ret; /* * Record static shared memory region info for later setting -- 2.34.1
[PATCH v2 7/7] xen/docs: Describe static shared memory when host address is not provided
From: Penny Zheng This commit describe the new scenario where host address is not provided in "xen,shared-mem" property and a new example is added to the page to explain in details. Take the occasion to fix some typos in the page. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - Add Michal R-by v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-10-penny.zh...@arm.com/ with some changes in the commit message. --- docs/misc/arm/device-tree/booting.txt | 52 --- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index bbd955e9c2f6..ac4bad6fe5e0 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -590,7 +590,7 @@ communication. An array takes a physical address, which is the base address of the shared memory region in host physical address space, a size, and a guest physical address, as the target address of the mapping. -e.g. xen,shared-mem = < [host physical address] [guest address] [size] > +e.g. xen,shared-mem = < [host physical address] [guest address] [size] >; It shall also meet the following criteria: 1) If the SHM ID matches with an existing region, the address range of the @@ -601,8 +601,8 @@ communication. The number of cells for the host address (and size) is the same as the guest pseudo-physical address and they are inherited from the parent node. -Host physical address is optional, when missing Xen decides the location -(currently unimplemented). +Host physical address is optional, when missing Xen decides the location. +e.g. xen,shared-mem = < [guest address] [size] >; - role (Optional) @@ -629,7 +629,7 @@ chosen { role = "owner"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x1000 0x1000>; -} +}; domU1 { compatible = "xen,domain"; @@ -640,25 +640,36 @@ chosen { vpl011; /* - * shared memory region identified as 0x0(xen,shm-id = <0x0>) - * is shared between Dom0 and DomU1. + * shared memory region "my-shared-mem-0" is shared + * between Dom0 and DomU1. */ domU1-shared-mem@1000 { compatible = "xen,domain-shared-memory-v1"; role = "borrower"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x5000 0x1000>; -} +}; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between DomU1 and DomU2. + * shared memory region "my-shared-mem-1" is shared between + * DomU1 and DomU2. */ domU1-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x6000 0x2000>; -} +}; + +/* + * shared memory region "my-shared-mem-2" is shared between + * DomU1 and DomU2. + */ +domU1-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "owner"; +xen,shared-mem = <0x8000 0x2000>; +}; .. @@ -672,14 +683,21 @@ chosen { cpus = <1>; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between domU1 and domU2. + * shared memory region "my-shared-mem-1" is shared between + * domU1 and domU2. */ domU2-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x7000 0x2000>; -} +}; + +domU2-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "borrower"; +xen,shared-mem = <0x9000 0x2000>; +}; .. }; @@ -699,3 +717,11 @@ shared between DomU1 and DomU2. It will get mapped at 0x6000 in DomU1 guest physical address space, and at 0x7000 in DomU2 guest physical address space. DomU1 and DomU2 are both the borrower domain, the owner domain is the default owner domain DOMID_IO. + +For the static shared memory region "my-shared-mem-2", since host physical +address is not provided by user, Xen will automatically allocate 512MB +from heap a
[PATCH v2 3/7] xen/p2m: put reference for level 2 superpage
From: Penny Zheng We are doing foreign memory mapping for static shared memory, and there is a great possibility that it could be super mapped. But today, p2m_put_l3_page could not handle superpages. This commits implements a new function p2m_put_l2_superpage to handle 2MB superpages, specifically for helping put extra references for foreign superpages. Modify relinquish_p2m_mapping as well to take into account preemption when type is foreign memory and order is above 9 (2MB). Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v2: - Do not handle 1GB super page as there might be some issue where a lot of calls to put_page(...) might be issued which could lead to free memory that is a long operation. v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/ --- xen/arch/arm/mmu/p2m.c | 57 +- 1 file changed, 40 insertions(+), 17 deletions(-) diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index 41fcca011cf4..29fdb3b87dd0 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, return rc; } -/* - * Put any references on the single 4K page referenced by pte. - * TODO: Handle superpages, for now we only take special references for leaf - * pages (specifically foreign ones, which can't be super mapped today). - */ -static void p2m_put_l3_page(const lpae_t pte) +/* Put any references on the single 4K page referenced by mfn. */ +static void p2m_put_l3_page(mfn_t mfn, p2m_type_t type) { -mfn_t mfn = lpae_get_mfn(pte); - -ASSERT(p2m_is_valid(pte)); - /* * TODO: Handle other p2m types * @@ -771,16 +763,44 @@ static void p2m_put_l3_page(const lpae_t pte) * flush the TLBs if the page is reallocated before the end of * this loop. */ -if ( p2m_is_foreign(pte.p2m.type) ) +if ( p2m_is_foreign(type) ) { ASSERT(mfn_valid(mfn)); put_page(mfn_to_page(mfn)); } /* Detect the xenheap page and mark the stored GFN as invalid. */ -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); } +/* Put any references on the superpage referenced by mfn. */ +static void p2m_put_l2_superpage(mfn_t mfn, p2m_type_t type) +{ +unsigned int i; +unsigned int level_order = XEN_PT_LEVEL_ORDER(3); + +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ ) +{ +p2m_put_l3_page(mfn, type); + +mfn = mfn_add(mfn, 1 << level_order); +} +} + +/* Put any references on the page referenced by pte. */ +static void p2m_put_page(const lpae_t pte, unsigned int level) +{ +mfn_t mfn = lpae_get_mfn(pte); + +ASSERT(p2m_is_valid(pte)); + +/* We have a second level 2M superpage */ +if ( p2m_is_superpage(pte, level) && (level == 2) ) +return p2m_put_l2_superpage(mfn, pte.p2m.type); +else if ( level == 3 ) +return p2m_put_l3_page(mfn, pte.p2m.type); +} + /* Free lpae sub-tree behind an entry */ static void p2m_free_entry(struct p2m_domain *p2m, lpae_t entry, unsigned int level) @@ -809,9 +829,10 @@ static void p2m_free_entry(struct p2m_domain *p2m, #endif p2m->stats.mappings[level]--; -/* Nothing to do if the entry is a super-page. */ -if ( level == 3 ) -p2m_put_l3_page(entry); +/* TODO: Currently we don't handle 1GB super-page. */ +if ( level >= 2 ) +p2m_put_page(entry, level); + return; } @@ -1558,9 +1579,11 @@ int relinquish_p2m_mapping(struct domain *d) count++; /* - * Arbitrarily preempt every 512 iterations. + * Arbitrarily preempt every 512 iterations or when type is foreign + * mapping and the order is above 9 (2MB). */ -if ( !(count % 512) && hypercall_preempt_check() ) +if ( (!(count % 512) || (p2m_is_foreign(t) && (order > 9))) && + hypercall_preempt_check() ) { rc = -ERESTART; break; -- 2.34.1
[PATCH v2 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
The current static shared memory code is using bootinfo banks when it needs to find the number of borrowers, so every time assign_shared_memory is called, the bank is searched in the bootinfo.shmem structure. There is nothing wrong with it, however the bank can be used also to retrieve the start address and size and also to pass less argument to assign_shared_memory. When retrieving the information from the bootinfo bank, it's also possible to move the checks on alignment to process_shm_node in the early stages. So create a new function find_shm_bank_by_id() which takes a 'struct shared_meminfo' structure and the shared memory ID, to look for a bank with a matching ID, take the physical host address and size from the bank, pass the bank to assign_shared_memory() removing the now unnecessary arguments and finally remove the acquire_nr_borrower_domain() function since now the information can be extracted from the passed bank. Move the "xen,shm-id" parsing early in process_shm to bail out quickly in case of errors (unlikely), as said above, move the checks on alignment to process_shm_node. Drawback of this change is that now the bootinfo are used also when the bank doesn't need to be allocated, however it will be convinient later to use it as an argument for assign_shared_memory when dealing with the use case where the Host physical address is not supplied by the user. Signed-off-by: Luca Fancellu --- v2 changes: - fix typo commit msg, renamed find_shm() to find_shm_bank_by_id(), swap region size check different from zero and size alignment, remove not necessary BUGON(). (Michal) --- xen/arch/arm/static-shmem.c | 101 +++- 1 file changed, 54 insertions(+), 47 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 78881dd1d3f7..0afc86c43f85 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -19,29 +19,22 @@ static void __init __maybe_unused build_assertions(void) offsetof(struct shared_meminfo, bank))); } -static int __init acquire_nr_borrower_domain(struct domain *d, - paddr_t pbase, paddr_t psize, - unsigned long *nr_borrowers) +static const struct membank __init * +find_shm_bank_by_id(const struct membanks *shmem, const char *shm_id) { -const struct membanks *shmem = bootinfo_get_shmem(); unsigned int bank; -/* Iterate reserved memory to find requested shm bank. */ for ( bank = 0 ; bank < shmem->nr_banks; bank++ ) { -paddr_t bank_start = shmem->bank[bank].start; -paddr_t bank_size = shmem->bank[bank].size; - -if ( (pbase == bank_start) && (psize == bank_size) ) +if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id, + MAX_SHM_ID_LENGTH) == 0 ) break; } if ( bank == shmem->nr_banks ) -return -ENOENT; +return NULL; -*nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers; - -return 0; +return >bank[bank]; } /* @@ -103,14 +96,18 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, return smfn; } -static int __init assign_shared_memory(struct domain *d, - paddr_t pbase, paddr_t psize, - paddr_t gbase) +static int __init assign_shared_memory(struct domain *d, paddr_t gbase, + const struct membank *shm_bank) { mfn_t smfn; int ret = 0; unsigned long nr_pages, nr_borrowers, i; struct page_info *page; +paddr_t pbase, psize; + +pbase = shm_bank->start; +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); @@ -135,14 +132,6 @@ static int __init assign_shared_memory(struct domain *d, } } -/* - * Get the right amount of references per page, which is the number of - * borrower domains. - */ -ret = acquire_nr_borrower_domain(d, pbase, psize, _borrowers); -if ( ret ) -return ret; - /* * Instead of letting borrower domain get a page ref, we add as many * additional reference as the number of borrowers when the owner @@ -199,6 +188,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, dt_for_each_child_node(node, shm_node) { +const struct membank *boot_shm_bank; const struct dt_property *prop; const __be32 *cells; uint32_t addr_cells, size_cells; @@ -212,6 +202,23 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1&
Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
> On 14 May 2024, at 22:06, Julien Grall wrote: > > Hi, > > On 14/05/2024 08:53, Luca Fancellu wrote: >> Hi Julien, >> Thanks for having a look on the patch, >>> On 13 May 2024, at 22:54, Julien Grall wrote: >>> >>> Hi Luca, >>> >>> On 25/04/2024 14:11, Luca Fancellu wrote: >>>> Currently the code is listing device tree reserve map regions >>>> as reserved memory for Xen, but they are not added into >>>> bootinfo.reserved_mem and they are fetched in multiple places >>>> using the same code sequence, causing duplication. Fix this >>>> by adding them to the bootinfo.reserved_mem at early stage. >>> >>> Do we have enough space in bootinfo.reserved_mem for them? >> So we have 255 banks, in my experience I would say I’ve never saw too many >> reserved regions >> in the DT, maybe a couple, but I’ve always had to deal with embedded >> platforms. >> I’ve tested this one with ADLINK AVA board, n1sdp, Juno, raspberry pi, qemu, >> fvp. >> In your experience, have you seen any numbers that could be concerning? > I know in the past we had to bump the memory banks a few times. But as you > tested on a few platforms, I think we should be ok. > > It would be best if this patch goes sooner than later to allow wider testing > before we release 4.19. > > Acked-by: Julien Grall Yes it would make sense, this patch makes sense on its own, would you/anyone commit it separately while I work on the second patch?
Re: [PATCH 3/7] xen/p2m: put reference for superpage
Hi Julien, > >> Note also that a foreign unmap resulting in a page free is also not >> the common case, as that should only happen when the foreign domain >> has been destroyed, or the page ballooned out, so to benchmark the >> worst case some effort will be needed in order to model this >> scenario. > > Good callout. It may be easier to reproduce it with some XTF tests. > Unfortunately, I don't have the bandwith to look at it. Maybe Luca can? Unfortunately I don’t have bandwidth for that, > > Otherwise, we may have to accept not supporting 1GB superpage for the time > being for shared memory. I will try to do it. Cheers, Luca
Re: [RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Hi Julien, Thanks for having a look on the patch, > On 13 May 2024, at 22:54, Julien Grall wrote: > > Hi Luca, > > On 25/04/2024 14:11, Luca Fancellu wrote: >> Currently the code is listing device tree reserve map regions >> as reserved memory for Xen, but they are not added into >> bootinfo.reserved_mem and they are fetched in multiple places >> using the same code sequence, causing duplication. Fix this >> by adding them to the bootinfo.reserved_mem at early stage. > > Do we have enough space in bootinfo.reserved_mem for them? So we have 255 banks, in my experience I would say I’ve never saw too many reserved regions in the DT, maybe a couple, but I’ve always had to deal with embedded platforms. I’ve tested this one with ADLINK AVA board, n1sdp, Juno, raspberry pi, qemu, fvp. In your experience, have you seen any numbers that could be concerning? Cheers, Luca
Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
> On 10 May 2024, at 10:32, Michal Orzel wrote: > > Hi Luca, > > On 10/05/2024 11:25, Luca Fancellu wrote: >> >> >>> On 10 May 2024, at 10:17, Michal Orzel 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 >>> >>> 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
Re: [PATCH 6/7] xen/arm: Implement the logic for static shared memory from Xen heap
> On 10 May 2024, at 10:17, Michal Orzel 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 > > 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
Re: [PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
Hi Michal, >> >>> +if ( shm_id_match ) >>>{ >>> -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 ) >>> -continue; >>>else >>>{ >> There is no need for this else and entire block given that the block within >> if either calls break or return > > There was a MISRA discussion about else at the end of if ... else if ... > (R15.7) and I don’t remember > the outcome Sorry I was misreading the code here, sure I’ll remove the else. Cheers, Luca
Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
> On 2 May 2024, at 19:35, Stefano Stabellini wrote: > > On Tue, 30 Apr 2024, Luca Fancellu wrote: >> Commit 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory >> bank structures") introduced a MISRA regression for Rule 1.1 because a >> flexible array member is introduced in the middle of a struct, furthermore >> this is using a GCC extension that is going to be deprecated in GCC 14 and >> a warning to identify such cases will be present >> (-Wflex-array-member-not-at-end) to identify such cases. >> >> In order to fix this issue, use the macro __struct_group to create a >> structure 'struct membanks_hdr' which will hold the common data among >> structures using the 'struct membanks' interface. >> >> Modify the 'struct shared_meminfo' and 'struct meminfo' to use this new >> structure, effectively removing the flexible array member from the middle >> of the structure and modify the code accessing the .common field to use >> the macro container_of to maintain the functionality of the interface. >> >> Given this change, container_of needs to be supplied with a type and so >> the macro 'kernel_info_get_mem' inside arm/include/asm/kernel.h can't be >> an option since it uses const and non-const types for struct membanks, so >> introduce two static inline, one of which will keep the const qualifier. >> >> Given the complexity of the interface, which carries a lot of benefit but >> on the other hand could be prone to developer confusion if the access is >> open-coded, introduce two static inline helper for the >> 'struct kernel_info' .shm_mem member and get rid the open-coding >> shm_mem.common access. >> >> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank >> structures") >> Signed-off-by: Luca Fancellu > > Reviewed-by: Stefano Stabellini Hi Stefano, Thanks! Is it possible to add, eventually on commit, this tag? Reported-by: Nicola Vetrini
Re: [PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
Hi Michal, > On 8 May 2024, at 13:09, Michal Orzel wrote: > > Hi Luca, > > On 23/04/2024 10:25, 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 don't 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 and ACPI and it's unlikely for these > also from shmem > >> bank to have the start address as INVALID_PADDR. The change holds >> because of this consideration. > On arm64 and LPAE arm32 we don't have this problem. In theory we could have a > bank > starting at INVALID_PADDR if PA range was 32bit but as the comment above the > function states, > wrapping around is not handled. You might want to use it as a justification > to be clear. Sure, I’ll rephrase it, is it ok something like this: [...] 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. The change holds because of this consideration. >> >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index 24e40495a481..1c03bb7f1882 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); > The dot at the end is not needed. Will fix >> >> -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: >> + * when host address is provided: > - when would read better Ok I’ll use hyphen instead of star, here and below > >> * 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; > parenthesis around INVALID_PADDR == paddr Ok > >> +bool shm_id_match = strncmp(shm_id, shmem_extra[i].shm_id, >> +MAX_SHM_ID_LENGTH) == 0; > why not if ( strncmp... given no other use of this variable other than the > one below? Yeah I think in some previous rework I was using multiple times and then I forgot to change here, I’ll fix > >> +if ( shm_id_match ) >> { >> -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) >> +
Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function
Hi Michal, int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, if ( dt_property_read_string(shm_node, "role", _str) == 0 ) owner_dom_io = false; >>> Looking at owner_dom_io, why don't you move parsing role and setting >>> owner_dom_io accordingly to handle_shared_mem_bank()? >> >> I think I wanted to keep all dt_* functions on the same level inside >> process_shm, otherwise yes, I could >> pass down shm_node and do the reading of role_str in handle_shared_mem_bank, >> or I could derive >> owner_dom_io from role_str being passed or not, something like: >> >> role_str = NULL; >> dt_property_read_string(shm_node, "role", _str) >> >> [inside handle_shared_mem_bank]: >> If ( role_str ) >>owner_dom_io = false; >> >> And pass only role_str to handle_shared_mem_bank. >> >> Is this comment to reduce the number of parameters passed? I guess it’s not >> for where we call > In this series as well as the previous one you limit the number of arguments > passed to quite a few functions. > So naturally I would expect it to be done here as well. owner_dom_io is used > only by handle_shared_mem_bank, so it makes more sense to move parsing to this > function so that it is self-contained. Ok I will, just to be on the same page here, you mean having dt_property_read_string inside handle_shared_mem_bank? Or the above example would work for you as well? That one would have role_str passed instead of shm_node. Cheers, Luca
Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
Hi Michal, >> @@ -440,6 +431,26 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, device_tree_get_reg(, address_cells, address_cells, , ); size = dt_next_cell(size_cells, ); +if ( !IS_ALIGNED(paddr, PAGE_SIZE) ) +{ +printk("fdt: physical address 0x%"PRIpaddr" is not suitably aligned.\n", + paddr); +return -EINVAL; +} + +if ( !IS_ALIGNED(gaddr, PAGE_SIZE) ) +{ +printk("fdt: guest address 0x%"PRIpaddr" is not suitably aligned.\n", + gaddr); +return -EINVAL; +} + +if ( !IS_ALIGNED(size, PAGE_SIZE) ) >>> What sense does it make to check for size being aligned before checking for >>> size being 0? It would pass this check. >> >> Yes, but in the end we are doing that to print a different error message, so >> it would pass >> for 0 and it’s totally fine, but in the end it will fail afterwards. I don’t >> see functional disruptions >> having this one before the other, what is the concern here? > It does not cause the functional disruption. It is more about code > readability and writing cleaner code. > It makes more sense to first check for size being 0 rather than whether it's > page aligned, since the latter can > pass if former is true and thus not making much sense. Ok then I will switch them and check it being different from 0 before the alignment check. Cheers, Luca
Re: [PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function
Hi Michal, >> >> +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, >> + bool owner_dom_io, >> + const char *role_str, >> + const struct membank *shm_bank) >> +{ >> +paddr_t pbase, psize; >> +int ret; >> + >> +BUG_ON(!shm_bank); > not needed > >> + >> +pbase = shm_bank->start; >> +psize = shm_bank->size; > please add empty line here Will do >> >> int __init process_shm(struct domain *d, struct kernel_info *kinfo, >>const struct dt_device_node *node) >> { >> @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct >> kernel_info *kinfo, >> if ( dt_property_read_string(shm_node, "role", _str) == 0 ) >> owner_dom_io = false; > Looking at owner_dom_io, why don't you move parsing role and setting > owner_dom_io accordingly to handle_shared_mem_bank()? I think I wanted to keep all dt_* functions on the same level inside process_shm, otherwise yes, I could pass down shm_node and do the reading of role_str in handle_shared_mem_bank, or I could derive owner_dom_io from role_str being passed or not, something like: role_str = NULL; dt_property_read_string(shm_node, "role", _str) [inside handle_shared_mem_bank]: If ( role_str ) owner_dom_io = false; And pass only role_str to handle_shared_mem_bank. Is this comment to reduce the number of parameters passed? I guess it’s not for where we call dt_property_read_string isn’t it? Cheers, Luca
Re: [PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
Hi Michal, Thanks for your review. > On 6 May 2024, at 14:24, Michal Orzel wrote: > > Hi Luca, > > On 23/04/2024 10:25, Luca Fancellu wrote: >> >> >> The current static shared memory code is using bootinfo banks when it >> needs to find the number of borrower, so every time assign_shared_memory > s/borrower/borrowers Will fix > >> is called, the bank is searched in the bootinfo.shmem structure. >> >> There is nothing wrong with it, however the bank can be used also to >> retrieve the start address and size and also to pass less argument to >> assign_shared_memory. When retrieving the information from the bootinfo >> bank, it's also possible to move the checks on alignment to >> process_shm_node in the early stages. > Is this change really required for what you want to achieve? At the moment > the alignment checks > are done before first use, which requires these values to be aligned. FDT > processing part does not need it. That’s true, but it would separate better the parsing part, in the end what is the point of failing later if, for example, some value are passed but not aligned? > >> >> So create a new function find_shm() which takes a 'struct shared_meminfo' > Can we name it find_shm_bank() or find_shm_bank_by_id()? > I agree that it's better to use a unique ID rather than matching by > address/size Yes either names are good for me, I would use find_shm_bank_by_id > >> structure and the shared memory ID, to look for a bank with a matching ID, >> take the physical host address and size from the bank, pass the bank to >> assign_shared_memory() removing the now unnecessary arguments and finally >> remove the acquire_nr_borrower_domain() function since now the information >> can be extracted from the passed bank. >> Move the "xen,shm-id" parsing early in process_shm to bail out quickly in >> case of errors (unlikely), as said above, move the checks on alignment >> to process_shm_node. >> >> Drawback of this change is that now the bootinfo are used also when the >> bank doesn't need to be allocated, however it will be convinient later >> to use it as an argument for assign_shared_memory when dealing with >> the use case where the Host physical address is not supplied by the user. >> >> Signed-off-by: Luca Fancellu >> --- >> xen/arch/arm/static-shmem.c | 105 >> 1 file changed, 58 insertions(+), 47 deletions(-) >> >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index 09f474ec6050..f6cf74e58a83 100644 >> --- a/xen/arch/arm/static-shmem.c >> +++ b/xen/arch/arm/static-shmem.c >> @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void) >> offsetof(struct shared_meminfo, bank))); >> } >> >> -static int __init acquire_nr_borrower_domain(struct domain *d, >> - paddr_t pbase, paddr_t psize, >> - unsigned long *nr_borrowers) >> +static const struct membank __init *find_shm(const struct membanks *shmem, >> + const char *shm_id) >> { >> -const struct membanks *shmem = bootinfo_get_shmem(); >> unsigned int bank; >> >> -/* Iterate reserved memory to find requested shm bank. */ >> +BUG_ON(!shmem || !shm_id); > Is it really necessary? For example, before calling find_shm(), strlen is > used on shm_id So, I guess I did that to have more robust code, in case someone changes the code in the future and perhaps removes something we rely on. If you object to them I will remove though, here and the other related points below. > >> + >> for ( bank = 0 ; bank < shmem->nr_banks; bank++ ) >> { >> -paddr_t bank_start = shmem->bank[bank].start; >> -paddr_t bank_size = shmem->bank[bank].size; >> - >> -if ( (pbase == bank_start) && (psize == bank_size) ) >> +if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id, >> + MAX_SHM_ID_LENGTH) == 0 ) > Why not strcmp? AFAICS it's been validated many times already > >> break; >> } >> >> if ( bank == shmem->nr_banks ) >> -return -ENOENT; >> - >> -*nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers; >> +return NULL; >> >> -return 0; >> +return >bank[bank]; >> } >> >> /* >> @@ -103,14 +98,20 @@ static mfn_t __init acquire_sha
Re: [PATCH 3/7] xen/p2m: put reference for superpage
Hi Julien, > On 7 May 2024, at 14:20, Julien Grall wrote: > > Hi Luca, > > On 23/04/2024 09:25, Luca Fancellu wrote: >> From: Penny Zheng >> We are doing foreign memory mapping for static shared memory, and >> there is a great possibility that it could be super mapped. > > Is this because we are mapping more than one page at the time? Can you point > me to the code? So, to be honest this patch was originally in Penny’s serie, my knowledge of this side of the codebase is very limited and so I pushed this one basically untouched. From what I can see in the serie the mappings are made in handle_shared_mem_bank, and map_regions_p2mt is called for one page at the time (allocated through the function allocate_domheap_memory (new function introduced in the serie). So is that the case that this patch is not needed? > >> But today, p2m_put_l3_page could not handle superpages. > > This was done on purpose. Xen is not preemptible and therefore we need to be > cautious how much work is done within the p2m code. > > With the below proposal, for 1GB mapping, we may end up to call put_page() up > to 512 * 512 = 262144 times. put_page() can free memory. This could be a very > long operation. > > Have you benchmark how long it would take? I did not, since its purpose was unclear to me and was not commented in the last serie from Penny. Cheers, Luca
Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
> On 2 May 2024, at 11:30, Jan Beulich wrote: > > On 02.05.2024 12:12, Luca Fancellu wrote: >>>> In any case it would be used soon also for other architectures using >>>> bootinfo. >>> >>> Oh, would it? >> >> PPC people have plans on putting that interface in common: > > I'm aware, but ... > >> https://patchwork.kernel.org/project/xen-devel/patch/451705505ff7f80ec66c78cc2830196fa6e4090c.1712893887.git.sanasta...@raptorengineering.com/ > > ... I can't spot any flexible array members in this patch. I guess v5 of that patch will have that, because it would be rebased on the latest state of xen/arch/arm/include/asm/setup.h, moving that interface in xen/include/xen/bootfdt.h, in order to use part of the code taken out from xen/arch/arm/setup.c and put in xen/common/device-tree/bootinfo.c
Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
> >> In any case it would be used soon also for other architectures using >> bootinfo. > > Oh, would it? PPC people have plans on putting that interface in common: https://patchwork.kernel.org/project/xen-devel/patch/451705505ff7f80ec66c78cc2830196fa6e4090c.1712893887.git.sanasta...@raptorengineering.com/
Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
> On 2 May 2024, at 07:43, Jan Beulich wrote: > > On 02.05.2024 08:33, Luca Fancellu wrote: >> >> >>> On 2 May 2024, at 07:14, Jan Beulich wrote: >>> >>> On 01.05.2024 08:57, Luca Fancellu wrote: >>>> Hi Jan, >>>> >>>>> On 30 Apr 2024, at 12:37, Jan Beulich wrote: >>>>> >>>>> On 30.04.2024 13:09, Luca Fancellu wrote: >>>>>> --- a/xen/arch/arm/include/asm/setup.h >>>>>> +++ b/xen/arch/arm/include/asm/setup.h >>>>>> @@ -64,18 +64,20 @@ struct membank { >>>>>> }; >>>>>> >>>>>> struct membanks { >>>>>> -unsigned int nr_banks; >>>>>> -unsigned int max_banks; >>>>>> +__struct_group(membanks_hdr, common, , >>>>>> +unsigned int nr_banks; >>>>>> +unsigned int max_banks; >>>>>> +); >>>>>> struct membank bank[]; >>>>>> }; >>>>> >>>>> I'm afraid I can't spot why __struct_group() is needed here. Why would >>>>> just >>>>> one of the two more straightforward >>>>> >>>>> struct membanks { >>>>> struct membanks_hdr { >>>>> unsigned int nr_banks; >>>>> unsigned int max_banks; >>>>> ); >>>>> struct membank bank[]; >>>>> }; >>>>> >>>> >>>> At the first sight I thought this solution could have worked, however GCC >>>> brought me back down to earth >>>> remembering me that flexible array members can’t be left alone in an empty >>>> structure: >>>> >>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: >>>> error: declaration does not declare anything [-Werror] >>>> 70 | }; >>>> | ^ >>>> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: >>>> error: flexible array member in a struct with no named members >>>> 71 | struct membank bank[]; >>>> | ^~~~ >>>> [...] >>> >>> Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution >>> to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of >>> borrowing __struct_group(), we could consider borrowing this as well. Or >>> open-code it just here, for the time being (perhaps my preference). Yet >>> it's not clear to me that doing so will actually be enough to make things >>> work for you. >> >> I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group() >> was enough for my purpose, can I ask the technical reasons why it would be >> your >> preference? Is there something in that construct that is a concern for you? > > I don't like either construct very much, but of the two __DECLARE_FLEX_ARRAY() > looks slightly more "natural" for what is wanted and how it's done. > __struct_group() introducing twice the (effectively) same structure feels > pretty odd, for now at least. It's not even entirely clear to me whether there > aren't pitfalls, seeing that the C spec differentiates named and unnamed > struct fields in a few cases. For __DECLARE_FLEX_ARRAY(), otoh, I can't > presently see any reason to suspect possible corner cases. > > Yet as said before - I'm not sure __DECLARE_FLEX_ARRAY() alone would be enough > for what you want to achieve. Mmm yes, I think I would still have problems because container_of wants a named member, so __DECLARE_FLEX_ARRAY() doesn’t help much alone, if I’m not missing something obvious. If you believe however that importing __struct_group() only for this instance is not enough to justify its presence in the codebase, I could open-code it, provided that maintainers are ok with that. In any case it would be used soon also for other architectures using bootinfo.
Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
> On 2 May 2024, at 07:14, Jan Beulich wrote: > > On 01.05.2024 08:57, Luca Fancellu wrote: >> Hi Jan, >> >>> On 30 Apr 2024, at 12:37, Jan Beulich wrote: >>> >>> On 30.04.2024 13:09, Luca Fancellu wrote: >>>> --- a/xen/arch/arm/include/asm/setup.h >>>> +++ b/xen/arch/arm/include/asm/setup.h >>>> @@ -64,18 +64,20 @@ struct membank { >>>> }; >>>> >>>> struct membanks { >>>> -unsigned int nr_banks; >>>> -unsigned int max_banks; >>>> +__struct_group(membanks_hdr, common, , >>>> +unsigned int nr_banks; >>>> +unsigned int max_banks; >>>> +); >>>>struct membank bank[]; >>>> }; >>> >>> I'm afraid I can't spot why __struct_group() is needed here. Why would just >>> one of the two more straightforward >>> >>> struct membanks { >>> struct membanks_hdr { >>> unsigned int nr_banks; >>> unsigned int max_banks; >>> ); >>> struct membank bank[]; >>> }; >>> >> >> At the first sight I thought this solution could have worked, however GCC >> brought me back down to earth >> remembering me that flexible array members can’t be left alone in an empty >> structure: >> >> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: >> error: declaration does not declare anything [-Werror] >> 70 | }; >> | ^ >> /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: >> error: flexible array member in a struct with no named members >> 71 | struct membank bank[]; >> | ^~~~ >> [...] > > Since for patch 1 you looked at Linux'es uapi/linux/stddef.h, the solution > to this lies there, in __DECLARE_FLEX_ARRAY(). Alongside or instead of > borrowing __struct_group(), we could consider borrowing this as well. Or > open-code it just here, for the time being (perhaps my preference). Yet > it's not clear to me that doing so will actually be enough to make things > work for you. I looked also into __DECLARE_FLEX_ARRAY(), but then decided __struct_group() was enough for my purpose, can I ask the technical reasons why it would be your preference? Is there something in that construct that is a concern for you?
Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux
> On 2 May 2024, at 07:09, Jan Beulich wrote: > > On 01.05.2024 08:54, Luca Fancellu wrote: >>> On 30 Apr 2024, at 12:43, Jan Beulich wrote: >>> On 30.04.2024 13:09, Luca Fancellu wrote: >>>> --- a/xen/include/xen/kernel.h >>>> +++ b/xen/include/xen/kernel.h >>>> @@ -54,6 +54,27 @@ >>>>typeof_field(type, member) *__mptr = (ptr); \ >>>>(type *)( (char *)__mptr - offsetof(type,member) );}) >>>> >>>> +/** >>>> + * __struct_group() - Create a mirrored named and anonyomous struct >>>> + * >>>> + * @TAG: The tag name for the named sub-struct (usually empty) >>>> + * @NAME: The identifier name of the mirrored sub-struct >>>> + * @ATTRS: Any struct attributes (usually empty) >>>> + * @MEMBERS: The member declarations for the mirrored structs >>>> + * >>>> + * Used to create an anonymous union of two structs with identical layout >>>> + * and size: one anonymous and one named. The former's members can be used >>>> + * normally without sub-struct naming, and the latter can be used to >>>> + * reason about the start, end, and size of the group of struct members. >>>> + * The named struct can also be explicitly tagged for layer reuse, as well >>>> + * as both having struct attributes appended. >>>> + */ >>>> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ >>>> +union { \ >>>> +struct { MEMBERS } ATTRS; \ >>>> +struct TAG { MEMBERS } ATTRS NAME; \ >>>> +} ATTRS >>> >>> Besides my hesitance towards having this construct, can you explain why >>> ATTR needs using 3 times, i.e. also on the wrapping union? >> >> The original commit didn’t have the third ATTRS, but afterwards it was >> introduced due to >> this: >> >> https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmanti...@yandex.ru/#25610045 > > Hmm. Since generally packing propagates to containing compound types, I'd > say this cannot go without a code comment, or at the very least a mention > in the description. But: Do we use this old ABI in Xen at all? IOW can we > get away without this 3rd instance? Yes, I think it won’t be a problem for Xen, is it something that can be done on commit? Anyway in case of comments on the second patch, I’ll push the change also for this one. Cheers, Luca
Re: [PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
Hi Jan, > On 30 Apr 2024, at 12:37, Jan Beulich wrote: > > On 30.04.2024 13:09, Luca Fancellu wrote: >> --- a/xen/arch/arm/include/asm/setup.h >> +++ b/xen/arch/arm/include/asm/setup.h >> @@ -64,18 +64,20 @@ struct membank { >> }; >> >> struct membanks { >> -unsigned int nr_banks; >> -unsigned int max_banks; >> +__struct_group(membanks_hdr, common, , >> +unsigned int nr_banks; >> +unsigned int max_banks; >> +); >> struct membank bank[]; >> }; > > I'm afraid I can't spot why __struct_group() is needed here. Why would just > one of the two more straightforward > > struct membanks { >struct membanks_hdr { >unsigned int nr_banks; >unsigned int max_banks; >); >struct membank bank[]; > }; > At the first sight I thought this solution could have worked, however GCC brought me back down to earth remembering me that flexible array members can’t be left alone in an empty structure: /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:70:6: error: declaration does not declare anything [-Werror] 70 | }; | ^ /data_sdc/lucfan01/gitlab_mickledore_xen/xen/xen/arch/arm/include/asm/setup.h:71:20: error: flexible array member in a struct with no named members 71 | struct membank bank[]; | ^~~~ [...] Cheers, Luca
Re: [PATCH 1/2] xen/kernel.h: Import __struct_group from Linux
Hi Jan, > On 30 Apr 2024, at 12:43, Jan Beulich wrote: > > On 30.04.2024 13:09, Luca Fancellu wrote: >> --- a/xen/include/xen/kernel.h >> +++ b/xen/include/xen/kernel.h >> @@ -54,6 +54,27 @@ >> typeof_field(type, member) *__mptr = (ptr); \ >> (type *)( (char *)__mptr - offsetof(type,member) );}) >> >> +/** >> + * __struct_group() - Create a mirrored named and anonyomous struct >> + * >> + * @TAG: The tag name for the named sub-struct (usually empty) >> + * @NAME: The identifier name of the mirrored sub-struct >> + * @ATTRS: Any struct attributes (usually empty) >> + * @MEMBERS: The member declarations for the mirrored structs >> + * >> + * Used to create an anonymous union of two structs with identical layout >> + * and size: one anonymous and one named. The former's members can be used >> + * normally without sub-struct naming, and the latter can be used to >> + * reason about the start, end, and size of the group of struct members. >> + * The named struct can also be explicitly tagged for layer reuse, as well >> + * as both having struct attributes appended. >> + */ >> +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ >> +union { \ >> +struct { MEMBERS } ATTRS; \ >> +struct TAG { MEMBERS } ATTRS NAME; \ >> +} ATTRS > > Besides my hesitance towards having this construct, can you explain why > ATTR needs using 3 times, i.e. also on the wrapping union? The original commit didn’t have the third ATTRS, but afterwards it was introduced due to this: https://patchwork.kernel.org/project/linux-wireless/patch/20231120110607.98956-1-dmanti...@yandex.ru/#25610045 Now, I have to say that for the Origin tag I used the SHA of the commit introducing the macro and the SHA doing this modification is different, how are these cases handled? Cheers, Luca
[PATCH 1/2] xen/kernel.h: Import __struct_group from Linux
Import __struct_group from Linux, commit 50d7bd38c3aa ("stddef: Introduce struct_group() helper macro"), in order to allow the access through the anonymous structure to the members without having to write also the name, e.g: struct foo { int one; struct { int two; int three, four; } thing; int five; }; would become: struct foo { int one; __struct_group(/* None */, thing, /* None */, int two; int three, four; ); int five; }; Allowing the users of this structure to access the .thing members by using .two/.three/.four on the struct foo. This construct will become useful in order to have some generalized interfaces that shares some common members. Origin: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 50d7bd38c3aa Signed-off-by: Luca Fancellu --- xen/include/xen/kernel.h | 21 + 1 file changed, 21 insertions(+) diff --git a/xen/include/xen/kernel.h b/xen/include/xen/kernel.h index bb6b0f38912d..bc2440b5f96e 100644 --- a/xen/include/xen/kernel.h +++ b/xen/include/xen/kernel.h @@ -54,6 +54,27 @@ typeof_field(type, member) *__mptr = (ptr); \ (type *)( (char *)__mptr - offsetof(type,member) );}) +/** + * __struct_group() - Create a mirrored named and anonyomous struct + * + * @TAG: The tag name for the named sub-struct (usually empty) + * @NAME: The identifier name of the mirrored sub-struct + * @ATTRS: Any struct attributes (usually empty) + * @MEMBERS: The member declarations for the mirrored structs + * + * Used to create an anonymous union of two structs with identical layout + * and size: one anonymous and one named. The former's members can be used + * normally without sub-struct naming, and the latter can be used to + * reason about the start, end, and size of the group of struct members. + * The named struct can also be explicitly tagged for layer reuse, as well + * as both having struct attributes appended. + */ +#define __struct_group(TAG, NAME, ATTRS, MEMBERS...) \ +union { \ +struct { MEMBERS } ATTRS; \ +struct TAG { MEMBERS } ATTRS NAME; \ +} ATTRS + /* * Check at compile time that something is of a particular type. * Always evaluates to 1 so you may use it easily in comparisons. -- 2.34.1
[PATCH 0/2] Fix MISRA regression about flexible array member not at the end
This small serie is fixing a MISRA regression on the R1.1 due to the introduction of a new interface that uses a flexible member array [1]. I've splitted the serie in two in order to keep the Linux imported code on a single commit, the imported macro is later used in the second patch of the serie. [1] https://lore.kernel.org/all/8be082b6d22d61c0b14910680d383...@bugseng.com/ Luca Fancellu (2): xen/kernel.h: Import __struct_group from Linux xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end xen/arch/arm/acpi/domain_build.c| 2 +- xen/arch/arm/domain_build.c | 6 +++--- xen/arch/arm/include/asm/kernel.h | 11 ++- xen/arch/arm/include/asm/setup.h| 18 ++ xen/arch/arm/include/asm/static-shmem.h | 12 xen/arch/arm/static-shmem.c | 19 +-- xen/include/xen/kernel.h| 21 + 7 files changed, 66 insertions(+), 23 deletions(-) -- 2.34.1
[PATCH 2/2] xen/arm: Fix MISRA regression on R1.1, flexible array member not at the end
Commit 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") introduced a MISRA regression for Rule 1.1 because a flexible array member is introduced in the middle of a struct, furthermore this is using a GCC extension that is going to be deprecated in GCC 14 and a warning to identify such cases will be present (-Wflex-array-member-not-at-end) to identify such cases. In order to fix this issue, use the macro __struct_group to create a structure 'struct membanks_hdr' which will hold the common data among structures using the 'struct membanks' interface. Modify the 'struct shared_meminfo' and 'struct meminfo' to use this new structure, effectively removing the flexible array member from the middle of the structure and modify the code accessing the .common field to use the macro container_of to maintain the functionality of the interface. Given this change, container_of needs to be supplied with a type and so the macro 'kernel_info_get_mem' inside arm/include/asm/kernel.h can't be an option since it uses const and non-const types for struct membanks, so introduce two static inline, one of which will keep the const qualifier. Given the complexity of the interface, which carries a lot of benefit but on the other hand could be prone to developer confusion if the access is open-coded, introduce two static inline helper for the 'struct kernel_info' .shm_mem member and get rid the open-coding shm_mem.common access. Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") Signed-off-by: Luca Fancellu --- xen/arch/arm/acpi/domain_build.c| 2 +- xen/arch/arm/domain_build.c | 6 +++--- xen/arch/arm/include/asm/kernel.h | 11 ++- xen/arch/arm/include/asm/setup.h| 18 ++ xen/arch/arm/include/asm/static-shmem.h | 12 xen/arch/arm/static-shmem.c | 19 +-- 6 files changed, 45 insertions(+), 23 deletions(-) diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index ed895dd8f926..2ce75543d004 100644 --- a/xen/arch/arm/acpi/domain_build.c +++ b/xen/arch/arm/acpi/domain_build.c @@ -451,7 +451,7 @@ static int __init estimate_acpi_efi_size(struct domain *d, struct acpi_table_rsdp *rsdp_tbl; struct acpi_table_header *table; -efi_size = estimate_efi_size(kernel_info_get_mem(kinfo)->nr_banks); +efi_size = estimate_efi_size(kernel_info_get_mem_const(kinfo)->nr_banks); acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8); acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8); diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 0784e4c5e315..f6550809cfdf 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -805,7 +805,7 @@ int __init make_memory_node(const struct kernel_info *kinfo, int addrcells, * static shared memory banks need to be listed as /memory node, so when * this function is handling the normal memory, add the banks. */ -if ( mem == kernel_info_get_mem(kinfo) ) +if ( mem == kernel_info_get_mem_const(kinfo) ) shm_mem_node_fill_reg_range(kinfo, reg, _cells, addrcells, sizecells); @@ -884,7 +884,7 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, { const struct membanks *mem = bootinfo_get_mem(); const struct membanks *mem_banks[] = { -kernel_info_get_mem(kinfo), +kernel_info_get_mem_const(kinfo), bootinfo_get_reserved_mem(), #ifdef CONFIG_STATIC_SHM bootinfo_get_shmem(), @@ -1108,7 +1108,7 @@ static int __init find_domU_holes(const struct kernel_info *kinfo, uint64_t bankend; const uint64_t bankbase[] = GUEST_RAM_BANK_BASES; const uint64_t banksize[] = GUEST_RAM_BANK_SIZES; -const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo); +const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo); int res = -ENOENT; for ( i = 0; i < GUEST_RAM_BANKS; i++ ) diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index 16a2bfc01e27..7e6e3c82a477 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -80,7 +80,16 @@ struct kernel_info { }; }; -#define kernel_info_get_mem(kinfo) (&(kinfo)->mem.common) +static inline struct membanks *kernel_info_get_mem(struct kernel_info *kinfo) +{ +return container_of(>mem.common, struct membanks, common); +} + +static inline const struct membanks * +kernel_info_get_mem_const(const struct kernel_info *kinfo) +{ +return container_of(>mem.common, const struct membanks, common); +} #ifdef CONFIG_STATIC_SHM #define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_SHMEM_BANKS, diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index 28fb659fe946..61c15806a7c4 100644 --
Re: [XEN PATCH] automation/eclair: reorganize pipelines
> On 25 Apr 2024, at 14:32, Nicola Vetrini wrote: > > On 2024-04-25 11:40, Federico Serafini wrote: >> On 25/04/24 02:06, Stefano Stabellini wrote: >>> On Tue, 23 Apr 2024, Federico Serafini wrote: >>>> From: Simone Ballarin >>>> Introduce accepted_guidelines.sh: a script to autogenerate the >>>> configuration file accepted.ecl from docs/misra/rules.rst which enables >>>> all accepted guidelines. >>>> Introduce monitored.ecl: a manual selection of accepted guidelines >>>> which are clean or almost clean, it is intended to be used for the >>>> analyses triggered by commits. >>>> Reorganize tagging.ecl: >>>> -Remove "accepted" tags: keeping track of accepted guidelines tagging >>>>them as "accepted" in the configuration file tagging.ecl is no >>>>longer needed since docs/rules.rst is keeping track of them. >>>> -Tag more guidelines as clean. >>>> Reorganize eclair pipelines: >>>> - Set1, Set2, Set3 are now obsolete: remove the corresponding >>>> pipelines and ecl files. >>>> - Amend scheduled eclair pipeline to use accepted.ecl. >>>> - Amend triggered eclair pipeline to use monitored.ecl. >>>> Rename and improve action_check_clean_regressions.sh to print a >>>> diagnostic in case a commit introduces a violation of a clean guideline. >>>> An example of diagnostic is the following: >>>> Failure: 13 regressions found for clean guidelines >>>> service MC3R1.R8.2: (required) Function types shall be in prototype form >>>> with named parameters: >>>>violation: 13 >>>> Signed-off-by: Simone Ballarin >>>> Signed-off-by: Federico Serafini >>>> Signed-off-by: Alessandro Zucchelli >>> Fantastic work, thank you! >>> Reviewed-by: Stefano Stabellini >>> Is this patch safe to commit now? Or would it cause gitlab-ci breakage? >> Yes, it is safe because the ECLAIR analysis is still allowed to fail. >> Committing this patch wouldn't break the CI but it will highlight some >> regressions with the orange badge and the following messages: >> arm: >> Failure: 5 regressions found for clean guidelines >> service MC3R1.R1.1: (required) The program shall contain no violations of >> the standard C syntax and constraints, and shall not exceed the >> implementation's translation limits: >> violation: 5 > Hi Nicola, > +Cc ARM maintainers, Luca Fancellu > > after some investigation on these regressions on R1.1, there are some > concerns with the current code: > 2209c1e35b479dff8ed3d3161001ffdefa0a704e introduced the struct > > struct membanks { >unsigned int nr_banks; >unsigned int max_banks; >struct membank bank[]; > }; > > with a flexible array member at the end; this is well-defined in C99, but > what is non-standard (a GNU extension) is having this struct as a member to > another struct/union in a position other than the last. > > This is still supported by GNU (albeit reliance on this is undocumented for > Xen), but what is concerning here is the following (taken from [1]): > > "A structure containing a C99 flexible array member, or a union containing > such a structure, is not the last field of another structure, for example: > > struct flex { int length; char data[]; }; > > struct mid_flex { int m; struct flex flex_data; int n; }; > > In the above, accessing a member of the array mid_flex.flex_data.data[] might > have undefined behavior. Compilers do not handle such a case consistently. > Any code relying on this case should be modified to ensure that flexible > array members only end up at the ends of structures. > Please use the warning option -Wflex-array-member-not-at-end to identify all > such cases in the source code and modify them. This extension is now > deprecated." > > It looks like this option, and the corresponding deprecation of the > extension, will be available starting from GCC 14, so this might impact > future compiler updates for Xen builds. > > Linux is also concerned about this (see [2]), so I think the changes in > struct layout introduced by the series [3] should be reviewed to determine > whether this change was deliberate or happened as a byproduct. If this is > determined not to be a legitimate concern, then this can be documented as an > use of the GNU extension. Thanks for letting us know, so the change was deliberate, the effect probably not, I guess we need to find a way to fix this in order to use this interface. Cheers, Luca
[RFC PATCH 2/2] xen/arm: Rework dt_unreserved_regions to avoid recursion
The function dt_unreserved_regions is currently using recursion to compute the non overlapping ranges of a passed region against the reserved memory banks, in the spirit of removing the recursion to improve safety and also improve the scalability of the function, rework its code to use an iterative algorithm with the same result. The function was taking an additional parameter 'first', but given the rework and given that the function was always initially called with this parameter as zero, remove the parameter and update the codebase to reflect the change. Signed-off-by: Luca Fancellu --- xen/arch/arm/include/asm/setup.h| 8 +- xen/arch/arm/include/asm/static-shmem.h | 7 +- xen/arch/arm/kernel.c | 2 +- xen/arch/arm/setup.c| 133 4 files changed, 96 insertions(+), 54 deletions(-) diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index fc6967f9a435..24519b9ed969 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -9,7 +9,12 @@ #define MAX_FDT_SIZE SZ_2M #define NR_MEM_BANKS 256 + +#ifdef CONFIG_STATIC_SHM #define NR_SHMEM_BANKS 32 +#else +#define NR_SHMEM_BANKS 0 +#endif #define MAX_MODULES 32 /* Current maximum useful modules */ @@ -215,8 +220,7 @@ void create_dom0(void); void discard_initial_modules(void); void fw_unreserved_regions(paddr_t s, paddr_t e, - void (*cb)(paddr_t ps, paddr_t pe), - unsigned int first); + void (*cb)(paddr_t ps, paddr_t pe)); size_t boot_fdt_info(const void *fdt, paddr_t paddr); const char *boot_fdt_cmdline(const void *fdt); diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index 3b6569e5703f..1b7c7ea0e17d 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -7,11 +7,11 @@ #include #include -#ifdef CONFIG_STATIC_SHM - /* Worst case /memory node reg element: (addrcells + sizecells) */ #define DT_MEM_NODE_REG_RANGE_SIZE ((NR_MEM_BANKS + NR_SHMEM_BANKS) * 4) +#ifdef CONFIG_STATIC_SHM + int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells, int sizecells); @@ -47,9 +47,6 @@ void shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, __be32 *reg, #else /* !CONFIG_STATIC_SHM */ -/* Worst case /memory node reg element: (addrcells + sizecells) */ -#define DT_MEM_NODE_REG_RANGE_SIZE (NR_MEM_BANKS * 4) - static inline int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells, int sizecells) { diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c index 674388fa11a2..ecbeec518754 100644 --- a/xen/arch/arm/kernel.c +++ b/xen/arch/arm/kernel.c @@ -247,7 +247,7 @@ static __init int kernel_decompress(struct bootmodule *mod, uint32_t offset) * Free the original kernel, update the pointers to the * decompressed kernel */ -fw_unreserved_regions(addr, addr + size, init_domheap_pages, 0); +fw_unreserved_regions(addr, addr + size, init_domheap_pages); return 0; } diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index c4e5c19b11d6..d737fe56e539 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -204,55 +204,97 @@ static void __init processor_id(void) } static void __init dt_unreserved_regions(paddr_t s, paddr_t e, - void (*cb)(paddr_t ps, paddr_t pe), - unsigned int first) + void (*cb)(paddr_t ps, paddr_t pe)) { -const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); -#ifdef CONFIG_STATIC_SHM -const struct membanks *shmem = bootinfo_get_shmem(); -unsigned int offset; -#endif -unsigned int i; - /* - * i is the current bootmodule we are evaluating across all possible - * kinds. + * The worst case scenario is to have N reserved region ovelapping the + * passed one, so having N+1 regions in the stack */ -for ( i = first; i < reserved_mem->nr_banks; i++ ) -{ -paddr_t r_s = reserved_mem->bank[i].start; -paddr_t r_e = r_s + reserved_mem->bank[i].size; - -if ( s < r_e && r_s < e ) -{ -dt_unreserved_regions(r_e, e, cb, i + 1); -dt_unreserved_regions(s, r_s, cb, i + 1); -return; -} -} - +struct { +paddr_t s; +paddr_t e; +unsigned int dict; +unsigned int bank; +} stack[NR_MEM_BANKS + NR_SHMEM_BANKS + 1]; +const struct membanks *mem_banks[] = { +bootinfo_get_reserved_mem(), #ifdef CONFIG_STATIC_SHM -/* - * When retrieving the corresponding shared memory addresses - * below, we need to index the shmem->bank starting from 0, h
[RFC PATCH 0/2] xen/arm: Remove recursion from dt_unreserved_regions
Hi, this is an RFC that is exploiting the new 'struct membank' interface introduced here 2209c1e35b479dff8ed3d3161001ffdefa0a704e ("xen/arm: Introduce a generic way to access memory bank structures") to start removing recursion from some function, in this serie the dt_unreserved_regions is reworked for that reason. This is an RFC to understand if the proposed approach can be accepted. Another function can benefit from this approach, consider_modules in arm32/mmu/mm.c, but it might require to rework also the 'struct bootmodules' to adhere to the new interface and have just a loop that can go through all the different structures. Comments are welcome. Luca Fancellu (2): xen/arm: Add DT reserve map regions to bootinfo.reserved_mem xen/arm: Rework dt_unreserved_regions to avoid recursion xen/arch/arm/arm32/mmu/mm.c | 29 + xen/arch/arm/bootfdt.c | 51 +--- xen/arch/arm/domain_build.c | 3 +- xen/arch/arm/include/asm/setup.h| 13 +- xen/arch/arm/include/asm/static-shmem.h | 7 +- xen/arch/arm/kernel.c | 2 +- xen/arch/arm/setup.c| 158 +--- 7 files changed, 135 insertions(+), 128 deletions(-) -- 2.34.1
[RFC PATCH 1/2] xen/arm: Add DT reserve map regions to bootinfo.reserved_mem
Currently the code is listing device tree reserve map regions as reserved memory for Xen, but they are not added into bootinfo.reserved_mem and they are fetched in multiple places using the same code sequence, causing duplication. Fix this by adding them to the bootinfo.reserved_mem at early stage. Signed-off-by: Luca Fancellu --- xen/arch/arm/arm32/mmu/mm.c | 29 + xen/arch/arm/bootfdt.c | 51 ++ xen/arch/arm/domain_build.c | 3 +- xen/arch/arm/include/asm/setup.h | 5 +++ xen/arch/arm/setup.c | 53 +--- 5 files changed, 53 insertions(+), 88 deletions(-) diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c index 23150122f7c4..be480c31ea05 100644 --- a/xen/arch/arm/arm32/mmu/mm.c +++ b/xen/arch/arm/arm32/mmu/mm.c @@ -73,33 +73,6 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, } } -/* Now check any fdt reserved areas. */ - -nr = fdt_num_mem_rsv(device_tree_flattened); - -for ( ; i < mi->nr_mods + nr; i++ ) -{ -paddr_t mod_s, mod_e; - -if ( fdt_get_mem_rsv_paddr(device_tree_flattened, - i - mi->nr_mods, - _s, _e ) < 0 ) -/* If we can't read it, pretend it doesn't exist... */ -continue; - -/* fdt_get_mem_rsv_paddr returns length */ -mod_e += mod_s; - -if ( s < mod_e && mod_s < e ) -{ -mod_e = consider_modules(mod_e, e, size, align, i+1); -if ( mod_e ) -return mod_e; - -return consider_modules(s, mod_s, size, align, i+1); -} -} - /* * i is the current bootmodule we are evaluating, across all * possible kinds of bootmodules. @@ -108,7 +81,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, * need to index the reserved_mem bank starting from 0, and only counting * the reserved-memory modules. Hence, we need to use i - nr. */ -nr += mi->nr_mods; +nr = mi->nr_mods; for ( ; i - nr < reserved_mem->nr_banks; i++ ) { paddr_t r_s = reserved_mem->bank[i - nr].start; diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 4d708442a19e..6e060111d95b 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -475,8 +475,7 @@ static void __init early_print_info(void) const struct membanks *mem_resv = bootinfo_get_reserved_mem(); struct bootmodules *mods = struct bootcmdlines *cmds = -unsigned int i, j; -int nr_rsvd; +unsigned int i; for ( i = 0; i < mi->nr_banks; i++ ) printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n", @@ -490,26 +489,11 @@ static void __init early_print_info(void) mods->module[i].start + mods->module[i].size, boot_module_kind_as_string(mods->module[i].kind)); -nr_rsvd = fdt_num_mem_rsv(device_tree_flattened); -if ( nr_rsvd < 0 ) -panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd); - -for ( i = 0; i < nr_rsvd; i++ ) -{ -paddr_t s, e; - -if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, , ) < 0 ) -continue; - -/* fdt_get_mem_rsv_paddr returns length */ -e += s; -printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, s, e); -} -for ( j = 0; j < mem_resv->nr_banks; j++, i++ ) +for ( i = 0; i < mem_resv->nr_banks; i++ ) { printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i, - mem_resv->bank[j].start, - mem_resv->bank[j].start + mem_resv->bank[j].size - 1); + mem_resv->bank[i].start, + mem_resv->bank[i].start + mem_resv->bank[i].size - 1); } early_print_info_shmem(); printk("\n"); @@ -550,7 +534,10 @@ static void __init swap_memory_node(void *_a, void *_b, size_t size) */ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr) { +struct membanks *reserved_mem = bootinfo_get_reserved_mem(); struct membanks *mem = bootinfo_get_mem(); +unsigned int i; +int nr_rsvd; int ret; ret = fdt_check_header(fdt); @@ -559,6 +546,30 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr) add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false); +nr_rsvd = fdt_num_mem_rsv(fdt); +if ( nr_rsvd < 0 ) +panic("Parsing FDT memory reserve map failed (%d)\n", nr_rsvd); + +for ( i = 0; i < nr_rsvd; i++ ) +{ +struct membank *bank; +paddr_t s, sz; + +if ( fdt_get_mem_rsv_paddr(device_tree_flattened, i, , ) < 0 ) +continue; + +if ( reserved_mem->nr_banks < reser
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi Julien, > > Rest LGTM: > Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. >>> I don't think there is a need to respin the whole series just for these >>> fixes. >>> You should wait for the committers opinion. >> AFAICT, there are multiple changes requested in various line. So I would >> rather prefer if this is respinned. >> If this is the only patch that requires to change. You could send a new one >> in reply-to this patch. I think b4 is clever enough to pick up the new >> version in that case. > > I was wrong. b4 didn't picked up the new version. Anyway, I have applied the > new patch and send to gitlab for testing. I will merge it once it passes. Thanks a lot for that! Cheers, Luca
[PATCH 5/7] xen/arm: Rework heap page allocation outside allocate_bank_memory
The function allocate_bank_memory allocates pages from the heap and map them to the guest using guest_physmap_add_page. As a preparation work to support static shared memory bank when the host physical address is not provided, Xen needs to allocate memory from the heap, so rework allocate_bank_memory moving out the page allocation in a new function called allocate_domheap_memory. The function allocate_domheap_memory takes a callback function and a pointer to some extra information passed to the callback and this function will be called for every page allocated, until a defined size is reached. In order to keep allocate_bank_memory functionality, the callback passed to allocate_domheap_memory is a wrapper for guest_physmap_add_page. Let allocate_domheap_memory be externally visible, in order to use it in the future from the static shared memory module. Take the opportunity to change the signature of allocate_bank_memory and remove the 'struct domain' parameter, which can be retrieved from 'struct kernel_info'. No functional changes is intended. Signed-off-by: Luca Fancellu --- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 77 + xen/arch/arm/include/asm/domain_build.h | 9 ++- 3 files changed, 62 insertions(+), 28 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 74f053c242f4..20ddf6f8f250 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -60,12 +60,12 @@ static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo) mem->nr_banks = 0; bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE), bank_size) ) goto fail; bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem); -if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), +if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE), bank_size) ) goto fail; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 0784e4c5e315..148db06b8ca3 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -417,26 +417,13 @@ static void __init allocate_memory_11(struct domain *d, } #ifdef CONFIG_DOM0LESS_BOOT -bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, - gfn_t sgfn, paddr_t tot_size) +bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size, +alloc_domheap_mem_cb cb, void *extra) { -struct membanks *mem = kernel_info_get_mem(kinfo); -int res; +unsigned int max_order = UINT_MAX; struct page_info *pg; -struct membank *bank; -unsigned int max_order = ~0; -/* - * allocate_bank_memory can be called with a tot_size of zero for - * the second memory bank. It is not an error and we can safely - * avoid creating a zero-size memory bank. - */ -if ( tot_size == 0 ) -return true; - -bank = >bank[mem->nr_banks]; -bank->start = gfn_to_gaddr(sgfn); -bank->size = tot_size; +BUG_ON(!cb); while ( tot_size > 0 ) { @@ -463,17 +450,61 @@ bool __init allocate_bank_memory(struct domain *d, struct kernel_info *kinfo, continue; } -res = guest_physmap_add_page(d, sgfn, page_to_mfn(pg), order); -if ( res ) -{ -dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +if ( cb(d, pg, order, extra) ) return false; -} -sgfn = gfn_add(sgfn, 1UL << order); tot_size -= (1ULL << (PAGE_SHIFT + order)); } +return true; +} + +static int __init guest_map_pages(struct domain *d, struct page_info *pg, + unsigned int order, void *extra) +{ +gfn_t *sgfn = (gfn_t *)extra; +int res; + +BUG_ON(!sgfn); +res = guest_physmap_add_page(d, *sgfn, page_to_mfn(pg), order); +if ( res ) +{ +dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res); +return res; +} + +*sgfn = gfn_add(*sgfn, 1UL << order); + +return 0; +} + +bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn, + paddr_t tot_size) +{ +struct membanks *mem = kernel_info_get_mem(kinfo); +struct domain *d = kinfo->d; +struct membank *bank; + +/* + * allocate_bank_memory can be called with a tot_size of zero for + * the second memory bank. It is not an error and we can safely + * avoid creating a zero-size memory bank. + */ +if ( tot_size == 0 ) +return true; + +bank = >bank[mem->nr_ba
[PATCH 2/7] xen/arm: Wrap shared memory mapping code in one function
Wrap the code and logic that is calling assign_shared_memory and map_regions_p2mt into a new function 'handle_shared_mem_bank', it will become useful later when the code will allow the user to don't pass the host physical address. Signed-off-by: Luca Fancellu --- xen/arch/arm/static-shmem.c | 71 +++-- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index f6cf74e58a83..24e40495a481 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -185,6 +185,47 @@ append_shm_bank_to_domain(struct shared_meminfo *kinfo_shm_mem, paddr_t start, return 0; } +static int __init handle_shared_mem_bank(struct domain *d, paddr_t gbase, + bool owner_dom_io, + const char *role_str, + const struct membank *shm_bank) +{ +paddr_t pbase, psize; +int ret; + +BUG_ON(!shm_bank); + +pbase = shm_bank->start; +psize = shm_bank->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 + * only find the borrowers. + */ +if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || + (!owner_dom_io && strcmp(role_str, "owner") == 0) ) +{ +/* + * 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); +if ( ret ) +return ret; +} + +if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) +{ +/* Set up P2M foreign mapping for borrower domain. */ +ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), + _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); +if ( ret ) +return ret; +} + +return 0; +} + int __init process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -249,32 +290,10 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, if ( dt_property_read_string(shm_node, "role", _str) == 0 ) owner_dom_io = false; -/* - * 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 - * only find the borrowers. - */ -if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) || - (!owner_dom_io && strcmp(role_str, "owner") == 0) ) -{ -/* - * 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, - boot_shm_bank); -if ( ret ) -return ret; -} - -if ( owner_dom_io || (strcmp(role_str, "borrower") == 0) ) -{ -/* Set up P2M foreign mapping for borrower domain. */ -ret = map_regions_p2mt(d, _gfn(PFN_UP(gbase)), PFN_DOWN(psize), - _mfn(PFN_UP(pbase)), p2m_map_foreign_rw); -if ( ret ) -return ret; -} +ret = handle_shared_mem_bank(d, gbase, owner_dom_io, role_str, + boot_shm_bank); +if ( ret ) +return ret; /* * Record static shared memory region info for later setting -- 2.34.1
[PATCH 3/7] xen/p2m: put reference for superpage
From: Penny Zheng We are doing foreign memory mapping for static shared memory, and there is a great possibility that it could be super mapped. But today, p2m_put_l3_page could not handle superpages. This commits implements a new function p2m_put_superpage to handle superpages, specifically for helping put extra references for foreign superpages. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-9-penny.zh...@arm.com/ --- xen/arch/arm/mmu/p2m.c | 58 +++--- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/xen/arch/arm/mmu/p2m.c b/xen/arch/arm/mmu/p2m.c index 41fcca011cf4..479a80fbd4cf 100644 --- a/xen/arch/arm/mmu/p2m.c +++ b/xen/arch/arm/mmu/p2m.c @@ -753,17 +753,9 @@ static int p2m_mem_access_radix_set(struct p2m_domain *p2m, gfn_t gfn, return rc; } -/* - * Put any references on the single 4K page referenced by pte. - * TODO: Handle superpages, for now we only take special references for leaf - * pages (specifically foreign ones, which can't be super mapped today). - */ -static void p2m_put_l3_page(const lpae_t pte) +/* Put any references on the single 4K page referenced by mfn. */ +static void p2m_put_l3_page(mfn_t mfn, unsigned type) { -mfn_t mfn = lpae_get_mfn(pte); - -ASSERT(p2m_is_valid(pte)); - /* * TODO: Handle other p2m types * @@ -771,16 +763,53 @@ static void p2m_put_l3_page(const lpae_t pte) * flush the TLBs if the page is reallocated before the end of * this loop. */ -if ( p2m_is_foreign(pte.p2m.type) ) +if ( p2m_is_foreign(type) ) { ASSERT(mfn_valid(mfn)); put_page(mfn_to_page(mfn)); } /* Detect the xenheap page and mark the stored GFN as invalid. */ -else if ( p2m_is_ram(pte.p2m.type) && is_xen_heap_mfn(mfn) ) +else if ( p2m_is_ram(type) && is_xen_heap_mfn(mfn) ) page_set_xenheap_gfn(mfn_to_page(mfn), INVALID_GFN); } +/* Put any references on the superpage referenced by mfn. */ +static void p2m_put_superpage(mfn_t mfn, unsigned int next_level, unsigned type) +{ +unsigned int i; +unsigned int level_order = XEN_PT_LEVEL_ORDER(next_level); + +for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ ) +{ +if ( next_level == 3 ) +p2m_put_l3_page(mfn, type); +else +p2m_put_superpage(mfn, next_level + 1, type); + +mfn = mfn_add(mfn, 1 << level_order); +} +} + +/* Put any references on the page referenced by pte. */ +static void p2m_put_page(const lpae_t pte, unsigned int level) +{ +mfn_t mfn = lpae_get_mfn(pte); + +ASSERT(p2m_is_valid(pte)); + +/* + * We are either having a first level 1G superpage or a + * second level 2M superpage. + */ +if ( p2m_is_superpage(pte, level) ) +return p2m_put_superpage(mfn, level + 1, pte.p2m.type); +else +{ +ASSERT(level == 3); +return p2m_put_l3_page(mfn, pte.p2m.type); +} +} + /* Free lpae sub-tree behind an entry */ static void p2m_free_entry(struct p2m_domain *p2m, lpae_t entry, unsigned int level) @@ -809,9 +838,8 @@ static void p2m_free_entry(struct p2m_domain *p2m, #endif p2m->stats.mappings[level]--; -/* Nothing to do if the entry is a super-page. */ -if ( level == 3 ) -p2m_put_l3_page(entry); +p2m_put_page(entry, level); + return; } -- 2.34.1
[PATCH 7/7] xen/docs: Describe static shared memory when host address is not provided
From: Penny Zheng This commit describe the new scenario where host address is not provided in "xen,shared-mem" property and a new example is added to the page to explain in details. Take the occasion to fix some typos in the page. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v1: - patch from https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-10-penny.zh...@arm.com/ with some changes in the commit message. --- docs/misc/arm/device-tree/booting.txt | 52 --- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt index bbd955e9c2f6..ac4bad6fe5e0 100644 --- a/docs/misc/arm/device-tree/booting.txt +++ b/docs/misc/arm/device-tree/booting.txt @@ -590,7 +590,7 @@ communication. An array takes a physical address, which is the base address of the shared memory region in host physical address space, a size, and a guest physical address, as the target address of the mapping. -e.g. xen,shared-mem = < [host physical address] [guest address] [size] > +e.g. xen,shared-mem = < [host physical address] [guest address] [size] >; It shall also meet the following criteria: 1) If the SHM ID matches with an existing region, the address range of the @@ -601,8 +601,8 @@ communication. The number of cells for the host address (and size) is the same as the guest pseudo-physical address and they are inherited from the parent node. -Host physical address is optional, when missing Xen decides the location -(currently unimplemented). +Host physical address is optional, when missing Xen decides the location. +e.g. xen,shared-mem = < [guest address] [size] >; - role (Optional) @@ -629,7 +629,7 @@ chosen { role = "owner"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x1000 0x1000>; -} +}; domU1 { compatible = "xen,domain"; @@ -640,25 +640,36 @@ chosen { vpl011; /* - * shared memory region identified as 0x0(xen,shm-id = <0x0>) - * is shared between Dom0 and DomU1. + * shared memory region "my-shared-mem-0" is shared + * between Dom0 and DomU1. */ domU1-shared-mem@1000 { compatible = "xen,domain-shared-memory-v1"; role = "borrower"; xen,shm-id = "my-shared-mem-0"; xen,shared-mem = <0x1000 0x5000 0x1000>; -} +}; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between DomU1 and DomU2. + * shared memory region "my-shared-mem-1" is shared between + * DomU1 and DomU2. */ domU1-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x6000 0x2000>; -} +}; + +/* + * shared memory region "my-shared-mem-2" is shared between + * DomU1 and DomU2. + */ +domU1-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "owner"; +xen,shared-mem = <0x8000 0x2000>; +}; .. @@ -672,14 +683,21 @@ chosen { cpus = <1>; /* - * shared memory region identified as 0x1(xen,shm-id = <0x1>) - * is shared between domU1 and domU2. + * shared memory region "my-shared-mem-1" is shared between + * domU1 and domU2. */ domU2-shared-mem@5000 { compatible = "xen,domain-shared-memory-v1"; xen,shm-id = "my-shared-mem-1"; xen,shared-mem = <0x5000 0x7000 0x2000>; -} +}; + +domU2-shared-mem-2 { +compatible = "xen,domain-shared-memory-v1"; +xen,shm-id = "my-shared-mem-2"; +role = "borrower"; +xen,shared-mem = <0x9000 0x2000>; +}; .. }; @@ -699,3 +717,11 @@ shared between DomU1 and DomU2. It will get mapped at 0x6000 in DomU1 guest physical address space, and at 0x7000 in DomU2 guest physical address space. DomU1 and DomU2 are both the borrower domain, the owner domain is the default owner domain DOMID_IO. + +For the static shared memory region "my-shared-mem-2", since host physical +address is not provided by user, Xen will automatically allocate 512MB +from heap as static shared memory to be shared between DomU1 and DomU2. +The automatically allocated static shared memory will get mapped at +0x8000 in DomU1 guest physical address space, and at 0x9000 in DomU2 +guest physical address space. DomU1 is explicitly defined as the owner domain, +and DomU2 is the borrower domain. -- 2.34.1
[PATCH 4/7] xen/arm: Parse xen,shared-mem when host phys address is not provided
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 don't 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 and ACPI and it's unlikely for these bank to have the start address as INVALID_PADDR. The change holds because of this consideration. Signed-off-by: Luca Fancellu --- xen/arch/arm/setup.c| 3 +- xen/arch/arm/static-shmem.c | 129 +--- 2 files changed, 93 insertions(+), 39 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index d242674381d4..f15d40a85a5f 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -297,7 +297,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 24e40495a481..1c03bb7f1882 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 = ; * TODO: pbase is optional. @@ -382,7 +388,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; @@ -437,24 +444,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(, address_cells, size_cells, , +); +else +{ +printk("fdt: invalid `xen,shared-mem` property.\n"); +return -EINVAL; +} } +else +{ +device_tree_get_reg(, address_cells, address_cells, , +); +size = dt_next_cell(size_cells, ); -cell = (const __be32 *)prop->data; -device_tree_get_reg(, address_cells, address_cells, , ); -size = dt_next_cell(size_cells, ); +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) ) @@ -476,41 +496,69 @@ 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); -retur
[PATCH 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 --- xen/arch/arm/static-shmem.c | 193 +--- 1 file changed, 157 insertions(+), 36 deletions(-) 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 #include +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: S
[PATCH 0/7] Static shared memory followup v2 - pt2
This serie is a partial rework of this other serie: https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ The original serie is addressing an issue of the static shared memory feature that impacts the memory footprint of other component when the feature is enabled, another issue impacts the device tree generation for the guests when the feature is enabled and used and the last one is a missing feature that is the option to have a static shared memory region that is not from the host address space. This serie is handling some comment on the original serie and it is splitting the rework in two part, this first part is addressing the memory footprint issue and the device tree generation and currently is fully reviewed by Michal (https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/), this serie is addressing the static shared memory allocation from the Xen heap. This serie is meant to be applied on top of: https://patchwork.kernel.org/project/xen-devel/cover/20240418073652.3622828-1-luca.fance...@arm.com/ where the last patch was amended in favour of: https://patchwork.kernel.org/project/xen-devel/patch/20240422110207.204968-1-luca.fance...@arm.com/ Luca Fancellu (5): xen/arm: Lookup bootinfo shm bank during the mapping xen/arm: Wrap shared memory mapping code in one function xen/arm: Parse xen,shared-mem when host phys address is not provided xen/arm: Rework heap page allocation outside allocate_bank_memory xen/arm: Implement the logic for static shared memory from Xen heap Penny Zheng (2): xen/p2m: put reference for superpage xen/docs: Describe static shared memory when host address is not provided docs/misc/arm/device-tree/booting.txt | 52 ++- xen/arch/arm/dom0less-build.c | 4 +- xen/arch/arm/domain_build.c | 77 +++-- xen/arch/arm/include/asm/domain_build.h | 9 +- xen/arch/arm/mmu/p2m.c | 58 +++- xen/arch/arm/setup.c| 3 +- xen/arch/arm/static-shmem.c | 430 +--- 7 files changed, 463 insertions(+), 170 deletions(-) -- 2.34.1
[PATCH 1/7] xen/arm: Lookup bootinfo shm bank during the mapping
The current static shared memory code is using bootinfo banks when it needs to find the number of borrower, so every time assign_shared_memory is called, the bank is searched in the bootinfo.shmem structure. There is nothing wrong with it, however the bank can be used also to retrieve the start address and size and also to pass less argument to assign_shared_memory. When retrieving the information from the bootinfo bank, it's also possible to move the checks on alignment to process_shm_node in the early stages. So create a new function find_shm() which takes a 'struct shared_meminfo' structure and the shared memory ID, to look for a bank with a matching ID, take the physical host address and size from the bank, pass the bank to assign_shared_memory() removing the now unnecessary arguments and finally remove the acquire_nr_borrower_domain() function since now the information can be extracted from the passed bank. Move the "xen,shm-id" parsing early in process_shm to bail out quickly in case of errors (unlikely), as said above, move the checks on alignment to process_shm_node. Drawback of this change is that now the bootinfo are used also when the bank doesn't need to be allocated, however it will be convinient later to use it as an argument for assign_shared_memory when dealing with the use case where the Host physical address is not supplied by the user. Signed-off-by: Luca Fancellu --- xen/arch/arm/static-shmem.c | 105 1 file changed, 58 insertions(+), 47 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 09f474ec6050..f6cf74e58a83 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -19,29 +19,24 @@ static void __init __maybe_unused build_assertions(void) offsetof(struct shared_meminfo, bank))); } -static int __init acquire_nr_borrower_domain(struct domain *d, - paddr_t pbase, paddr_t psize, - unsigned long *nr_borrowers) +static const struct membank __init *find_shm(const struct membanks *shmem, + const char *shm_id) { -const struct membanks *shmem = bootinfo_get_shmem(); unsigned int bank; -/* Iterate reserved memory to find requested shm bank. */ +BUG_ON(!shmem || !shm_id); + for ( bank = 0 ; bank < shmem->nr_banks; bank++ ) { -paddr_t bank_start = shmem->bank[bank].start; -paddr_t bank_size = shmem->bank[bank].size; - -if ( (pbase == bank_start) && (psize == bank_size) ) +if ( strncmp(shm_id, shmem->bank[bank].shmem_extra->shm_id, + MAX_SHM_ID_LENGTH) == 0 ) break; } if ( bank == shmem->nr_banks ) -return -ENOENT; - -*nr_borrowers = shmem->bank[bank].shmem_extra->nr_shm_borrowers; +return NULL; -return 0; +return >bank[bank]; } /* @@ -103,14 +98,20 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, return smfn; } -static int __init assign_shared_memory(struct domain *d, - paddr_t pbase, paddr_t psize, - paddr_t gbase) +static int __init assign_shared_memory(struct domain *d, paddr_t gbase, + const struct membank *shm_bank) { mfn_t smfn; int ret = 0; unsigned long nr_pages, nr_borrowers, i; struct page_info *page; +paddr_t pbase, psize; + +BUG_ON(!shm_bank || !shm_bank->shmem_extra); + +pbase = shm_bank->start; +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); @@ -135,14 +136,6 @@ static int __init assign_shared_memory(struct domain *d, } } -/* - * Get the right amount of references per page, which is the number of - * borrower domains. - */ -ret = acquire_nr_borrower_domain(d, pbase, psize, _borrowers); -if ( ret ) -return ret; - /* * Instead of letting borrower domain get a page ref, we add as many * additional reference as the number of borrowers when the owner @@ -199,6 +192,7 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, dt_for_each_child_node(node, shm_node) { +const struct membank *boot_shm_bank; const struct dt_property *prop; const __be32 *cells; uint32_t addr_cells, size_cells; @@ -212,6 +206,23 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) continue; +if ( dt_property_read_string(
[PATCH v3.2 12/12] xen/arm: List static shared memory regions as /memory nodes
Currently Xen is not exporting the static shared memory regions to the device tree as /memory node, this commit is fixing this issue. Given that now make_memory_node needs a parameter 'struct kernel_info' in order to call the new function shm_mem_node_fill_reg_range, take the occasion to remove the unused struct domain parameter. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3.2: - changed u64 to uint64_t and used %u as format specifier for var i in make_memory_node, changed u64 to paddr_t in shm_mem_node_fill_reg_range, add Michal R-by v3: - removed previous patch that was merging the intervals, rebase changes. v2: - try to use make_memory_node, don't add overlapping ranges of memory, commit message changed. v1: - new patch --- --- xen/arch/arm/dom0less-build.c | 2 +- xen/arch/arm/domain_build.c | 34 + xen/arch/arm/include/asm/domain_build.h | 2 +- xen/arch/arm/include/asm/static-shmem.h | 15 +++ xen/arch/arm/static-shmem.c | 23 + 5 files changed, 63 insertions(+), 13 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 51cf03221d56..74f053c242f4 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -642,7 +642,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; -ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, +ret = make_memory_node(kinfo, addrcells, sizecells, kernel_info_get_mem(kinfo)); if ( ret ) goto err; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 68532ddc084c..0784e4c5e315 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -756,15 +756,14 @@ int __init domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit) return fdt_begin_node(fdt, buf); } -int __init make_memory_node(const struct domain *d, -void *fdt, -int addrcells, int sizecells, -const struct membanks *mem) +int __init make_memory_node(const struct kernel_info *kinfo, int addrcells, +int sizecells, const struct membanks *mem) { +void *fdt = kinfo->fdt; unsigned int i; int res, reg_size = addrcells + sizecells; int nr_cells = 0; -__be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; +__be32 reg[DT_MEM_NODE_REG_RANGE_SIZE]; __be32 *cells; if ( mem->nr_banks == 0 ) @@ -797,14 +796,28 @@ int __init make_memory_node(const struct domain *d, if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN ) continue; -dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", - i, start, start + size); - nr_cells += reg_size; BUG_ON(nr_cells >= ARRAY_SIZE(reg)); dt_child_set_range(, addrcells, sizecells, start, size); } +/* + * static shared memory banks need to be listed as /memory node, so when + * this function is handling the normal memory, add the banks. + */ +if ( mem == kernel_info_get_mem(kinfo) ) +shm_mem_node_fill_reg_range(kinfo, reg, _cells, addrcells, +sizecells); + +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size ) +{ +uint64_t start = dt_read_number(cells, addrcells); +uint64_t size = dt_read_number(cells + addrcells, sizecells); + +dt_dprintk(" Bank %u: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); +} + dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells); res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg)); @@ -1783,7 +1796,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, if ( res ) return res; -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, +res = make_memory_node(kinfo, addrcells, sizecells, kernel_info_get_mem(kinfo)); if ( res ) return res; @@ -1794,8 +1807,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, */ if ( reserved_mem->nr_banks > 0 ) { -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, - reserved_mem); +res = make_memory_node(kinfo, addrcells, sizecells, reserved_mem); if ( res ) return res; } diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h index 026d975da28e..45936212ca21 100644 --- a/xen/arch/arm/include/asm/domain_build.h +++ b/xen/arch/arm/include/a
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
> On 22 Apr 2024, at 11:24, Julien Grall wrote: > > Hi, > > On 22/04/2024 10:26, Michal Orzel wrote: >> On 22/04/2024 10:07, Luca Fancellu wrote: >>> >>> >>> Hi Michal, >>> >>>>> +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += >>>>> reg_size ) >>>>> +{ >>>>> +u64 start = dt_read_number(cells, addrcells); >>>> We should no longer use Linux derived types like u64. Use uint64_t. >>>> >>>>> +u64 size = dt_read_number(cells + addrcells, sizecells); >>>>> + >>>>> +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", >>>>> + i, start, start + size); >>>> i is unsigned so the correct format specifier should be %u >>> >>> Right, should have been more careful when copying the code from above >>> >>>>> >>>>> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, >>>>> +__be32 *reg, int *nr_cells, >>>>> +int addrcells, int sizecells) >>>>> +{ >>>>> +const struct membanks *mem = >shm_mem.common; >>>>> +unsigned int i; >>>>> +__be32 *cells; >>>>> + >>>>> +BUG_ON(!nr_cells || !reg); >>>>> + >>>>> +cells = [*nr_cells]; >>>>> +for ( i = 0; i < mem->nr_banks; i++ ) >>>>> +{ >>>>> +u64 start = mem->bank[i].start; >>>> ditto >>> >>> Will fix, here paddr_t should be ok isn’t it? >> yes >>> >>>> >>>> Rest LGTM: >>>> Reviewed-by: Michal Orzel >>> >>> Thanks, I will send the next one shortly. >> I don't think there is a need to respin the whole series just for these >> fixes. >> You should wait for the committers opinion. > > AFAICT, there are multiple changes requested in various line. So I would > rather prefer if this is respinned. > > If this is the only patch that requires to change. You could send a new one > in reply-to this patch. I think b4 is clever enough to pick up the new > version in that case. All the other patches are already reviewed by Michal, if you agree with his review it’s ok for me to respin only this one Cheers, Luca > > Cheers, > > -- > Julien Grall
Re: [PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Hi Michal, >> +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += >> reg_size ) >> +{ >> +u64 start = dt_read_number(cells, addrcells); > We should no longer use Linux derived types like u64. Use uint64_t. > >> +u64 size = dt_read_number(cells + addrcells, sizecells); >> + >> +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", >> + i, start, start + size); > i is unsigned so the correct format specifier should be %u Right, should have been more careful when copying the code from above >> >> +void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo, >> +__be32 *reg, int *nr_cells, >> +int addrcells, int sizecells) >> +{ >> +const struct membanks *mem = >shm_mem.common; >> +unsigned int i; >> +__be32 *cells; >> + >> +BUG_ON(!nr_cells || !reg); >> + >> +cells = [*nr_cells]; >> +for ( i = 0; i < mem->nr_banks; i++ ) >> +{ >> +u64 start = mem->bank[i].start; > ditto Will fix, here paddr_t should be ok isn’t it? > > Rest LGTM: > Reviewed-by: Michal Orzel Thanks, I will send the next one shortly. Cheers, Luca
[PATCH v3 11/12] xen/arm: fix duplicate /reserved-memory node in Dom0
From: Penny Zheng In case there is a /reserved-memory node already present in the host dtb, current Xen codes would create yet another /reserved-memory node when the static shared memory feature is enabled and static shared memory regions are present. This would result in an incorrect device tree generation and hwdom would not be able to detect the static shared memory region. Avoid this issue by checking the presence of the /reserved-memory node and appending the nodes instead of generating a duplicate /reserved-memory. Make make_shm_memory_node externally visible and rename it to make_shm_resv_memory_node to make clear it produces childs for /reserved-memory. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - fix comment, remove function signature change, fixed commit msg - rename make_shm_memory_node to make_shm_resv_memory_node in order to make clear that it produces childs for /reserved-memory - Add Michal R-by v1: - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-11-penny.zh...@arm.com/ --- xen/arch/arm/domain_build.c | 23 --- xen/arch/arm/include/asm/static-shmem.h | 9 + xen/arch/arm/static-shmem.c | 8 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 0cc39b0bd7bb..68532ddc084c 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1640,6 +1640,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, DT_MATCH_PATH("/hypervisor"), { /* sentinel */ }, }; +static __initdata bool res_mem_node_found = false; struct dt_device_node *child; int res, i, nirq, irq_id; const char *name; @@ -1734,6 +1735,19 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, if ( res ) return res; +if ( dt_node_path_is_equal(node, "/reserved-memory") ) +{ +res_mem_node_found = true; +/* + * Avoid duplicate /reserved-memory nodes in Device Tree, so add the + * static shared memory nodes there. + */ +res = make_shm_resv_memory_node(kinfo, dt_n_addr_cells(node), +dt_n_size_cells(node)); +if ( res ) +return res; +} + for ( child = node->child; child != NULL; child = child->sibling ) { res = handle_node(d, kinfo, child, p2mt); @@ -1786,9 +1800,12 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } -res = make_resv_memory_node(kinfo, addrcells, sizecells); -if ( res ) -return res; +if ( !res_mem_node_found ) +{ +res = make_resv_memory_node(kinfo, addrcells, sizecells); +if ( res ) +return res; +} } res = fdt_end_node(kinfo->fdt); diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index 2e8b138eb989..7495a91e7a31 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -34,6 +34,9 @@ int remove_shm_from_rangeset(const struct kernel_info *kinfo, int remove_shm_holes_for_domU(const struct kernel_info *kinfo, struct membanks *ext_regions); +int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells, + int sizecells); + #else /* !CONFIG_STATIC_SHM */ static inline int make_resv_memory_node(const struct kernel_info *kinfo, @@ -77,6 +80,12 @@ static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo, return 0; } +static inline int make_shm_resv_memory_node(const struct kernel_info *kinfo, +int addrcells, int sizecells) +{ +return 0; +} + #endif /* CONFIG_STATIC_SHM */ #endif /* __ASM_STATIC_SHMEM_H_ */ diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 12e2df939915..c85f60dd1bf7 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -297,8 +297,8 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, return 0; } -static int __init make_shm_memory_node(const struct kernel_info *kinfo, - int addrcells, int sizecells) +int __init make_shm_resv_memory_node(const struct kernel_info *kinfo, + int addrcells, int sizecells) { const struct membanks *mem = >shm_mem.common; void *fdt = kinfo->fdt; @@ -306,7 +306,7 @@ static int __init make_shm_memory_node(const struct kernel_info *kinfo, int res = 0; if ( mem->nr_banks == 0 ) -return -ENOENT; +return 0; /* * For each shared memory region, a r
Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
> On 18 Apr 2024, at 07:28, Jan Beulich wrote: > > On 09.04.2024 13:45, Luca Fancellu wrote: >> --- a/xen/arch/x86/extable.c >> +++ b/xen/arch/x86/extable.c >> @@ -23,7 +23,8 @@ static inline unsigned long ex_cont(const struct >> exception_table_entry *x) >> return EX_FIELD(x, cont); >> } >> >> -static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b) >> +static int init_or_livepatch cf_check cmp_ex(const void *a, const void *b, >> + const void *data) >> { >> const struct exception_table_entry *l = a, *r = b; >> unsigned long lip = ex_addr(l); >> @@ -53,7 +54,7 @@ void init_or_livepatch sort_exception_table(struct >> exception_table_entry *start, >> const struct exception_table_entry *stop) >> { >> sort(start, stop - start, >> - sizeof(struct exception_table_entry), cmp_ex, swap_ex); >> + sizeof(struct exception_table_entry), cmp_ex, swap_ex, NULL); >> } > > Not the least because of this addition of an entirely useless parameter / > argument Well it’s not useless in this patch, given that without it I couldn’t know the size of the address element, however ... > I'm not in favor of ... > >> --- a/xen/include/xen/sort.h >> +++ b/xen/include/xen/sort.h >> @@ -23,8 +23,8 @@ >> extern gnu_inline >> #endif >> void sort(void *base, size_t num, size_t size, >> - int (*cmp)(const void *a, const void *b), >> - void (*swap)(void *a, void *b, size_t size)) >> + int (*cmp)(const void *a, const void *b, const void *data), >> + void (*swap)(void *a, void *b, size_t size), const void *cmp_data) >> { > > ... this change. Consider you were doing this on a C library you cannot > change. > You'd have to find a different solution anyway. I get your point here, we should not change standard functions. > And the way we have sort() > right now is matching the C spec. The change to do renders things unexpected > to > anyone wanting to use this function in a spec-compliant way. One approach may > be to make an adjustment to data representation, such that the extra reference > data is accessible through the pointers already being passed. > > Jan > Anyway in the end this patch was dropped for other reasons. Cheers, Luca
[PATCH v3 12/12] xen/arm: List static shared memory regions as /memory nodes
Currently Xen is not exporting the static shared memory regions to the device tree as /memory node, this commit is fixing this issue. Given that now make_memory_node needs a parameter 'struct kernel_info' in order to call the new function shm_mem_node_fill_reg_range, take the occasion to remove the unused struct domain parameter. Signed-off-by: Luca Fancellu --- v3: - removed previous patch that was merging the intervals, rebase changes. v2: - try to use make_memory_node, don't add overlapping ranges of memory, commit message changed. v1: - new patch --- --- xen/arch/arm/dom0less-build.c | 2 +- xen/arch/arm/domain_build.c | 34 + xen/arch/arm/include/asm/domain_build.h | 2 +- xen/arch/arm/include/asm/static-shmem.h | 15 +++ xen/arch/arm/static-shmem.c | 23 + 5 files changed, 63 insertions(+), 13 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index 51cf03221d56..74f053c242f4 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -642,7 +642,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; -ret = make_memory_node(d, kinfo->fdt, addrcells, sizecells, +ret = make_memory_node(kinfo, addrcells, sizecells, kernel_info_get_mem(kinfo)); if ( ret ) goto err; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 68532ddc084c..15f888169c4e 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -756,15 +756,14 @@ int __init domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit) return fdt_begin_node(fdt, buf); } -int __init make_memory_node(const struct domain *d, -void *fdt, -int addrcells, int sizecells, -const struct membanks *mem) +int __init make_memory_node(const struct kernel_info *kinfo, int addrcells, +int sizecells, const struct membanks *mem) { +void *fdt = kinfo->fdt; unsigned int i; int res, reg_size = addrcells + sizecells; int nr_cells = 0; -__be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; +__be32 reg[DT_MEM_NODE_REG_RANGE_SIZE]; __be32 *cells; if ( mem->nr_banks == 0 ) @@ -797,14 +796,28 @@ int __init make_memory_node(const struct domain *d, if ( mem->bank[i].type == MEMBANK_STATIC_DOMAIN ) continue; -dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", - i, start, start + size); - nr_cells += reg_size; BUG_ON(nr_cells >= ARRAY_SIZE(reg)); dt_child_set_range(, addrcells, sizecells, start, size); } +/* + * static shared memory banks need to be listed as /memory node, so when + * this function is handling the normal memory, add the banks. + */ +if ( mem == kernel_info_get_mem(kinfo) ) +shm_mem_node_fill_reg_range(kinfo, reg, _cells, addrcells, +sizecells); + +for ( cells = reg, i = 0; cells < reg + nr_cells; i++, cells += reg_size ) +{ +u64 start = dt_read_number(cells, addrcells); +u64 size = dt_read_number(cells + addrcells, sizecells); + +dt_dprintk(" Bank %d: %#"PRIx64"->%#"PRIx64"\n", + i, start, start + size); +} + dt_dprintk("(reg size %d, nr cells %d)\n", reg_size, nr_cells); res = fdt_property(fdt, "reg", reg, nr_cells * sizeof(*reg)); @@ -1783,7 +1796,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, if ( res ) return res; -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, +res = make_memory_node(kinfo, addrcells, sizecells, kernel_info_get_mem(kinfo)); if ( res ) return res; @@ -1794,8 +1807,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, */ if ( reserved_mem->nr_banks > 0 ) { -res = make_memory_node(d, kinfo->fdt, addrcells, sizecells, - reserved_mem); +res = make_memory_node(kinfo, addrcells, sizecells, reserved_mem); if ( res ) return res; } diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h index 026d975da28e..45936212ca21 100644 --- a/xen/arch/arm/include/asm/domain_build.h +++ b/xen/arch/arm/include/asm/domain_build.h @@ -14,7 +14,7 @@ int make_chosen_node(const struct kernel_info *kinfo); int make_cpus_node(const struct domain *d, void *fdt); int make_hypervisor_node(struct domain *d, const struct kernel_info
[PATCH v3 09/12] xen/arm: Reduce struct membank size on static shared memory
Currently the memory footprint of the static shared memory feature is impacting all the struct meminfo instances with memory space that is not going to be used. To solve this issue, rework the static shared memory extra information linked to the memory bank to another structure, struct shmem_membank_extra, and exploit the struct membank padding to host a pointer to that structure in a union with the enum membank_type, with this trick the 'struct membank' has the same size with or without the static shared memory, given that the 'type' and 'shmem_extra' are never used at the same time, hence the 'struct membank' won't grow in size. Afterwards, create a new structure 'struct shared_meminfo' which has the same interface of 'struct meminfo', but requires less banks, defined by the number in NR_SHMEM_BANKS, which is 32 at the moment and should be enough for the current use cases, the value might be increased in te future if needed. Finally, this structure hosts also the extra information for the static shared memory banks. The fields 'bank' and 'extra' of this structure are meant to be linked by the index (e.g. extra[idx] will have the information for the bank[idx], for i=0..NR_SHMEM_BANKS), the convinient pointer 'shmem_extra' of 'struct membank' is then linked to the related 'extra' bank to ease the fruition when a function has access only to the 'struct membanks common' of 'struct shared_meminfo'. The last part of this work is to move the allocation of the static shared memory banks from the 'reserved_mem' to a new 'shmem' member of the 'struct bootinfo'. Change also the 'shm_mem' member type to be 'struct shared_meminfo' in order to match the above changes and allow a memory space reduction also in 'struct kernel_info'. Now that the structure Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3: - rebase changes, improved comment in find_unallocated_memory - Add Michal R-by v2: - Made clear that the struct membank is not growing in size in the commit message. - Add static assert for the struct shared_meminfo bank field. v1: - new patch --- --- xen/arch/arm/arm32/mmu/mm.c | 24 xen/arch/arm/arm64/mmu/mm.c | 2 + xen/arch/arm/bootfdt.c | 1 + xen/arch/arm/domain_build.c | 5 ++ xen/arch/arm/include/asm/kernel.h | 4 +- xen/arch/arm/include/asm/setup.h| 41 - xen/arch/arm/include/asm/static-shmem.h | 8 +++ xen/arch/arm/setup.c| 25 +++- xen/arch/arm/static-shmem.c | 79 +++-- 9 files changed, 165 insertions(+), 24 deletions(-) diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c index 84c89da909bf..23150122f7c4 100644 --- a/xen/arch/arm/arm32/mmu/mm.c +++ b/xen/arch/arm/arm32/mmu/mm.c @@ -8,6 +8,7 @@ #include #include #include +#include static unsigned long opt_xenheap_megabytes __initdata; integer_param("xenheap_megabytes", opt_xenheap_megabytes); @@ -43,6 +44,9 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, int first_mod) { const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); +#ifdef CONFIG_STATIC_SHM +const struct membanks *shmem = bootinfo_get_shmem(); +#endif const struct bootmodules *mi = int i; int nr; @@ -119,6 +123,25 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, return consider_modules(s, r_s, size, align, i + 1); } } + +#ifdef CONFIG_STATIC_SHM +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; + +if ( s < r_e && r_s < e ) +{ +r_e = consider_modules(r_e, e, size, align, i + 1); +if ( r_e ) +return r_e; + +return consider_modules(s, r_s, size, align, i + 1); +} +} +#endif + return e; } @@ -295,6 +318,7 @@ void __init setup_mm(void) mfn_to_maddr(directmap_mfn_end)); init_staticmem_pages(); +init_sharedmem_pages(); } /* diff --git a/xen/arch/arm/arm64/mmu/mm.c b/xen/arch/arm/arm64/mmu/mm.c index f8aaf4ac18be..293acb67e09c 100644 --- a/xen/arch/arm/arm64/mmu/mm.c +++ b/xen/arch/arm/arm64/mmu/mm.c @@ -6,6 +6,7 @@ #include #include +#include /* Override macros from asm/page.h to make them work with mfn_t */ #undef virt_to_mfn @@ -236,6 +237,7 @@ void __init setup_mm(void) max_page = PFN_DOWN(ram_end); init_staticmem_pages(); +init_sharedmem_pages(); } /* diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 4c80962f79d4..4d708442a19e 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -511,6 +511,7 @@ static void __init early_print_info(void) mem_resv->bank[j].start,
[PATCH v3 10/12] xen/arm: remove shm holes from extended regions
From: Penny Zheng Static shared memory acts as reserved memory in guest, so it shall be excluded from extended regions. Extended regions are taken care of under three different scenarios: normal DomU, direct-map domain with iommu on, and direct-map domain with iommu off. For normal DomU, we create a new function "remove_shm_holes_for_domU", to firstly transfer original outputs into the format of "struct rangeset", then use "remove_shm_from_rangeset" to remove static shm from them. For direct-map domain with iommu on, after we get guest shm info from "kinfo", we use "remove_shm_from_rangeset" to remove static shm. For direct-map domain with iommu off, as static shm has already been taken care of through find_unallocated_memory, we do nothing. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu --- v3: - Put size -1 inside PFN_DOWN so that error message has the 'end' unchanged. - Add expanation comment inside remove_shm_holes_for_domU v2: - Fixed commit title, fixed comment, moved call of remove_shm_from_rangeset after populating rangeset in find_memory_holes, print error code when possible - used PFN_DOWN where needed v1: - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-penny.zh...@arm.com/ --- --- xen/arch/arm/domain_build.c | 16 - xen/arch/arm/include/asm/domain_build.h | 2 + xen/arch/arm/include/asm/static-shmem.h | 18 + xen/arch/arm/static-shmem.c | 94 + 4 files changed, 127 insertions(+), 3 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 2fc7feeae3c3..0cc39b0bd7bb 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -816,8 +816,8 @@ int __init make_memory_node(const struct domain *d, return res; } -static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, - void *data) +int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, + void *data) { struct membanks *ext_regions = data; paddr_t start, size; @@ -990,6 +990,8 @@ static int __init handle_pci_range(const struct dt_device_node *dev, * - MMIO * - Host RAM * - PCI aperture + * - Static shared memory regions, which are described by special property + * "xen,shared-mem" */ static int __init find_memory_holes(const struct kernel_info *kinfo, struct membanks *ext_regions) @@ -1017,6 +1019,11 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, goto out; } +/* Remove static shared memory regions */ +res = remove_shm_from_rangeset(kinfo, mem_holes); +if ( res ) +goto out; + /* * Remove regions described by "reg" and "ranges" properties where * the memory is addressable (MMIO, RAM, PCI BAR, etc). @@ -1109,7 +1116,10 @@ static int __init find_domU_holes(const struct kernel_info *kinfo, res = 0; } -return res; +if ( res ) +return res; + +return remove_shm_holes_for_domU(kinfo, ext_regions); } int __init make_hypervisor_node(struct domain *d, diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h index a6f276cc4263..026d975da28e 100644 --- a/xen/arch/arm/include/asm/domain_build.h +++ b/xen/arch/arm/include/asm/domain_build.h @@ -51,6 +51,8 @@ static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo) int prepare_acpi(struct domain *d, struct kernel_info *kinfo); #endif +int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data); + #endif /* diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index 90aafc81e740..2e8b138eb989 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -28,6 +28,12 @@ void early_print_info_shmem(void); void init_sharedmem_pages(void); +int remove_shm_from_rangeset(const struct kernel_info *kinfo, + struct rangeset *rangeset); + +int remove_shm_holes_for_domU(const struct kernel_info *kinfo, + struct membanks *ext_regions); + #else /* !CONFIG_STATIC_SHM */ static inline int make_resv_memory_node(const struct kernel_info *kinfo, @@ -59,6 +65,18 @@ static inline void early_print_info_shmem(void) {}; static inline void init_sharedmem_pages(void) {}; +static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo, + struct rangeset *rangeset) +{ +return 0; +} + +static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo, +struct membanks *ext_regions) +{ +return 0; +} + #endif /* CONFIG_STATIC_SHM */ #endif /* __ASM_STATIC_SHMEM_H_
[PATCH v3 07/12] xen/arm: Avoid code duplication in check_reserved_regions_overlap
The function check_reserved_regions_overlap is calling 'meminfo_overlap_check' on the same type of structure, this code can be written in a way to avoid code duplication, so rework the function to do that. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3: - add Michal R-by v2: - no changes v1: - new patch --- --- xen/arch/arm/setup.c | 24 +--- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index e00dbb135113..003b0446af27 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -340,25 +340,27 @@ void __init fw_unreserved_regions(paddr_t s, paddr_t e, bool __init check_reserved_regions_overlap(paddr_t region_start, paddr_t region_size) { +const struct membanks *mem_banks[] = { +bootinfo_get_reserved_mem(), +#ifdef CONFIG_ACPI +bootinfo_get_acpi(), +#endif +}; +unsigned int i; + /* - * Check if input region is overlapping with bootinfo_get_reserved_mem() - * banks + * Check if input region is overlapping with reserved memory banks or + * ACPI EfiACPIReclaimMemory (when ACPI feature is enabled) */ -if ( meminfo_overlap_check(bootinfo_get_reserved_mem(), - region_start, region_size) ) -return true; +for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) +if ( meminfo_overlap_check(mem_banks[i], region_start, region_size) ) +return true; /* Check if input region is overlapping with bootmodules */ if ( bootmodules_overlap_check(, region_start, region_size) ) return true; -#ifdef CONFIG_ACPI -/* Check if input region is overlapping with ACPI EfiACPIReclaimMemory */ -if ( meminfo_overlap_check(bootinfo_get_acpi(), region_start, region_size) ) -return true; -#endif - return false; } -- 2.34.1
[PATCH v3 08/12] xen/arm: Introduce helper for static memory pages
Introduce a new helper function in the static-memory module that can be called to manage static memory banks, this is done to reuse the code when other modules would like to manage static memory banks that are not part of the reserved_mem structure, this is done because the static shared memory banks will be removed from reserved_mem. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - Add Michal R-by - Changed commit msg v1: - new patch --- --- xen/arch/arm/include/asm/static-memory.h | 13 + xen/arch/arm/static-memory.c | 12 +--- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/xen/arch/arm/include/asm/static-memory.h b/xen/arch/arm/include/asm/static-memory.h index 3e3efd70c38d..804166e541ef 100644 --- a/xen/arch/arm/include/asm/static-memory.h +++ b/xen/arch/arm/include/asm/static-memory.h @@ -3,10 +3,23 @@ #ifndef __ASM_STATIC_MEMORY_H_ #define __ASM_STATIC_MEMORY_H_ +#include #include #ifdef CONFIG_STATIC_MEMORY +static inline void init_staticmem_bank(const struct membank *bank) +{ +mfn_t bank_start = _mfn(PFN_UP(bank->start)); +unsigned long bank_pages = PFN_DOWN(bank->size); +mfn_t bank_end = mfn_add(bank_start, bank_pages); + +if ( mfn_x(bank_end) <= mfn_x(bank_start) ) +return; + +unprepare_staticmem_pages(mfn_to_page(bank_start), bank_pages, false); +} + void allocate_static_memory(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node); void assign_static_memory_11(struct domain *d, struct kernel_info *kinfo, diff --git a/xen/arch/arm/static-memory.c b/xen/arch/arm/static-memory.c index 34bd12696a53..d4585c5a0633 100644 --- a/xen/arch/arm/static-memory.c +++ b/xen/arch/arm/static-memory.c @@ -265,17 +265,7 @@ void __init init_staticmem_pages(void) for ( bank = 0 ; bank < reserved_mem->nr_banks; bank++ ) { if ( reserved_mem->bank[bank].type == MEMBANK_STATIC_DOMAIN ) -{ -mfn_t bank_start = _mfn(PFN_UP(reserved_mem->bank[bank].start)); -unsigned long bank_pages = PFN_DOWN(reserved_mem->bank[bank].size); -mfn_t bank_end = mfn_add(bank_start, bank_pages); - -if ( mfn_x(bank_end) <= mfn_x(bank_start) ) -return; - -unprepare_staticmem_pages(mfn_to_page(bank_start), - bank_pages, false); -} +init_staticmem_bank(_mem->bank[bank]); } } -- 2.34.1
[PATCH v3 06/12] xen/arm: Avoid code duplication in find_unallocated_memory
The function find_unallocated_memory is using the same code to loop through 2 structure of the same type, in order to avoid code duplication, rework the code to have only one loop that goes through all the structures, this will be used to avoid duplication when the static shared memory banks will be introduced as a separate structure from reserved memory. Take the occasion to add the error code to the error message in case 'rangeset_remove_range' fails. Signed-off-by: Luca Fancellu --- v3: - Fixed the wrong logic, now the function correctly adds the available ram to the rangeset and afterwards removes the Dom0 memory and reserved memory from it. - take the occasion to print the error code in the error message as explained in the commit msg. v2: - Add comment in the loop inside find_unallocated_memory to improve readability v1: - new patch --- --- xen/arch/arm/domain_build.c | 53 - 1 file changed, 23 insertions(+), 30 deletions(-) diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 02e4dcafe78f..7c7038254473 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -868,12 +868,14 @@ static int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, static int __init find_unallocated_memory(const struct kernel_info *kinfo, struct membanks *ext_regions) { -const struct membanks *kinfo_mem = kernel_info_get_mem(kinfo); const struct membanks *mem = bootinfo_get_mem(); -const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); +const struct membanks *mem_banks[] = { +kernel_info_get_mem(kinfo), +bootinfo_get_reserved_mem(), +}; struct rangeset *unalloc_mem; paddr_t start, end; -unsigned int i; +unsigned int i, j; int res; dt_dprintk("Find unallocated memory for extended regions\n"); @@ -897,35 +899,26 @@ static int __init find_unallocated_memory(const struct kernel_info *kinfo, } } -/* Remove RAM assigned to Dom0 */ -for ( i = 0; i < kinfo_mem->nr_banks; i++ ) -{ -start = kinfo_mem->bank[i].start; -end = kinfo_mem->bank[i].start + kinfo_mem->bank[i].size; -res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), -PFN_DOWN(end - 1)); -if ( res ) -{ -printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); -goto out; -} -} - -/* Remove reserved-memory regions */ -for ( i = 0; i < reserved_mem->nr_banks; i++ ) -{ -start = reserved_mem->bank[i].start; -end = reserved_mem->bank[i].start + reserved_mem->bank[i].size; -res = rangeset_remove_range(unalloc_mem, PFN_DOWN(start), -PFN_DOWN(end - 1)); -if ( res ) +/* + * Exclude the following regions: + * 1) Remove RAM assigned to Dom0 + * 2) Remove reserved memory + */ +for ( i = 0; i < ARRAY_SIZE(mem_banks); i++ ) +for ( j = 0; j < mem_banks[i]->nr_banks; j++ ) { -printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", - start, end); -goto out; +start = mem_banks[i]->bank[j].start; +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)); +if ( res ) +{ +printk(XENLOG_ERR + "Failed to add: %#"PRIpaddr"->%#"PRIpaddr", error %d\n", + start, end, res); +goto out; +} } -} /* Remove grant table region */ if ( kinfo->gnttab_size ) -- 2.34.1
[PATCH v3 01/12] xen/arm: remove stale addr_cells/size_cells in assign_shared_memory
From: Penny Zheng Function parameters {addr_cells,size_cells} are stale parameters in assign_shared_memory, so we shall remove them. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - no change v1: - This is this patch: https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-2-penny.zh...@arm.com/ --- --- xen/arch/arm/static-shmem.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 9097bc8b1511..cb268cd2edf1 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -90,7 +90,6 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, } static int __init assign_shared_memory(struct domain *d, - uint32_t addr_cells, uint32_t size_cells, paddr_t pbase, paddr_t psize, paddr_t gbase) { @@ -252,7 +251,6 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, * specified, so they should be assigned to dom_io. */ ret = assign_shared_memory(owner_dom_io ? dom_io : d, - addr_cells, size_cells, pbase, psize, gbase); if ( ret ) return ret; -- 2.34.1
[PATCH v3 05/12] xen/arm: Conditional compilation of kernel_info.shm_mem member
The user of shm_mem member of the 'struct kernel_info' is only the code managing the static shared memory feature, which can be compiled out using CONFIG_STATIC_SHM, so in case the feature is not requested, that member won't be used and will waste memory space. To address this issue, protect the member with the Kconfig parameter and modify the signature of the only function using it to remove any reference to the member from outside the static-shmem module. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - add Michal R-by - Removed the signature modification of make_resv_memory_node from this patch v1: - new patch --- --- xen/arch/arm/include/asm/kernel.h | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h index d46f29ee6ce5..eb3cb7809ccf 100644 --- a/xen/arch/arm/include/asm/kernel.h +++ b/xen/arch/arm/include/asm/kernel.h @@ -39,7 +39,9 @@ struct kernel_info { void *fdt; /* flat device tree */ paddr_t unassigned_mem; /* RAM not (yet) assigned to a bank */ struct meminfo mem; +#ifdef CONFIG_STATIC_SHM struct meminfo shm_mem; +#endif /* kernel entry point */ paddr_t entry; @@ -80,10 +82,16 @@ struct kernel_info { #define kernel_info_get_mem(kinfo) (&(kinfo)->mem.common) +#ifdef CONFIG_STATIC_SHM +#define KERNEL_INFO_SHM_MEM_INIT .shm_mem.common.max_banks = NR_MEM_BANKS, +#else +#define KERNEL_INFO_SHM_MEM_INIT +#endif + #define KERNEL_INFO_INIT\ { \ .mem.common.max_banks = NR_MEM_BANKS, \ -.shm_mem.common.max_banks = NR_MEM_BANKS, \ +KERNEL_INFO_SHM_MEM_INIT\ } /* -- 2.34.1
[PATCH v3 03/12] xen/arm: Pass struct kernel_info parameter to make_{resv,shm}_memory_node
The struct domain parameter is not used in make_resv_memory_node and in its called function make_shm_memory_node, so drop it from both functions, also, take the occasion to pass directly struct kernel_info, from which we can infer other parameters passed to the functions and drop them as well. Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3: - Fix commit title/msg, add Michal R-by v2: - new patch --- --- xen/arch/arm/dom0less-build.c | 3 +-- xen/arch/arm/domain_build.c | 3 +-- xen/arch/arm/include/asm/static-shmem.h | 9 - xen/arch/arm/static-shmem.c | 16 +--- 4 files changed, 15 insertions(+), 16 deletions(-) diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c index fb63ec6fd111..0edc5357caef 100644 --- a/xen/arch/arm/dom0less-build.c +++ b/xen/arch/arm/dom0less-build.c @@ -645,8 +645,7 @@ static int __init prepare_dtb_domU(struct domain *d, struct kernel_info *kinfo) if ( ret ) goto err; -ret = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, ->shm_mem); +ret = make_resv_memory_node(kinfo, addrcells, sizecells); if ( ret ) goto err; diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 54232ed4cb9d..11d4e7d0b1ea 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -1769,8 +1769,7 @@ static int __init handle_node(struct domain *d, struct kernel_info *kinfo, return res; } -res = make_resv_memory_node(d, kinfo->fdt, addrcells, sizecells, ->shm_mem); +res = make_resv_memory_node(kinfo, addrcells, sizecells); if ( res ) return res; } diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h index 1536ff18b895..680594b6843d 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -7,8 +7,8 @@ #ifdef CONFIG_STATIC_SHM -int make_resv_memory_node(const struct domain *d, void *fdt, int addrcells, - int sizecells, const struct meminfo *mem); +int make_resv_memory_node(const struct kernel_info *kinfo, int addrcells, + int sizecells); int process_shm(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node); @@ -26,9 +26,8 @@ int process_shm_node(const void *fdt, int node, uint32_t address_cells, #else /* !CONFIG_STATIC_SHM */ -static inline int make_resv_memory_node(const struct domain *d, void *fdt, -int addrcells, int sizecells, -const struct meminfo *mem) +static inline int make_resv_memory_node(const struct kernel_info *kinfo, +int addrcells, int sizecells) { return 0; } diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 40a0e860c79d..349b85667684 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -277,10 +277,11 @@ int __init process_shm(struct domain *d, struct kernel_info *kinfo, return 0; } -static int __init make_shm_memory_node(const struct domain *d, void *fdt, - int addrcells, int sizecells, - const struct meminfo *mem) +static int __init make_shm_memory_node(const struct kernel_info *kinfo, + int addrcells, int sizecells) { +const struct meminfo *mem = >shm_mem; +void *fdt = kinfo->fdt; unsigned int i = 0; int res = 0; @@ -488,10 +489,11 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, return 0; } -int __init make_resv_memory_node(const struct domain *d, void *fdt, - int addrcells, int sizecells, - const struct meminfo *mem) +int __init make_resv_memory_node(const struct kernel_info *kinfo, int addrcells, + int sizecells) { +const struct meminfo *mem = >shm_mem; +void *fdt = kinfo->fdt; int res = 0; /* Placeholder for reserved-memory\0 */ const char resvbuf[16] = "reserved-memory"; @@ -518,7 +520,7 @@ int __init make_resv_memory_node(const struct domain *d, void *fdt, if ( res ) return res; -res = make_shm_memory_node(d, fdt, addrcells, sizecells, mem); +res = make_shm_memory_node(kinfo, addrcells, sizecells); if ( res ) return res; -- 2.34.1
[PATCH v3 02/12] xen/arm: avoid repetitive checking in process_shm_node
From: Penny Zheng Putting overlap and overflow checking in the loop is causing repetitive operation, so this commit extracts both checking outside the loop. Signed-off-by: Penny Zheng Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v2: - add Michal R-by v1: - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-3-penny.zh...@arm.com/ - use strncmp to match the branch above - drop Michal R-by given the change --- --- xen/arch/arm/static-shmem.c | 39 +++-- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index cb268cd2edf1..40a0e860c79d 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -349,7 +349,7 @@ 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; +paddr_t paddr, gaddr, size, end; struct meminfo *mem = _mem; unsigned int i; int len; @@ -422,6 +422,13 @@ 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++ ) { /* @@ -441,30 +448,13 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, return -EINVAL; } } +else if ( strncmp(shm_id, mem->bank[i].shm_id, MAX_SHM_ID_LENGTH) != 0 ) +continue; else { -paddr_t end = paddr + size; -paddr_t bank_end = mem->bank[i].start + mem->bank[i].size; - -if ( (end <= paddr) || (bank_end <= mem->bank[i].start) ) -{ -printk("fdt: static shared memory region %s overflow\n", shm_id); -return -EINVAL; -} - -if ( check_reserved_regions_overlap(paddr, size) ) -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; -} -} +printk("fdt: different shared memory region could not share the same shm ID %s\n", + shm_id); +return -EINVAL; } } @@ -472,6 +462,9 @@ int __init process_shm_node(const void *fdt, int node, uint32_t address_cells, { if ( i < NR_MEM_BANKS ) { +if ( check_reserved_regions_overlap(paddr, size) ) +return -EINVAL; + /* Static shared memory shall be reserved from any other use. */ safe_strcpy(mem->bank[mem->nr_banks].shm_id, shm_id); mem->bank[mem->nr_banks].start = paddr; -- 2.34.1
[PATCH v3 04/12] xen/arm: Introduce a generic way to access memory bank structures
Currently the 'struct meminfo' is defining a static defined array of 'struct membank' of NR_MEM_BANKS elements, some features like shared memory don't require such amount of memory allocation but might want to reuse existing code to manipulate this kind of structure that is just as 'struct meminfo' but less bulky. For this reason introduce a generic way to access this kind of structure using a new structure 'struct membanks', which implements all the fields needed by a structure related to memory banks without the need to specify at build time the size of the 'struct membank' array, using a flexible array member. Modify 'struct meminfo' to implement the field related to the new introduced structure, given the change all usage of this structure are updated in this way: - code accessing bootinfo.{mem,reserved_mem,acpi} field now uses 3 new introduced static inline helpers to access the new field of 'struct meminfo' named 'common'. - code accessing 'struct kernel_info *' member 'mem' now use the new introduced macro 'kernel_info_get_mem(...)' to access the new field of 'struct meminfo' named 'common'. - introduced KERNEL_INFO_INIT and BOOTINFO_INIT that from now on will be used to initialize 'struct kernel_info' and 'struct bootinfo' respectively, in order to initialize their 'struct meminfo' .common.max_banks members. Constify pointers where needed. Suggested-by: Julien Grall Signed-off-by: Luca Fancellu Reviewed-by: Michal Orzel --- v3: - mention KERNEL_INFO_INIT and BOOTINFO_INIT in the commit msg, back to the macro for struct kernel_info as in v1 because the static inline solution needed two different static inline to handle 'struct kernel_info *kinfo' and 'const struct kernel_info *kinfo'. - add Michal R-by v2: - Fixed typos in commit message and mention flexible array member - Add static assert for struct membanks - use static inline for the kernel_info structure instead of macro - use xzalloc_flex_struct inside make_hypervisor_node instead of xzalloc - Fix trailing backslash style v1: - new patch --- --- xen/arch/arm/acpi/domain_build.c| 6 +- xen/arch/arm/arm32/mmu/mm.c | 44 +- xen/arch/arm/arm64/mmu/mm.c | 2 +- xen/arch/arm/bootfdt.c | 39 ++--- xen/arch/arm/dom0less-build.c | 16 ++-- xen/arch/arm/domain_build.c | 103 +--- xen/arch/arm/efi/efi-boot.h | 8 +- xen/arch/arm/efi/efi-dom0.c | 13 +-- xen/arch/arm/include/asm/domain_build.h | 2 +- xen/arch/arm/include/asm/kernel.h | 8 ++ xen/arch/arm/include/asm/setup.h| 40 - xen/arch/arm/kernel.c | 12 +-- xen/arch/arm/setup.c| 58 +++-- xen/arch/arm/static-memory.c| 27 --- xen/arch/arm/static-shmem.c | 34 15 files changed, 249 insertions(+), 163 deletions(-) diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index b58389ce9e9f..ed895dd8f926 100644 --- a/xen/arch/arm/acpi/domain_build.c +++ b/xen/arch/arm/acpi/domain_build.c @@ -444,14 +444,14 @@ static int __init acpi_create_fadt(struct domain *d, struct membank tbl_add[]) } static int __init estimate_acpi_efi_size(struct domain *d, - struct kernel_info *kinfo) + const struct kernel_info *kinfo) { size_t efi_size, acpi_size, madt_size; u64 addr; struct acpi_table_rsdp *rsdp_tbl; struct acpi_table_header *table; -efi_size = estimate_efi_size(kinfo->mem.nr_banks); +efi_size = estimate_efi_size(kernel_info_get_mem(kinfo)->nr_banks); acpi_size = ROUNDUP(sizeof(struct acpi_table_fadt), 8); acpi_size += ROUNDUP(sizeof(struct acpi_table_stao), 8); @@ -546,7 +546,7 @@ int __init prepare_acpi(struct domain *d, struct kernel_info *kinfo) acpi_map_other_tables(d); acpi_create_efi_system_table(d, tbl_add); -acpi_create_efi_mmap_table(d, >mem, tbl_add); +acpi_create_efi_mmap_table(d, kernel_info_get_mem(kinfo), tbl_add); /* Map the EFI and ACPI tables to Dom0 */ rc = map_regions_p2mt(d, diff --git a/xen/arch/arm/arm32/mmu/mm.c b/xen/arch/arm/arm32/mmu/mm.c index 0ab6ae52a63b..84c89da909bf 100644 --- a/xen/arch/arm/arm32/mmu/mm.c +++ b/xen/arch/arm/arm32/mmu/mm.c @@ -42,6 +42,7 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, uint32_t size, paddr_t align, int first_mod) { +const struct membanks *reserved_mem = bootinfo_get_reserved_mem(); const struct bootmodules *mi = int i; int nr; @@ -100,15 +101,14 @@ static paddr_t __init consider_modules(paddr_t s, paddr_t e, * possible kinds of bootmodules. * * When retrieving the corresponding reserved-memory addresses, we - * need
[PATCH v3 00/12] Static shared memory followup v2 - pt1
This serie is a partial rework of this other serie: https://patchwork.kernel.org/project/xen-devel/cover/20231206090623.1932275-1-penny.zh...@arm.com/ The original serie is addressing an issue of the static shared memory feature that impacts the memory footprint of other component when the feature is enabled, another issue impacts the device tree generation for the guests when the feature is enabled and used and the last one is a missing feature that is the option to have a static shared memory region that is not from the host address space. This serie is handling some comment on the original serie and it is splitting the rework in two part, this first part is addressing the memory footprint issue and the device tree generation, there will be a following serie addressing the last missing feature soon. >From v2: - Removed merging of the /memory interval >From v1: - Add new patches, moved the patch related to the static memory helper. Luca Fancellu (8): xen/arm: Pass struct kernel_info parameter to make_{resv,shm}_memory_node xen/arm: Introduce a generic way to access memory bank structures xen/arm: Conditional compilation of kernel_info.shm_mem member xen/arm: Avoid code duplication in find_unallocated_memory xen/arm: Avoid code duplication in check_reserved_regions_overlap xen/arm: Introduce helper for static memory pages xen/arm: Reduce struct membank size on static shared memory xen/arm: List static shared memory regions as /memory nodes Penny Zheng (4): xen/arm: remove stale addr_cells/size_cells in assign_shared_memory xen/arm: avoid repetitive checking in process_shm_node xen/arm: remove shm holes from extended regions xen/arm: fix duplicate /reserved-memory node in Dom0 xen/arch/arm/acpi/domain_build.c | 6 +- xen/arch/arm/arm32/mmu/mm.c | 68 -- xen/arch/arm/arm64/mmu/mm.c | 4 +- xen/arch/arm/bootfdt.c | 40 ++-- xen/arch/arm/dom0less-build.c| 19 +- xen/arch/arm/domain_build.c | 213 ++ xen/arch/arm/efi/efi-boot.h | 8 +- xen/arch/arm/efi/efi-dom0.c | 13 +- xen/arch/arm/include/asm/domain_build.h | 6 +- xen/arch/arm/include/asm/kernel.h| 18 +- xen/arch/arm/include/asm/setup.h | 81 ++- xen/arch/arm/include/asm/static-memory.h | 13 ++ xen/arch/arm/include/asm/static-shmem.h | 59 - xen/arch/arm/kernel.c| 12 +- xen/arch/arm/setup.c | 97 ++--- xen/arch/arm/static-memory.c | 35 ++- xen/arch/arm/static-shmem.c | 263 ++- 17 files changed, 689 insertions(+), 266 deletions(-) -- 2.34.1
Re: [PATCH] public: xen: Define missing guest handle for int32_t
Hi Michal, > On 17 Apr 2024, at 13:14, Michal Orzel wrote: > > Commit afab29d0882f ("public: s/int/int32_t") replaced int with int32_t > in XEN_GUEST_HANDLE() in memory.h but there is no guest handle defined > for it. This results in a build failure. Example on Arm: > > ./include/public/arch-arm.h:205:41: error: unknown type name > ‘__guest_handle_64_int32_t’ > 205 | #define __XEN_GUEST_HANDLE(name)__guest_handle_64_ ## name > | ^~ > ./include/public/arch-arm.h:206:41: note: in expansion of macro > ‘__XEN_GUEST_HANDLE’ > 206 | #define XEN_GUEST_HANDLE(name) __XEN_GUEST_HANDLE(name) > | ^~ > ./include/public/memory.h:277:5: note: in expansion of macro > ‘XEN_GUEST_HANDLE’ > 277 | XEN_GUEST_HANDLE(int32_t) errs; > > Fix it. Also, drop guest handle definition for int given no further use. > > Fixes: afab29d0882f ("public: s/int/int32_t") > Signed-off-by: Michal Orzel > --- I’ve build it for arm64, arm32 and x86 Reviewed-by: Luca Fancellu Tested-by: Luca Fancellu
Re: docs/misra: add R21.6 R21.14 R21.15 R21.16
Hi Stefano, > Other deviations: > - > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst > index b7b447e152..00db02ad34 100644 > --- a/docs/misra/rules.rst > +++ b/docs/misra/rules.rst > @@ -652,12 +652,38 @@ maintainers if you want to suggest a change. >declared > - See comment for Rule 21.1 > > + * - `Rule 21.6 > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_06.c>`_ > + - Required > + - The Standard Library input/output routines shall not be used > + - See the snprintf() and vsnprintf() deviation in deviations.rst > + >* - `Rule 21.13 > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_13.c>`_ > - Mandatory > - Any value passed to a function in shall be representable as > an >unsigned char or be the value EOF > - > > + * - `Rule 21.14 > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_14.c>`_ > + - Required > + - The Standard Library function memcmp shall not be used to compare > + null terminated strings > + - > + > + * - `Rule 21.15 > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_15.c>`_ > + - Required > + - The pointer arguments to the Standard Library functions memcpy, > + memmove and memcmp shall be pointers to qualified or unqualified > + versions of compatible types There is a trailing space at the end of this line > + - void* arguments are allowed, see deviations.rst > + > + * - `Rule 21.16 > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_16.c>`_ > + - Required > + - The pointer arguments to the Standard Library function memcmp > + shall point to either a pointer type, an essentially signed type, > + an essentially unsigned type, an essentially Boolean type or an > + essentially enum type Also here. > + - > + >* - `Rule 21.17 > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_21_17.c>`_ > - Mandatory > - Use of the string handling functions from shall not result > in > Apart from them, that I guess can be addressed on commit, it looks good to me, I’ve also tested that the changes don’t break convert_misra_doc.py build. Reviewed-by: Luca Fancellu Tested-by: Luca Fancellu
Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
> On 16 Apr 2024, at 10:06, Julien Grall wrote: > > > > On 16/04/2024 09:59, Luca Fancellu wrote: >>> On 16 Apr 2024, at 09:50, Julien Grall wrote: >>> >>> >>> >>> On 16/04/2024 07:27, Luca Fancellu wrote: >>>> Hi Julien, >>> >>> Hi Luca, >>> >>>>> On 15 Apr 2024, at 19:41, Julien Grall wrote: >>>>> >>>>> Hi Luca, >>>>> >>>>> On 09/04/2024 12:45, Luca Fancellu wrote: >>>>>> Currently Xen is not exporting the static shared memory regions >>>>>> to the device tree as /memory node, this commit is fixing this >>>>>> issue. >>>>>> The static shared memory banks can be part of the memory range >>>>>> available for the domain, so if they are overlapping with the >>>>>> normal memory banks, they need to be merged together in order >>>>>> to produce a /memory node with non overlapping ranges in 'reg'. >>>>> >>>>> Before reviewing the code in more details, I would like to understand a >>>>> bit more the use case and whether it should be valid. >>>>> >>>>> From my understanding, the case you are trying to prevent is the >>>>> following setup: >>>>> 1. The Guest Physical region 0x to 0x8000 is used for RAM >>>>> 2. The Guest Physical region 0x to 0x4000 is used for static memory >>>> So far, it was possible to map guest physical regions inside the memory >>>> range given to the guest, >>>> so the above configuration was allowed and the underlying host physical >>>> regions were of course >>>> different and enforced with checks. So I’m not trying to prevent this >>>> behaviour, however ... >>>>> >>>>> The underlying Host Physical regions may be different. Xen doesn't >>>>> guarantee in which order the regions will be mapped, So whether the >>>>> overlapped region will point to the memory or the shared region is >>>>> unknown (we don't guarantee the order of the mapping). So nothing good >>>>> will happen to the guest. >>>> ... now here I don’t understand if this was wrong from the beginning or >>>> not, shall we enforce also that >>>> guest physical regions for static shared memory are outside the memory >>>> given to the guest? >>> >>> Nothing good will happen if you are trying to overwrite mappings. So I >>> think this should be enforced. However, this is a more general problem. At >>> the moment, this is pretty much as mess because you can overwrite any >>> mapping (e.g. map MMIO on top of the RAM). >>> >>> I think the easiest way to enforce is to do it in the P2M code like x86 >>> does for certain mappings. >>> >>> Anyway, I don't think the problem should be solved here or by you (this is >>> likely going to be a can of worms). For now, I would consider to simply >>> drop the patches that are trying to do the merge. >>> >>> Any thoughts? >> Yes I agree with you, I’ll drop the patch that tries to do the merge, I was >> thinking about checking that the guest phys static mem region is >> inside these boundaries: >> #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE } >> #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE } >> and also that doesn’t overlap with (struct kernel_info).mem, does it sounds >> right to you? > > I don't fully understand what you want to do. But as I wrote before, the > overlaps happen with many different regions (what if you try to use the GIC > Distributor regions for the shared memory?). > > So if we want some overlaps check, then it has to be generic. After a chat in Matrix now I understand what you mean, thanks, I will just drop the patch 12 and update this one. Cheers, Luca
Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
> On 16 Apr 2024, at 09:50, Julien Grall wrote: > > > > On 16/04/2024 07:27, Luca Fancellu wrote: >> Hi Julien, > > Hi Luca, > >>> On 15 Apr 2024, at 19:41, Julien Grall wrote: >>> >>> Hi Luca, >>> >>> On 09/04/2024 12:45, Luca Fancellu wrote: >>>> Currently Xen is not exporting the static shared memory regions >>>> to the device tree as /memory node, this commit is fixing this >>>> issue. >>>> The static shared memory banks can be part of the memory range >>>> available for the domain, so if they are overlapping with the >>>> normal memory banks, they need to be merged together in order >>>> to produce a /memory node with non overlapping ranges in 'reg'. >>> >>> Before reviewing the code in more details, I would like to understand a bit >>> more the use case and whether it should be valid. >>> >>> From my understanding, the case you are trying to prevent is the following >>> setup: >>> 1. The Guest Physical region 0x to 0x8000 is used for RAM >>> 2. The Guest Physical region 0x to 0x4000 is used for static memory >> So far, it was possible to map guest physical regions inside the memory >> range given to the guest, >> so the above configuration was allowed and the underlying host physical >> regions were of course >> different and enforced with checks. So I’m not trying to prevent this >> behaviour, however ... >>> >>> The underlying Host Physical regions may be different. Xen doesn't >>> guarantee in which order the regions will be mapped, So whether the >>> overlapped region will point to the memory or the shared region is unknown >>> (we don't guarantee the order of the mapping). So nothing good will happen >>> to the guest. >> ... now here I don’t understand if this was wrong from the beginning or not, >> shall we enforce also that >> guest physical regions for static shared memory are outside the memory given >> to the guest? > > Nothing good will happen if you are trying to overwrite mappings. So I think > this should be enforced. However, this is a more general problem. At the > moment, this is pretty much as mess because you can overwrite any mapping > (e.g. map MMIO on top of the RAM). > > I think the easiest way to enforce is to do it in the P2M code like x86 does > for certain mappings. > > Anyway, I don't think the problem should be solved here or by you (this is > likely going to be a can of worms). For now, I would consider to simply drop > the patches that are trying to do the merge. > > Any thoughts? Yes I agree with you, I’ll drop the patch that tries to do the merge, I was thinking about checking that the guest phys static mem region is inside these boundaries: #define GUEST_RAM_BANK_BASES { GUEST_RAM0_BASE, GUEST_RAM1_BASE } #define GUEST_RAM_BANK_SIZES { GUEST_RAM0_SIZE, GUEST_RAM1_SIZE } and also that doesn’t overlap with (struct kernel_info).mem, does it sounds right to you? So in this patch I will only populate the /memory nodes with the static shared memory banks. > > Cheers, > > -- > Julien Grall
Re: [PATCH v2 13/13] xen/arm: List static shared memory regions as /memory nodes
Hi Julien, > On 15 Apr 2024, at 19:41, Julien Grall wrote: > > Hi Luca, > > On 09/04/2024 12:45, Luca Fancellu wrote: >> Currently Xen is not exporting the static shared memory regions >> to the device tree as /memory node, this commit is fixing this >> issue. >> The static shared memory banks can be part of the memory range >> available for the domain, so if they are overlapping with the >> normal memory banks, they need to be merged together in order >> to produce a /memory node with non overlapping ranges in 'reg'. > > Before reviewing the code in more details, I would like to understand a bit > more the use case and whether it should be valid. > > From my understanding, the case you are trying to prevent is the following > setup: > 1. The Guest Physical region 0x to 0x8000 is used for RAM > 2. The Guest Physical region 0x to 0x4000 is used for static memory So far, it was possible to map guest physical regions inside the memory range given to the guest, so the above configuration was allowed and the underlying host physical regions were of course different and enforced with checks. So I’m not trying to prevent this behaviour, however ... > > The underlying Host Physical regions may be different. Xen doesn't guarantee > in which order the regions will be mapped, So whether the overlapped region > will point to the memory or the shared region is unknown (we don't guarantee > the order of the mapping). So nothing good will happen to the guest. ... now here I don’t understand if this was wrong from the beginning or not, shall we enforce also that guest physical regions for static shared memory are outside the memory given to the guest? > > Did I understand correctly? If so, shouldn't this be a configuration we > should forbid? > > Cheers, > > -- > Julien Grall Cheers, Luca
Re: [PATCH] docs: arm: Update where Xen should be loaded in memory
> On 12 Apr 2024, at 07:16, Michal Orzel wrote: > > Since commit 6cd046c501bc ("xen/arm: Enlarge identity map space to 10TB") > Xen can be loaded below 10 TiB. Update docs accordingly. > > Take the opportunity to update stale links to Linux docs. > > Signed-off-by: Michal Orzel > --- Reviewed-by: Luca Fancellu
Re: [PATCH 1/5] gzip: colocate gunzip code files
>>> create mode 100644 xen/common/gzip/Makefile >>> rename xen/common/{ => gzip}/gunzip.c (100%) >>> rename xen/common/{ => gzip}/inflate.c (100%) >> For inflate.c you will need to update also docs/misra/exclude-list.json > > Something like this? > > diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-list.json > index 36bad9e54f7d..095636415897 100644 > --- a/docs/misra/exclude-list.json > +++ b/docs/misra/exclude-list.json > @@ -118,7 +118,7 @@ > "comment": "Imported from Linux, ignore for now" > }, > { > -"rel_path": "common/inflate.c", > +"rel_path": "common/gzip/inflate.c", > "comment": "Imported from Linux, ignore for now" > }, > { Yes indeed
Re: [PATCH 1/5] gzip: colocate gunzip code files
> On 11 Apr 2024, at 16:25, Daniel P. Smith > wrote: > > This patch moves the gunzip code files to common/gzip. Makefiles are adjusted > accordingly. > > Signed-off-by: Daniel P. Smith > --- > xen/common/Makefile | 2 +- > xen/common/gzip/Makefile| 1 + > xen/common/{ => gzip}/gunzip.c | 0 > xen/common/{ => gzip}/inflate.c | 0 > 4 files changed, 2 insertions(+), 1 deletion(-) > create mode 100644 xen/common/gzip/Makefile > rename xen/common/{ => gzip}/gunzip.c (100%) > rename xen/common/{ => gzip}/inflate.c (100%) For inflate.c you will need to update also docs/misra/exclude-list.json
Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
> > I’ve just spotted an issue with the algorithm, the fix is this one: > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c > index 24914a80d03b..262385a041a8 100644 > --- a/xen/common/device_tree.c > +++ b/xen/common/device_tree.c > @@ -2360,6 +2360,10 @@ int __init > dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells, > __be32 *tmp_last_cell_size = last_cell + addrcells; > > dt_set_cell(_last_cell_size, sizecells, new_size); > + > +/* Last interval updated, so the end is changed */ > +end_last = start_last + size_last; > + > /* > * This current interval is merged with the last one, so remove > this > * interval and shift left all the remaining elements > Apologies, this is the fix: diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 24914a80d03b..9a2f5b27aa9b 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -2360,6 +2360,10 @@ int __init dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells, __be32 *tmp_last_cell_size = last_cell + addrcells; dt_set_cell(_last_cell_size, sizecells, new_size); + +/* Last interval updated, so the end is changed */ +end_last = start_last + new_size; + /* * This current interval is merged with the last one, so remove this * interval and shift left all the remaining elements So instead of “size_last” -> “new_size”. Sorry for the noise. Cheers, Luca
Re: [PATCH v2 12/13] xen/device_tree: Introduce function to merge overlapping intervals
> On 9 Apr 2024, at 12:45, Luca Fancellu wrote: > > Introduce a function that given an array of cells containing > (address,size) intervals, merges the overlapping ones, returning > an array with no overlapping intervals. > > The algorithm needs to sort the intervals by ascending order > address, so the sort() function already included in the codebase > is used, however in this case additional data is needed for the > compare function, to be able to extract the address from the > interval. > So add one argument to the sort() function and its compare > callback to have additional data and be able to pass, in this > case, the address length. In case the argument is not needed, > NULL can be provided. > > Signed-off-by: Luca Fancellu > --- Hi all, I’ve just spotted an issue with the algorithm, the fix is this one: diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 24914a80d03b..262385a041a8 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -2360,6 +2360,10 @@ int __init dt_merge_overlapping_addr_size_intervals(__be32 *reg, int *nr_cells, __be32 *tmp_last_cell_size = last_cell + addrcells; dt_set_cell(_last_cell_size, sizecells, new_size); + +/* Last interval updated, so the end is changed */ +end_last = start_last + size_last; + /* * This current interval is merged with the last one, so remove this * interval and shift left all the remaining elements Now, I would like to write something about the algorithm to ease the reviewers, the problem is that we have some intervals and we would like to merge the overlapping ones, a simple algorithm can be found here: https://www.interviewbit.com/blog/merge-intervals/ Limitation now is that when merging the intervals, we don’t want to exceed the space needed to store the information, for example: sizecells: 1 (meaning one __be32, 4 byte) Int1: start 0x0, size 0x Int2: start 0x, size 0x1000 We can’t merge them because the new size would be over 4 byte. During the development of this algorithm I’ve prototyped it in Python, I’ll attach my script here so that it’s easier to understand: #!/usr/bin/env python3 def merge_intervals_inplace(intervals, size_limit): merged = intervals[:] last_idx = 0 i = 1 count = len(merged) if count == 1: return merged last_cell = merged[last_idx] start_last = last_cell[0] size_last = last_cell[1] end_last = start_last + size_last while i < count: start_current = merged[i][0] size_current = merged[i][1] end_current = start_current + size_current overlap = end_last >= start_current new_size = max(end_last, end_current) - start_last #print((f"last ({start_last},{end_last})," # f" curr ({start_current},{end_current})," # f" newsize: {new_size}" #)) # If the current interval doesn't overlap with the last one, or even if # they overlap but the computed new size would be over the imposed # limit, then advance the last element by one position if (not overlap) or (overlap and new_size > size_limit): #print("advance last") last_idx += 1 last_cell = merged[last_idx] start_last = last_cell[0] size_last = last_cell[1] end_last = start_last + size_last else: #print("merge") # Set new size for the last element, merging the last interval with # the current one merged[last_idx] = (start_last, new_size) # Update last elem interval end end_last = start_last + new_size # The current interval (i) is merged with the last, so remove it and # shift left all the remaining intervals merged = merged[:i] + merged[i+1:] # Now the array has less element since we merged two intervals count -= 1 # Next iteration needs to start from the current index, skip # increment continue i += 1 return merged def print_interval(intervals): print("[", end='') for interval in intervals: s = interval[0] sz = interval[1] print(f" ({s},{sz}) ", end='') print("] -> ", end='') print("[", end='') for interval in intervals: s = interval[0] e = interval[0] + interval[1] print(f" ({s},{e}) ", end='') print("]") def main(argv): limit=20 # Array of intervals (start address, size) #banks = [(0,2), (5,1), (0,10), (10,7), (2,6)] banks = [(0,20), (20,5), (10,15), (5,15)] for interval in
Re: [PATCH v2 4/4] xen/virtual-region: Drop setup_virtual_regions()
> On 10 Apr 2024, at 19:42, Andrew Cooper wrote: > > All other actions it used to perform have been converted to build-time > initialisation. The extable setup can done at build time too. > > This is one fewer setup step required to get exceptions working. > > Take the opportunity to move 'core' into read_mostly, where it probably should > have lived all along. > > Signed-off-by: Andrew Cooper For the arm part: Reviewed-by: Luca Fancellu #arm I’ve also tested the serie on arm32 and arm64 on qemu Tested-by: Luca Fancellu
Re: [PATCH] xen/nospec: Remove unreachable code
> On 10 Apr 2024, at 20:26, Andrew Cooper wrote: > > When CONFIG_SPECULATIVE_HARDEN_LOCK is active, this reads: > > static always_inline bool lock_evaluate_nospec(bool condition) > { > return arch_lock_evaluate_nospec(condition); > return condition; > } > > Insert an #else to take out the second return. > > Fixes: 7ef0084418e1 ("x86/spinlock: introduce support for blocking > speculation into critical regions") > Signed-off-by: Andrew Cooper > --- Reviewed-by: Luca Fancellu
Re: [PATCH] xen/vPCI: Remove shadowed variable
> On 10 Apr 2024, at 20:33, Andrew Cooper wrote: > > Resolves a MISRA R5.3 violation. > > Fixes: 622bdd962822 ("vpci/header: handle p2m range sets per BAR") > Signed-off-by: Andrew Cooper > --- Reviewed-by: Luca Fancellu
Re: [PATCH] xen/spinlock: Adjust LOCK_DEBUG_INITVAL to placate MISRA
> On 10 Apr 2024, at 20:35, Andrew Cooper wrote: > > Resolves an R7.2 violation. > > Fixes: c286bb93d20c ("xen/spinlock: support higher number of cpus") > Signed-off-by: Andrew Cooper Yes makes sense Reviewed-by: Luca Fancellu