Re: [PATCH] xen/arm: Reduce redundant clear root pages when teardown p2m

[Julien Grall <julien@xxxxxxx>]

Hi Henry,

On 12/12/2022 06:41, Henry Wang wrote:
> Currently, p2m for a domain will be teardown from two paths:
> (1) The normal path when a domain is destroyed.
> (2) The arch_domain_destroy() in the failure path of domain creation.
>
> When tearing down p2m from (1), the part to clear and clean the root
> is only needed to do once rather than for every call of p2m_teardown().
> If the p2m teardown is from (2), the clear and clean of the root
> is unnecessary because the domain is not scheduled.
>
> Therefore, this patch introduces a helper `p2m_clear_root_pages()` to
> do the clear and clean of the root, and move this logic outside of
> p2m_teardown(). With this movement, the `page_list_empty(&p2m->pages)`
> check can be dropped.
>
> Signed-off-by: Henry Wang <Henry.Wang@xxxxxxx>
> ---
>   xen/arch/arm/domain.c          |  5 +++++
>   xen/arch/arm/include/asm/p2m.h |  1 +
>   xen/arch/arm/p2m.c             | 23 ++++++++++++-----------
>   3 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 99577adb6c..29c189aab4 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -1022,6 +1022,11 @@ int domain_relinquish_resources(struct domain *d)
>               return ret;
>   
>       PROGRESS(p2m):
> +        /*
> +         * We are about to free the intermediate page-tables, so clear the
> +         * root to prevent any walk to use them.
> +         */
> +        p2m_clear_root_pages(&d->arch.p2m);
The commit message suggests this should only be called once. However, 
you will still end up to call p2m_clear_root_pages() multiple time if 
p2m_teardown() is preempted.
>           ret = p2m_teardown(d, true);
>           if ( ret )
>               return ret;
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index 91df922e1c..bf5183e53a 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -281,6 +281,7 @@ int p2m_set_entry(struct p2m_domain *p2m,
>   
>   bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
>   
> +void p2m_clear_root_pages(struct p2m_domain *p2m);
>   void p2m_invalidate_root(struct p2m_domain *p2m);
>   
>   /*
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 948f199d84..0c942c5923 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1314,6 +1314,18 @@ static void p2m_invalidate_table(struct p2m_domain *p2m, 
> mfn_t mfn)
>       p2m->need_flush = true;
>   }
>   
> +void p2m_clear_root_pages(struct p2m_domain *p2m)
> +{
> +    unsigned int i;
> +
> +    p2m_write_lock(p2m);
> +
> +    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
> +        clear_and_clean_page(p2m->root + i);
> +
> +    p2m_write_unlock(p2m);
> +}
> +
>   /*
>    * Invalidate all entries in the root page-tables. This is
>    * useful to get fault on entry and do an action.
> @@ -1698,21 +1710,10 @@ int p2m_teardown(struct domain *d, bool 
> allow_preemption)
>       struct p2m_domain *p2m = p2m_get_hostp2m(d);
>       unsigned long count = 0;
>       struct page_info *pg;
> -    unsigned int i;
>       int rc = 0;
>   
> -    if ( page_list_empty(&p2m->pages) )
> -        return 0;
> -
>       p2m_write_lock(p2m);
>   
> -    /*
> -     * We are about to free the intermediate page-tables, so clear the
> -     * root to prevent any walk to use them.
> -     */
> -    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
> -        clear_and_clean_page(p2m->root + i);
> -
>       /*
>        * The domain will not be scheduled anymore, so in theory we should
>        * not need to flush the TLBs. Do it for safety purpose.
This flush needs to be moved in p2m_clear_root_pages().

Cheers,

--
Julien Grall