[Qemu-devel] [PATCH 0/7] armv8m: Implement TT, and other bugfixes

[Qemu-devel] [PATCH 0/7] armv8m: Implement TT, and other bugfixes

[Peter Maydell]

The main aim of this patchseries is to implement the new-for-v8M
TT/TTT/TTA/TTAT instructions (which take an address and do an
MPU/SAU lookup and tell you the security state and access
permissions for the address).

The first part of the series is some smaller bugfixes that
I noticed along the way, followed by a bit of refactoring
before finally implementing TT in the last patch.

(Notably, patch 4 splits the existing MNegPri and MSNegPri
MMU indexes into two new indexes each -- when I originally
added them I hadn't realized that "negative execution priority"
was orthogonal to "user vs privileged", rather than implying
privileged. This brings us up to 8 MMU indexes, but that
should not cost us any more than having 7, so it seems a
better approach than a more convoluted other option (which
you can find briefly described in the patch 4 commit message).

thanks
-- PMM

Peter Maydell (7):
  target/arm: Handle SPSEL and current stack being out of sync in
    MSP/PSP reads
  target/arm: Allow explicit writes to CONTROL.SPSEL in Handler mode
  target/arm: Add missing M profile case to regime_is_user()
  target/arm: Split M profile MNegPri mmu index into user and priv
  target/arm: Create new arm_v7m_mmu_idx_for_secstate_and_priv()
  target/arm: Factor MPU lookup code out of get_phys_addr_pmsav8()
  target/arm: Implement TT instruction

 target/arm/cpu.h       |  68 ++++++++-----
 target/arm/helper.h    |   2 +
 target/arm/internals.h |   6 +-
 target/arm/helper.c    | 268 +++++++++++++++++++++++++++++++++++++------------
 target/arm/translate.c |  37 ++++++-
 5 files changed, 289 insertions(+), 92 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] target/arm: Handle SPSEL and current stack being out of sync in MSP/PSP reads

[Peter Maydell]

For v8M it is possible for the CONTROL.SPSEL bit value and the
current stack to be out of sync. This means we need to update
the checks used in reads and writes of the PSP and MSP special
registers to use v7m_using_psp() rather than directly checking
the SPSEL bit in the control register.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 91a9300..88394d4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9953,11 +9953,9 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 
     switch (reg) {
     case 8: /* MSP */
-        return (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) ?
-            env->v7m.other_sp : env->regs[13];
+        return v7m_using_psp(env) ? env->v7m.other_sp : env->regs[13];
     case 9: /* PSP */
-        return (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) ?
-            env->regs[13] : env->v7m.other_sp;
+        return v7m_using_psp(env) ? env->regs[13] : env->v7m.other_sp;
     case 16: /* PRIMASK */
         return env->v7m.primask[env->v7m.secure];
     case 17: /* BASEPRI */
@@ -10059,14 +10057,14 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
         }
         break;
     case 8: /* MSP */
