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

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 17/10/2022 20:12, Andrew Cooper wrote:
> From: Henry Wang <Henry.Wang@xxxxxxx>
>
> The XSA-409 fixes discovered that the GICv2 path tries to create P2M mappings
> in the domain_create() path.  This fails, as the P2M pool is empty before a
> XEN_DOMCTL_SHADOW_OP_SET_ALLOCATION hypercall.
>
> As a stopgap, automatically give domains 16 pages of P2M memory.  This is
> large enough to allow the GICv2 case to work, but small enough to not
> introduce a continuation worry.
>
> A consequence is that, for later error paths domain_create(), we end up in
> p2m_final_teardown() with a nonzero P2M pool.  Such a domain has no vCPUs, and
> has never been scheduled, so free the memory directly.
>
> Fixes: cbea5a1149ca ("xen/arm: Allocate and free P2M pages from the P2M pool")
> Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
This is not really in the spirit of my original suggestion anymore... In 
fact, you drop all the explanations regarding how the code is fragile 
(e.g. we are relying on early mapping to not take any extra reference). 
Maybe you don't care, but I do as Henry and I spent ages to figure out 
all the corner cases. In addition to that...
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Henry Wang <Henry.Wang@xxxxxxx>
> ---
>   xen/arch/arm/p2m.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 41 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 6826f6315080..76a0e31c6c8c 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1736,8 +1736,36 @@ void p2m_final_teardown(struct domain *d)
>       if ( !p2m->domain )
>           return;
>   
> -    ASSERT(page_list_empty(&p2m->pages));
> -    ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
> +    /*
> +     * On the domain_create() error path only, we can end up here with a
> +     * non-zero P2M pool.
> +     *
> +     * At present, this is a maximum of 16 pages, spread between p2m->pages
> +     * and the free list.  The domain has never been scheduled (it has no
> +     * vcpus), so there is TLB maintenance to perform; just free everything.
> +     */
> +    if ( !page_list_empty(&p2m->pages) ||
> +         !page_list_empty(&d->arch.paging.p2m_freelist) )
> +    {
> +        struct page_info *pg;
> +
> +        /*
> +         * There's no sensible "in the domain_create() error path" predicate,
> +         * so simply sanity check that we don't have unexpected work to do.
> +         */
> +        ASSERT(d->arch.paging.p2m_total_pages <= 16);
> +
> +        spin_lock(&d->arch.paging.lock);
> +
> +        while ( (pg = page_list_remove_head(&p2m->pages)) )
> +            free_domheap_page(pg);
> +        while ( (pg = page_list_remove_head(&d->arch.paging.p2m_freelist)) )
> +            free_domheap_page(pg);
> +
> +        d->arch.paging.p2m_total_pages = 0;
> +
> +        spin_unlock(&d->arch.paging.lock);
> +    }
... you are hardcoding both p2m_teardown() and p2m_set_allocation().
IMO this is not an improvement at all. It is just making the code more 
complex than necessary and lack all the explanation on the assumptions.
So while I am fine with your patch #1 (already reviewed it), there is a 
better patch from Henry on the ML. So we should take his (rebased) 
instead of yours.
Cheers,

--
Julien Grall