| Message ID | 20200419095030.2081-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Avoid to open-code the relinquish state machine | expand |
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 >
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;
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(-)