Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in arch_domain_create()

[Julien Grall <julien@xxxxxxx>]

On 13/10/2022 10:21, Henry Wang wrote:
> Hi Julien,
Hi Henry,

> > -----Original Message-----
> > From: Julien Grall <julien@xxxxxxx>
> > Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in
> > arch_domain_create()
> >
> > Hi Henry,
> >
> > On 13/10/2022 09:38, Henry Wang wrote:
> > > Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
> > > when the domain is created. Considering the worst case of page tables
> > Can you describe in the commit message what is the worst case scenario?
> The two pages will be consecutive but not necessarily in the same L3 page
> table so the worst case is 4 + 2, is that correct?
So I agree that the worse case is 6. But I don't understand what you 
mean by '4 + 2' here.
> > > and keep a buffer, populate 16 pages as the default value to the P2M
> > > pages pool in arch_domain_create() at the domain creation stage to
> > > satisfy the GICv2 requirement.
> > >
> > > Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M
> > pool")
> > > Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> > > ---
> > > This should also be backported to 4.13, 4.14, 4.15 and 4.16.
> > > ---
> > >    xen/arch/arm/domain.c | 14 ++++++++++++++
> > >    1 file changed, 14 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > index 2c84e6dbbb..e40e2bcba1 100644
> > > --- a/xen/arch/arm/domain.c
> > > +++ b/xen/arch/arm/domain.c
> > > @@ -740,6 +740,20 @@ int arch_domain_create(struct domain *d,
> > >            BUG();
> > >        }
> > >
> > > +    spin_lock(&d->arch.paging.lock);
> > > +    /*
> > > +     * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2
> > area
> >
> > The wording suggests that this is only necessary for GICv2. But below
> > this is done unconditionally. I am happy with this been done
> > unconditionally, but I think this should be clarified here.
> Sure, I will add "For GICv3, the above-mentioned P2M mapping is not
> necessary, but since the allocated 16 pages here would not be lost, hence
> populate these pages unconditionally" if it is ok to you.
Sounds good to me.

> > > +     * when the domain is created. Considering the worst case for page
> > > +     * tables and keep a buffer, populate 16 pages to the P2M pages pool
> > here.
> > > +     */
> > > +    if ( (rc = p2m_set_allocation(d, 16, NULL)) != 0 )
> > > +    {
> > > +        p2m_set_allocation(d, 0, NULL);
> > Shouldn't this be done in p2m_fiinal_teardown() to cover so the pages
> > will be freed anything after this call will fail (include in the caller
> > domain_create())?
> Hmm, yes, I will remove this p2m_set_allocation(d, 0, NULL); in v2.
Just to clarify, I meant that a call in p2m_final_teardown() *is* 
missing in p2m_final_teardown() (or wherever we decide to add).
This would make this one redundant.

Cheers,

--
Julien Grall