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

[Julien Grall <julien@xxxxxxx>]

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?

> 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.
> +     * 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())?
> +        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