| Message ID | 20160816221714.22041-17-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, On 16/08/16 23:16, Sergej Proskurin wrote: > +static int altp2m_init_helper(struct domain *d, unsigned int idx) > +{ > + int rc; > + struct p2m_domain *p2m = d->arch.altp2m_p2m[idx]; > + > + ASSERT(p2m == NULL); > + > + /* Allocate a new, zeroed altp2m view. */ > + p2m = xzalloc(struct p2m_domain); > + if ( p2m == NULL) > + { > + rc = -ENOMEM; > + goto err; > + } This could be simplified with just return -ENOMEM. > + > + p2m->p2m_class = p2m_alternate; > + > + /* Initialize the new altp2m view. */ > + rc = p2m_init_one(d, p2m); > + if ( rc ) > + goto err; > + > + p2m->access_required = false; > + _atomic_set(&p2m->active_vcpus, 0); the p2m is initialized to 0, so both access_required and active_vcpus initialization is not necessary. > + > + d->arch.altp2m_p2m[idx] = p2m; > + > + return rc; > + > +err: > + if ( p2m ) > + xfree(p2m); xfree is able to handle NULL pointer. > + > + d->arch.altp2m_p2m[idx] = NULL; > + > + return rc; > +} > + > +int altp2m_init_by_id(struct domain *d, unsigned int idx) > +{ > + int rc = -EINVAL; > + > + if ( idx >= MAX_ALTP2M ) > + return rc; > + > + altp2m_lock(d); > + > + if ( d->arch.altp2m_p2m[idx] == NULL ) > + rc = altp2m_init_helper(d, idx); > + > + altp2m_unlock(d); > + > + return rc; > +} > + Regards,
Hi Julien, On 09/09/2016 07:14 PM, Julien Grall wrote: > Hello Sergej, > > On 16/08/16 23:16, Sergej Proskurin wrote: >> +static int altp2m_init_helper(struct domain *d, unsigned int idx) >> +{ >> + int rc; >> + struct p2m_domain *p2m = d->arch.altp2m_p2m[idx]; >> + >> + ASSERT(p2m == NULL); >> + >> + /* Allocate a new, zeroed altp2m view. */ >> + p2m = xzalloc(struct p2m_domain); >> + if ( p2m == NULL) >> + { >> + rc = -ENOMEM; >> + goto err; >> + } > > This could be simplified with just return -ENOMEM. > True. I will do that. >> + >> + p2m->p2m_class = p2m_alternate; >> + >> + /* Initialize the new altp2m view. */ >> + rc = p2m_init_one(d, p2m); >> + if ( rc ) >> + goto err; >> + >> + p2m->access_required = false; >> + _atomic_set(&p2m->active_vcpus, 0); > > the p2m is initialized to 0, so both access_required and active_vcpus > initialization is not necessary. > I will remove these initializations in the next patch. >> + >> + d->arch.altp2m_p2m[idx] = p2m; >> + >> + return rc; >> + >> +err: >> + if ( p2m ) >> + xfree(p2m); > > xfree is able to handle NULL pointer. > Ok, thank you. >> + >> + d->arch.altp2m_p2m[idx] = NULL; >> + >> + return rc; >> +} >> + >> +int altp2m_init_by_id(struct domain *d, unsigned int idx) >> +{ >> + int rc = -EINVAL; >> + >> + if ( idx >= MAX_ALTP2M ) >> + return rc; >> + >> + altp2m_lock(d); >> + >> + if ( d->arch.altp2m_p2m[idx] == NULL ) >> + rc = altp2m_init_helper(d, idx); >> + >> + altp2m_unlock(d); >> + >> + return rc; >> +} >> + Cheers, ~Sergej
Hello Sergej, On 16/08/16 23:16, Sergej Proskurin wrote: > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 180154e..c69da36 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -83,8 +83,40 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) > break; > > case HVMOP_altp2m_set_domain_state: > - rc = -EOPNOTSUPP; > + { > + struct vcpu *v; > + bool_t ostate, nstate; > + > + ostate = d->arch.altp2m_active; > + nstate = !!a.u.domain_state.state; > + > + /* If the alternate p2m state has changed, handle appropriately */ > + if ( (nstate != ostate) && > + (ostate || !(rc = altp2m_init_by_id(d, 0))) ) > + { > + for_each_vcpu( d, v ) > + { > + if ( !ostate ) > + { > + altp2m_vcpu_initialise(v); > + d->arch.altp2m_active = nstate; Why do you set d->arch.altp2m_active for each vCPU? > + } > + else > + { > + d->arch.altp2m_active = nstate; > + altp2m_vcpu_destroy(v); > + } > + } > + > + /* > + * The altp2m_active state has been deactivated. It is now safe to > + * flush all altp2m views -- including altp2m[0]. > + */ > + if ( ostate ) > + altp2m_flush(d); > + } > break; > + } > > case HVMOP_altp2m_vcpu_enable_notify: > rc = -EOPNOTSUPP; Regards,
diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c index 02cffd7..02a52ec 100644 --- a/xen/arch/arm/altp2m.c +++ b/xen/arch/arm/altp2m.c @@ -20,6 +20,108 @@ #include <asm/p2m.h> #include <asm/altp2m.h> +struct p2m_domain *altp2m_get_altp2m(struct vcpu *v) +{ + unsigned int index = altp2m_vcpu(v).p2midx; + + if ( index == INVALID_ALTP2M ) + return NULL; + + BUG_ON(index >= MAX_ALTP2M); + + return v->domain->arch.altp2m_p2m[index]; +} + +static void altp2m_vcpu_reset(struct vcpu *v) +{ + struct altp2mvcpu *av = &altp2m_vcpu(v); + + av->p2midx = INVALID_ALTP2M; +} + +void altp2m_vcpu_initialise(struct vcpu *v) +{ + if ( v != current ) + vcpu_pause(v); + + altp2m_vcpu(v).p2midx = 0; + atomic_inc(&altp2m_get_altp2m(v)->active_vcpus); + + if ( v != current ) + vcpu_unpause(v); +} + +void altp2m_vcpu_destroy(struct vcpu *v) +{ + struct p2m_domain *p2m; + + if ( v != current ) + vcpu_pause(v); + + if ( (p2m = altp2m_get_altp2m(v)) ) + atomic_dec(&p2m->active_vcpus); + + altp2m_vcpu_reset(v); + + if ( v != current ) + vcpu_unpause(v); +} + +static int altp2m_init_helper(struct domain *d, unsigned int idx) +{ + int rc; + struct p2m_domain *p2m = d->arch.altp2m_p2m[idx]; + + ASSERT(p2m == NULL); + + /* Allocate a new, zeroed altp2m view. */ + p2m = xzalloc(struct p2m_domain); + if ( p2m == NULL) + { + rc = -ENOMEM; + goto err; + } + + p2m->p2m_class = p2m_alternate; + + /* Initialize the new altp2m view. */ + rc = p2m_init_one(d, p2m); + if ( rc ) + goto err; + + p2m->access_required = false; + _atomic_set(&p2m->active_vcpus, 0); + + d->arch.altp2m_p2m[idx] = p2m; + + return rc; + +err: + if ( p2m ) + xfree(p2m); + + d->arch.altp2m_p2m[idx] = NULL; + + return rc; +} + +int altp2m_init_by_id(struct domain *d, unsigned int idx) +{ + int rc = -EINVAL; + + if ( idx >= MAX_ALTP2M ) + return rc; + + altp2m_lock(d); + + if ( d->arch.altp2m_p2m[idx] == NULL ) + rc = altp2m_init_helper(d, idx); + + altp2m_unlock(d); + + return rc; +} + int altp2m_init(struct domain *d) { unsigned int i; diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 180154e..c69da36 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -83,8 +83,40 @@ static int do_altp2m_op(XEN_GUEST_HANDLE_PARAM(void) arg) break; case HVMOP_altp2m_set_domain_state: - rc = -EOPNOTSUPP; + { + struct vcpu *v; + bool_t ostate, nstate; + + ostate = d->arch.altp2m_active; + nstate = !!a.u.domain_state.state; + + /* If the alternate p2m state has changed, handle appropriately */ + if ( (nstate != ostate) && + (ostate || !(rc = altp2m_init_by_id(d, 0))) ) + { + for_each_vcpu( d, v ) + { + if ( !ostate ) + { + altp2m_vcpu_initialise(v); + d->arch.altp2m_active = nstate; + } + else + { + d->arch.altp2m_active = nstate; + altp2m_vcpu_destroy(v); + } + } + + /* + * The altp2m_active state has been deactivated. It is now safe to + * flush all altp2m views -- including altp2m[0]. + */ + if ( ostate ) + altp2m_flush(d); + } break; + } case HVMOP_altp2m_vcpu_enable_notify: rc = -EOPNOTSUPP; diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h index 4c15b75..f604ffd 100644 --- a/xen/include/asm-arm/altp2m.h +++ b/xen/include/asm-arm/altp2m.h @@ -22,6 +22,10 @@ #include <xen/sched.h> +#define INVALID_ALTP2M 0xffff + +#define altp2m_vcpu(v) ((v)->arch.avcpu) + #define altp2m_lock(d) spin_lock(&(d)->arch.altp2m_lock) #define altp2m_unlock(d) spin_unlock(&(d)->arch.altp2m_lock) @@ -42,6 +46,16 @@ static inline uint16_t altp2m_vcpu_idx(const struct vcpu *v) int altp2m_init(struct domain *d); void altp2m_teardown(struct domain *d); +void altp2m_vcpu_initialise(struct vcpu *v); +void altp2m_vcpu_destroy(struct vcpu *v); + +/* Get current alternate p2m table. */ +struct p2m_domain *altp2m_get_altp2m(struct vcpu *v); + +/* Make a specific alternate p2m valid. */ +int altp2m_init_by_id(struct domain *d, + unsigned int idx); + /* Flush all the alternate p2m's for a domain. */ void altp2m_flush(struct domain *d); diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index a4e4762..99aa3bc 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -137,6 +137,10 @@ struct arch_domain struct p2m_domain *altp2m_p2m[MAX_ALTP2M]; } __cacheline_aligned; +struct altp2mvcpu { + uint16_t p2midx; /* alternate p2m index */ +}; + struct arch_vcpu { struct { @@ -266,6 +270,9 @@ struct arch_vcpu struct vtimer phys_timer; struct vtimer virt_timer; bool_t vtimer_initialized; + + /* Alternate p2m context */ + struct altp2mvcpu avcpu; } __cacheline_aligned; void vcpu_show_execution_state(struct vcpu *); diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index de0c90a..978125a 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -9,6 +9,8 @@ #include <xen/p2m-common.h> #include <public/memory.h> +#include <asm/atomic.h> + #define MAX_ALTP2M 10 /* ARM might contain an arbitrary number of altp2m views. */ #define paddr_bits PADDR_BITS @@ -102,6 +104,9 @@ struct p2m_domain { */ struct radix_tree_root mem_access_settings; + /* Alternate p2m: count of vcpu's currently using this p2m. */ + atomic_t active_vcpus; + /* Choose between: host/alternate. */ p2m_class_t p2m_class;
The HVMOP_altp2m_set_domain_state allows to activate altp2m on a specific domain. This commit adopts the x86 HVMOP_altp2m_set_domain_state implementation. Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> --- v2: Dynamically allocate memory for altp2m views only when needed. Move altp2m related helpers to altp2m.c. p2m_flush_tlb is made publicly accessible. v3: Cosmetic fixes. Removed call to "p2m_alloc_table" in "altp2m_init_helper" as the entire p2m allocation is now done within the function "p2m_init_one". The same applies to the call of the function "p2m_flush_tlb" from "p2m_init_one". Removed the "altp2m_enabled" check in HVMOP_altp2m_set_domain_state case as it has been moved in front of the switch statement in "do_altp2m_op". Changed the order of setting the new altp2m state (depending on setting/resetting the state) in HVMOP_altp2m_set_domain_state case. Removed the call to altp2m_vcpu_reset from altp2m_vcpu_initialise, as the p2midx is set right after the call to 0, representing the default view. Moved the define "vcpu_altp2m" from domain.h to altp2m.h to avoid defining altp2m-related functionality in multiple files. Also renamed "vcpu_altp2m" to "altp2m_vcpu". Declared the function "p2m_flush_tlb" as static, as it is not called from altp2m.h anymore. Exported the function "altp2m_get_altp2m" in altp2m.h. Exchanged the check "altp2m_vttbr[idx] == INVALID_VTTBR" for "altp2m_p2m[idx] == NULL" in "altp2m_init_by_id". Set the field p2m->access_required to false by default. --- xen/arch/arm/altp2m.c | 102 +++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/hvm.c | 34 ++++++++++++++- xen/include/asm-arm/altp2m.h | 14 ++++++ xen/include/asm-arm/domain.h | 7 +++ xen/include/asm-arm/p2m.h | 5 +++ 5 files changed, 161 insertions(+), 1 deletion(-)