-        if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) {
+        if (v7m_using_psp(env)) {
             env->v7m.other_sp = val;
         } else {
             env->regs[13] = val;
         }
         break;
     case 9: /* PSP */
-        if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) {
+        if (v7m_using_psp(env)) {
             env->regs[13] = val;
         } else {
             env->v7m.other_sp = val;
-- 
2.7.4

[Qemu-devel] [PATCH 2/7] target/arm: Allow explicit writes to CONTROL.SPSEL in Handler mode

[Peter Maydell]

In ARMv7M the CPU ignores explicit writes to CONTROL.SPSEL
in Handler mode. In v8M the behaviour is slightly different:
writes to the bit are permitted but will have no effect.

We've already done the hard work to handle the value in
CONTROL.SPSEL being out of sync with what stack pointer is
actually in use, so all we need to do to fix this last loose
end is to update the condition we use to guard whether we
call write_v7m_control_spsel() on the register write.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 88394d4..f21c142 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -10091,8 +10091,11 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
          * thread mode; other bits can be updated by any privileged code.
          * write_v7m_control_spsel() deals with updating the SPSEL bit in
          * env->v7m.control, so we only need update the others.
+         * For v7M, we must just ignore explicit writes to SPSEL in handler
+         * mode; for v8M the write is permitted but will have no effect.
          */
-        if (!arm_v7m_is_handler_mode(env)) {
+        if (arm_feature(env, ARM_FEATURE_V8) ||
+            !arm_v7m_is_handler_mode(env)) {
             write_v7m_control_spsel(env, (val & R_V7M_CONTROL_SPSEL_MASK) != 0);
         }
         env->v7m.control[env->v7m.secure] &= ~R_V7M_CONTROL_NPRIV_MASK;
-- 
2.7.4

[Qemu-devel] [PATCH 3/7] target/arm: Add missing M profile case to regime_is_user()

[Peter Maydell]

When we added the ARMMMUIdx_MSUser MMU index we forgot to
add it to the case statement in regime_is_user(), so we
weren't treating it as unprivileged when doing MPU lookups.
Correct the omission.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index f21c142..c4c8d5a 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8016,6 +8016,7 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
     case ARMMMUIdx_S1SE0:
     case ARMMMUIdx_S1NSE0:
     case ARMMMUIdx_MUser:
+    case ARMMMUIdx_MSUser:
         return true;
     default:
         return false;
-- 
2.7.4

[Qemu-devel] [PATCH 4/7] target/arm: Split M profile MNegPri mmu index into user and priv

[Peter Maydell]

For M profile, we currently have an mmu index MNegPri for
"requested execution priority negative". This fails to
distinguish "requested execution priority negative, privileged"
from "requested execution priority negative, usermode", but
the two can return different results for MPU lookups. Fix this
by splitting MNegPri into MNegPriPriv and MNegPriUser, and
similarly for the Secure equivalent MSNegPri.

This takes us from 6 M profile MMU modes to 8, which means
we need to bump NB_MMU_MODES; this is OK since the point
where we are forced to reduce TLB sizes is 9 MMU modes.

(It would in theory be possible to stick with 6 MMU indexes:
{mpu-disabled,user,privileged} x {secure,nonsecure} since
in the MPU-disabled case the result of an MPU lookup is
always the same for both user and privileged code. However
we would then need to rework the TB flags handling to put
user/priv into the TB flags separately from the mmuidx.
Adding an extra couple of mmu indexes is simpler.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 49 ++++++++++++++++++++++++++++---------------------
 target/arm/internals.h |  6 ++++--
 target/arm/helper.c    | 14 ++++++++++----
 target/arm/translate.c |  8 ++++++--
 4 files changed, 48 insertions(+), 29 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 89d49cd..d228fe6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -112,7 +112,7 @@ enum {
 #define ARM_CPU_VIRQ 2
 #define ARM_CPU_VFIQ 3
 
-#define NB_MMU_MODES 7
+#define NB_MMU_MODES 8
 /* ARM-specific extra insn start words:
  * 1: Conditional execution bits
  * 2: Partial exception syndrome for data aborts
@@ -2226,13 +2226,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
  * They have the following different MMU indexes:
  *  User
  *  Privileged
- *  Execution priority negative (this is like privileged, but the
- *  MPU HFNMIENA bit means that it may have different access permission
- *  check results to normal privileged code, so can't share a TLB).
+ *  User, execution priority negative (ie the MPU HFNMIENA bit may apply)
+ *  Privileged, execution priority negative (ditto)
  * If the CPU supports the v8M Security Extension then there are also:
  *  Secure User
  *  Secure Privileged
- *  Secure, execution priority negative
+ *  Secure User, execution priority negative
+ *  Secure Privileged, execution priority negative
  *
  * The ARMMMUIdx and the mmu index value used by the core QEMU TLB code
  * are not quite the same -- different CPU types (most notably M profile
@@ -2251,6 +2251,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
  * The constant names here are patterned after the general style of the names
  * of the AT/ATS operations.
  * The values used are carefully arranged to make mmu_idx => EL lookup easy.
+ * For M profile we arrange them to have a bit for priv, a bit for negpri
+ * and a bit for secure.
  */
 #define ARM_MMU_IDX_A 0x10 /* A profile */
 #define ARM_MMU_IDX_NOTLB 0x20 /* does not have a TLB */
@@ -2269,10 +2271,12 @@ typedef enum ARMMMUIdx {
     ARMMMUIdx_S2NS = 6 | ARM_MMU_IDX_A,
     ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
     ARMMMUIdx_MPriv = 1 | ARM_MMU_IDX_M,
-    ARMMMUIdx_MNegPri = 2 | ARM_MMU_IDX_M,
-    ARMMMUIdx_MSUser = 3 | ARM_MMU_IDX_M,
-    ARMMMUIdx_MSPriv = 4 | ARM_MMU_IDX_M,
-    ARMMMUIdx_MSNegPri = 5 | ARM_MMU_IDX_M,
+    ARMMMUIdx_MUserNegPri = 2 | ARM_MMU_IDX_M,
+    ARMMMUIdx_MPrivNegPri = 3 | ARM_MMU_IDX_M,
+    ARMMMUIdx_MSUser = 4 | ARM_MMU_IDX_M,
+    ARMMMUIdx_MSPriv = 5 | ARM_MMU_IDX_M,
+    ARMMMUIdx_MSUserNegPri = 6 | ARM_MMU_IDX_M,
+    ARMMMUIdx_MSPrivNegPri = 7 | ARM_MMU_IDX_M,
     /* Indexes below here don't have TLBs and are used only for AT system
      * instructions or for the first stage of an S12 page table walk.
      */
@@ -2293,10 +2297,12 @@ typedef enum ARMMMUIdxBit {
     ARMMMUIdxBit_S2NS = 1 << 6,
     ARMMMUIdxBit_MUser = 1 << 0,
     ARMMMUIdxBit_MPriv = 1 << 1,
-    ARMMMUIdxBit_MNegPri = 1 << 2,
-    ARMMMUIdxBit_MSUser = 1 << 3,
-    ARMMMUIdxBit_MSPriv = 1 << 4,
-    ARMMMUIdxBit_MSNegPri = 1 << 5,
+    ARMMMUIdxBit_MUserNegPri = 1 << 2,
+    ARMMMUIdxBit_MPrivNegPri = 1 << 3,
+    ARMMMUIdxBit_MSUser = 1 << 4,
+    ARMMMUIdxBit_MSPriv = 1 << 5,
+    ARMMMUIdxBit_MSUserNegPri = 1 << 6,
+    ARMMMUIdxBit_MSPrivNegPri = 1 << 7,
 } ARMMMUIdxBit;
 
 #define MMU_USER_IDX 0
@@ -2322,8 +2328,7 @@ static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
     case ARM_MMU_IDX_A:
         return mmu_idx & 3;
     case ARM_MMU_IDX_M:
-        return (mmu_idx == ARMMMUIdx_MUser || mmu_idx == ARMMMUIdx_MSUser)
-            ? 0 : 1;
+        return mmu_idx & 1;
     default:
         g_assert_not_reached();
     }
@@ -2334,16 +2339,18 @@ static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
                                                      bool secstate)
 {
     int el = arm_current_el(env);
-    ARMMMUIdx mmu_idx;
+    ARMMMUIdx mmu_idx = ARM_MMU_IDX_M;
 
-    if (el == 0) {
-        mmu_idx = secstate ? ARMMMUIdx_MSUser : ARMMMUIdx_MUser;
-    } else {
-        mmu_idx = secstate ? ARMMMUIdx_MSPriv : ARMMMUIdx_MPriv;
+    if (el != 0) {
+        mmu_idx |= 1;
     }
 
     if (armv7m_nvic_neg_prio_requested(env->nvic, secstate)) {
-        mmu_idx = secstate ? ARMMMUIdx_MSNegPri : ARMMMUIdx_MNegPri;
+        mmu_idx |= 2;
+    }
+
+    if (secstate) {
+        mmu_idx |= 4;
     }
 
     return mmu_idx;
diff --git a/target/arm/internals.h b/target/arm/internals.h
index d9cc75e..aa9c91b 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -544,15 +544,17 @@ static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
     case ARMMMUIdx_S1NSE1:
     case ARMMMUIdx_S1E2:
     case ARMMMUIdx_S2NS:
+    case ARMMMUIdx_MPrivNegPri:
+    case ARMMMUIdx_MUserNegPri:
     case ARMMMUIdx_MPriv:
-    case ARMMMUIdx_MNegPri:
     case ARMMMUIdx_MUser:
         return false;
     case ARMMMUIdx_S1E3:
     case ARMMMUIdx_S1SE0:
     case ARMMMUIdx_S1SE1:
+    case ARMMMUIdx_MSPrivNegPri:
+    case ARMMMUIdx_MSUserNegPri:
     case ARMMMUIdx_MSPriv:
-    case ARMMMUIdx_MSNegPri:
     case ARMMMUIdx_MSUser:
         return true;
     default:
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c4c8d5a..14ab1f4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7856,11 +7856,13 @@ static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
     case ARMMMUIdx_S1SE1:
     case ARMMMUIdx_S1NSE0:
     case ARMMMUIdx_S1NSE1:
+    case ARMMMUIdx_MPrivNegPri:
+    case ARMMMUIdx_MUserNegPri:
     case ARMMMUIdx_MPriv:
-    case ARMMMUIdx_MNegPri:
     case ARMMMUIdx_MUser:
+    case ARMMMUIdx_MSPrivNegPri:
+    case ARMMMUIdx_MSUserNegPri:
     case ARMMMUIdx_MSPriv:
-    case ARMMMUIdx_MSNegPri:
     case ARMMMUIdx_MSUser:
         return 1;
     default:
@@ -7883,8 +7885,10 @@ static inline bool regime_translation_disabled(CPUARMState *env,
                 (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
         case R_V7M_MPU_CTRL_ENABLE_MASK:
             /* Enabled, but not for HardFault and NMI */
-            return mmu_idx == ARMMMUIdx_MNegPri ||
-                mmu_idx == ARMMMUIdx_MSNegPri;
+            return mmu_idx == ARMMMUIdx_MUserNegPri ||
+                mmu_idx == ARMMMUIdx_MPrivNegPri ||
+                mmu_idx == ARMMMUIdx_MSUserNegPri ||
+                mmu_idx == ARMMMUIdx_MSPrivNegPri;
         case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
             /* Enabled for all cases */
             return false;
@@ -8017,6 +8021,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
     case ARMMMUIdx_S1NSE0:
     case ARMMMUIdx_MUser:
     case ARMMMUIdx_MSUser:
+    case ARMMMUIdx_MUserNegPri:
+    case ARMMMUIdx_MSUserNegPri:
         return true;
     default:
         return false;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4afb0c8..6df4d90 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -159,12 +159,16 @@ static inline int get_a32_user_mem_index(DisasContext *s)
         return arm_to_core_mmu_idx(ARMMMUIdx_S1SE0);
     case ARMMMUIdx_MUser:
     case ARMMMUIdx_MPriv:
-    case ARMMMUIdx_MNegPri:
         return arm_to_core_mmu_idx(ARMMMUIdx_MUser);
+    case ARMMMUIdx_MUserNegPri:
+    case ARMMMUIdx_MPrivNegPri:
+        return arm_to_core_mmu_idx(ARMMMUIdx_MUserNegPri);
     case ARMMMUIdx_MSUser:
     case ARMMMUIdx_MSPriv:
-    case ARMMMUIdx_MSNegPri:
         return arm_to_core_mmu_idx(ARMMMUIdx_MSUser);
+    case ARMMMUIdx_MSUserNegPri:
+    case ARMMMUIdx_MSPrivNegPri:
+        return arm_to_core_mmu_idx(ARMMMUIdx_MSUserNegPri);
     case ARMMMUIdx_S2NS:
     default:
         g_assert_not_reached();
-- 
2.7.4

[Qemu-devel] [PATCH 5/7] target/arm: Create new arm_v7m_mmu_idx_for_secstate_and_priv()

[Peter Maydell]

The TT instruction is going to need to look up the MMU index
for a specified security and privilege state. Refactor the
existing arm_v7m_mmu_idx_for_secstate() into a version that
lets you specify the privilege state and one that uses the
current state of the CPU.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index d228fe6..1f414fd 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2334,14 +2334,16 @@ static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
     }
 }
 
-/* Return the MMU index for a v7M CPU in the specified security state */
-static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
-                                                     bool secstate)
+/* Return the MMU index for a v7M CPU in the specified security and
+ * privilege state
+ */
+static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate_and_priv(CPUARMState *env,
+                                                              bool secstate,
+                                                              bool priv)
 {
-    int el = arm_current_el(env);
     ARMMMUIdx mmu_idx = ARM_MMU_IDX_M;
 
-    if (el != 0) {
+    if (priv) {
         mmu_idx |= 1;
     }
 
@@ -2356,6 +2358,15 @@ static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
     return mmu_idx;
 }
 
+/* Return the MMU index for a v7M CPU in the specified security state */
+static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
+                                                     bool secstate)
+{
+    bool priv = arm_current_el(env) != 0;
+
+    return arm_v7m_mmu_idx_for_secstate_and_priv(env, secstate, priv);
+}
+
 /* Determine the current mmu_idx to use for normal loads/stores */
 static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
 {
-- 
2.7.4

[Qemu-devel] [PATCH 6/7] target/arm: Factor MPU lookup code out of get_phys_addr_pmsav8()

[Peter Maydell]

For the TT instruction we're going to need to do an MPU lookup that
also tells us which MPU region the access hit. This requires us
to do the MPU lookup without first doing the SAU security access
check, so pull the MPU lookup parts of get_phys_addr_pmsav8()
out into their own function.

The TT instruction also needs to know the MPU region number which
the lookup hit, so provide this information to the caller of the
MPU lookup code, even though get_phys_addr_pmsav8() doesn't
need to know it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.c | 130 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 79 insertions(+), 51 deletions(-)

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 14ab1f4..be8d797 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9351,67 +9351,28 @@ static void v8m_security_lookup(CPUARMState *env, uint32_t address,
     }
 }
 
-static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
-                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
-                                 hwaddr *phys_ptr, MemTxAttrs *txattrs,
-                                 int *prot, uint32_t *fsr)
+static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
+                              MMUAccessType access_type, ARMMMUIdx mmu_idx,
+                              hwaddr *phys_ptr, MemTxAttrs *txattrs,
+                              int *prot, uint32_t *fsr, uint32_t *mregion)
 {
+    /* Perform a PMSAv8 MPU lookup (without also doing the SAU check
+     * that a full phys-to-virt translation does).
+     * mregion is (if not NULL) set to the region number which matched,
+     * or -1 if no region number is returned (MPU off, address did not
+     * hit a region, address hit in multiple regions).
+     */
     ARMCPU *cpu = arm_env_get_cpu(env);
     bool is_user = regime_is_user(env, mmu_idx);
     uint32_t secure = regime_is_secure(env, mmu_idx);
     int n;
     int matchregion = -1;
     bool hit = false;
-    V8M_SAttributes sattrs = {};
 
     *phys_ptr = address;
     *prot = 0;
-
-    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
-        v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs);
-        if (access_type == MMU_INST_FETCH) {
-            /* Instruction fetches always use the MMU bank and the
-             * transaction attribute determined by the fetch address,
-             * regardless of CPU state. This is painful for QEMU
-             * to handle, because it would mean we need to encode
-             * into the mmu_idx not just the (user, negpri) information
-             * for the current security state but also that for the
-             * other security state, which would balloon the number
-             * of mmu_idx values needed alarmingly.
-             * Fortunately we can avoid this because it's not actually
-             * possible to arbitrarily execute code from memory with
-             * the wrong security attribute: it will always generate
-             * an exception of some kind or another, apart from the
-             * special case of an NS CPU executing an SG instruction
-             * in S&NSC memory. So we always just fail the translation
-             * here and sort things out in the exception handler
-             * (including possibly emulating an SG instruction).
-             */
-            if (sattrs.ns != !secure) {
-                *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT;
-                return true;
-            }
-        } else {
-            /* For data accesses we always use the MMU bank indicated
-             * by the current CPU state, but the security attributes
-             * might downgrade a secure access to nonsecure.
-             */
-            if (sattrs.ns) {
-                txattrs->secure = false;
-            } else if (!secure) {
-                /* NS access to S memory must fault.
-                 * Architecturally we should first check whether the
-                 * MPU information for this address indicates that we
-                 * are doing an unaligned access to Device memory, which
-                 * should generate a UsageFault instead. QEMU does not
-                 * currently check for that kind of unaligned access though.
-                 * If we added it we would need to do so as a special case
-                 * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt().
-                 */
-                *fsr = M_FAKE_FSR_SFAULT;
-                return true;
-            }
-        }
+    if (mregion) {
+        *mregion = -1;
     }
 
     /* Unlike the ARM ARM pseudocode, we don't need to check whether this
@@ -9500,12 +9461,79 @@ static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
         /* We don't need to look the attribute up in the MAIR0/MAIR1
          * registers because that only tells us about cacheability.
          */
+        if (mregion) {
+            *mregion = matchregion;
+        }
     }
 
     *fsr = 0x00d; /* Permission fault */
     return !(*prot & (1 << access_type));
 }
 
+
+static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
+                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
+                                 hwaddr *phys_ptr, MemTxAttrs *txattrs,
+                                 int *prot, uint32_t *fsr)
+{
+    uint32_t secure = regime_is_secure(env, mmu_idx);
+    V8M_SAttributes sattrs = {};
+
+    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
+        v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs);
+        if (access_type == MMU_INST_FETCH) {
+            /* Instruction fetches always use the MMU bank and the
+             * transaction attribute determined by the fetch address,
+             * regardless of CPU state. This is painful for QEMU
+             * to handle, because it would mean we need to encode
+             * into the mmu_idx not just the (user, negpri) information
+             * for the current security state but also that for the
+             * other security state, which would balloon the number
+             * of mmu_idx values needed alarmingly.
+             * Fortunately we can avoid this because it's not actually
+             * possible to arbitrarily execute code from memory with
+             * the wrong security attribute: it will always generate
+             * an exception of some kind or another, apart from the
+             * special case of an NS CPU executing an SG instruction
+             * in S&NSC memory. So we always just fail the translation
+             * here and sort things out in the exception handler
+             * (including possibly emulating an SG instruction).
+             */
+            if (sattrs.ns != !secure) {
+                *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT;
+                *phys_ptr = address;
+                *prot = 0;
+                return true;
+            }
+        } else {
+            /* For data accesses we always use the MMU bank indicated
+             * by the current CPU state, but the security attributes
+             * might downgrade a secure access to nonsecure.
+             */
+            if (sattrs.ns) {
+                txattrs->secure = false;
+            } else if (!secure) {
+                /* NS access to S memory must fault.
+                 * Architecturally we should first check whether the
+                 * MPU information for this address indicates that we
+                 * are doing an unaligned access to Device memory, which
+                 * should generate a UsageFault instead. QEMU does not
+                 * currently check for that kind of unaligned access though.
+                 * If we added it we would need to do so as a special case
+                 * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt().
+                 */
+                *fsr = M_FAKE_FSR_SFAULT;
+                *phys_ptr = address;
+                *prot = 0;
+                return true;
+            }
+        }
+    }
+
+    return pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr,
+                             txattrs, prot, fsr, NULL);
+}
+
 static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
                                  MMUAccessType access_type, ARMMMUIdx mmu_idx,
                                  hwaddr *phys_ptr, int *prot, uint32_t *fsr)
