[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH v5 5/8] xen/arm: Add additional reference to owner domain when the owner is allocated
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xxxxxxx> > Sent: Saturday, June 25, 2022 3:18 AM > To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx > Cc: Wei Chen <Wei.Chen@xxxxxxx>; Stefano Stabellini > <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx> > 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@xxxxxxx> > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > > --- > > 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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |