| Message ID | 20160801171028.11615-9-proskurin@sec.in.tum.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Sergej, On 01/08/16 18:10, Sergej Proskurin wrote: > 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. Note that the function > altp2m_flush is currently implemented in form of a stub. > > 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. > --- > xen/arch/arm/altp2m.c | 116 +++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/hvm.c | 34 +++++++++++- > xen/arch/arm/p2m.c | 2 +- > xen/include/asm-arm/altp2m.h | 15 ++++++ > xen/include/asm-arm/domain.h | 9 ++++ > xen/include/asm-arm/flushtlb.h | 4 ++ > xen/include/asm-arm/p2m.h | 11 ++++ > 7 files changed, 189 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c > index abbd39a..767f233 100644 > --- a/xen/arch/arm/altp2m.c > +++ b/xen/arch/arm/altp2m.c > @@ -19,6 +19,122 @@ > > #include <asm/p2m.h> > #include <asm/altp2m.h> > +#include <asm/flushtlb.h> > + > +struct p2m_domain *altp2m_get_altp2m(struct vcpu *v) > +{ > + unsigned int index = vcpu_altp2m(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 = &vcpu_altp2m(v); > + > + av->p2midx = INVALID_ALTP2M; > +} > + > +void altp2m_vcpu_initialise(struct vcpu *v) > +{ > + if ( v != current ) > + vcpu_pause(v); > + > + altp2m_vcpu_reset(v); I don't understand why you call altp2m_vcpu_reset which will set p2midx to INVALID_ALTP2M but a line after you set to 0. > + vcpu_altp2m(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]; > + > + if ( p2m == NULL ) > + { > + /* Allocate a new, zeroed altp2m view. */ > + p2m = xzalloc(struct p2m_domain); > + if ( p2m == NULL) > + { > + rc = -ENOMEM; > + goto err; > + } > + } Why don't you re-allocate the p2m from scratch? > + > + /* Initialize the new altp2m view. */ > + rc = p2m_init_one(d, p2m); > + if ( rc ) > + goto err; > + > + /* Allocate a root table for the altp2m view. */ > + rc = p2m_alloc_table(p2m); > + if ( rc ) > + goto err; > + > + p2m->p2m_class = p2m_alternate; > + p2m->access_required = 1; Please use true here. Although, I am not sure why you want to enable the access by default. > + _atomic_set(&p2m->active_vcpus, 0); > + > + d->arch.altp2m_p2m[idx] = p2m; > + d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr; > + > + /* > + * Make sure that all TLBs corresponding to the current VMID are flushed > + * before using it. > + */ > + p2m_flush_tlb(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_vttbr[idx] == INVALID_VTTBR ) > + rc = altp2m_init_helper(d, idx); > + > + altp2m_unlock(d); > + > + return rc; > +} > > int altp2m_init(struct domain *d) > { > diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c > index 01a3243..78370c6 100644 > --- a/xen/arch/arm/hvm.c > +++ b/xen/arch/arm/hvm.c > @@ -80,8 +80,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; > + > + if ( !altp2m_enabled(d) ) > + { > + rc = -EINVAL; > + break; > + } > + > + ostate = d->arch.altp2m_active; > + d->arch.altp2m_active = !!a.u.domain_state.state; > + > + /* If the alternate p2m state has changed, handle appropriately */ > + if ( (d->arch.altp2m_active != ostate) && > + (ostate || !(rc = altp2m_init_by_id(d, 0))) ) > + { > + for_each_vcpu( d, v ) > + { > + if ( !ostate ) > + altp2m_vcpu_initialise(v); > + else > + altp2m_vcpu_destroy(v); > + } The implementation of this hvmop param looks racy to me. What does prevent to CPU running in this function at the same time? One will destroy, whilst the other one will initialize. It might even be possible to have both doing the initialization because there is no synchronization barrier for altp2m_active. > + > + /* > + * The altp2m_active state has been deactivated. It is now safe to > + * flush all altp2m views -- including altp2m[0]. > + */ > + if ( ostate ) > + altp2m_flush(d); The function altp2m_flush is defined afterwards (in patch #9). Please make sure that all the patches compile one by one. > + } > break; > + } > > case HVMOP_altp2m_vcpu_enable_notify: > rc = -EOPNOTSUPP; > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 29ec5e5..8afea11 100644 > --- a/xen/arch/arm/p2m.c > +++ b/xen/arch/arm/p2m.c > @@ -139,7 +139,7 @@ void p2m_restore_state(struct vcpu *n) > isb(); > } > > -static void p2m_flush_tlb(struct p2m_domain *p2m) > +void p2m_flush_tlb(struct p2m_domain *p2m) This should ideally be in a separate patch. > { > unsigned long flags = 0; > uint64_t ovttbr; > diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h > index 79ea66b..a33c740 100644 > --- a/xen/include/asm-arm/altp2m.h > +++ b/xen/include/asm-arm/altp2m.h > @@ -22,6 +22,8 @@ > > #include <xen/sched.h> > > +#define INVALID_ALTP2M 0xffff > + > #define altp2m_lock(d) spin_lock(&(d)->arch.altp2m_lock) > #define altp2m_unlock(d) spin_unlock(&(d)->arch.altp2m_lock) > > @@ -44,4 +46,17 @@ 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); > + > +/* 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 */ > +static inline void altp2m_flush(struct domain *d) > +{ > + /* Not yet implemented. */ > +} > + > #endif /* __ASM_ARM_ALTP2M_H */ > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index 3c25ea5..63a9650 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -135,6 +135,12 @@ struct arch_domain > spinlock_t altp2m_lock; > } __cacheline_aligned; > > +struct altp2mvcpu { > + uint16_t p2midx; /* alternate p2m index */ > +}; > + > +#define vcpu_altp2m(v) ((v)->arch.avcpu) Please avoid to have half of altp2m defined in altp2m.h and the other half in domain.h. > + > struct arch_vcpu > { > struct { > @@ -264,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/flushtlb.h b/xen/include/asm-arm/flushtlb.h > index 329fbb4..57c3c34 100644 > --- a/xen/include/asm-arm/flushtlb.h > +++ b/xen/include/asm-arm/flushtlb.h > @@ -2,6 +2,7 @@ > #define __ASM_ARM_FLUSHTLB_H__ > > #include <xen/cpumask.h> > +#include <asm/p2m.h> > > /* > * Filter the given set of CPUs, removing those that definitely flushed their > @@ -25,6 +26,9 @@ do { \ > /* Flush specified CPUs' TLBs */ > void flush_tlb_mask(const cpumask_t *mask); > > +/* Flush CPU's TLBs for the specified domain */ > +void p2m_flush_tlb(struct p2m_domain *p2m); > + This function should be declared in p2m.h and not flushtlb.h. > #endif /* __ASM_ARM_FLUSHTLB_H__ */ > /* > * Local variables: > diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h > index 24a1f61..f13f285 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 > @@ -86,6 +88,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; > > @@ -214,6 +219,12 @@ void guest_physmap_remove_page(struct domain *d, > > mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); > > +/* Allocates page table for a p2m. */ > +int p2m_alloc_table(struct p2m_domain *p2m); > + > +/* Initialize the p2m structure. */ > +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); These declarations belong to the patch that exported them not. Not here. > + > /* Release resources held by the p2m structure. */ > void p2m_free_one(struct p2m_domain *p2m); > > Regards,
Hi Julien, On 08/03/2016 08:41 PM, Julien Grall wrote: > Hello Sergej, > > On 01/08/16 18:10, Sergej Proskurin wrote: >> 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. Note that the function >> altp2m_flush is currently implemented in form of a stub. >> >> 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. >> --- >> xen/arch/arm/altp2m.c | 116 >> +++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/hvm.c | 34 +++++++++++- >> xen/arch/arm/p2m.c | 2 +- >> xen/include/asm-arm/altp2m.h | 15 ++++++ >> xen/include/asm-arm/domain.h | 9 ++++ >> xen/include/asm-arm/flushtlb.h | 4 ++ >> xen/include/asm-arm/p2m.h | 11 ++++ >> 7 files changed, 189 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c >> index abbd39a..767f233 100644 >> --- a/xen/arch/arm/altp2m.c >> +++ b/xen/arch/arm/altp2m.c >> @@ -19,6 +19,122 @@ >> >> #include <asm/p2m.h> >> #include <asm/altp2m.h> >> +#include <asm/flushtlb.h> >> + >> +struct p2m_domain *altp2m_get_altp2m(struct vcpu *v) >> +{ >> + unsigned int index = vcpu_altp2m(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 = &vcpu_altp2m(v); >> + >> + av->p2midx = INVALID_ALTP2M; >> +} >> + >> +void altp2m_vcpu_initialise(struct vcpu *v) >> +{ >> + if ( v != current ) >> + vcpu_pause(v); >> + >> + altp2m_vcpu_reset(v); > > I don't understand why you call altp2m_vcpu_reset which will set > p2midx to INVALID_ALTP2M but a line after you set to 0. > It is a leftover from the x86 implementation. Since we do not need to reset further fields, I can remove the call to altp2m_vcpu_reset. >> + vcpu_altp2m(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]; >> + >> + if ( p2m == NULL ) >> + { >> + /* Allocate a new, zeroed altp2m view. */ >> + p2m = xzalloc(struct p2m_domain); >> + if ( p2m == NULL) >> + { >> + rc = -ENOMEM; >> + goto err; >> + } >> + } > > Why don't you re-allocate the p2m from scratch? > >> + >> + /* Initialize the new altp2m view. */ >> + rc = p2m_init_one(d, p2m); >> + if ( rc ) >> + goto err; >> + >> + /* Allocate a root table for the altp2m view. */ >> + rc = p2m_alloc_table(p2m); >> + if ( rc ) >> + goto err; >> + >> + p2m->p2m_class = p2m_alternate; >> + p2m->access_required = 1; > > Please use true here. Although, I am not sure why you want to enable > the access by default. > >> + _atomic_set(&p2m->active_vcpus, 0); >> + >> + d->arch.altp2m_p2m[idx] = p2m; >> + d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr; >> + >> + /* >> + * Make sure that all TLBs corresponding to the current VMID are >> flushed >> + * before using it. >> + */ >> + p2m_flush_tlb(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_vttbr[idx] == INVALID_VTTBR ) >> + rc = altp2m_init_helper(d, idx); >> + >> + altp2m_unlock(d); >> + >> + return rc; >> +} >> >> int altp2m_init(struct domain *d) >> { >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index 01a3243..78370c6 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -80,8 +80,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; >> + >> + if ( !altp2m_enabled(d) ) >> + { >> + rc = -EINVAL; >> + break; >> + } >> + >> + ostate = d->arch.altp2m_active; >> + d->arch.altp2m_active = !!a.u.domain_state.state; >> + >> + /* If the alternate p2m state has changed, handle >> appropriately */ >> + if ( (d->arch.altp2m_active != ostate) && >> + (ostate || !(rc = altp2m_init_by_id(d, 0))) ) >> + { >> + for_each_vcpu( d, v ) >> + { >> + if ( !ostate ) >> + altp2m_vcpu_initialise(v); >> + else >> + altp2m_vcpu_destroy(v); >> + } > > The implementation of this hvmop param looks racy to me. What does > prevent to CPU running in this function at the same time? One will > destroy, whilst the other one will initialize. > > It might even be possible to have both doing the initialization > because there is no synchronization barrier for altp2m_active. > >> + >> + /* >> + * The altp2m_active state has been deactivated. It is >> now safe to >> + * flush all altp2m views -- including altp2m[0]. >> + */ >> + if ( ostate ) >> + altp2m_flush(d); > > The function altp2m_flush is defined afterwards (in patch #9). Please > make sure that all the patches compile one by one. > >> + } >> break; >> + } >> >> case HVMOP_altp2m_vcpu_enable_notify: >> rc = -EOPNOTSUPP; >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 29ec5e5..8afea11 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -139,7 +139,7 @@ void p2m_restore_state(struct vcpu *n) >> isb(); >> } >> >> -static void p2m_flush_tlb(struct p2m_domain *p2m) >> +void p2m_flush_tlb(struct p2m_domain *p2m) > > This should ideally be in a separate patch. > >> { >> unsigned long flags = 0; >> uint64_t ovttbr; >> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h >> index 79ea66b..a33c740 100644 >> --- a/xen/include/asm-arm/altp2m.h >> +++ b/xen/include/asm-arm/altp2m.h >> @@ -22,6 +22,8 @@ >> >> #include <xen/sched.h> >> >> +#define INVALID_ALTP2M 0xffff >> + >> #define altp2m_lock(d) spin_lock(&(d)->arch.altp2m_lock) >> #define altp2m_unlock(d) spin_unlock(&(d)->arch.altp2m_lock) >> >> @@ -44,4 +46,17 @@ 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); >> + >> +/* 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 */ >> +static inline void altp2m_flush(struct domain *d) >> +{ >> + /* Not yet implemented. */ >> +} >> + >> #endif /* __ASM_ARM_ALTP2M_H */ >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 3c25ea5..63a9650 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -135,6 +135,12 @@ struct arch_domain >> spinlock_t altp2m_lock; >> } __cacheline_aligned; >> >> +struct altp2mvcpu { >> + uint16_t p2midx; /* alternate p2m index */ >> +}; >> + >> +#define vcpu_altp2m(v) ((v)->arch.avcpu) > > > Please avoid to have half of altp2m defined in altp2m.h and the other > half in domain.h. > >> + >> struct arch_vcpu >> { >> struct { >> @@ -264,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/flushtlb.h >> b/xen/include/asm-arm/flushtlb.h >> index 329fbb4..57c3c34 100644 >> --- a/xen/include/asm-arm/flushtlb.h >> +++ b/xen/include/asm-arm/flushtlb.h >> @@ -2,6 +2,7 @@ >> #define __ASM_ARM_FLUSHTLB_H__ >> >> #include <xen/cpumask.h> >> +#include <asm/p2m.h> >> >> /* >> * Filter the given set of CPUs, removing those that definitely >> flushed their >> @@ -25,6 +26,9 @@ do >> { \ >> /* Flush specified CPUs' TLBs */ >> void flush_tlb_mask(const cpumask_t *mask); >> >> +/* Flush CPU's TLBs for the specified domain */ >> +void p2m_flush_tlb(struct p2m_domain *p2m); >> + > > This function should be declared in p2m.h and not flushtlb.h. > >> #endif /* __ASM_ARM_FLUSHTLB_H__ */ >> /* >> * Local variables: >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index 24a1f61..f13f285 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 >> @@ -86,6 +88,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; >> >> @@ -214,6 +219,12 @@ void guest_physmap_remove_page(struct domain *d, >> >> mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); >> >> +/* Allocates page table for a p2m. */ >> +int p2m_alloc_table(struct p2m_domain *p2m); >> + >> +/* Initialize the p2m structure. */ >> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); > > These declarations belong to the patch that exported them not. Not here. > >> + >> /* Release resources held by the p2m structure. */ >> void p2m_free_one(struct p2m_domain *p2m); >> >> > > Regards, >
(I did not finish answering all questions in the previous mail) Hi Julien, On 08/03/2016 08:41 PM, Julien Grall wrote: > Hello Sergej, > > On 01/08/16 18:10, Sergej Proskurin wrote: >> 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. Note that the function >> altp2m_flush is currently implemented in form of a stub. >> >> 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. >> --- >> xen/arch/arm/altp2m.c | 116 >> +++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/hvm.c | 34 +++++++++++- >> xen/arch/arm/p2m.c | 2 +- >> xen/include/asm-arm/altp2m.h | 15 ++++++ >> xen/include/asm-arm/domain.h | 9 ++++ >> xen/include/asm-arm/flushtlb.h | 4 ++ >> xen/include/asm-arm/p2m.h | 11 ++++ >> 7 files changed, 189 insertions(+), 2 deletions(-) >> >> diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c >> index abbd39a..767f233 100644 >> --- a/xen/arch/arm/altp2m.c >> +++ b/xen/arch/arm/altp2m.c >> @@ -19,6 +19,122 @@ >> >> #include <asm/p2m.h> >> #include <asm/altp2m.h> >> +#include <asm/flushtlb.h> >> + >> +struct p2m_domain *altp2m_get_altp2m(struct vcpu *v) >> +{ >> + unsigned int index = vcpu_altp2m(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 = &vcpu_altp2m(v); >> + >> + av->p2midx = INVALID_ALTP2M; >> +} >> + >> +void altp2m_vcpu_initialise(struct vcpu *v) >> +{ >> + if ( v != current ) >> + vcpu_pause(v); >> + >> + altp2m_vcpu_reset(v); > > I don't understand why you call altp2m_vcpu_reset which will set > p2midx to INVALID_ALTP2M but a line after you set to 0. > It is a leftover from the x86 implementation. Since we do not need to reset further fields, I can remove the call to altp2m_vcpu_reset. >> + vcpu_altp2m(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]; >> + >> + if ( p2m == NULL ) >> + { >> + /* Allocate a new, zeroed altp2m view. */ >> + p2m = xzalloc(struct p2m_domain); >> + if ( p2m == NULL) >> + { >> + rc = -ENOMEM; >> + goto err; >> + } >> + } > > Why don't you re-allocate the p2m from scratch? > The reason is still the reuse of already allocated altp2m's, e.g., within the context of altp2m_propagate_change and altp2m_reset. We have already discussed this in patch #07. >> + >> + /* Initialize the new altp2m view. */ >> + rc = p2m_init_one(d, p2m); >> + if ( rc ) >> + goto err; >> + >> + /* Allocate a root table for the altp2m view. */ >> + rc = p2m_alloc_table(p2m); >> + if ( rc ) >> + goto err; >> + >> + p2m->p2m_class = p2m_alternate; >> + p2m->access_required = 1; > > Please use true here. Although, I am not sure why you want to enable > the access by default. > Will do. p2m->access_required is true by default in the x86 implementation. Also, there is currently no way to manually set access_required on altp2m. Besides, I do not see a scenario, where it makes sense to run altp2m without access_required set to true. >> + _atomic_set(&p2m->active_vcpus, 0); >> + >> + d->arch.altp2m_p2m[idx] = p2m; >> + d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr; >> + >> + /* >> + * Make sure that all TLBs corresponding to the current VMID are >> flushed >> + * before using it. >> + */ >> + p2m_flush_tlb(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_vttbr[idx] == INVALID_VTTBR ) >> + rc = altp2m_init_helper(d, idx); >> + >> + altp2m_unlock(d); >> + >> + return rc; >> +} >> >> int altp2m_init(struct domain *d) >> { >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c >> index 01a3243..78370c6 100644 >> --- a/xen/arch/arm/hvm.c >> +++ b/xen/arch/arm/hvm.c >> @@ -80,8 +80,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; >> + >> + if ( !altp2m_enabled(d) ) >> + { >> + rc = -EINVAL; >> + break; >> + } >> + >> + ostate = d->arch.altp2m_active; >> + d->arch.altp2m_active = !!a.u.domain_state.state; >> + >> + /* If the alternate p2m state has changed, handle >> appropriately */ >> + if ( (d->arch.altp2m_active != ostate) && >> + (ostate || !(rc = altp2m_init_by_id(d, 0))) ) >> + { >> + for_each_vcpu( d, v ) >> + { >> + if ( !ostate ) >> + altp2m_vcpu_initialise(v); >> + else >> + altp2m_vcpu_destroy(v); >> + } > > The implementation of this hvmop param looks racy to me. What does > prevent to CPU running in this function at the same time? One will > destroy, whilst the other one will initialize. > > It might even be possible to have both doing the initialization > because there is no synchronization barrier for altp2m_active. We have discussed this issue in patch #01. > >> + >> + /* >> + * The altp2m_active state has been deactivated. It is >> now safe to >> + * flush all altp2m views -- including altp2m[0]. >> + */ >> + if ( ostate ) >> + altp2m_flush(d); > > The function altp2m_flush is defined afterwards (in patch #9). Please > make sure that all the patches compile one by one. > The patches compile one by one. Please note that there is an altp2m_flush stub inside of this patch. +/* Flush all the alternate p2m's for a domain */ +static inline void altp2m_flush(struct domain *d) +{ + /* Not yet implemented. */ +} >> + } >> break; >> + } >> >> case HVMOP_altp2m_vcpu_enable_notify: >> rc = -EOPNOTSUPP; >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index 29ec5e5..8afea11 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -139,7 +139,7 @@ void p2m_restore_state(struct vcpu *n) >> isb(); >> } >> >> -static void p2m_flush_tlb(struct p2m_domain *p2m) >> +void p2m_flush_tlb(struct p2m_domain *p2m) > > This should ideally be in a separate patch. > Ok. >> { >> unsigned long flags = 0; >> uint64_t ovttbr; >> diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h >> index 79ea66b..a33c740 100644 >> --- a/xen/include/asm-arm/altp2m.h >> +++ b/xen/include/asm-arm/altp2m.h >> @@ -22,6 +22,8 @@ >> >> #include <xen/sched.h> >> >> +#define INVALID_ALTP2M 0xffff >> + >> #define altp2m_lock(d) spin_lock(&(d)->arch.altp2m_lock) >> #define altp2m_unlock(d) spin_unlock(&(d)->arch.altp2m_lock) >> >> @@ -44,4 +46,17 @@ 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); >> + >> +/* 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 */ >> +static inline void altp2m_flush(struct domain *d) >> +{ >> + /* Not yet implemented. */ >> +} >> + >> #endif /* __ASM_ARM_ALTP2M_H */ >> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h >> index 3c25ea5..63a9650 100644 >> --- a/xen/include/asm-arm/domain.h >> +++ b/xen/include/asm-arm/domain.h >> @@ -135,6 +135,12 @@ struct arch_domain >> spinlock_t altp2m_lock; >> } __cacheline_aligned; >> >> +struct altp2mvcpu { >> + uint16_t p2midx; /* alternate p2m index */ >> +}; >> + >> +#define vcpu_altp2m(v) ((v)->arch.avcpu) > > > Please avoid to have half of altp2m defined in altp2m.h and the other > half in domain.h. > Ok, thank you. >> + >> struct arch_vcpu >> { >> struct { >> @@ -264,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/flushtlb.h >> b/xen/include/asm-arm/flushtlb.h >> index 329fbb4..57c3c34 100644 >> --- a/xen/include/asm-arm/flushtlb.h >> +++ b/xen/include/asm-arm/flushtlb.h >> @@ -2,6 +2,7 @@ >> #define __ASM_ARM_FLUSHTLB_H__ >> >> #include <xen/cpumask.h> >> +#include <asm/p2m.h> >> >> /* >> * Filter the given set of CPUs, removing those that definitely >> flushed their >> @@ -25,6 +26,9 @@ do >> { \ >> /* Flush specified CPUs' TLBs */ >> void flush_tlb_mask(const cpumask_t *mask); >> >> +/* Flush CPU's TLBs for the specified domain */ >> +void p2m_flush_tlb(struct p2m_domain *p2m); >> + > > This function should be declared in p2m.h and not flushtlb.h. > Ok, thank you. >> #endif /* __ASM_ARM_FLUSHTLB_H__ */ >> /* >> * Local variables: >> diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h >> index 24a1f61..f13f285 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 >> @@ -86,6 +88,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; >> >> @@ -214,6 +219,12 @@ void guest_physmap_remove_page(struct domain *d, >> >> mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); >> >> +/* Allocates page table for a p2m. */ >> +int p2m_alloc_table(struct p2m_domain *p2m); >> + >> +/* Initialize the p2m structure. */ >> +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); > > These declarations belong to the patch that exported them not. Not here. > I will change that. >> + >> /* Release resources held by the p2m structure. */ >> void p2m_free_one(struct p2m_domain *p2m); >> >> > Best regards, ~Sergej
Hello Sergej, On 06/08/2016 10:36, Sergej Proskurin wrote: > (I did not finish answering all questions in the previous mail) > On 08/03/2016 08:41 PM, Julien Grall wrote: >> On 01/08/16 18:10, Sergej Proskurin wrote: [...] >>> + >>> + /* Initialize the new altp2m view. */ >>> + rc = p2m_init_one(d, p2m); >>> + if ( rc ) >>> + goto err; >>> + >>> + /* Allocate a root table for the altp2m view. */ >>> + rc = p2m_alloc_table(p2m); >>> + if ( rc ) >>> + goto err; >>> + >>> + p2m->p2m_class = p2m_alternate; >>> + p2m->access_required = 1; >> >> Please use true here. Although, I am not sure why you want to enable >> the access by default. >> > > Will do. > > p2m->access_required is true by default in the x86 implementation. Also, > there is currently no way to manually set access_required on altp2m. > Besides, I do not see a scenario, where it makes sense to run altp2m > without access_required set to true. I am afraid to say that "x86 does it" is not an argument. When I am reading an ARM series I don't necessary look at the x86, which by way does not give any explanation which it set to true by default. Please document it in the code. Regards,
Hello Sergej, On 06/08/2016 10:36, Sergej Proskurin wrote: > (I did not finish answering all questions in the previous mail) > On 08/03/2016 08:41 PM, Julien Grall wrote: >> On 01/08/16 18:10, Sergej Proskurin wrote: [...] >>> + >>> + /* Initialize the new altp2m view. */ >>> + rc = p2m_init_one(d, p2m); >>> + if ( rc ) >>> + goto err; >>> + >>> + /* Allocate a root table for the altp2m view. */ >>> + rc = p2m_alloc_table(p2m); >>> + if ( rc ) >>> + goto err; >>> + >>> + p2m->p2m_class = p2m_alternate; >>> + p2m->access_required = 1; >> >> Please use true here. Although, I am not sure why you want to enable >> the access by default. >> > > Will do. > > p2m->access_required is true by default in the x86 implementation. Also, > there is currently no way to manually set access_required on altp2m. > Besides, I do not see a scenario, where it makes sense to run altp2m > without access_required set to true. I am afraid to say that "x86 does it" is not an argument. When I am reading an ARM series I don't necessary look at the x86, which by way does not give any explanation which it set to true by default. Please document it in the code. Regards,
Hello Sergej, On 06/08/2016 11:36, Sergej Proskurin wrote: >>> + >>> + /* Initialize the new altp2m view. */ >>> + rc = p2m_init_one(d, p2m); >>> + if ( rc ) >>> + goto err; >>> + >>> + /* Allocate a root table for the altp2m view. */ >>> + rc = p2m_alloc_table(p2m); >>> + if ( rc ) >>> + goto err; >>> + >>> + p2m->p2m_class = p2m_alternate; >>> + p2m->access_required = 1; >> >> Please use true here. Although, I am not sure why you want to enable >> the access by default. >> > > Will do. > > p2m->access_required is true by default in the x86 implementation. Also, > there is currently no way to manually set access_required on altp2m. > Besides, I do not see a scenario, where it makes sense to run altp2m > without access_required set to true. Please add a comment in the code to explain it. [...] >> >>> + >>> + /* >>> + * The altp2m_active state has been deactivated. It is >>> now safe to >>> + * flush all altp2m views -- including altp2m[0]. >>> + */ >>> + if ( ostate ) >>> + altp2m_flush(d); >> >> The function altp2m_flush is defined afterwards (in patch #9). Please >> make sure that all the patches compile one by one. >> > > The patches compile one by one. Please note that there is an > altp2m_flush stub inside of this patch. > > +/* Flush all the alternate p2m's for a domain */ > +static inline void altp2m_flush(struct domain *d) > +{ > + /* Not yet implemented. */ > +} I don't want to see stubs that are been replaced later on within the same series. The patch #9 does not seem to depend on patch #8, so I don't see any reason why you can't swap the 2 patches. Regards,
diff --git a/xen/arch/arm/altp2m.c b/xen/arch/arm/altp2m.c index abbd39a..767f233 100644 --- a/xen/arch/arm/altp2m.c +++ b/xen/arch/arm/altp2m.c @@ -19,6 +19,122 @@ #include <asm/p2m.h> #include <asm/altp2m.h> +#include <asm/flushtlb.h> + +struct p2m_domain *altp2m_get_altp2m(struct vcpu *v) +{ + unsigned int index = vcpu_altp2m(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 = &vcpu_altp2m(v); + + av->p2midx = INVALID_ALTP2M; +} + +void altp2m_vcpu_initialise(struct vcpu *v) +{ + if ( v != current ) + vcpu_pause(v); + + altp2m_vcpu_reset(v); + vcpu_altp2m(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]; + + if ( p2m == NULL ) + { + /* Allocate a new, zeroed altp2m view. */ + p2m = xzalloc(struct p2m_domain); + if ( p2m == NULL) + { + rc = -ENOMEM; + goto err; + } + } + + /* Initialize the new altp2m view. */ + rc = p2m_init_one(d, p2m); + if ( rc ) + goto err; + + /* Allocate a root table for the altp2m view. */ + rc = p2m_alloc_table(p2m); + if ( rc ) + goto err; + + p2m->p2m_class = p2m_alternate; + p2m->access_required = 1; + _atomic_set(&p2m->active_vcpus, 0); + + d->arch.altp2m_p2m[idx] = p2m; + d->arch.altp2m_vttbr[idx] = p2m->vttbr.vttbr; + + /* + * Make sure that all TLBs corresponding to the current VMID are flushed + * before using it. + */ + p2m_flush_tlb(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_vttbr[idx] == INVALID_VTTBR ) + rc = altp2m_init_helper(d, idx); + + altp2m_unlock(d); + + return rc; +} int altp2m_init(struct domain *d) { diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c index 01a3243..78370c6 100644 --- a/xen/arch/arm/hvm.c +++ b/xen/arch/arm/hvm.c @@ -80,8 +80,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; + + if ( !altp2m_enabled(d) ) + { + rc = -EINVAL; + break; + } + + ostate = d->arch.altp2m_active; + d->arch.altp2m_active = !!a.u.domain_state.state; + + /* If the alternate p2m state has changed, handle appropriately */ + if ( (d->arch.altp2m_active != ostate) && + (ostate || !(rc = altp2m_init_by_id(d, 0))) ) + { + for_each_vcpu( d, v ) + { + if ( !ostate ) + altp2m_vcpu_initialise(v); + else + 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/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 29ec5e5..8afea11 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -139,7 +139,7 @@ void p2m_restore_state(struct vcpu *n) isb(); } -static void p2m_flush_tlb(struct p2m_domain *p2m) +void p2m_flush_tlb(struct p2m_domain *p2m) { unsigned long flags = 0; uint64_t ovttbr; diff --git a/xen/include/asm-arm/altp2m.h b/xen/include/asm-arm/altp2m.h index 79ea66b..a33c740 100644 --- a/xen/include/asm-arm/altp2m.h +++ b/xen/include/asm-arm/altp2m.h @@ -22,6 +22,8 @@ #include <xen/sched.h> +#define INVALID_ALTP2M 0xffff + #define altp2m_lock(d) spin_lock(&(d)->arch.altp2m_lock) #define altp2m_unlock(d) spin_unlock(&(d)->arch.altp2m_lock) @@ -44,4 +46,17 @@ 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); + +/* 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 */ +static inline void altp2m_flush(struct domain *d) +{ + /* Not yet implemented. */ +} + #endif /* __ASM_ARM_ALTP2M_H */ diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 3c25ea5..63a9650 100644 --- a/xen/include/asm-arm/domain.h +++ b/xen/include/asm-arm/domain.h @@ -135,6 +135,12 @@ struct arch_domain spinlock_t altp2m_lock; } __cacheline_aligned; +struct altp2mvcpu { + uint16_t p2midx; /* alternate p2m index */ +}; + +#define vcpu_altp2m(v) ((v)->arch.avcpu) + struct arch_vcpu { struct { @@ -264,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/flushtlb.h b/xen/include/asm-arm/flushtlb.h index 329fbb4..57c3c34 100644 --- a/xen/include/asm-arm/flushtlb.h +++ b/xen/include/asm-arm/flushtlb.h @@ -2,6 +2,7 @@ #define __ASM_ARM_FLUSHTLB_H__ #include <xen/cpumask.h> +#include <asm/p2m.h> /* * Filter the given set of CPUs, removing those that definitely flushed their @@ -25,6 +26,9 @@ do { \ /* Flush specified CPUs' TLBs */ void flush_tlb_mask(const cpumask_t *mask); +/* Flush CPU's TLBs for the specified domain */ +void p2m_flush_tlb(struct p2m_domain *p2m); + #endif /* __ASM_ARM_FLUSHTLB_H__ */ /* * Local variables: diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 24a1f61..f13f285 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 @@ -86,6 +88,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; @@ -214,6 +219,12 @@ void guest_physmap_remove_page(struct domain *d, mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); +/* Allocates page table for a p2m. */ +int p2m_alloc_table(struct p2m_domain *p2m); + +/* Initialize the p2m structure. */ +int p2m_init_one(struct domain *d, struct p2m_domain *p2m); + /* Release resources held by the p2m structure. */ void p2m_free_one(struct p2m_domain *p2m);
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. Note that the function altp2m_flush is currently implemented in form of a stub. 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. --- xen/arch/arm/altp2m.c | 116 +++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/hvm.c | 34 +++++++++++- xen/arch/arm/p2m.c | 2 +- xen/include/asm-arm/altp2m.h | 15 ++++++ xen/include/asm-arm/domain.h | 9 ++++ xen/include/asm-arm/flushtlb.h | 4 ++ xen/include/asm-arm/p2m.h | 11 ++++ 7 files changed, 189 insertions(+), 2 deletions(-)