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

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 17/10/2022 17:51, Henry Wang wrote:
> @@ -1736,7 +1739,20 @@ void p2m_final_teardown(struct domain *d)
>       if ( !p2m->domain )
>           return;
>   
> +    /*
> +     * 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 )
> @@ -1784,6 +1800,8 @@ int p2m_init(struct domain *d)
As Andrew pointed out the change in p2m_init() will end up leaking 
either the VMID or the root table.
Andrew's patch #1 [1] should help to solve the problem. So I would 
suggest to rebase on top of it.
Other than that, the logic looks good to me. This is even knowning that 
Andrew said the code is buggy. I spent some time starring at the code 
and can't figure out where the issue lies because p2m_teardown() will do 
nothing when the list is empty. If it is not empty, then it is 
guaranteed that the VMID and root table is allocated. So the code looks 
functional but just not efficient.
We already discussed the latter point earlier in the review and agreed 
to look at improve it post 4.17.
For the former, I am happy to be proven wrong. But this is going to 
require more substantial explanations.
Cheers,

--
Julien Grall