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

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Henry,

On 16/01/2023 02:58, 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>
> ---
> Was: [PATCH v2] xen/arm: Reduce redundant clear root pages when
> teardown p2m. Picked to this series with changes in original v1:
> 1. Introduce a new PROGRESS for p2m_clear_root_pages() to avoid
>    multiple calling when p2m_teardown() is preempted.
> 2. Move p2m_force_tlb_flush_sync() to p2m_clear_root_pages().
> ---
>  xen/arch/arm/domain.c          | 12 ++++++++++++
>  xen/arch/arm/include/asm/p2m.h |  1 +
>  xen/arch/arm/p2m.c             | 34 ++++++++++++++--------------------
>  3 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 99577adb6c..961dab9166 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -959,6 +959,7 @@ enum {
>      PROG_xen,
>      PROG_page,
>      PROG_mapping,
> +    PROG_p2m_root,
>      PROG_p2m,
>      PROG_p2m_pool,
>      PROG_done,
> @@ -1021,6 +1022,17 @@ int domain_relinquish_resources(struct domain *d)
>          if ( ret )
>              return ret;
> 
> +    PROGRESS(p2m_root):
> +        /*
> +         * We are about to free the intermediate page-tables, so clear the
> +         * root to prevent any walk to use them.
The comment from here...
> +         * The domain will not be scheduled anymore, so in theory we should
> +         * not need to flush the TLBs. Do it for safety purpose.
> +         * Note that all the devices have already been de-assigned. So we 
> don't
> +         * need to flush the IOMMU TLB here.
> +         */
to here does not make a lot of sense in this place and should be moved to 
p2m_clear_root_pages
where a user can see the call to p2m_force_tlb_flush_sync.

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal