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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Andrew,

> -----Original Message-----
> From: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>
> Subject: Re: [PATCH] xen/arm: p2m: Populate pages for GICv2 mapping in
> arch_domain_create()
> 
> 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
> > 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
> > +     * 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);
> > +        spin_unlock(&d->arch.paging.lock);
> > +        goto fail;
> > +    }
> > +    spin_unlock(&d->arch.paging.lock);
> 
> Generally, this would be better written as
> 
> spin_lock();
> if ( rc = p2m_set_allocation(16) )
>     p2m_set_allocation(0)
> spin_unlock();
> 
> if ( rc )
>     goto fail;
> 
> to reduce the number of spin_unlock() calls and make the error paths
> more clear.  However...

I think in Arm's arch_domain_create(), all the error handling are the
same style using:

if ( (rc = <function>) !=0 )
    goto fail;

and we need to keep them the same? But I think I will drop the
p2m_set_allocation(d, 0, NULL); as the arch_domain_destroy(d) in

fail:
    d->is_dying = DOMDYING_dead;
    arch_domain_destroy(d);

will clean-up the pool.

Kind regards,
Henry

> 
> > +
> >      if ( (rc = domain_vgic_register(d, &count)) != 0 )
> >          goto fail;
> >
> 
> ... you've got a problem on this error path, so the set allocation to 0
> needs to be in the fail: path with suitable locking.
> 
> There are perhaps better ways of doing it in 4.15(?) and later, but not
> in earlier versions.  As this is a fix to a bug in a security patch,
> simplicity is generally the better approach.
> 
> ~Andrew