Re: [PATCH v6 5/9] xen/arm: Add additional reference to owner domain when the owner is allocated

[Stefano Stabellini <sstabellini@kernel.org>]

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
>