[PATCH] xen: arm: correctly configure NSACR.

[PATCH] xen: arm: correctly configure NSACR.

[Ian Campbell]

From: Ian Campbell <ian.campbell@citrix.com>

Previously we were setting it up twice, the second time neglecting to set the
NS_SMP bit.

NSACR.NS_SMP is a processor specific bit which on Cortex-A7 and -A15 regulates
access to the (also processor specific) ACTLR.SMP bit. Not setting NSACR.NS_SMP
meant that Xen's attempts to set ACTLR.SMP was silently ignored. Setting this
bit is required in order to cause the processor to take part in cache and TLB
coherency protocols. Failure to set this bit leads to random memory corruption
in guests (although nothing like as catestrophic as you might expect!).

An alternative fix would have been to set ACTLR.SMP when in Secure World,
however Linux expects to set ACTLR.SMP itself in NS mode, so it's a good bet
that bootloaders will set NSACR.NS_SMP instead.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/mode_switch.S | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm32/mode_switch.S b/xen/arch/arm/arm32/mode_switch.S
index c92a1cf..c18251f 100644
--- a/xen/arch/arm/arm32/mode_switch.S
+++ b/xen/arch/arm/arm32/mode_switch.S
@@ -104,7 +104,8 @@ enter_hyp_mode:
          * memory-mapped control registers live, we can't find out the
          * right frequency. */
         mcr   CP32(r0, CNTFRQ)
-        ldr   r0, =0x40c00           /* SMP, c11, c10 in non-secure mode */
+        ldr   r0, =0x3fff            /* Allow access to all co-processors in NS mode */
+        orr   r0, r0, #(1<<18)       /* CA7/CA15: Allow access to ACTLR.SMP in NS mode */
         mcr   CP32(r0, NSACR)
 
         add   r0, r1, #GIC_DR_OFFSET
@@ -143,9 +144,6 @@ skip_spis:
         mov   r0, #0
         mcr   CP32(r0, FCSEIDR)
         mcr   CP32(r0, CONTEXTIDR)
-        /* Allow non-secure access to coprocessors, FIQs, VFP and NEON */
-        ldr   r1, =0x3fff            /* 14 CP bits set, all others clear */
-        mcr   CP32(r1, NSACR)
 
         mrs   r0, cpsr               /* Copy the CPSR */
         add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
-- 
1.8.3.2

[PATCH v2] xen: arm: correctly configure NSACR.

[Ian Campbell]

From: Ian Campbell <ian.campbell@citrix.com>

Previously we were setting it up twice, the second time neglecting to set the
NS_SMP bit.

NSACR.NS_SMP is a processor specific bit which on Cortex-A7 and -A15 regulates
access to the (also processor specific) ACTLR.SMP bit. Not setting NSACR.NS_SMP
meant that Xen's attempts to set ACTLR.SMP was silently ignored. Setting this
bit is required in order to cause the processor to take part in cache and TLB
coherency protocols. Failure to set this bit leads to random memory corruption
in guests (although nothing like as catestrophic as you might expect!).

An alternative fix would have been to set ACTLR.SMP when in Secure World,
however Linux expects to set ACTLR.SMP itself in NS mode, so it's a good bet
that bootloaders will set NSACR.NS_SMP instead.

While here switch to a read-modify-write of NSACR to preserve any existing bits
-- seems safer.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v2: Modify rather than overwrite NSACR.
---
 xen/arch/arm/arm32/mode_switch.S | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/arm32/mode_switch.S b/xen/arch/arm/arm32/mode_switch.S
index c92a1cf..3500eb0 100644
--- a/xen/arch/arm/arm32/mode_switch.S
+++ b/xen/arch/arm/arm32/mode_switch.S
@@ -104,7 +104,11 @@ enter_hyp_mode:
          * memory-mapped control registers live, we can't find out the
          * right frequency. */
         mcr   CP32(r0, CNTFRQ)
-        ldr   r0, =0x40c00           /* SMP, c11, c10 in non-secure mode */
+
+        mrc   CP32(r0,NSACR)
+        ldr   r4, =0x3fff            /* Allow access to all co-processors in NS mode */
+        orr   r0, r0, r4
+        orr   r0, r0, #(1<<18)       /* CA7/CA15: Allow access to ACTLR.SMP in NS mode */
         mcr   CP32(r0, NSACR)
 
         add   r0, r1, #GIC_DR_OFFSET