-- 
2.7.4

[Qemu-devel] [PATCH 7/7] target/arm: Implement TT instruction

[Peter Maydell]

Implement the TT instruction which queries the security
state and access permissions of a memory location.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/helper.h    |   2 +
 target/arm/helper.c    | 108 +++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/translate.c |  29 ++++++++++++-
 3 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 439d228..066729e 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -66,6 +66,8 @@ DEF_HELPER_2(v7m_mrs, i32, env, i32)
 DEF_HELPER_2(v7m_bxns, void, env, i32)
 DEF_HELPER_2(v7m_blxns, void, env, i32)
 
+DEF_HELPER_3(v7m_tt, i32, env, i32, i32)
+
 DEF_HELPER_4(access_check_cp_reg, void, env, ptr, i32, i32)
 DEF_HELPER_3(set_cp_reg, void, env, ptr, i32)
 DEF_HELPER_2(get_cp_reg, i32, env, ptr)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index be8d797..5368ad8 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5947,6 +5947,28 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
     g_assert_not_reached();
 }
 
+uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t addr, uint32_t op)
+{
+    /* The TT instructions can be used by unprivileged code, but in
+     * user-only emulation we don't have the MPU.
+     * Luckily since we know we are NonSecure unprivileged (and that in
+     * turn means that the A flag wasn't specified), all the bits in the
+     * register must be zero:
+     *  IREGION: 0 because IRVALID is 0
+     *  IRVALID: 0 because NS
+     *  S: 0 because NS
+     *  NSRW: 0 because NS
+     *  NSR: 0 because NS
+     *  RW: 0 because unpriv and A flag not set
+     *  R: 0 because unpriv and A flag not set
+     *  SRVALID: 0 because NS
+     *  MRVALID: 0 because unpriv and A flag not set
+     *  SREGION: 0 becaus SRVALID is 0
+     *  MREGION: 0 because MRVALID is 0
+     */
+    return 0;
+}
+
 void switch_mode(CPUARMState *env, int mode)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
