diff mbox series

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

Message ID 20221212064119.2632626-1-Henry.Wang@arm.com (mailing list archive)
State Superseded
Headers show
Series xen/arm: Reduce redundant clear root pages when teardown p2m | expand

Commit Message

Henry Wang Dec. 12, 2022, 6:41 a.m. UTC
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(-)

Comments

Julien Grall Dec. 12, 2022, 11:22 a.m. UTC | #1
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,
Henry Wang Dec. 12, 2022, 11:27 a.m. UTC | #2
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
Julien Grall Dec. 13, 2022, 9:34 p.m. UTC | #3
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,
diff mbox series

Patch

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.