@@ -143,9 +147,6 @@ skip_spis:
         mov   r0, #0
         mcr   CP32(r0, FCSEIDR)
         mcr   CP32(r0, CONTEXTIDR)
-        /* Allow non-secure access to coprocessors, FIQs, VFP and NEON */
-        ldr   r1, =0x3fff            /* 14 CP bits set, all others clear */
-        mcr   CP32(r1, NSACR)
 
         mrs   r0, cpsr               /* Copy the CPSR */
         add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
-- 
1.8.3.2

Re: [PATCH v2] xen: arm: correctly configure NSACR.

[Stefano Stabellini]

On Mon, 15 Jul 2013, Ian Campbell wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> Previously we were setting it up twice, the second time neglecting to set the
> NS_SMP bit.
> 
> NSACR.NS_SMP is a processor specific bit which on Cortex-A7 and -A15 regulates
> access to the (also processor specific) ACTLR.SMP bit. Not setting NSACR.NS_SMP
> meant that Xen's attempts to set ACTLR.SMP was silently ignored. Setting this
> bit is required in order to cause the processor to take part in cache and TLB
> coherency protocols. Failure to set this bit leads to random memory corruption
> in guests (although nothing like as catestrophic as you might expect!).
> 
> An alternative fix would have been to set ACTLR.SMP when in Secure World,
> however Linux expects to set ACTLR.SMP itself in NS mode, so it's a good bet
> that bootloaders will set NSACR.NS_SMP instead.
> 
> While here switch to a read-modify-write of NSACR to preserve any existing bits
> -- seems safer.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Modify rather than overwrite NSACR.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  xen/arch/arm/arm32/mode_switch.S | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/mode_switch.S b/xen/arch/arm/arm32/mode_switch.S
> index c92a1cf..3500eb0 100644
> --- a/xen/arch/arm/arm32/mode_switch.S
> +++ b/xen/arch/arm/arm32/mode_switch.S
> @@ -104,7 +104,11 @@ enter_hyp_mode:
>           * memory-mapped control registers live, we can't find out the
>           * right frequency. */
>          mcr   CP32(r0, CNTFRQ)
> -        ldr   r0, =0x40c00           /* SMP, c11, c10 in non-secure mode */
> +
> +        mrc   CP32(r0,NSACR)
> +        ldr   r4, =0x3fff            /* Allow access to all co-processors in NS mode */
> +        orr   r0, r0, r4
> +        orr   r0, r0, #(1<<18)       /* CA7/CA15: Allow access to ACTLR.SMP in NS mode */
>          mcr   CP32(r0, NSACR)
>  
>          add   r0, r1, #GIC_DR_OFFSET
> @@ -143,9 +147,6 @@ skip_spis:
>          mov   r0, #0
>          mcr   CP32(r0, FCSEIDR)
>          mcr   CP32(r0, CONTEXTIDR)
> -        /* Allow non-secure access to coprocessors, FIQs, VFP and NEON */
> -        ldr   r1, =0x3fff            /* 14 CP bits set, all others clear */
> -        mcr   CP32(r1, NSACR)
>  
>          mrs   r0, cpsr               /* Copy the CPSR */
>          add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
> -- 
> 1.8.3.2
> 

Re: [PATCH v2] xen: arm: correctly configure NSACR.

[Julien Grall]

On 07/15/2013 12:22 PM, Stefano Stabellini wrote:
> On Mon, 15 Jul 2013, Ian Campbell wrote:
>> From: Ian Campbell <ian.campbell@citrix.com>
>>
>> Previously we were setting it up twice, the second time neglecting to set the
>> NS_SMP bit.
>>
>> NSACR.NS_SMP is a processor specific bit which on Cortex-A7 and -A15 regulates
>> access to the (also processor specific) ACTLR.SMP bit. Not setting NSACR.NS_SMP
>> meant that Xen's attempts to set ACTLR.SMP was silently ignored. Setting this
>> bit is required in order to cause the processor to take part in cache and TLB
>> coherency protocols. Failure to set this bit leads to random memory corruption
>> in guests (although nothing like as catestrophic as you might expect!).
                                          ^
                                       catastrophic
