| Message ID | 20220721132115.3015761-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | static shared memory on dom0less system | expand |
Hi Penny, On 21/07/2022 14:21, Penny Zheng wrote: > Borrower domain will fail to get a page ref using the owner domain > during allocation, when the owner is created after borrower. > > So here, we decide to get and add the right amount of reference, which > is the number of borrowers, when the owner is allocated. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> IMHO, this tag should not have been kept given... > --- > v6 change: > - adapt to the change of "nr_shm_borrowers" ... this change. 'reviewed-by' means that the person reviewed the code and therefore agree with the logic. So I would only keep the tag if the change is trivial (including typo, coding style) and would drop it (or confirm with the person) otherwise. Stefano, can you confirm you are happy that your reviewed-by tag is kept? > - add in-code comment to explain if the borrower is created first, we intend to > add pages in the P2M without reference. > --- > v5 change: > - no change > --- > v4 changes: > - no change > --- > v3 change: > - printk rather than dprintk since it is a serious error > --- > v2 change: > - new commit > --- > xen/arch/arm/domain_build.c | 60 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index a7e95c34a7..e891e800a7 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -761,6 +761,30 @@ static void __init assign_static_memory_11(struct domain *d, > } > > #ifdef CONFIG_STATIC_SHM > +static int __init acquire_nr_borrower_domain(struct domain *d, > + paddr_t pbase, paddr_t psize, > + unsigned long *nr_borrowers) > +{ > + unsigned long bank; NIT: AFAICT nr_banks is an "unsigned int". Other than that: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
On Thu, 1 Sep 2022, Julien Grall wrote: > Hi Penny, > > On 21/07/2022 14:21, Penny Zheng wrote: > > Borrower domain will fail to get a page ref using the owner domain > > during allocation, when the owner is created after borrower. > > > > So here, we decide to get and add the right amount of reference, which > > is the number of borrowers, when the owner is allocated. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > IMHO, this tag should not have been kept given... > > > --- > > v6 change: > > - adapt to the change of "nr_shm_borrowers" > > ... this change. 'reviewed-by' means that the person reviewed the code and > therefore agree with the logic. So I would only keep the tag if the change is > trivial (including typo, coding style) and would drop it (or confirm with the > person) otherwise. > > Stefano, can you confirm you are happy that your reviewed-by tag is kept? Yes I confirm Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > - add in-code comment to explain if the borrower is created first, we intend > > to > > add pages in the P2M without reference. > > --- > > v5 change: > > - no change > > --- > > v4 changes: > > - no change > > --- > > v3 change: > > - printk rather than dprintk since it is a serious error > > --- > > v2 change: > > - new commit > > --- > > xen/arch/arm/domain_build.c | 60 +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index a7e95c34a7..e891e800a7 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -761,6 +761,30 @@ static void __init assign_static_memory_11(struct > > domain *d, > > } > > #ifdef CONFIG_STATIC_SHM > > +static int __init acquire_nr_borrower_domain(struct domain *d, > > + paddr_t pbase, paddr_t psize, > > + unsigned long *nr_borrowers) > > +{ > > + unsigned long bank; > > NIT: AFAICT nr_banks is an "unsigned int". > > Other than that: > > Acked-by: Julien Grall <jgrall@amazon.com> > > Cheers, > > -- > Julien Grall >
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index a7e95c34a7..e891e800a7 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -761,6 +761,30 @@ static void __init assign_static_memory_11(struct domain *d, } #ifdef CONFIG_STATIC_SHM +static int __init acquire_nr_borrower_domain(struct domain *d, + paddr_t pbase, paddr_t psize, + unsigned long *nr_borrowers) +{ + unsigned long bank; + + /* Iterate reserved memory to find requested shm bank. */ + for ( bank = 0 ; bank < bootinfo.reserved_mem.nr_banks; bank++ ) + { + paddr_t bank_start = bootinfo.reserved_mem.bank[bank].start; + paddr_t bank_size = bootinfo.reserved_mem.bank[bank].size; + + if ( (pbase == bank_start) && (psize == bank_size) ) + break; + } + + if ( bank == bootinfo.reserved_mem.nr_banks ) + return -ENOENT; + + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_borrowers; + + return 0; +} + /* * This function checks whether the static shared memory region is * already allocated to dom_io. @@ -823,6 +847,8 @@ static int __init allocate_shared_memory(struct domain *d, { mfn_t smfn; int ret = 0; + unsigned long nr_pages, nr_borrowers, i; + struct page_info *page; dprintk(XENLOG_INFO, "%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -836,6 +862,7 @@ static int __init allocate_shared_memory(struct domain *d, * DOMID_IO is the domain, like DOMID_XEN, that is not auto-translated. * It sees RAM 1:1 and we do not need to create P2M mapping for it */ + nr_pages = PFN_DOWN(psize); if ( d != dom_io ) { ret = guest_physmap_add_pages(d, gaddr_to_gfn(gbase), smfn, @@ -847,6 +874,39 @@ static int __init allocate_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, &nr_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 + * is allocated, since there is a chance that owner is created + * after borrower. + * So if the borrower is created first, it will cause adding pages + * in the P2M without reference. + */ + page = mfn_to_page(smfn); + for ( i = 0; i < nr_pages; i++ ) + { + if ( !get_page_nr(page + i, d, nr_borrowers) ) + { + printk(XENLOG_ERR + "Failed to add %lu references to page %"PRI_mfn".\n", + nr_borrowers, mfn_x(smfn) + i); + goto fail; + } + } + + return 0; + + fail: + while ( --i >= 0 ) + put_page_nr(page + i, nr_borrowers); return ret; }