[PATCH] xen/arm: Avoid to open-code the relinquish state machine

Julien Grall posted 1 patch 3 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20200419095030.2081-1-julien@xen.org
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Julien Grall <julien@xen.org>, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
xen/arch/arm/domain.c        | 60 ++++++++++++++++++++++--------------
xen/include/asm-arm/domain.h |  9 +-----
2 files changed, 38 insertions(+), 31 deletions(-)
[PATCH] xen/arm: Avoid to open-code the relinquish state machine
Posted by Julien Grall 3 years, 11 months ago
In commit 0dfffe01d5 "x86: Improve the efficiency of
domain_relinquish_resources()", the x86 version of the function has been
reworked to avoid open-coding the state machine and also add more
documentation.

Bring the Arm version on part with x86 by introducing a documented
PROGRESS() macro to avoid latent bugs and make the new PROG_* states
private to domain_relinquish_resources().

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/domain.c        | 60 ++++++++++++++++++++++--------------
 xen/include/asm-arm/domain.h |  9 +-----
 2 files changed, 38 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6627be2922..31169326b2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -674,7 +674,6 @@ int arch_domain_create(struct domain *d,
     int rc, count = 0;
 
     BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
-    d->arch.relmem = RELMEM_not_started;
 
     /* Idle domains do not need this setup */
     if ( is_idle_domain(d) )
@@ -950,13 +949,41 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
     return ret;
 }
 
