| Message ID | 20220620051114.210118-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | static shared memory on dom0less system | expand |
Hi Penny, On 20/06/2022 06:11, 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> > --- > 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 | 62 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 62 insertions(+) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index d4fd64e2bd..650d18f5ef 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -799,6 +799,34 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, > > } > > +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; > + > + if ( d == dom_io ) > + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain; > + else > + /* Exclude the owner domain itself. */ NIT: I think this comment wants to be just above the 'if' and expanded to explain why the "dom_io" is not included. AFAIU, this is because "dom_io" is not described in the Device-Tree, so it would not be taken into account for nr_shm_domain. > + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1; TBH, given the use here. I would have consider to not increment nr_shm_domain if the role was owner in parsing code. This is v5 now, so I would be OK with the comment above. But I would suggest to consider it as a follow-up. > + > + return 0; > +} > + > /* > * Func allocate_shared_memory is supposed to be only called > * from the owner. > @@ -810,6 +838,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, > "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", > @@ -824,6 +854,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, PFN_DOWN(psize)); > @@ -835,6 +866,37 @@ static int __init allocate_shared_memory(struct domain *d, > } > } > > + /* > + * Get the right amount of references per page, which is the number of > + * borrow domains. > + */ > + ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers); > + if ( ret ) > + return ret; > + > + /* > + * Instead of let borrower domain get a page ref, we add as many Typo: s/let/letting/ > + * additional reference as the number of borrowers when the owner > + * is allocated, since there is a chance that owner is created > + * after borrower. What if the borrower is created first? Wouldn't this result to add pages in the P2M without reference? If yes, then I think this is worth an explaination. > + */ > + page = mfn_to_page(smfn); Where do you validate the range [smfn, nr_pages]? > + 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; > } > Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Saturday, June 25, 2022 3:18 AM > 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 v5 5/8] xen/arm: Add additional reference to owner > domain when the owner is allocated > > Hi Penny, > > On 20/06/2022 06:11, 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> > > --- > > 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 | 62 > +++++++++++++++++++++++++++++++++++++ > > 1 file changed, 62 insertions(+) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index d4fd64e2bd..650d18f5ef 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -799,6 +799,34 @@ static mfn_t __init > > acquire_shared_memory_bank(struct domain *d, > > > > } > > > > +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; > > + > > + if ( d == dom_io ) > > + *nr_borrowers = > bootinfo.reserved_mem.bank[bank].nr_shm_domain; > > + else > > + /* Exclude the owner domain itself. */ > NIT: I think this comment wants to be just above the 'if' and expanded to > explain why the "dom_io" is not included. AFAIU, this is because "dom_io" is > not described in the Device-Tree, so it would not be taken into account for > nr_shm_domain. > > > + *nr_borrowers = > > + bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1; > > TBH, given the use here. I would have consider to not increment > nr_shm_domain if the role was owner in parsing code. This is v5 now, so I > would be OK with the comment above. > > But I would suggest to consider it as a follow-up. > LTM, it is not a big change, I'll try to include it in the next serie~ > > + > > + return 0; > > +} > > + > > /* > > * Func allocate_shared_memory is supposed to be only called > > * from the owner. > > @@ -810,6 +838,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, > > "Allocate static shared memory BANK > > %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -824,6 +854,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, > > PFN_DOWN(psize)); @@ -835,6 +866,37 @@ static int __init > allocate_shared_memory(struct domain *d, > > } > > } > > > > + /* > > + * Get the right amount of references per page, which is the number of > > + * borrow domains. > > + */ > > + ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers); > > + if ( ret ) > > + return ret; > > + > > + /* > > + * Instead of let borrower domain get a page ref, we add as many > > Typo: s/let/letting/ > > > + * additional reference as the number of borrowers when the owner > > + * is allocated, since there is a chance that owner is created > > + * after borrower. > > What if the borrower is created first? Wouldn't this result to add pages in the > P2M without reference? > > If yes, then I think this is worth an explaination. > Yes, it is intended to be the way you said, and I'll add a comment to explain. > > + */ > > + page = mfn_to_page(smfn); > > Where do you validate the range [smfn, nr_pages]? > > > + 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; > > } > > > > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index d4fd64e2bd..650d18f5ef 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -799,6 +799,34 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, } +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; + + if ( d == dom_io ) + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain; + else + /* Exclude the owner domain itself. */ + *nr_borrowers = bootinfo.reserved_mem.bank[bank].nr_shm_domain - 1; + + return 0; +} + /* * Func allocate_shared_memory is supposed to be only called * from the owner. @@ -810,6 +838,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, "Allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", @@ -824,6 +854,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, PFN_DOWN(psize)); @@ -835,6 +866,37 @@ static int __init allocate_shared_memory(struct domain *d, } } + /* + * Get the right amount of references per page, which is the number of + * borrow domains. + */ + ret = acquire_nr_borrower_domain(d, pbase, psize, &nr_borrowers); + if ( ret ) + return ret; + + /* + * Instead of let 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. + */ + 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; }