>>
>> An alternative fix would have been to set ACTLR.SMP when in Secure World,
>> however Linux expects to set ACTLR.SMP itself in NS mode, so it's a good bet
>> that bootloaders will set NSACR.NS_SMP instead.
>>
>> While here switch to a read-modify-write of NSACR to preserve any existing bits
>> -- seems safer.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> ---
>> v2: Modify rather than overwrite NSACR.
> 
> Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Except the minor typo:
Acked-by: Julien Grall <julien.grall@linaro.org>

> 
>>  xen/arch/arm/arm32/mode_switch.S | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/mode_switch.S b/xen/arch/arm/arm32/mode_switch.S
>> index c92a1cf..3500eb0 100644
>> --- a/xen/arch/arm/arm32/mode_switch.S
>> +++ b/xen/arch/arm/arm32/mode_switch.S
>> @@ -104,7 +104,11 @@ enter_hyp_mode:
>>           * memory-mapped control registers live, we can't find out the
>>           * right frequency. */
>>          mcr   CP32(r0, CNTFRQ)
>> -        ldr   r0, =0x40c00           /* SMP, c11, c10 in non-secure mode */
>> +
>> +        mrc   CP32(r0,NSACR)
>> +        ldr   r4, =0x3fff            /* Allow access to all co-processors in NS mode */
>> +        orr   r0, r0, r4
>> +        orr   r0, r0, #(1<<18)       /* CA7/CA15: Allow access to ACTLR.SMP in NS mode */
>>          mcr   CP32(r0, NSACR)
>>  
>>          add   r0, r1, #GIC_DR_OFFSET
>> @@ -143,9 +147,6 @@ skip_spis:
>>          mov   r0, #0
>>          mcr   CP32(r0, FCSEIDR)
>>          mcr   CP32(r0, CONTEXTIDR)
>> -        /* Allow non-secure access to coprocessors, FIQs, VFP and NEON */
>> -        ldr   r1, =0x3fff            /* 14 CP bits set, all others clear */
>> -        mcr   CP32(r1, NSACR)
>>  
>>          mrs   r0, cpsr               /* Copy the CPSR */
>>          add   r0, r0, #0x4           /* 0x16 (Monitor) -> 0x1a (Hyp) */
>> -- 
>> 1.8.3.2
>>
> 

Re: [PATCH v2] xen: arm: correctly configure NSACR.

[Ian Campbell]

On Mon, 2013-07-15 at 14:00 +0100, Julien Grall wrote:
> On 07/15/2013 12:22 PM, Stefano Stabellini wrote:
> > On Mon, 15 Jul 2013, Ian Campbell wrote:
> >> From: Ian Campbell <ian.campbell@citrix.com>
> >>
> >> Previously we were setting it up twice, the second time neglecting to set the
> >> NS_SMP bit.
> >>
> >> NSACR.NS_SMP is a processor specific bit which on Cortex-A7 and -A15 regulates
> >> access to the (also processor specific) ACTLR.SMP bit. Not setting NSACR.NS_SMP
> >> meant that Xen's attempts to set ACTLR.SMP was silently ignored. Setting this
> >> bit is required in order to cause the processor to take part in cache and TLB
> >> coherency protocols. Failure to set this bit leads to random memory corruption
> >> in guests (although nothing like as catestrophic as you might expect!).
>                                           ^
>                                        catastrophic

I knew being a spelling pendant would eventually come back and bite
me ;-)

> >>
> >> An alternative fix would have been to set ACTLR.SMP when in Secure World,
> >> however Linux expects to set ACTLR.SMP itself in NS mode, so it's a good bet
> >> that bootloaders will set NSACR.NS_SMP instead.
> >>
> >> While here switch to a read-modify-write of NSACR to preserve any existing bits
> >> -- seems safer.
> >>
> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >> ---
> >> v2: Modify rather than overwrite NSACR.
> > 
> > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Except the minor typo:
> Acked-by: Julien Grall <julien.grall@linaro.org>

Typo fixed + applied, thanks.