@@ -10143,6 +10165,92 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
     }
 }
 
+uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t addr, uint32_t op)
+{
+    /* Implement the TT instruction. op is bits [7:6] of the insn. */
+    bool forceunpriv = op & 1;
+    bool alt = op & 2;
+    V8M_SAttributes sattrs = {};
+    uint32_t tt_resp;
+    bool r, rw, nsr, nsrw, mrvalid;
+    int prot;
+    MemTxAttrs attrs = {};
+    hwaddr phys_addr;
+    uint32_t fsr;
+    ARMMMUIdx mmu_idx;
+    uint32_t mregion;
+    bool targetpriv;
+    bool targetsec = env->v7m.secure;
+
+    /* Work out what the security state and privilege level we're
+     * interested in is...
+     */
+    if (alt) {
+        targetsec = !targetsec;
+    }
+
+    if (forceunpriv) {
+        targetpriv = false;
+    } else {
+        targetpriv = arm_v7m_is_handler_mode(env) ||
+            !(env->v7m.control[targetsec] & R_V7M_CONTROL_NPRIV_MASK);
+    }
+
+    /* ...and then figure out which MMU index this is */
+    mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, targetsec, targetpriv);
+
+    /* We know that the MPU and SAU don't care about the access type
+     * for our purposes beyond that we don't want to claim to be
+     * an insn fetch, so we arbitrarily call this a read.
+     */
+
+    /* MPU region info only available for privileged or if
+     * inspecting the other MPU state.
+     */
+    if (arm_current_el(env) != 0 || alt) {
+        /* We can ignore the return value as prot is always set */
+        pmsav8_mpu_lookup(env, addr, MMU_DATA_LOAD, mmu_idx,
+                          &phys_addr, &attrs, &prot, &fsr, &mregion);
+        if (mregion == -1) {
+            mrvalid = false;
+            mregion = 0;
+        } else {
+            mrvalid = true;
+        }
+        r = prot & PAGE_READ;
+        rw = prot & PAGE_WRITE;
+    } else {
+        r = false;
+        rw = false;
+        mrvalid = false;
+        mregion = 0;
+    }
+
+    if (env->v7m.secure) {
+        v8m_security_lookup(env, addr, MMU_DATA_LOAD, mmu_idx, &sattrs);
+        nsr = sattrs.ns && r;
+        nsrw = sattrs.ns && rw;
+    } else {
+        sattrs.ns = true;
+        nsr = false;
+        nsrw = false;
+    }
+
+    tt_resp = (sattrs.iregion << 24) |
+        (sattrs.irvalid << 23) |
+        ((!sattrs.ns) << 22) |
+        (nsrw << 21) |
+        (nsr << 20) |
+        (rw << 19) |
+        (r << 18) |
+        (sattrs.srvalid << 17) |
+        (mrvalid << 16) |
+        (sattrs.sregion << 8) |
+        mregion;
+
+    return tt_resp;
+}
+
 #endif
 
 void HELPER(dc_zva)(CPUARMState *env, uint64_t vaddr_in)
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6df4d90..002055b 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9810,7 +9810,7 @@ static int disas_thumb2_insn(DisasContext *s, uint32_t insn)
         if (insn & (1 << 22)) {
             /* 0b1110_100x_x1xx_xxxx_xxxx_xxxx_xxxx_xxxx
              * - load/store doubleword, load/store exclusive, ldacq/strel,
-             *   table branch.
+             *   table branch, TT.
              */
             if (insn == 0xe97fe97f && arm_dc_feature(s, ARM_FEATURE_M) &&
                 arm_dc_feature(s, ARM_FEATURE_V8)) {
@@ -9887,8 +9887,35 @@ static int disas_thumb2_insn(DisasContext *s, uint32_t insn)
             } else if ((insn & (1 << 23)) == 0) {
                 /* 0b1110_1000_010x_xxxx_xxxx_xxxx_xxxx_xxxx
                  * - load/store exclusive word
+                 * - TT (v8M only)
                  */
                 if (rs == 15) {
+                    if (!(insn & (1 << 20)) &&
+                        arm_dc_feature(s, ARM_FEATURE_M) &&
+                        arm_dc_feature(s, ARM_FEATURE_V8)) {
+                        /* 0b1110_1000_0100_xxxx_1111_xxxx_xxxx_xxxx
+                         *  - TT (v8M only)
+                         */
+                        bool alt = insn & (1 << 7);
+                        TCGv_i32 addr, op, ttresp;
+
+                        if ((insn & 0x3f) || rd == 13 || rd == 15 || rn == 15) {
+                            /* we UNDEF for these UNPREDICTABLE cases */
+                            goto illegal_op;
+                        }
+
+                        if (alt && !s->v8m_secure) {
+                            goto illegal_op;
+                        }
+
+                        addr = load_reg(s, rn);
+                        op = tcg_const_i32(extract32(insn, 6, 2));
+                        ttresp = tcg_temp_new_i32();
+                        gen_helper_v7m_tt(ttresp, cpu_env, addr, op);
+                        tcg_temp_free_i32(addr);
+                        tcg_temp_free_i32(op);
+                        store_reg(s, rd, ttresp);
+                    }
                     goto illegal_op;
                 }
                 addr = tcg_temp_local_new_i32();
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/7] target/arm: Handle SPSEL and current stack being out of sync in MSP/PSP reads

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> For v8M it is possible for the CONTROL.SPSEL bit value and the
> current stack to be out of sync. This means we need to update
> the checks used in reads and writes of the PSP and MSP special
> registers to use v7m_using_psp() rather than directly checking
> the SPSEL bit in the control register.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 2/7] target/arm: Allow explicit writes to CONTROL.SPSEL in Handler mode

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> In ARMv7M the CPU ignores explicit writes to CONTROL.SPSEL
> in Handler mode. In v8M the behaviour is slightly different:
> writes to the bit are permitted but will have no effect.
> 
> We've already done the hard work to handle the value in
> CONTROL.SPSEL being out of sync with what stack pointer is
> actually in use, so all we need to do to fix this last loose
> end is to update the condition we use to guard whether we
> call write_v7m_control_spsel() on the register write.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 3/7] target/arm: Add missing M profile case to regime_is_user()

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> When we added the ARMMMUIdx_MSUser MMU index we forgot to
> add it to the case statement in regime_is_user(), so we
> weren't treating it as unprivileged when doing MPU lookups.
> Correct the omission.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 4/7] target/arm: Split M profile MNegPri mmu index into user and priv

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> For M profile, we currently have an mmu index MNegPri for
> "requested execution priority negative". This fails to
> distinguish "requested execution priority negative, privileged"
> from "requested execution priority negative, usermode", but
> the two can return different results for MPU lookups. Fix this
> by splitting MNegPri into MNegPriPriv and MNegPriUser, and
> similarly for the Secure equivalent MSNegPri.
> 
> This takes us from 6 M profile MMU modes to 8, which means
> we need to bump NB_MMU_MODES; this is OK since the point
> where we are forced to reduce TLB sizes is 9 MMU modes.
> 
> (It would in theory be possible to stick with 6 MMU indexes:
> {mpu-disabled,user,privileged} x {secure,nonsecure} since
> in the MPU-disabled case the result of an MPU lookup is
> always the same for both user and privileged code. However
> we would then need to rework the TB flags handling to put
> user/priv into the TB flags separately from the mmuidx.
> Adding an extra couple of mmu indexes is simpler.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       | 49 ++++++++++++++++++++++++++++---------------------
>  target/arm/internals.h |  6 ++++--
>  target/arm/helper.c    | 14 ++++++++++----
>  target/arm/translate.c |  8 ++++++--
>  4 files changed, 48 insertions(+), 29 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 5/7] target/arm: Create new arm_v7m_mmu_idx_for_secstate_and_priv()

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> The TT instruction is going to need to look up the MMU index
> for a specified security and privilege state. Refactor the
> existing arm_v7m_mmu_idx_for_secstate() into a version that
> lets you specify the privilege state and one that uses the
> current state of the CPU.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 6/7] target/arm: Factor MPU lookup code out of get_phys_addr_pmsav8()

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> For the TT instruction we're going to need to do an MPU lookup that
> also tells us which MPU region the access hit. This requires us
> to do the MPU lookup without first doing the SAU security access
> check, so pull the MPU lookup parts of get_phys_addr_pmsav8()
> out into their own function.
> 
> The TT instruction also needs to know the MPU region number which
> the lookup hit, so provide this information to the caller of the
> MPU lookup code, even though get_phys_addr_pmsav8() doesn't
> need to know it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.c | 130 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 79 insertions(+), 51 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 7/7] target/arm: Implement TT instruction

