| Message ID | 20221115025235.1378931-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory | expand |
Hi Penny, On 15/11/2022 02:52, Penny Zheng wrote: > when host address is not provided in "xen,shared-mem", we let Xen > allocate requested shared memory from heap, and once the shared memory is > allocated, it will be marked as static(PGC_static), which means that it will be > reserved as static memory, and will not go back to heap even on freeing. Please don't move pages from the {xen,dom}heap to the static heap. If you need to keep the pages for longer, then get an extra reference so they will not be released. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/domain_build.c | 83 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 82 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index fbb196d8a4..3de96882a5 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -835,6 +835,72 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank) > return true; > } > > +static int __init mark_shared_memory_static(struct shm_membank *shm_membank) > +{ > + unsigned int bank; > + unsigned long i, nr_mfns; > + struct page_info *pg; > + struct meminfo *meminfo; > + > + BUG_ON(!shm_membank->mem.banks.meminfo); > + meminfo = shm_membank->mem.banks.meminfo; > + for ( bank = 0; bank < meminfo->nr_banks; bank++ ) > + { > + pg = mfn_to_page(maddr_to_mfn(meminfo->bank[bank].start)); mfn_to_page(maddr_to_mfn(...)) is equivalent to maddr_to_page(..). > + nr_mfns = PFN_DOWN(meminfo->bank[bank].size); > + > + for ( i = 0; i < nr_mfns; i++ ) > + { > + /* The page should be already allocated from heap. */ > + if ( !pg[i].count_info & PGC_state_inuse ) I don't think this is doing what you want because ! will take precedence over &. You likely want to add parenthese: !(... & ...) > + { > + printk(XENLOG_ERR > + "pg[%lu] MFN %"PRI_mfn" c=%#lx\n", > + i, mfn_x(page_to_mfn(pg)) + i, pg[i].count_info); > + goto fail; > + } > + > + pg[i].count_info |= PGC_static; > + } > + } > + > + return 0; > + > + fail: > + while ( bank >= 0 ) > + { > + while ( --i >= 0 ) > + pg[i].count_info &= ~PGC_static; > + i = PFN_DOWN(meminfo->bank[--bank].size); > + } > + > + return -EINVAL; > +} > + > +static int __init allocate_shared_memory(struct shm_membank *shm_membank, > + paddr_t psize) > +{ > + struct meminfo *banks; > + int ret; > + > + BUG_ON(shm_membank->mem.banks.meminfo != NULL); > + > + banks = xmalloc_bytes(sizeof(struct meminfo)); Where is this freed? > + if ( banks == NULL ) > + return -ENOMEM; > + shm_membank->mem.banks.meminfo = banks; > + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct meminfo)); > + > + if ( !allocate_domheap_memory(NULL, psize, shm_membank->mem.banks.meminfo) ) > + return -EINVAL; > + > + ret = mark_shared_memory_static(shm_membank); > + if ( ret ) > + return ret; > + > + return 0; > +} > + > static mfn_t __init acquire_shared_memory_bank(struct domain *d, > paddr_t pbase, paddr_t psize) > { > @@ -975,7 +1041,7 @@ static 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; > + bool owner_dom_io = true, paddr_assigned = true; > struct shm_membank *shm_membank; > > if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) > @@ -1035,6 +1101,21 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo, > return -ENOENT; > } > > + /* > + * When host address is not provided in "xen,shared-mem", > + * we let Xen allocate requested memory from heap at first domain. > + */ > + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo ) > + { > + ret = allocate_shared_memory(shm_membank, psize); > + if ( ret ) > + { > + printk("%pd: failed to allocate shared memory bank(%"PRIpaddr"MB) from heap: %d\n", > + d, psize >> 20, ret); > + return ret; > + } > + } > + > /* > * 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 Cheers,
> -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Sunday, January 8, 2023 8:23 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap > when host address not provided > > Hi Penny, > Hi Julien, > On 15/11/2022 02:52, Penny Zheng wrote: > > when host address is not provided in "xen,shared-mem", we let Xen > > allocate requested shared memory from heap, and once the shared > memory > > is allocated, it will be marked as static(PGC_static), which means > > that it will be reserved as static memory, and will not go back to heap even > on freeing. > > Please don't move pages from the {xen,dom}heap to the static heap. If you > need to keep the pages for longer, then get an extra reference so they will > not be released. > > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > xen/arch/arm/domain_build.c | 83 > ++++++++++++++++++++++++++++++++++++- > > 1 file changed, 82 insertions(+), 1 deletion(-) > > > > +static int __init allocate_shared_memory(struct shm_membank > *shm_membank, > > + paddr_t psize) { > > + struct meminfo *banks; > > + int ret; > > + > > + BUG_ON(shm_membank->mem.banks.meminfo != NULL); > > + > > + banks = xmalloc_bytes(sizeof(struct meminfo)); > > Where is this freed? > These kinds of info will be only used in boot-time, so maybe I shall free them in init_done()? Or just after process_shm() ? > > + if ( banks == NULL ) > > + return -ENOMEM; > > + shm_membank->mem.banks.meminfo = banks; > > + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct > > + meminfo)); > > + > > + if ( !allocate_domheap_memory(NULL, psize, shm_membank- > >mem.banks.meminfo) ) > > + return -EINVAL; > > + > > + ret = mark_shared_memory_static(shm_membank); > > + if ( ret ) > > + return ret; > > + > > + return 0; > > +} > > + > > static mfn_t __init acquire_shared_memory_bank(struct domain *d, > > paddr_t pbase, paddr_t psize) > > { > > @@ -975,7 +1041,7 @@ static 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; > > + bool owner_dom_io = true, paddr_assigned = true; > > struct shm_membank *shm_membank; > > > > if ( !dt_device_is_compatible(shm_node, > > "xen,domain-shared-memory-v1") ) @@ -1035,6 +1101,21 @@ static int > __init process_shm(struct domain *d, struct kernel_info *kinfo, > > return -ENOENT; > > } > > > > + /* > > + * When host address is not provided in "xen,shared-mem", > > + * we let Xen allocate requested memory from heap at first domain. > > + */ > > + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo ) > > + { > > + ret = allocate_shared_memory(shm_membank, psize); > > + if ( ret ) > > + { > > + printk("%pd: failed to allocate shared memory > bank(%"PRIpaddr"MB) from heap: %d\n", > > + d, psize >> 20, ret); > > + return ret; > > + } > > + } > > + > > /* > > * 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 > > Cheers, > > -- > Julien Grall Cheers, -- Penny Zheng
Hi Penny, On 09/01/2023 07:50, Penny Zheng wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Sunday, January 8, 2023 8:23 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH v1 05/13] xen/arm: allocate shared memory from heap >> when host address not provided >> >> Hi Penny, >> > > Hi Julien, > >> On 15/11/2022 02:52, Penny Zheng wrote: >>> when host address is not provided in "xen,shared-mem", we let Xen >>> allocate requested shared memory from heap, and once the shared >> memory >>> is allocated, it will be marked as static(PGC_static), which means >>> that it will be reserved as static memory, and will not go back to heap even >> on freeing. >> >> Please don't move pages from the {xen,dom}heap to the static heap. If you >> need to keep the pages for longer, then get an extra reference so they will >> not be released. >> >>> >>> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >>> --- >>> xen/arch/arm/domain_build.c | 83 >> ++++++++++++++++++++++++++++++++++++- >>> 1 file changed, 82 insertions(+), 1 deletion(-) >>> >>> +static int __init allocate_shared_memory(struct shm_membank >> *shm_membank, >>> + paddr_t psize) { >>> + struct meminfo *banks; >>> + int ret; >>> + >>> + BUG_ON(shm_membank->mem.banks.meminfo != NULL); >>> + >>> + banks = xmalloc_bytes(sizeof(struct meminfo)); >> >> Where is this freed? >> > > These kinds of info will be only used in boot-time, so maybe I shall > free them in init_done()? I don't think you can free it in init_done() because we don't keep a pointer to kinfo past construct_dom*(). > Or just after process_shm() ? This might work. But I think it would be better to avoid the dynamic memory allocation if we can. Cheers,
On 11/14/22 21:52, Penny Zheng wrote: > when host address is not provided in "xen,shared-mem", we let Xen > allocate requested shared memory from heap, and once the shared memory is > allocated, it will be marked as static(PGC_static), which means that it will be > reserved as static memory, and will not go back to heap even on freeing. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/domain_build.c | 83 ++++++++++++++++++++++++++++++++++++- > 1 file changed, 82 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index fbb196d8a4..3de96882a5 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -835,6 +835,72 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank) > return true; > } > > +static int __init mark_shared_memory_static(struct shm_membank *shm_membank) > +{ > + unsigned int bank; > + unsigned long i, nr_mfns; > + struct page_info *pg; > + struct meminfo *meminfo; > + > + BUG_ON(!shm_membank->mem.banks.meminfo); > + meminfo = shm_membank->mem.banks.meminfo; > + for ( bank = 0; bank < meminfo->nr_banks; bank++ ) > + { > + pg = mfn_to_page(maddr_to_mfn(meminfo->bank[bank].start)); > + nr_mfns = PFN_DOWN(meminfo->bank[bank].size); > + > + for ( i = 0; i < nr_mfns; i++ ) > + { > + /* The page should be already allocated from heap. */ > + if ( !pg[i].count_info & PGC_state_inuse ) > + { > + printk(XENLOG_ERR > + "pg[%lu] MFN %"PRI_mfn" c=%#lx\n", > + i, mfn_x(page_to_mfn(pg)) + i, pg[i].count_info); > + goto fail; > + } > + > + pg[i].count_info |= PGC_static; > + } > + } > + > + return 0; > + > + fail: > + while ( bank >= 0 ) When building with EXTRA_CFLAGS_XEN_CORE="-Wtype-limits -Wno-error=type-limits", we get the following warning: arch/arm/domain_build.c: In function ‘mark_shared_memory_static’: arch/arm/domain_build.c:879:18: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits] 879 | while ( bank >= 0 ) | ^~ > + { > + while ( --i >= 0 ) Similarly here: arch/arm/domain_build.c:881:21: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits] 881 | while ( --i >= 0 ) | ^~ > + pg[i].count_info &= ~PGC_static; > + i = PFN_DOWN(meminfo->bank[--bank].size); > + } > + > + return -EINVAL; > +} > + > +static int __init allocate_shared_memory(struct shm_membank *shm_membank, > + paddr_t psize) > +{ > + struct meminfo *banks; > + int ret; > + > + BUG_ON(shm_membank->mem.banks.meminfo != NULL); > + > + banks = xmalloc_bytes(sizeof(struct meminfo)); > + if ( banks == NULL ) > + return -ENOMEM; > + shm_membank->mem.banks.meminfo = banks; > + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct meminfo)); > + > + if ( !allocate_domheap_memory(NULL, psize, shm_membank->mem.banks.meminfo) ) > + return -EINVAL; > + > + ret = mark_shared_memory_static(shm_membank); > + if ( ret ) > + return ret; > + > + return 0; > +} > + > static mfn_t __init acquire_shared_memory_bank(struct domain *d, > paddr_t pbase, paddr_t psize) > { > @@ -975,7 +1041,7 @@ static 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; > + bool owner_dom_io = true, paddr_assigned = true; > struct shm_membank *shm_membank; > > if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) > @@ -1035,6 +1101,21 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo, > return -ENOENT; > } > > + /* > + * When host address is not provided in "xen,shared-mem", > + * we let Xen allocate requested memory from heap at first domain. > + */ > + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo ) > + { > + ret = allocate_shared_memory(shm_membank, psize); > + if ( ret ) > + { > + printk("%pd: failed to allocate shared memory bank(%"PRIpaddr"MB) from heap: %d\n", > + d, psize >> 20, ret); > + return ret; > + } > + } > + > /* > * 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 > -- > 2.25.1 > >
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index fbb196d8a4..3de96882a5 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -835,6 +835,72 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank) return true; } +static int __init mark_shared_memory_static(struct shm_membank *shm_membank) +{ + unsigned int bank; + unsigned long i, nr_mfns; + struct page_info *pg; + struct meminfo *meminfo; + + BUG_ON(!shm_membank->mem.banks.meminfo); + meminfo = shm_membank->mem.banks.meminfo; + for ( bank = 0; bank < meminfo->nr_banks; bank++ ) + { + pg = mfn_to_page(maddr_to_mfn(meminfo->bank[bank].start)); + nr_mfns = PFN_DOWN(meminfo->bank[bank].size); + + for ( i = 0; i < nr_mfns; i++ ) + { + /* The page should be already allocated from heap. */ + if ( !pg[i].count_info & PGC_state_inuse ) + { + printk(XENLOG_ERR + "pg[%lu] MFN %"PRI_mfn" c=%#lx\n", + i, mfn_x(page_to_mfn(pg)) + i, pg[i].count_info); + goto fail; + } + + pg[i].count_info |= PGC_static; + } + } + + return 0; + + fail: + while ( bank >= 0 ) + { + while ( --i >= 0 ) + pg[i].count_info &= ~PGC_static; + i = PFN_DOWN(meminfo->bank[--bank].size); + } + + return -EINVAL; +} + +static int __init allocate_shared_memory(struct shm_membank *shm_membank, + paddr_t psize) +{ + struct meminfo *banks; + int ret; + + BUG_ON(shm_membank->mem.banks.meminfo != NULL); + + banks = xmalloc_bytes(sizeof(struct meminfo)); + if ( banks == NULL ) + return -ENOMEM; + shm_membank->mem.banks.meminfo = banks; + memset(shm_membank->mem.banks.meminfo, 0, sizeof(struct meminfo)); + + if ( !allocate_domheap_memory(NULL, psize, shm_membank->mem.banks.meminfo) ) + return -EINVAL; + + ret = mark_shared_memory_static(shm_membank); + if ( ret ) + return ret; + + return 0; +} + static mfn_t __init acquire_shared_memory_bank(struct domain *d, paddr_t pbase, paddr_t psize) { @@ -975,7 +1041,7 @@ static 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; + bool owner_dom_io = true, paddr_assigned = true; struct shm_membank *shm_membank; if ( !dt_device_is_compatible(shm_node, "xen,domain-shared-memory-v1") ) @@ -1035,6 +1101,21 @@ static int __init process_shm(struct domain *d, struct kernel_info *kinfo, return -ENOENT; } + /* + * When host address is not provided in "xen,shared-mem", + * we let Xen allocate requested memory from heap at first domain. + */ + if ( !paddr_assigned && !shm_membank->mem.banks.meminfo ) + { + ret = allocate_shared_memory(shm_membank, psize); + if ( ret ) + { + printk("%pd: failed to allocate shared memory bank(%"PRIpaddr"MB) from heap: %d\n", + d, psize >> 20, ret); + return ret; + } + } + /* * 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
when host address is not provided in "xen,shared-mem", we let Xen allocate requested shared memory from heap, and once the shared memory is allocated, it will be marked as static(PGC_static), which means that it will be reserved as static memory, and will not go back to heap even on freeing. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/arch/arm/domain_build.c | 83 ++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-)