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

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

Posted by Henry Wang 1 year, 5 months ago

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@arm.com>
---
 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);
         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.
-- 
2.25.1

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

Posted by Julien Grall 1 year, 5 months ago

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@arm.com>
> ---
>   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

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

Posted by Henry Wang 1 year, 5 months ago

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH] xen/arm: Reduce redundant clear root pages when
> teardown p2m
> 
> Hi Henry,
> 
> >       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.

Thanks for confirming this, would adding another PROGRESS stage
such as PROGRESS(p2m_clean) before PROGRESS(p2m) seem ok to you?

> 
> >           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;
> >   }
> >
> >       /*
> >        * 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().

Sure, will move this in v2.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

Posted by Julien Grall 1 year, 5 months ago

On 12/12/2022 11:27, Henry Wang wrote:
> Hi Julien,

Hi,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH] xen/arm: Reduce redundant clear root pages when
>> teardown p2m
>>
>> Hi Henry,
>>
>>>        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.
> 
> Thanks for confirming this, would adding another PROGRESS stage
> such as PROGRESS(p2m_clean) before PROGRESS(p2m) seem ok to you?

The would be fine with me.

Cheers,

-- 
Julien Grall