[Richard Henderson]

On 12/01/2017 10:44 AM, Peter Maydell wrote:
> Implement the TT instruction which queries the security
> state and access permissions of a memory location.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/helper.h    |   2 +
>  target/arm/helper.c    | 108 +++++++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/translate.c |  29 ++++++++++++-
>  3 files changed, 138 insertions(+), 1 deletion(-)


Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/7] target/arm: Handle SPSEL and current stack being out of sync in MSP/PSP reads

[Philippe Mathieu-Daudé]

On 12/01/2017 03:44 PM, Peter Maydell wrote:
> For v8M it is possible for the CONTROL.SPSEL bit value and the
> current stack to be out of sync. This means we need to update
> the checks used in reads and writes of the PSP and MSP special
> registers to use v7m_using_psp() rather than directly checking
> the SPSEL bit in the control register.

good catch.

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

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 91a9300..88394d4 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9953,11 +9953,9 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
>  
>      switch (reg) {
>      case 8: /* MSP */
> -        return (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) ?
> -            env->v7m.other_sp : env->regs[13];
> +        return v7m_using_psp(env) ? env->v7m.other_sp : env->regs[13];
>      case 9: /* PSP */
> -        return (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) ?
> -            env->regs[13] : env->v7m.other_sp;
> +        return v7m_using_psp(env) ? env->regs[13] : env->v7m.other_sp;
>      case 16: /* PRIMASK */
>          return env->v7m.primask[env->v7m.secure];
>      case 17: /* BASEPRI */
> @@ -10059,14 +10057,14 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
>          }
>          break;
>      case 8: /* MSP */
> -        if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) {
> +        if (v7m_using_psp(env)) {
>              env->v7m.other_sp = val;
>          } else {
>              env->regs[13] = val;
>          }
>          break;
>      case 9: /* PSP */
> -        if (env->v7m.control[env->v7m.secure] & R_V7M_CONTROL_SPSEL_MASK) {
> +        if (v7m_using_psp(env)) {
>              env->regs[13] = val;
>          } else {
>              env->v7m.other_sp = val;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/7] target/arm: Add missing M profile case to regime_is_user()

[Philippe Mathieu-Daudé]

On 12/01/2017 03:44 PM, Peter Maydell wrote:
> When we added the ARMMMUIdx_MSUser MMU index we forgot to
> add it to the case statement in regime_is_user(), so we
> weren't treating it as unprivileged when doing MPU lookups.
> Correct the omission.

Eh commit e7b921c2d9e wasn't reviewed!

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

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index f21c142..c4c8d5a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -8016,6 +8016,7 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>      case ARMMMUIdx_S1SE0:
>      case ARMMMUIdx_S1NSE0:
>      case ARMMMUIdx_MUser:
> +    case ARMMMUIdx_MSUser:
>          return true;
>      default:
>          return false;
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] target/arm: Split M profile MNegPri mmu index into user and priv

[Philippe Mathieu-Daudé]

Hi Peter,

On 12/01/2017 03:44 PM, Peter Maydell wrote:
> For M profile, we currently have an mmu index MNegPri for
> "requested execution priority negative". This fails to
> distinguish "requested execution priority negative, privileged"
> from "requested execution priority negative, usermode", but
> the two can return different results for MPU lookups. Fix this
> by splitting MNegPri into MNegPriPriv and MNegPriUser, and
> similarly for the Secure equivalent MSNegPri.
> 
> This takes us from 6 M profile MMU modes to 8, which means
> we need to bump NB_MMU_MODES; this is OK since the point
> where we are forced to reduce TLB sizes is 9 MMU modes.
> 
> (It would in theory be possible to stick with 6 MMU indexes:
> {mpu-disabled,user,privileged} x {secure,nonsecure} since
> in the MPU-disabled case the result of an MPU lookup is
> always the same for both user and privileged code. However
> we would then need to rework the TB flags handling to put
> user/priv into the TB flags separately from the mmuidx.
> Adding an extra couple of mmu indexes is simpler.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       | 49 ++++++++++++++++++++++++++++---------------------
>  target/arm/internals.h |  6 ++++--
>  target/arm/helper.c    | 14 ++++++++++----
>  target/arm/translate.c |  8 ++++++--
>  4 files changed, 48 insertions(+), 29 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 89d49cd..d228fe6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -112,7 +112,7 @@ enum {
>  #define ARM_CPU_VIRQ 2
>  #define ARM_CPU_VFIQ 3
>  
> -#define NB_MMU_MODES 7
> +#define NB_MMU_MODES 8
>  /* ARM-specific extra insn start words:
>   * 1: Conditional execution bits
>   * 2: Partial exception syndrome for data aborts
> @@ -2226,13 +2226,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>   * They have the following different MMU indexes:
>   *  User
>   *  Privileged
> - *  Execution priority negative (this is like privileged, but the
> - *  MPU HFNMIENA bit means that it may have different access permission
> - *  check results to normal privileged code, so can't share a TLB).
> + *  User, execution priority negative (ie the MPU HFNMIENA bit may apply)
> + *  Privileged, execution priority negative (ditto)
>   * If the CPU supports the v8M Security Extension then there are also:
>   *  Secure User
>   *  Secure Privileged
> - *  Secure, execution priority negative
> + *  Secure User, execution priority negative
> + *  Secure Privileged, execution priority negative
>   *
>   * The ARMMMUIdx and the mmu index value used by the core QEMU TLB code
>   * are not quite the same -- different CPU types (most notably M profile
> @@ -2251,6 +2251,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
>   * The constant names here are patterned after the general style of the names
>   * of the AT/ATS operations.
>   * The values used are carefully arranged to make mmu_idx => EL lookup easy.
> + * For M profile we arrange them to have a bit for priv, a bit for negpri
> + * and a bit for secure.
>   */
>  #define ARM_MMU_IDX_A 0x10 /* A profile */
>  #define ARM_MMU_IDX_NOTLB 0x20 /* does not have a TLB */
> @@ -2269,10 +2271,12 @@ typedef enum ARMMMUIdx {
>      ARMMMUIdx_S2NS = 6 | ARM_MMU_IDX_A,
>      ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
>      ARMMMUIdx_MPriv = 1 | ARM_MMU_IDX_M,
> -    ARMMMUIdx_MNegPri = 2 | ARM_MMU_IDX_M,
> -    ARMMMUIdx_MSUser = 3 | ARM_MMU_IDX_M,
> -    ARMMMUIdx_MSPriv = 4 | ARM_MMU_IDX_M,
> -    ARMMMUIdx_MSNegPri = 5 | ARM_MMU_IDX_M,
> +    ARMMMUIdx_MUserNegPri = 2 | ARM_MMU_IDX_M,
> +    ARMMMUIdx_MPrivNegPri = 3 | ARM_MMU_IDX_M,
> +    ARMMMUIdx_MSUser = 4 | ARM_MMU_IDX_M,
> +    ARMMMUIdx_MSPriv = 5 | ARM_MMU_IDX_M,
> +    ARMMMUIdx_MSUserNegPri = 6 | ARM_MMU_IDX_M,
> +    ARMMMUIdx_MSPrivNegPri = 7 | ARM_MMU_IDX_M,
>      /* Indexes below here don't have TLBs and are used only for AT system
>       * instructions or for the first stage of an S12 page table walk.
>       */
> @@ -2293,10 +2297,12 @@ typedef enum ARMMMUIdxBit {
>      ARMMMUIdxBit_S2NS = 1 << 6,
>      ARMMMUIdxBit_MUser = 1 << 0,
>      ARMMMUIdxBit_MPriv = 1 << 1,
> -    ARMMMUIdxBit_MNegPri = 1 << 2,
> -    ARMMMUIdxBit_MSUser = 1 << 3,
> -    ARMMMUIdxBit_MSPriv = 1 << 4,
> -    ARMMMUIdxBit_MSNegPri = 1 << 5,
> +    ARMMMUIdxBit_MUserNegPri = 1 << 2,
> +    ARMMMUIdxBit_MPrivNegPri = 1 << 3,
> +    ARMMMUIdxBit_MSUser = 1 << 4,
> +    ARMMMUIdxBit_MSPriv = 1 << 5,
> +    ARMMMUIdxBit_MSUserNegPri = 1 << 6,
> +    ARMMMUIdxBit_MSPrivNegPri = 1 << 7,
>  } ARMMMUIdxBit;

(I think the ARMMMUIdxBit enum is misleading, since this is a mask)

The patch is correct, but I don't like having magic values.

Can you add these ARM_MMU_IDX_M_* defines/enums?

enum {
    ARM_MMU_IDX_M_EL        0x01, /* Exception Level */
    ARM_MMU_IDX_M_NEG       0x02,
    ARM_MMU_IDX_M_SEC       0x04, /* Secure */
    ARM_MMU_IDX_A           0x10, /* A profile */
    ARM_MMU_IDX_NOTLB       0x20, /* does not have a TLB */
    ARM_MMU_IDX_M           0x40, /* M profile */
};

>  
>  #define MMU_USER_IDX 0
> @@ -2322,8 +2328,7 @@ static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
>      case ARM_MMU_IDX_A:
>          return mmu_idx & 3;
>      case ARM_MMU_IDX_M:
> -        return (mmu_idx == ARMMMUIdx_MUser || mmu_idx == ARMMMUIdx_MSUser)
> -            ? 0 : 1;
> +        return mmu_idx & 1;

So here we can use:

           return mmu_idx & ARM_MMU_IDX_M_EL;

>      default:
>          g_assert_not_reached();
>      }
> @@ -2334,16 +2339,18 @@ static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
>                                                       bool secstate)
>  {
>      int el = arm_current_el(env);
> -    ARMMMUIdx mmu_idx;
> +    ARMMMUIdx mmu_idx = ARM_MMU_IDX_M;
>  
> -    if (el == 0) {
> -        mmu_idx = secstate ? ARMMMUIdx_MSUser : ARMMMUIdx_MUser;
> -    } else {
> -        mmu_idx = secstate ? ARMMMUIdx_MSPriv : ARMMMUIdx_MPriv;
> +    if (el != 0) {
> +        mmu_idx |= 1;

           mmu_idx |= ARM_MMU_IDX_M_EL;

>      }
> 
>      if (armv7m_nvic_neg_prio_requested(env->nvic, secstate)) {
> -        mmu_idx = secstate ? ARMMMUIdx_MSNegPri : ARMMMUIdx_MNegPri;
> +        mmu_idx |= 2;

           mmu_idx |= ARM_MMU_IDX_M_NEG;

> +    }
> +
> +    if (secstate) {
> +        mmu_idx |= 4;

           mmu_idx |= ARM_MMU_IDX_M_SEC;

>      }
>  
>      return mmu_idx;
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index d9cc75e..aa9c91b 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -544,15 +544,17 @@ static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>      case ARMMMUIdx_S1NSE1:
>      case ARMMMUIdx_S1E2:
>      case ARMMMUIdx_S2NS:
> +    case ARMMMUIdx_MPrivNegPri:
> +    case ARMMMUIdx_MUserNegPri:
>      case ARMMMUIdx_MPriv:
> -    case ARMMMUIdx_MNegPri:
>      case ARMMMUIdx_MUser:
>          return false;
>      case ARMMMUIdx_S1E3:
>      case ARMMMUIdx_S1SE0:
>      case ARMMMUIdx_S1SE1:
> +    case ARMMMUIdx_MSPrivNegPri:
> +    case ARMMMUIdx_MSUserNegPri:
>      case ARMMMUIdx_MSPriv:
> -    case ARMMMUIdx_MSNegPri:
>      case ARMMMUIdx_MSUser:
>          return true;
>      default:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index c4c8d5a..14ab1f4 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7856,11 +7856,13 @@ static inline uint32_t regime_el(CPUARMState *env, ARMMMUIdx mmu_idx)
>      case ARMMMUIdx_S1SE1:
>      case ARMMMUIdx_S1NSE0:
>      case ARMMMUIdx_S1NSE1:
> +    case ARMMMUIdx_MPrivNegPri:
> +    case ARMMMUIdx_MUserNegPri:
>      case ARMMMUIdx_MPriv:
> -    case ARMMMUIdx_MNegPri:
>      case ARMMMUIdx_MUser:
> +    case ARMMMUIdx_MSPrivNegPri:
> +    case ARMMMUIdx_MSUserNegPri:
>      case ARMMMUIdx_MSPriv:
> -    case ARMMMUIdx_MSNegPri:
>      case ARMMMUIdx_MSUser:
>          return 1;
>      default:
> @@ -7883,8 +7885,10 @@ static inline bool regime_translation_disabled(CPUARMState *env,
>                  (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
>          case R_V7M_MPU_CTRL_ENABLE_MASK:
>              /* Enabled, but not for HardFault and NMI */
> -            return mmu_idx == ARMMMUIdx_MNegPri ||
> -                mmu_idx == ARMMMUIdx_MSNegPri;
> +            return mmu_idx == ARMMMUIdx_MUserNegPri ||
> +                mmu_idx == ARMMMUIdx_MPrivNegPri ||
> +                mmu_idx == ARMMMUIdx_MSUserNegPri ||
> +                mmu_idx == ARMMMUIdx_MSPrivNegPri;

And here we can simplify:

               return mmu_idx & ARM_MMU_IDX_M_NEG;

>          case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
>              /* Enabled for all cases */
>              return false;
> @@ -8017,6 +8021,8 @@ static inline bool regime_is_user(CPUARMState *env, ARMMMUIdx mmu_idx)
>      case ARMMMUIdx_S1NSE0:
>      case ARMMMUIdx_MUser:
>      case ARMMMUIdx_MSUser:
> +    case ARMMMUIdx_MUserNegPri:
> +    case ARMMMUIdx_MSUserNegPri:
>          return true;
>      default:
>          return false;
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 4afb0c8..6df4d90 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -159,12 +159,16 @@ static inline int get_a32_user_mem_index(DisasContext *s)
>          return arm_to_core_mmu_idx(ARMMMUIdx_S1SE0);
>      case ARMMMUIdx_MUser:
>      case ARMMMUIdx_MPriv:
> -    case ARMMMUIdx_MNegPri:
>          return arm_to_core_mmu_idx(ARMMMUIdx_MUser);
> +    case ARMMMUIdx_MUserNegPri:
> +    case ARMMMUIdx_MPrivNegPri:
> +        return arm_to_core_mmu_idx(ARMMMUIdx_MUserNegPri);
>      case ARMMMUIdx_MSUser:
>      case ARMMMUIdx_MSPriv:
> -    case ARMMMUIdx_MSNegPri:
>          return arm_to_core_mmu_idx(ARMMMUIdx_MSUser);
> +    case ARMMMUIdx_MSUserNegPri:
> +    case ARMMMUIdx_MSPrivNegPri:
> +        return arm_to_core_mmu_idx(ARMMMUIdx_MSUserNegPri);
>      case ARMMMUIdx_S2NS:
>      default:
>          g_assert_not_reached();
> 

Regards,

Phil.

Re: [Qemu-devel] [PATCH 5/7] target/arm: Create new arm_v7m_mmu_idx_for_secstate_and_priv()

[Philippe Mathieu-Daudé]

On 12/01/2017 03:44 PM, Peter Maydell wrote:
> The TT instruction is going to need to look up the MMU index
> for a specified security and privilege state. Refactor the
> existing arm_v7m_mmu_idx_for_secstate() into a version that
> lets you specify the privilege state and one that uses the
> current state of the CPU.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.h | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d228fe6..1f414fd 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2334,14 +2334,16 @@ static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
>      }
>  }
>  
> -/* Return the MMU index for a v7M CPU in the specified security state */
> -static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
> -                                                     bool secstate)
> +/* Return the MMU index for a v7M CPU in the specified security and
> + * privilege state
> + */
> +static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate_and_priv(CPUARMState *env,
> +                                                              bool secstate,
> +                                                              bool priv)
>  {
> -    int el = arm_current_el(env);
>      ARMMMUIdx mmu_idx = ARM_MMU_IDX_M;
>  
> -    if (el != 0) {
> +    if (priv) {
>          mmu_idx |= 1;
>      }
>  
> @@ -2356,6 +2358,15 @@ static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
>      return mmu_idx;
>  }
>  
> +/* Return the MMU index for a v7M CPU in the specified security state */
> +static inline ARMMMUIdx arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
> +                                                     bool secstate)
> +{
> +    bool priv = arm_current_el(env) != 0;
> +
> +    return arm_v7m_mmu_idx_for_secstate_and_priv(env, secstate, priv);
> +}
> +
>  /* Determine the current mmu_idx to use for normal loads/stores */
>  static inline int cpu_mmu_index(CPUARMState *env, bool ifetch)
>  {
> 

Re: [Qemu-devel] [PATCH 6/7] target/arm: Factor MPU lookup code out of get_phys_addr_pmsav8()

[Philippe Mathieu-Daudé]

On 12/01/2017 03:44 PM, Peter Maydell wrote:
> For the TT instruction we're going to need to do an MPU lookup that
> also tells us which MPU region the access hit. This requires us
> to do the MPU lookup without first doing the SAU security access
> check, so pull the MPU lookup parts of get_phys_addr_pmsav8()
> out into their own function.
> 
> The TT instruction also needs to know the MPU region number which
> the lookup hit, so provide this information to the caller of the
> MPU lookup code, even though get_phys_addr_pmsav8() doesn't
> need to know it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Elegant refactor!

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/helper.c | 130 +++++++++++++++++++++++++++++++---------------------
>  1 file changed, 79 insertions(+), 51 deletions(-)
> 
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 14ab1f4..be8d797 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9351,67 +9351,28 @@ static void v8m_security_lookup(CPUARMState *env, uint32_t address,
>      }
>  }
>  
> -static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
> -                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
> -                                 hwaddr *phys_ptr, MemTxAttrs *txattrs,
> -                                 int *prot, uint32_t *fsr)
> +static bool pmsav8_mpu_lookup(CPUARMState *env, uint32_t address,
> +                              MMUAccessType access_type, ARMMMUIdx mmu_idx,
> +                              hwaddr *phys_ptr, MemTxAttrs *txattrs,
> +                              int *prot, uint32_t *fsr, uint32_t *mregion)
>  {
> +    /* Perform a PMSAv8 MPU lookup (without also doing the SAU check
> +     * that a full phys-to-virt translation does).
> +     * mregion is (if not NULL) set to the region number which matched,
> +     * or -1 if no region number is returned (MPU off, address did not
> +     * hit a region, address hit in multiple regions).
> +     */
>      ARMCPU *cpu = arm_env_get_cpu(env);
>      bool is_user = regime_is_user(env, mmu_idx);
>      uint32_t secure = regime_is_secure(env, mmu_idx);
>      int n;
>      int matchregion = -1;
>      bool hit = false;
> -    V8M_SAttributes sattrs = {};
>  
>      *phys_ptr = address;
>      *prot = 0;
> -
> -    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> -        v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs);
> -        if (access_type == MMU_INST_FETCH) {
> -            /* Instruction fetches always use the MMU bank and the
> -             * transaction attribute determined by the fetch address,
> -             * regardless of CPU state. This is painful for QEMU
> -             * to handle, because it would mean we need to encode
> -             * into the mmu_idx not just the (user, negpri) information
> -             * for the current security state but also that for the
> -             * other security state, which would balloon the number
> -             * of mmu_idx values needed alarmingly.
> -             * Fortunately we can avoid this because it's not actually
> -             * possible to arbitrarily execute code from memory with
> -             * the wrong security attribute: it will always generate
> -             * an exception of some kind or another, apart from the
> -             * special case of an NS CPU executing an SG instruction
> -             * in S&NSC memory. So we always just fail the translation
> -             * here and sort things out in the exception handler
> -             * (including possibly emulating an SG instruction).
> -             */
> -            if (sattrs.ns != !secure) {
> -                *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT;
> -                return true;
> -            }
> -        } else {
> -            /* For data accesses we always use the MMU bank indicated
> -             * by the current CPU state, but the security attributes
> -             * might downgrade a secure access to nonsecure.
> -             */
> -            if (sattrs.ns) {
> -                txattrs->secure = false;
> -            } else if (!secure) {
> -                /* NS access to S memory must fault.
> -                 * Architecturally we should first check whether the
> -                 * MPU information for this address indicates that we
> -                 * are doing an unaligned access to Device memory, which
> -                 * should generate a UsageFault instead. QEMU does not
> -                 * currently check for that kind of unaligned access though.
> -                 * If we added it we would need to do so as a special case
> -                 * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt().
> -                 */
> -                *fsr = M_FAKE_FSR_SFAULT;
> -                return true;
> -            }
> -        }
> +    if (mregion) {
> +        *mregion = -1;
>      }
>  
>      /* Unlike the ARM ARM pseudocode, we don't need to check whether this
> @@ -9500,12 +9461,79 @@ static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
>          /* We don't need to look the attribute up in the MAIR0/MAIR1
>           * registers because that only tells us about cacheability.
>           */
> +        if (mregion) {
> +            *mregion = matchregion;
> +        }
>      }
>  
>      *fsr = 0x00d; /* Permission fault */
>      return !(*prot & (1 << access_type));
>  }
>  
> +
> +static bool get_phys_addr_pmsav8(CPUARMState *env, uint32_t address,
> +                                 MMUAccessType access_type, ARMMMUIdx mmu_idx,
> +                                 hwaddr *phys_ptr, MemTxAttrs *txattrs,
> +                                 int *prot, uint32_t *fsr)
> +{
> +    uint32_t secure = regime_is_secure(env, mmu_idx);
> +    V8M_SAttributes sattrs = {};
> +
> +    if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> +        v8m_security_lookup(env, address, access_type, mmu_idx, &sattrs);
> +        if (access_type == MMU_INST_FETCH) {
> +            /* Instruction fetches always use the MMU bank and the
> +             * transaction attribute determined by the fetch address,
> +             * regardless of CPU state. This is painful for QEMU
> +             * to handle, because it would mean we need to encode
> +             * into the mmu_idx not just the (user, negpri) information
> +             * for the current security state but also that for the
> +             * other security state, which would balloon the number
> +             * of mmu_idx values needed alarmingly.
> +             * Fortunately we can avoid this because it's not actually
> +             * possible to arbitrarily execute code from memory with
> +             * the wrong security attribute: it will always generate
> +             * an exception of some kind or another, apart from the
> +             * special case of an NS CPU executing an SG instruction
> +             * in S&NSC memory. So we always just fail the translation
> +             * here and sort things out in the exception handler
> +             * (including possibly emulating an SG instruction).
> +             */
> +            if (sattrs.ns != !secure) {
> +                *fsr = sattrs.nsc ? M_FAKE_FSR_NSC_EXEC : M_FAKE_FSR_SFAULT;
> +                *phys_ptr = address;
> +                *prot = 0;
> +                return true;
> +            }
> +        } else {
> +            /* For data accesses we always use the MMU bank indicated
> +             * by the current CPU state, but the security attributes
> +             * might downgrade a secure access to nonsecure.
> +             */
> +            if (sattrs.ns) {
> +                txattrs->secure = false;
> +            } else if (!secure) {
> +                /* NS access to S memory must fault.
> +                 * Architecturally we should first check whether the
> +                 * MPU information for this address indicates that we
> +                 * are doing an unaligned access to Device memory, which
> +                 * should generate a UsageFault instead. QEMU does not
> +                 * currently check for that kind of unaligned access though.
> +                 * If we added it we would need to do so as a special case
> +                 * for M_FAKE_FSR_SFAULT in arm_v7m_cpu_do_interrupt().
> +                 */
> +                *fsr = M_FAKE_FSR_SFAULT;
> +                *phys_ptr = address;
> +                *prot = 0;
> +                return true;
> +            }
> +        }
> +    }
> +
> +    return pmsav8_mpu_lookup(env, address, access_type, mmu_idx, phys_ptr,
> +                             txattrs, prot, fsr, NULL);
> +}
> +
>  static bool get_phys_addr_pmsav5(CPUARMState *env, uint32_t address,
>                                   MMUAccessType access_type, ARMMMUIdx mmu_idx,
>                                   hwaddr *phys_ptr, int *prot, uint32_t *fsr)
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] target/arm: Split M profile MNegPri mmu index into user and priv

