Hi Julien
> -----Original Message-----
> From: Julien Grall <jul...@xen.org>
> Sent: Saturday, June 25, 2022 3:18 AM
> To: Penny Zheng <penny.zh...@arm.com>; xen-devel@lists.xenproject.org
> Cc: Wei Chen <wei.c...@arm.com>; Stefano Stabellini
> <sstabell...@kernel.org>; Bertrand Marquis <bertrand.marq...@arm.com>;
> Volodymyr Babchuk <volodymyr_babc...@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.zh...@arm.com>
> > Reviewed-by: Stefano Stabellini <sstabell...@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
