| Message ID | 1372950066-14379-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Thu, 2013-07-04 at 16:01 +0100, Julien Grall wrote: > @@ -452,6 +451,12 @@ int vcpu_initialise(struct vcpu *v) > return rc; > > v->arch.sctlr = SCTLR_BASE; > + v->arch.actlr = READ_SYSREG32(ACTLR_EL1); > + /* XXX: Handle other than CA15 cpus */ > + if ( v->domain->max_vcpus > 1 ) > + v->arch.actlr |= ACTLR_CA15_SMP; > + else > + v->arch.actlr &= ~ACTLR_CA15_SMP; I'm not 100% sure about this last bit. A CONFIG_SMP=y kernel will want to set this regardless of there only being one processor since the Cortex A15 (and now A7) are both MPCore processors. Granted they don't actually check or panic etc when we ignore them, so maybe it doesn't matter. I'd be tempted to simply expose the raw underling ACTLR_EL1 to guests (r/o of course), it neatly sidesteps any need to know about specific processors too. Anyone got any other thoughts? > > if ( (rc = vcpu_vgic_init(v)) != 0 ) > return rc; > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > index 398d209..bbd60aa 100644 > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -62,7 +62,8 @@ void __cpuinit init_traps(void) > WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2); > > /* Setup hypervisor traps */ > - WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI|HCR_TSC, HCR_EL2); > + WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI|HCR_TSC| > + HCR_TAC, HCR_EL2); > isb(); > } > > @@ -836,6 +837,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, > { > struct hsr_cp32 cp32 = hsr.cp32; > uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg); > + struct vcpu *v = current; > > if ( !cp32.ccvalid ) { > dprintk(XENLOG_ERR, "cp_15(32): need to handle invalid condition codes\n"); > @@ -889,6 +891,10 @@ static void do_cp15_32(struct cpu_user_regs *regs, > domain_crash_synchronous(); > } > break; > + case HSR_CPREG32(ACTLR): > + if ( cp32.read ) > + *r = v->arch.actlr; > + break; > default: > printk("%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n", > cp32.read ? "mrc" : "mcr",
On Wed, 17 Jul 2013, Ian Campbell wrote: > On Thu, 2013-07-04 at 16:01 +0100, Julien Grall wrote: > > @@ -452,6 +451,12 @@ int vcpu_initialise(struct vcpu *v) > > return rc; > > > > v->arch.sctlr = SCTLR_BASE; > > + v->arch.actlr = READ_SYSREG32(ACTLR_EL1); > > + /* XXX: Handle other than CA15 cpus */ > > + if ( v->domain->max_vcpus > 1 ) > > + v->arch.actlr |= ACTLR_CA15_SMP; > > + else > > + v->arch.actlr &= ~ACTLR_CA15_SMP; > > I'm not 100% sure about this last bit. A CONFIG_SMP=y kernel will want > to set this regardless of there only being one processor since the > Cortex A15 (and now A7) are both MPCore processors. Granted they don't > actually check or panic etc when we ignore them, so maybe it doesn't > matter. > > I'd be tempted to simply expose the raw underling ACTLR_EL1 to guests > (r/o of course), it neatly sidesteps any need to know about specific > processors too. Anyone got any other thoughts? The only concern would be about migration or big.LITTLE machines, where at some point the value of the ACTLR (if it's even present) would suddenly change underneath Linux's feet. I don't think that would cause problems, but it's worth keeping it in mind. > > > > if ( (rc = vcpu_vgic_init(v)) != 0 ) > > return rc; > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c > > index 398d209..bbd60aa 100644 > > --- a/xen/arch/arm/traps.c > > +++ b/xen/arch/arm/traps.c > > @@ -62,7 +62,8 @@ void __cpuinit init_traps(void) > > WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2); > > > > /* Setup hypervisor traps */ > > - WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI|HCR_TSC, HCR_EL2); > > + WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI|HCR_TSC| > > + HCR_TAC, HCR_EL2); > > isb(); > > } > > > > @@ -836,6 +837,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, > > { > > struct hsr_cp32 cp32 = hsr.cp32; > > uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg); > > + struct vcpu *v = current; > > > > if ( !cp32.ccvalid ) { > > dprintk(XENLOG_ERR, "cp_15(32): need to handle invalid condition codes\n"); > > @@ -889,6 +891,10 @@ static void do_cp15_32(struct cpu_user_regs *regs, > > domain_crash_synchronous(); > > } > > break; > > + case HSR_CPREG32(ACTLR): > > + if ( cp32.read ) > > + *r = v->arch.actlr; > > + break; > > default: > > printk("%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n", > > cp32.read ? "mrc" : "mcr", > >
On Wed, 2013-07-17 at 14:57 +0100, Stefano Stabellini wrote: > On Wed, 17 Jul 2013, Ian Campbell wrote: > > On Thu, 2013-07-04 at 16:01 +0100, Julien Grall wrote: > > > @@ -452,6 +451,12 @@ int vcpu_initialise(struct vcpu *v) > > > return rc; > > > > > > v->arch.sctlr = SCTLR_BASE; > > > + v->arch.actlr = READ_SYSREG32(ACTLR_EL1); > > > + /* XXX: Handle other than CA15 cpus */ > > > + if ( v->domain->max_vcpus > 1 ) > > > + v->arch.actlr |= ACTLR_CA15_SMP; > > > + else > > > + v->arch.actlr &= ~ACTLR_CA15_SMP; > > > > I'm not 100% sure about this last bit. A CONFIG_SMP=y kernel will want > > to set this regardless of there only being one processor since the > > Cortex A15 (and now A7) are both MPCore processors. Granted they don't > > actually check or panic etc when we ignore them, so maybe it doesn't > > matter. > > > > I'd be tempted to simply expose the raw underling ACTLR_EL1 to guests > > (r/o of course), it neatly sidesteps any need to know about specific > > processors too. Anyone got any other thoughts? > > The only concern would be about migration or big.LITTLE machines, where > at some point the value of the ACTLR (if it's even present) would > suddenly change underneath Linux's feet. > I don't think that would cause problems, but it's worth keeping it in > mind. OK. so I've acked + applied this one, we can always redo it later when we start to investigate these use cases. It occurs to me now that the behaviour we have (statically configuring the SMP bit) is no different to the situation where the Secure World firmware has configured it and not set the NSACR.NS_SMP bit, so its not unexpected for the kernels attempts to change it to silently be ignored. Ian.
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c index f465ab7..6937abf 100644 --- a/xen/arch/arm/domain.c +++ b/xen/arch/arm/domain.c @@ -28,6 +28,7 @@ #include <asm/irq.h> #include <asm/cpufeature.h> #include <asm/vfp.h> +#include <asm/processor-ca15.h> #include <asm/gic.h> #include "vtimer.h" @@ -61,7 +62,6 @@ static void ctxt_switch_from(struct vcpu *p) p->arch.csselr = READ_SYSREG(CSSELR_EL1); /* Control Registers */ - p->arch.actlr = READ_SYSREG(ACTLR_EL1); p->arch.sctlr = READ_SYSREG(SCTLR_EL1); p->arch.cpacr = READ_SYSREG(CPACR_EL1); @@ -182,7 +182,6 @@ static void ctxt_switch_to(struct vcpu *n) isb(); /* Control Registers */ - WRITE_SYSREG(n->arch.actlr, ACTLR_EL1); WRITE_SYSREG(n->arch.sctlr, SCTLR_EL1); WRITE_SYSREG(n->arch.cpacr, CPACR_EL1); @@ -452,6 +451,12 @@ int vcpu_initialise(struct vcpu *v) return rc; v->arch.sctlr = SCTLR_BASE; + v->arch.actlr = READ_SYSREG32(ACTLR_EL1); + /* XXX: Handle other than CA15 cpus */ + if ( v->domain->max_vcpus > 1 ) + v->arch.actlr |= ACTLR_CA15_SMP; + else + v->arch.actlr &= ~ACTLR_CA15_SMP; if ( (rc = vcpu_vgic_init(v)) != 0 ) return rc; diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 398d209..bbd60aa 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -62,7 +62,8 @@ void __cpuinit init_traps(void) WRITE_SYSREG((vaddr_t)hyp_traps_vector, VBAR_EL2); /* Setup hypervisor traps */ - WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI|HCR_TSC, HCR_EL2); + WRITE_SYSREG(HCR_PTW|HCR_BSU_OUTER|HCR_AMO|HCR_IMO|HCR_VM|HCR_TWI|HCR_TSC| + HCR_TAC, HCR_EL2); isb(); } @@ -836,6 +837,7 @@ static void do_cp15_32(struct cpu_user_regs *regs, { struct hsr_cp32 cp32 = hsr.cp32; uint32_t *r = (uint32_t*)select_user_reg(regs, cp32.reg); + struct vcpu *v = current; if ( !cp32.ccvalid ) { dprintk(XENLOG_ERR, "cp_15(32): need to handle invalid condition codes\n"); @@ -889,6 +891,10 @@ static void do_cp15_32(struct cpu_user_regs *regs, domain_crash_synchronous(); } break; + case HSR_CPREG32(ACTLR): + if ( cp32.read ) + *r = v->arch.actlr; + break; default: printk("%s p15, %d, r%d, cr%d, cr%d, %d @ 0x%"PRIregister"\n", cp32.read ? "mrc" : "mcr",
On Cortex-A15 ACTLR is used to set the SMP bit. If the guest has the control on this register, it can disable SMP support and so TLB broadcast. Implement the access to ACTRL as read-only register with SMP bit set to one if the guest has multiple VCPUs. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/domain.c | 9 +++++++-- xen/arch/arm/traps.c | 8 +++++++- 2 files changed, 14 insertions(+), 3 deletions(-)