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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 17/10/2022 08:46, Henry Wang wrote:
>       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;

p2m_set_allocation() wants to be called after 'p->domain' is set. So 
p2m_teardown_final() will not return early and leak memory (as Andrew 
pointed out).
For simplicity I would move the code right at the end of the function. 
But if you want to keep it here then...
> +
>       p2m->vmid = INVALID_VMID;
... this needs to be done first as well.

>   
>       rc = p2m_alloc_vmid(d);
Note that if you move the code at the end. Then you will need to add:

if ( rc )
  return rc;

after the call to p2m_alloc_table();

Other than that the code looks good to me.

Cheers,

--
Julien Grall