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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 17/10/2022 08:46, Henry Wang wrote:
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f17500ddf3..8c9ddf58e1 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1736,7 +1739,20 @@ void p2m_final_teardown(struct domain *d)
>      if ( !p2m->domain )
>          return;
>  

Everything below here is "dead" code, because...


> +    /*
> +     * No need to call relinquish_p2m_mapping() here because
> +     * p2m_final_teardown() is called either after 
> domain_relinquish_resources()
> +     * where relinquish_p2m_mapping() has been called, or from failure path 
> of
> +     * domain_create()/arch_domain_create() where mappings that require
> +     * p2m_put_l3_page() should never be created. For the latter case, also 
> see
> +     * comment on top of the p2m_set_entry() for more info.
> +     */
> +
> +    BUG_ON(p2m_teardown(d, false));
>      ASSERT(page_list_empty(&p2m->pages));
> +
> +    while ( p2m_teardown_allocation(d) == -ERESTART )
> +        continue; /* No preemption support here */
>      ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
>  
>      if ( p2m->root )
> @@ -1762,6 +1778,20 @@ int p2m_init(struct domain *d)
>      INIT_PAGE_LIST_HEAD(&p2m->pages);
>      INIT_PAGE_LIST_HEAD(&d->arch.paging.p2m_freelist);
>  
> +    /*
> +     * 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.
> +     * 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.
> +     */
> +    spin_lock(&d->arch.paging.lock);
> +    rc = p2m_set_allocation(d, 16, NULL);
> +    spin_unlock(&d->arch.paging.lock);
> +    if ( rc != 0 )
> +        return rc;

... this early exit is ahead of p2m_init() settings p2m->domain = d.

In particular, you introduce a bug whereby ...

> +
>      p2m->vmid = INVALID_VMID;
>  
>      rc = p2m_alloc_vmid(d);

... this error path now leaks the 16 page p2m allocation.


This change is overly complex.  You add a set_allocation(16) path in
p2m_init(), so should only be adding a single set_allocation(0) to
compensate.

The `while ( p2m_teardown_allocation(d) == -ERESTART ) continue;` is
especially silly because you're specifically wasting time ignoring the
preemption wrapper around the non-preemptible function that you actually
want to use.

Looking at between 4.13 and staging, you want to be calling
set_allocation(0) in p2m_final_teardown() ahead of of the p->domain
check.  Except idempotency which is going to be irritating here.

It will be a no-op from the normal domain destroy path (as relinquish
resource will have emptied the whole pool), while in domain_create error
path, it has a maximum of 16 pages to release.

I'll draft a patch.

~Andrew