| Message ID | 20240312130331.78418-10-luca.fancellu@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Static shared memory followup v2 - pt1 | expand |
Hi Luca, NIT: title s/for/from? On 12/03/2024 14:03, Luca Fancellu wrote: > > > From: Penny Zheng <Penny.Zheng@arm.com> > > 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 reserved memory banks, we do nothing. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> > --- > v1: > - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-Penny.Zheng@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 | 86 +++++++++++++++++++++++++ > 4 files changed, 119 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 9fad9e8b2c40..740c483ea2db 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -817,8 +817,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; > @@ -969,6 +969,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,domain-shared-memory-v1" I'm not sure if providing a compatible here makes sense. If at all, I would put "xen,shared-mem" which holds the addresses. > */ > static int __init find_memory_holes(const struct kernel_info *kinfo, > struct membanks *ext_regions) > @@ -985,6 +987,11 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, > if ( !mem_holes ) > return -ENOMEM; > > + /* Remove static shared memory regions */ > + res = remove_shm_from_rangeset(kinfo, mem_holes); > + if ( res ) > + goto out; How can you remove from a rangeset without first adding to it? This should be moved after rangeset_add_range(). Also, usually (and this is the case in this function) we pass frames to rangeset and not addresses (argument is of type ul). However... > + > /* Start with maximum possible addressable physical memory range */ > start = 0; > end = (1ULL << p2m_ipa_bits) - 1; > @@ -1089,7 +1096,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 c6fac9906656..2f70aed53ac7 100644 > --- a/xen/arch/arm/include/asm/static-shmem.h > +++ b/xen/arch/arm/include/asm/static-shmem.h > @@ -29,6 +29,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 domain *d, > @@ -61,6 +67,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_ */ > diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c > index 6143f52cb991..b3e2105dd3f2 100644 > --- a/xen/arch/arm/static-shmem.c > +++ b/xen/arch/arm/static-shmem.c > @@ -1,6 +1,7 @@ > /* SPDX-License-Identifier: GPL-2.0-only */ > > #include <xen/libfdt/libfdt.h> > +#include <xen/rangeset.h> > #include <xen/sched.h> > > #include <asm/domain_build.h> > @@ -564,6 +565,91 @@ void __init init_sharedmem_pages(void) > init_staticmem_bank(&shmem->bank[bank]); > } > > +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo, > + struct rangeset *rangeset) > +{ > + const struct membanks *shm_mem = &kinfo->shm_mem.common; > + unsigned int i; > + > + /* Remove static shared memory regions */ > + for ( i = 0; i < shm_mem->nr_banks; i++ ) > + { > + paddr_t start, end; ... here, these hold physical addresses and... > + int res; > + > + start = shm_mem->bank[i].start; > + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1; > + res = rangeset_remove_range(rangeset, start, end); you will end up in a mix which won't work. Switch to PFN_DOWN() > + if ( res ) What's the point of res variable if it is not printed below? > + { > + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", > + start, end); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo, > + struct membanks *ext_regions) > +{ > + const struct membanks *shm_mem = &kinfo->shm_mem.common; > + struct rangeset *guest_holes; > + unsigned int i; > + paddr_t start; > + paddr_t end; > + int res; > + > + /* No static shared memory region. */ > + if ( shm_mem->nr_banks == 0 ) > + return 0; > + > + dt_dprintk("Remove static shared memory holes for extended regions of DomU\n"); > + > + guest_holes = rangeset_new(NULL, NULL, 0); > + if ( !guest_holes ) > + return -ENOMEM; > + > + /* Copy extended regions sets into the rangeset */ > + for ( i = 0; i < ext_regions->nr_banks; i++ ) > + { > + start = ext_regions->bank[i].start; > + end = start + ext_regions->bank[i].size - 1; > + > + res = rangeset_add_range(guest_holes, start, end); Ditto, PFN_DOWN(). ~Michal
> On 22 Mar 2024, at 15:09, Michal Orzel <michal.orzel@amd.com> wrote: > > Hi Luca, > > NIT: title s/for/from? > > On 12/03/2024 14:03, Luca Fancellu wrote: >> >> >> From: Penny Zheng <Penny.Zheng@arm.com> >> >> 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 reserved memory banks, we do nothing. >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> Signed-off-by: Luca Fancellu <luca.fancellu@arm.com> >> --- >> v1: >> - Rework of https://patchwork.kernel.org/project/xen-devel/patch/20231206090623.1932275-8-Penny.Zheng@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 | 86 +++++++++++++++++++++++++ >> 4 files changed, 119 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 9fad9e8b2c40..740c483ea2db 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -817,8 +817,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; >> @@ -969,6 +969,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,domain-shared-memory-v1" > I'm not sure if providing a compatible here makes sense. If at all, I would put "xen,shared-mem" which holds the addresses. > >> */ >> static int __init find_memory_holes(const struct kernel_info *kinfo, >> struct membanks *ext_regions) >> @@ -985,6 +987,11 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, >> if ( !mem_holes ) >> return -ENOMEM; >> >> + /* Remove static shared memory regions */ >> + res = remove_shm_from_rangeset(kinfo, mem_holes); >> + if ( res ) >> + goto out; > How can you remove from a rangeset without first adding to it? > This should be moved after rangeset_add_range(). You are very right > Also, usually (and this is the case in this function) we pass frames > to rangeset and not addresses (argument is of type ul). However... > >> + >> /* Start with maximum possible addressable physical memory range */ >> start = 0; >> end = (1ULL << p2m_ipa_bits) - 1; >> @@ -1089,7 +1096,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 c6fac9906656..2f70aed53ac7 100644 >> --- a/xen/arch/arm/include/asm/static-shmem.h >> +++ b/xen/arch/arm/include/asm/static-shmem.h >> @@ -29,6 +29,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 domain *d, >> @@ -61,6 +67,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_ */ >> diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c >> index 6143f52cb991..b3e2105dd3f2 100644 >> --- a/xen/arch/arm/static-shmem.c >> +++ b/xen/arch/arm/static-shmem.c >> @@ -1,6 +1,7 @@ >> /* SPDX-License-Identifier: GPL-2.0-only */ >> >> #include <xen/libfdt/libfdt.h> >> +#include <xen/rangeset.h> >> #include <xen/sched.h> >> >> #include <asm/domain_build.h> >> @@ -564,6 +565,91 @@ void __init init_sharedmem_pages(void) >> init_staticmem_bank(&shmem->bank[bank]); >> } >> >> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo, >> + struct rangeset *rangeset) >> +{ >> + const struct membanks *shm_mem = &kinfo->shm_mem.common; >> + unsigned int i; >> + >> + /* Remove static shared memory regions */ >> + for ( i = 0; i < shm_mem->nr_banks; i++ ) >> + { >> + paddr_t start, end; > ... here, these hold physical addresses and... > >> + int res; >> + >> + start = shm_mem->bank[i].start; >> + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1; >> + res = rangeset_remove_range(rangeset, start, end); > you will end up in a mix which won't work. Switch to PFN_DOWN() Will fix > >> + if ( res ) > What's the point of res variable if it is not printed below? I guess this part was copied from find_memory_holes which doesn’t print out res, anyway you are right, we should print it. > >> + { >> + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", >> + start, end); >> + return -EINVAL; >> + } >> + } >> + >> + return 0; >> +} >> + >> +int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo, >> + struct membanks *ext_regions) >> +{ >> + const struct membanks *shm_mem = &kinfo->shm_mem.common; >> + struct rangeset *guest_holes; >> + unsigned int i; >> + paddr_t start; >> + paddr_t end; >> + int res; >> + >> + /* No static shared memory region. */ >> + if ( shm_mem->nr_banks == 0 ) >> + return 0; >> + >> + dt_dprintk("Remove static shared memory holes for extended regions of DomU\n"); >> + >> + guest_holes = rangeset_new(NULL, NULL, 0); >> + if ( !guest_holes ) >> + return -ENOMEM; >> + >> + /* Copy extended regions sets into the rangeset */ >> + for ( i = 0; i < ext_regions->nr_banks; i++ ) >> + { >> + start = ext_regions->bank[i].start; >> + end = start + ext_regions->bank[i].size - 1; >> + >> + res = rangeset_add_range(guest_holes, start, end); > Ditto, PFN_DOWN(). Thanks for the review, I will fix the issues in the next push, I’ll just wait for the serie to be fully reviewed, or if you don’t want to check the remaining two patches before I fix the previous comments, just let me know. > > ~Michal
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 9fad9e8b2c40..740c483ea2db 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -817,8 +817,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; @@ -969,6 +969,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,domain-shared-memory-v1" */ static int __init find_memory_holes(const struct kernel_info *kinfo, struct membanks *ext_regions) @@ -985,6 +987,11 @@ static int __init find_memory_holes(const struct kernel_info *kinfo, if ( !mem_holes ) return -ENOMEM; + /* Remove static shared memory regions */ + res = remove_shm_from_rangeset(kinfo, mem_holes); + if ( res ) + goto out; + /* Start with maximum possible addressable physical memory range */ start = 0; end = (1ULL << p2m_ipa_bits) - 1; @@ -1089,7 +1096,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 c6fac9906656..2f70aed53ac7 100644 --- a/xen/arch/arm/include/asm/static-shmem.h +++ b/xen/arch/arm/include/asm/static-shmem.h @@ -29,6 +29,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 domain *d, @@ -61,6 +67,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_ */ diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c index 6143f52cb991..b3e2105dd3f2 100644 --- a/xen/arch/arm/static-shmem.c +++ b/xen/arch/arm/static-shmem.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: GPL-2.0-only */ #include <xen/libfdt/libfdt.h> +#include <xen/rangeset.h> #include <xen/sched.h> #include <asm/domain_build.h> @@ -564,6 +565,91 @@ void __init init_sharedmem_pages(void) init_staticmem_bank(&shmem->bank[bank]); } +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo, + struct rangeset *rangeset) +{ + const struct membanks *shm_mem = &kinfo->shm_mem.common; + unsigned int i; + + /* Remove static shared memory regions */ + for ( i = 0; i < shm_mem->nr_banks; i++ ) + { + paddr_t start, end; + int res; + + start = shm_mem->bank[i].start; + end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1; + res = rangeset_remove_range(rangeset, start, end); + if ( res ) + { + printk(XENLOG_ERR "Failed to remove: %#"PRIpaddr"->%#"PRIpaddr"\n", + start, end); + return -EINVAL; + } + } + + return 0; +} + +int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo, + struct membanks *ext_regions) +{ + const struct membanks *shm_mem = &kinfo->shm_mem.common; + struct rangeset *guest_holes; + unsigned int i; + paddr_t start; + paddr_t end; + int res; + + /* No static shared memory region. */ + if ( shm_mem->nr_banks == 0 ) + return 0; + + dt_dprintk("Remove static shared memory holes for extended regions of DomU\n"); + + guest_holes = rangeset_new(NULL, NULL, 0); + if ( !guest_holes ) + return -ENOMEM; + + /* Copy extended regions sets into the rangeset */ + for ( i = 0; i < ext_regions->nr_banks; i++ ) + { + start = ext_regions->bank[i].start; + end = start + ext_regions->bank[i].size - 1; + + res = rangeset_add_range(guest_holes, start, end); + if ( res ) + { + printk(XENLOG_ERR "Failed to add: %#"PRIpaddr"->%#"PRIpaddr"\n", + start, end); + goto out; + } + } + + /* Remove static shared memory regions */ + res = remove_shm_from_rangeset(kinfo, guest_holes); + if ( res ) + goto out; + + i = ext_regions->nr_banks - 1; + start = ext_regions->bank[0].start; + end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1; + + /* Reset original extended regions to hold new value */ + ext_regions->nr_banks = 0; + res = rangeset_report_ranges(guest_holes, start, end, add_ext_regions, + ext_regions); + if ( res ) + ext_regions->nr_banks = 0; + else if ( !ext_regions->nr_banks ) + res = -ENOENT; + + out: + rangeset_destroy(guest_holes); + + return res; +} + /* * Local variables: * mode: C