[PATCH] xen/arm: Trap the ACTLR register

[PATCH] xen/arm: Trap the ACTLR register

[Julien Grall]

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(-)

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",
-- 
1.7.10.4

Re: [PATCH] xen/arm: Trap the ACTLR register

[Ian Campbell]

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",

Re: [PATCH] xen/arm: Trap the ACTLR register

[Stefano Stabellini]

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",
> 
> 

Re: [PATCH] xen/arm: Trap the ACTLR register

[Ian Campbell]

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.