diff mbox series

target/arm: Implement NSACR gating of floating point

Message ID 20190411153942.4533-1-peter.maydell@linaro.org
State Superseded
Headers show
Series target/arm: Implement NSACR gating of floating point | expand

Commit Message

Peter Maydell April 11, 2019, 3:39 p.m. UTC 
The NSACR register allows secure code to configure the FPU
to be inaccessible to non-secure code. If the NSACR.CP10
bit is set then:
 * NS accesses to the FPU trap as UNDEF (ie to NS EL1 or EL2)
 * CPACR.{CP10,CP11} behave as if RAZ/WI
 * HCPTR.{TCP11,TCP10} behave as if RAO/WI

Note that we do not implement the NSACR.NSASEDIS bit which
gates only access to Advanced SIMD, in the same way that
we don't implement the equivalent CPACR.ASEDIS and HCPTR.TASE.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

---
I noticed we were missing NSACR handling here while looking at
M profile floating point, but it turns out that M profile NSACR
isn't really the same, so the two don't share code. This patch
fixes the A-profile NSACR handling. Note that this only affects
CPUs with an AArch32 EL3.

 target/arm/helper.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 73 insertions(+), 2 deletions(-)

-- 
2.20.1

Comments

Richard Henderson April 13, 2019, 7:07 a.m. UTC | #1 
On 4/11/19 5:39 AM, Peter Maydell wrote:
> +static uint64_t cptr_el2_read(CPUARMState *env, const ARMCPRegInfo *ri)

