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 <jgr...@amazon.com>
> Signed-off-by: Henry Wang <henry.w...@arm.com>
> ---
> 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...
> +
> 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