[Peter Maydell]

On 5 December 2017 at 21:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> The patch is correct, but I don't like having magic values.

Yeah, you're right this would be better. I've already put this
patch into target-arm.next, so I'll just squash in this change:

--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2258,6 +2258,11 @@ static inline bool arm_excp_unmasked(CPUState
*cs, unsigned int excp_idx,
 #define ARM_MMU_IDX_NOTLB 0x20 /* does not have a TLB */
 #define ARM_MMU_IDX_M 0x40 /* M profile */

+/* meanings of the bits for M profile mmu idx values */
+#define ARM_MMU_IDX_M_PRIV 0x1
+#define ARM_MMU_IDX_M_NEGPRI 0x2
+#define ARM_MMU_IDX_M_S 0x4
+
 #define ARM_MMU_IDX_TYPE_MASK (~0x7)
 #define ARM_MMU_IDX_COREIDX_MASK 0x7

@@ -2328,7 +2333,7 @@ static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
     case ARM_MMU_IDX_A:
         return mmu_idx & 3;
     case ARM_MMU_IDX_M:
-        return mmu_idx & 1;
+        return mmu_idx & ARM_MMU_IDX_M_PRIV;
     default:
         g_assert_not_reached();
     }
@@ -2342,15 +2347,15 @@ static inline ARMMMUIdx
arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
     ARMMMUIdx mmu_idx = ARM_MMU_IDX_M;

     if (el != 0) {
-        mmu_idx |= 1;
+        mmu_idx |= ARM_MMU_IDX_M_PRIV;
     }

     if (armv7m_nvic_neg_prio_requested(env->nvic, secstate)) {
-        mmu_idx |= 2;
+        mmu_idx |= ARM_MMU_IDX_M_NEGPRI;
     }

     if (secstate) {
-        mmu_idx |= 4;
+        mmu_idx |= ARM_MMU_IDX_M_S;
     }

     return mmu_idx;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 14ab1f4..70cf313 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -7885,10 +7885,7 @@ static inline bool