+/*
+ * Record the current progress. Subsequent hypercall continuations will
+ * logically restart work from this point.
+ *
+ * PROGRESS() markers must not be in the middle of loops. The loop
+ * variable isn't preserved accross a continuation.
+ *
+ * To avoid redundant work, there should be a marker before each
+ * function which may return -ERESTART.
+ */
+enum {
+    PROG_tee = 1,
+    PROG_xen,
+    PROG_page,
+    PROG_mapping,
+    PROG_done,
+};
+
+#define PROGRESS(x)                         \
+    d->arch.rel_priv = PROG_ ## x;          \
+    /* Fallthrough */                       \
+    case PROG_ ## x
+
 int domain_relinquish_resources(struct domain *d)
 {
     int ret = 0;
 
-    switch ( d->arch.relmem )
+    /*
+     * This hypercall can take minutes of wallclock time to complete.  This
+     * logic implements a co-routine, stashing state in struct domain across
+     * hypercall continuation boundaries.
+     */
+    switch ( d->arch.rel_priv )
     {
-    case RELMEM_not_started:
+    case 0:
         ret = iommu_release_dt_devices(d);
         if ( ret )
             return ret;
@@ -967,42 +994,27 @@ int domain_relinquish_resources(struct domain *d)
          */
         domain_vpl011_deinit(d);
 
-        d->arch.relmem = RELMEM_tee;
-        /* Fallthrough */
-
-    case RELMEM_tee:
+    PROGRESS(tee):
         ret = tee_relinquish_resources(d);
         if (ret )
             return ret;
 
-        d->arch.relmem = RELMEM_xen;
-        /* Fallthrough */
-
-    case RELMEM_xen:
+    PROGRESS(xen):
         ret = relinquish_memory(d, &d->xenpage_list);
         if ( ret )
             return ret;
 
-        d->arch.relmem = RELMEM_page;
-        /* Fallthrough */
-
-    case RELMEM_page:
+    PROGRESS(page):
         ret = relinquish_memory(d, &d->page_list);
         if ( ret )
             return ret;
 
-        d->arch.relmem = RELMEM_mapping;
-        /* Fallthrough */
-
-    case RELMEM_mapping:
+    PROGRESS(mapping):
         ret = relinquish_p2m_mapping(d);
         if ( ret )
             return ret;
 
-        d->arch.relmem = RELMEM_done;
-        /* Fallthrough */
-
-    case RELMEM_done:
+    PROGRESS(done):
         break;
 
     default:
@@ -1012,6 +1024,8 @@ int domain_relinquish_resources(struct domain *d)
     return 0;
 }
 
+#undef PROGRESS
+
 void arch_dump_domain_info(struct domain *d)
 {
     p2m_dump_info(d);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d39477a939..d2142c6707 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -56,14 +56,7 @@ struct arch_domain
     struct vmmio vmmio;
 
     /* Continuable domain_relinquish_resources(). */
-    enum {
-        RELMEM_not_started,
-        RELMEM_tee,
-        RELMEM_xen,
-        RELMEM_page,
-        RELMEM_mapping,
-        RELMEM_done,
-    } relmem;
+    unsigned int rel_priv;
 
     struct {
         uint64_t offset;
-- 
2.17.1


Re: [PATCH] xen/arm: Avoid to open-code the relinquish state machine
Posted by Stefano Stabellini 3 years, 11 months ago
On Sun, 19 Apr 2020, Julien Grall wrote:
> In commit 0dfffe01d5 "x86: Improve the efficiency of
> domain_relinquish_resources()", the x86 version of the function has been
> reworked to avoid open-coding the state machine and also add more
> documentation.
> 
> Bring the Arm version on part with x86 by introducing a documented
> PROGRESS() macro to avoid latent bugs and make the new PROG_* states
> private to domain_relinquish_resources().
> 
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>  xen/arch/arm/domain.c        | 60 ++++++++++++++++++++++--------------
>  xen/include/asm-arm/domain.h |  9 +-----
>  2 files changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6627be2922..31169326b2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -674,7 +674,6 @@ int arch_domain_create(struct domain *d,
>      int rc, count = 0;
>  
>      BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
> -    d->arch.relmem = RELMEM_not_started;
>  
>      /* Idle domains do not need this setup */
>      if ( is_idle_domain(d) )
> @@ -950,13 +949,41 @@ static int relinquish_memory(struct domain *d, struct page_list_head *list)
>      return ret;
>  }
>  
> +/*
> + * Record the current progress. Subsequent hypercall continuations will
> + * logically restart work from this point.
> + *
> + * PROGRESS() markers must not be in the middle of loops. The loop
> + * variable isn't preserved accross a continuation.
> + *
> + * To avoid redundant work, there should be a marker before each
> + * function which may return -ERESTART.
> + */
> +enum {
> +    PROG_tee = 1,
> +    PROG_xen,
> +    PROG_page,
> +    PROG_mapping,
> +    PROG_done,
> +};
> +
> +#define PROGRESS(x)                         \
> +    d->arch.rel_priv = PROG_ ## x;          \
> +    /* Fallthrough */                       \
> +    case PROG_ ## x
> +
>  int domain_relinquish_resources(struct domain *d)
>  {
>      int ret = 0;
>  
> -    switch ( d->arch.relmem )
> +    /*
> +     * This hypercall can take minutes of wallclock time to complete.  This
> +     * logic implements a co-routine, stashing state in struct domain across
> +     * hypercall continuation boundaries.
> +     */
> +    switch ( d->arch.rel_priv )
>      {
> -    case RELMEM_not_started:
> +    case 0:
>          ret = iommu_release_dt_devices(d);
>          if ( ret )
>              return ret;
> @@ -967,42 +994,27 @@ int domain_relinquish_resources(struct domain *d)
>           */
>          domain_vpl011_deinit(d);
>  
> -        d->arch.relmem = RELMEM_tee;
> -        /* Fallthrough */
> -
> -    case RELMEM_tee:
> +    PROGRESS(tee):
>          ret = tee_relinquish_resources(d);
>          if (ret )
>              return ret;
>  
> -        d->arch.relmem = RELMEM_xen;
> -        /* Fallthrough */
> -
> -    case RELMEM_xen:
> +    PROGRESS(xen):
>          ret = relinquish_memory(d, &d->xenpage_list);
>          if ( ret )
>              return ret;
>  
> -        d->arch.relmem = RELMEM_page;
> -        /* Fallthrough */
> -
> -    case RELMEM_page:
> +    PROGRESS(page):
>          ret = relinquish_memory(d, &d->page_list);
>          if ( ret )
>              return ret;
>  
> -        d->arch.relmem = RELMEM_mapping;
> -        /* Fallthrough */
> -
> -    case RELMEM_mapping:
> +    PROGRESS(mapping):
>          ret = relinquish_p2m_mapping(d);
>          if ( ret )
>              return ret;
>  
> -        d->arch.relmem = RELMEM_done;
> -        /* Fallthrough */
> -
> -    case RELMEM_done:
> +    PROGRESS(done):
>          break;
>  
>      default:
> @@ -1012,6 +1024,8 @@ int domain_relinquish_resources(struct domain *d)
>      return 0;
>  }
>  
> +#undef PROGRESS
> +
>  void arch_dump_domain_info(struct domain *d)
>  {
>      p2m_dump_info(d);
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index d39477a939..d2142c6707 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -56,14 +56,7 @@ struct arch_domain
>      struct vmmio vmmio;
>  
>      /* Continuable domain_relinquish_resources(). */
> -    enum {
> -        RELMEM_not_started,
> -        RELMEM_tee,
> -        RELMEM_xen,
> -        RELMEM_page,
> -        RELMEM_mapping,
> -        RELMEM_done,
> -    } relmem;
> +    unsigned int rel_priv;
>  
>      struct {
>          uint64_t offset;
> -- 
> 2.17.1
>