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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
>  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
>