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
> Signed-off-by: Julien Grall
Reviewed-by: Stefano Stabellini
> ---
> 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, >xenpage_list);
> if ( ret )
> return ret;
>
> -d->arch.relmem = RELMEM_page;
> -/* Fallthrough */
> -
> -case RELMEM_page:
> +PROGRESS(page):
> ret = relinquish_memory(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
>