> +{

> +    /*

> +     * For A-profile AArch32 EL3, if NSACR.CP10

> +     * is 0 then HCPTR.{TCP11,TCP10} ignore writes and read as 1.

> +     */

> +    uint64_t value = env->cp15.cptr_el[2];

> +

> +    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&

> +        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {

> +        value &= ~(0x3 << 10);


Read as 1, and yet you're clearing the value?  Cut-n-paste error from CPACR?
Surely better to do nothing on read, but set on write (to either HCPTR or NSACR).

> +static uint64_t cpacr_read(CPUARMState *env, const ARMCPRegInfo *ri)

> +{

> +    /*

> +     * For A-profile AArch32 EL3 (but not M-profile secure mode), if NSACR.CP10

> +     * is 0 then CPACR.{CP11,CP10} ignore writes and read as 0b00.

> +     */

> +    uint64_t value = env->cp15.cpacr_el1;

> +

> +    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&

> +        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {

> +        value &= ~(0xf << 20);

> +    }


This one does do the right thing, but better to clear the bits on write to
NSACR.  This lets you avoid the change to fp_exception_el, and the missing
change to sve_exception_el.


r~
Peter Maydell April 14, 2019, 6:02 p.m. UTC | #2 
On Sat, 13 Apr 2019 at 08:07, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 4/11/19 5:39 AM, Peter Maydell wrote:

> > +static uint64_t cptr_el2_read(CPUARMState *env, const ARMCPRegInfo *ri)

> > +{

> > +    /*

> > +     * For A-profile AArch32 EL3, if NSACR.CP10

> > +     * is 0 then HCPTR.{TCP11,TCP10} ignore writes and read as 1.

> > +     */

> > +    uint64_t value = env->cp15.cptr_el[2];

> > +

> > +    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&

> > +        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {

> > +        value &= ~(0x3 << 10);

>

> Read as 1, and yet you're clearing the value?  Cut-n-paste error from CPACR?

> Surely better to do nothing on read, but set on write (to either HCPTR or NSACR).


The spec says the HCPTR bits must "behave as RAO/WI, regardless of
their actual value", which I interpret as being 'we must make
read and write do RAO/WI but preserve whatever the underlying
bit values happen to be'. You're right that I've incorrectly
made it clear the bits rather than set them, though.

> > +static uint64_t cpacr_read(CPUARMState *env, const ARMCPRegInfo *ri)

> > +{

> > +    /*

> > +     * For A-profile AArch32 EL3 (but not M-profile secure mode), if NSACR.CP10

> > +     * is 0 then CPACR.{CP11,CP10} ignore writes and read as 0b00.

> > +     */

> > +    uint64_t value = env->cp15.cpacr_el1;

> > +

> > +    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&

> > +        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {

> > +        value &= ~(0xf << 20);

> > +    }

>

> This one does do the right thing, but better to clear the bits on write to

> NSACR.  This lets you avoid the change to fp_exception_el, and the missing

> change to sve_exception_el.


There's similar wording for the effect of NSACR on CPACR, so
again I think we need to actually make the bits RAZ/WI
regardless of their underlying value, not just force them
to 0.

thanks
-- PMM
Richard Henderson April 14, 2019, 7:40 p.m. UTC | #3 
On 4/14/19 8:02 AM, Peter Maydell wrote:
> There's similar wording for the effect of NSACR on CPACR, so

> again I think we need to actually make the bits RAZ/WI

> regardless of their underlying value, not just force them

> to 0.


I don't see that language for CPACR, just "the corresponding bits in the CPACR
ignore writes and read as 0b00".  I'm willing to believe the manual is sloppy
on this point though, and the "correct" language appears only once, somewhere.

You might want to something akin to arm_hcr_el2_eff so that you don't have to
replicate the NSACR logic in too many places...


r~
Peter Maydell April 15, 2019, 8:34 a.m. UTC | #4 
On Sun, 14 Apr 2019 at 20:40, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 4/14/19 8:02 AM, Peter Maydell wrote:

> > There's similar wording for the effect of NSACR on CPACR, so

> > again I think we need to actually make the bits RAZ/WI

> > regardless of their underlying value, not just force them

> > to 0.

>

> I don't see that language for CPACR, just "the corresponding bits in the CPACR

> ignore writes and read as 0b00".  I'm willing to believe the manual is sloppy

> on this point though, and the "correct" language appears only once, somewhere.


It's in the documentation of CPACR cp10/cp11.

thanks
-- PMM
Peter Maydell May 3, 2019, 1:13 p.m. UTC | #5 
On Sat, 13 Apr 2019 at 08:07, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 4/11/19 5:39 AM, Peter Maydell wrote:

> > +static uint64_t cptr_el2_read(CPUARMState *env, const ARMCPRegInfo *ri)

> > +{

> > +    /*

> > +     * For A-profile AArch32 EL3, if NSACR.CP10

> > +     * is 0 then HCPTR.{TCP11,TCP10} ignore writes and read as 1.

> > +     */

> > +    uint64_t value = env->cp15.cptr_el[2];

> > +

> > +    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&

> > +        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {

> > +        value &= ~(0x3 << 10);

>

> Read as 1, and yet you're clearing the value?  Cut-n-paste error from CPACR?

> Surely better to do nothing on read, but set on write (to either HCPTR or NSACR).

>

> > +static uint64_t cpacr_read(CPUARMState *env, const ARMCPRegInfo *ri)

> > +{

> > +    /*

> > +     * For A-profile AArch32 EL3 (but not M-profile secure mode), if NSACR.CP10

> > +     * is 0 then CPACR.{CP11,CP10} ignore writes and read as 0b00.

> > +     */

> > +    uint64_t value = env->cp15.cpacr_el1;

> > +

> > +    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&

> > +        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {

> > +        value &= ~(0xf << 20);

> > +    }

>

> This one does do the right thing, but better to clear the bits on write to

> NSACR.  This lets you avoid the change to fp_exception_el, and the missing

> change to sve_exception_el.


Hi Richard -- I was just going through the review comments on this
patchset, and I saw this bit. Could you clarify what you mean by
"the missing change to sve_exception_el" ? Since SVE is AArch64 only,
there can't be any configs where we have SVE and EL3 is AArch32,
so I don't think these two features should be able to interact.

thanks
-- PMM
Richard Henderson May 3, 2019, 5:29 p.m. UTC | #6 
On 5/3/19 6:13 AM, Peter Maydell wrote:
> On Sat, 13 Apr 2019 at 08:07, Richard Henderson

>> This one does do the right thing, but better to clear the bits on write to

>> NSACR.  This lets you avoid the change to fp_exception_el, and the missing

>> change to sve_exception_el.

> 

> Hi Richard -- I was just going through the review comments on this

> patchset, and I saw this bit. Could you clarify what you mean by

> "the missing change to sve_exception_el" ? Since SVE is AArch64 only,

> there can't be any configs where we have SVE and EL3 is AArch32,

> so I don't think these two features should be able to interact.


You're right.  I'm going to assume I had been insufficiently caffeinated at the
time.


r~
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index a0dd1f99974..26105f13e70 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -903,9 +903,36 @@  static void cpacr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         }
         value &= mask;
     }
+
+    /*
+     * For A-profile AArch32 EL3 (but not M-profile secure mode), if NSACR.CP10
+     * is 0 then CPACR.{CP11,CP10} ignore writes and read as 0b00.
+     */
+    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&
+        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {
+        value &= ~(0xf << 20);
+        value |= env->cp15.cpacr_el1 & (0xf << 20);
+    }
+
     env->cp15.cpacr_el1 = value;
 }
 
+static uint64_t cpacr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /*
+     * For A-profile AArch32 EL3 (but not M-profile secure mode), if NSACR.CP10
+     * is 0 then CPACR.{CP11,CP10} ignore writes and read as 0b00.
+     */
+    uint64_t value = env->cp15.cpacr_el1;
+
+    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&
+        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {
+        value &= ~(0xf << 20);
+    }
+    return value;
+}
+
+
 static void cpacr_reset(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     /* Call cpacr_write() so that we reset with the correct RAO bits set
@@ -971,7 +998,7 @@  static const ARMCPRegInfo v6_cp_reginfo[] = {
     { .name = "CPACR", .state = ARM_CP_STATE_BOTH, .opc0 = 3,
       .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 2, .accessfn = cpacr_access,
       .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.cpacr_el1),
-      .resetfn = cpacr_reset, .writefn = cpacr_write },
+      .resetfn = cpacr_reset, .writefn = cpacr_write, .readfn = cpacr_read },
     REGINFO_SENTINEL
 };
 
@@ -4656,6 +4683,36 @@  uint64_t arm_hcr_el2_eff(CPUARMState *env)
     return ret;
 }
 
+static void cptr_el2_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    /*
+     * For A-profile AArch32 EL3, if NSACR.CP10
+     * is 0 then HCPTR.{TCP11,TCP10} ignore writes and read as 1.
+     */
+    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&
+        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {
+        value &= ~(0x3 << 10);
+        value |= env->cp15.cptr_el[2] & (0x3 << 10);
+    }
+    env->cp15.cptr_el[2] = value;
+}
+
+static uint64_t cptr_el2_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    /*
+     * For A-profile AArch32 EL3, if NSACR.CP10
+     * is 0 then HCPTR.{TCP11,TCP10} ignore writes and read as 1.
+     */
+    uint64_t value = env->cp15.cptr_el[2];
+
+    if (arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&
+        !arm_is_secure(env) && !extract32(env->cp15.nsacr, 10, 1)) {
+        value &= ~(0x3 << 10);
+    }
+    return value;
+}
+
 static const ARMCPRegInfo el2_cp_reginfo[] = {
     { .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
       .type = ARM_CP_IO,
@@ -4703,7 +4760,8 @@  static const ARMCPRegInfo el2_cp_reginfo[] = {
     { .name = "CPTR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 2,
       .access = PL2_RW, .accessfn = cptr_access, .resetvalue = 0,
-      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[2]) },
+      .fieldoffset = offsetof(CPUARMState, cp15.cptr_el[2]),
+      .readfn = cptr_el2_read, .writefn = cptr_el2_write },
     { .name = "MAIR_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 10, .crm = 2, .opc2 = 0,
       .access = PL2_RW, .fieldoffset = offsetof(CPUARMState, cp15.mair_el[2]),
@@ -12804,6 +12862,19 @@  int fp_exception_el(CPUARMState *env, int cur_el)
         break;
     }
 
+    /*
+     * The NSACR allows A-profile AArch32 EL3 and M-profile secure mode
+     * to control non-secure access to the FPU. It doesn't have any
+     * effect if EL3 is AArch64 or if EL3 doesn't exist at all.
+     */
+    if ((arm_feature(env, ARM_FEATURE_EL3) && !arm_el_is_aa64(env, 3) &&
+         cur_el <= 2 && !arm_is_secure_below_el3(env))) {
+        if (!extract32(env->cp15.nsacr, 10, 1)) {
+            /* FP insns act as UNDEF */
+            return cur_el == 2 ? 2 : 1;
+        }
+    }
+
     /* For the CPTR registers we don't need to guard with an ARM_FEATURE
      * check because zero bits in the registers mean "don't trap".
      */