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

[Julien Grall <julien@xxxxxxx>]

On 20/01/2023 09:43, Michal Orzel wrote:
> 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.
+1

> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>
Acked-by: Julien Grall <jgrall@xxxxxxxxxx>

Cheers,

--
Julien Grall