regime_translation_disabled(CPUARMState *env,
                 (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
         case R_V7M_MPU_CTRL_ENABLE_MASK:
             /* Enabled, but not for HardFault and NMI */
-            return mmu_idx == ARMMMUIdx_MUserNegPri ||
-                mmu_idx == ARMMMUIdx_MPrivNegPri ||
-                mmu_idx == ARMMMUIdx_MSUserNegPri ||
-                mmu_idx == ARMMMUIdx_MSPrivNegPri;
+            return mmu_idx & ARM_MMU_IDX_M_NEGPRI;
         case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
             /* Enabled for all cases */
             return false;


thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] target/arm: Split M profile MNegPri mmu index into user and priv

[Philippe Mathieu-Daudé]

On 12/07/2017 08:07 AM, Peter Maydell wrote:
> On 5 December 2017 at 21:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> The patch is correct, but I don't like having magic values.
> 
> Yeah, you're right this would be better. I've already put this
> patch into target-arm.next, so I'll just squash in this change:

Thanks!

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2258,6 +2258,11 @@ static inline bool arm_excp_unmasked(CPUState
> *cs, unsigned int excp_idx,
>  #define ARM_MMU_IDX_NOTLB 0x20 /* does not have a TLB */
>  #define ARM_MMU_IDX_M 0x40 /* M profile */
> 
> +/* meanings of the bits for M profile mmu idx values */
> +#define ARM_MMU_IDX_M_PRIV 0x1
> +#define ARM_MMU_IDX_M_NEGPRI 0x2
> +#define ARM_MMU_IDX_M_S 0x4
> +
>  #define ARM_MMU_IDX_TYPE_MASK (~0x7)
>  #define ARM_MMU_IDX_COREIDX_MASK 0x7
> 
> @@ -2328,7 +2333,7 @@ static inline int arm_mmu_idx_to_el(ARMMMUIdx mmu_idx)
>      case ARM_MMU_IDX_A:
>          return mmu_idx & 3;
>      case ARM_MMU_IDX_M:
> -        return mmu_idx & 1;
> +        return mmu_idx & ARM_MMU_IDX_M_PRIV;
>      default:
>          g_assert_not_reached();
>      }
> @@ -2342,15 +2347,15 @@ static inline ARMMMUIdx
> arm_v7m_mmu_idx_for_secstate(CPUARMState *env,
>      ARMMMUIdx mmu_idx = ARM_MMU_IDX_M;
> 
>      if (el != 0) {
> -        mmu_idx |= 1;
> +        mmu_idx |= ARM_MMU_IDX_M_PRIV;
>      }
> 
>      if (armv7m_nvic_neg_prio_requested(env->nvic, secstate)) {
> -        mmu_idx |= 2;
> +        mmu_idx |= ARM_MMU_IDX_M_NEGPRI;
>      }
> 
>      if (secstate) {
> -        mmu_idx |= 4;
> +        mmu_idx |= ARM_MMU_IDX_M_S;
>      }
> 
>      return mmu_idx;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 14ab1f4..70cf313 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -7885,10 +7885,7 @@ static inline bool
> regime_translation_disabled(CPUARMState *env,
>                  (R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK)) {
>          case R_V7M_MPU_CTRL_ENABLE_MASK:
>              /* Enabled, but not for HardFault and NMI */
> -            return mmu_idx == ARMMMUIdx_MUserNegPri ||
> -                mmu_idx == ARMMMUIdx_MPrivNegPri ||
> -                mmu_idx == ARMMMUIdx_MSUserNegPri ||
> -                mmu_idx == ARMMMUIdx_MSPrivNegPri;
> +            return mmu_idx & ARM_MMU_IDX_M_NEGPRI;
>          case R_V7M_MPU_CTRL_ENABLE_MASK | R_V7M_MPU_CTRL_HFNMIENA_MASK:
>              /* Enabled for all cases */
>              return false;
> 
> 
> thanks
> -- PMM
>