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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Julien,

> -----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?

> 
> > 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.

> 
> > +     * 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.

Kind regards,
Henry

> 
> > +        spin_unlock(&d->arch.paging.lock);
> > +        goto fail;
> > +    }
> > +    spin_unlock(&d->arch.paging.lock);
> > +
> >       if ( (rc = domain_vgic_register(d, &count)) != 0 )
> >           goto fail;
> >
> 
> Cheers,
> 
> --
> Julien Grall