[Qemu-devel] [PATCH v2 0/4] target-arm: Clean up trap/undef handling of SRS

[Qemu-devel] [PATCH v2 0/4] target-arm: Clean up trap/undef handling of SRS

[Peter Maydell]

The SRS instruction is a bit of an oddity because it isn't
used by Linux these days. Nonetheless it has a bunch of
UNPREDICTABLE, UNDEF and trapping behaviour that we weren't
correctly implementing:

 - trap to EL3 if EL3 is AArch64 and we are at Secure EL1
 - UNDEFINED in Hyp mode
 - UNPREDICTABLE in User or System mode
 - UNPREDICTABLE if the specified mode is:
 -- not implemented
 -- not a valid mode number
 -- a mode that's at a higher exception level
 -- Monitor, if we are Non-secure

This series implements the checks we were missing and makes
us UNDEF for all the UNPREDICTABLE cases.

Patch 1 does the easy checks that can be done at translate time;
patches 2 and 3 are code motion in preparation for patch 4, which
puts in a run-time check for the one awkward case we don't have
enough information to UNDEF at translate time.

Changes v1->v2: drop the user-mode-only assertions from the
get_/set_r13_banked() functions as suggested in review.

These have all now been reviewed so I'm just sending them out
to the list for completeness -- I'll put the patches in
target-arm.next in a moment.


thanks
-- PMM


Peter Maydell (4):
  target-arm: Clean up trap/undef handling of SRS
  target-arm: Move get/set_r13_banked() to op_helper.c
  target-arm: Combine user-only and softmmu get/set_r13_banked()
  target-arm: UNDEF in the UNPREDICTABLE SRS-from-System case

 target-arm/helper.c    | 33 -------------------------
 target-arm/op_helper.c | 26 ++++++++++++++++++++
 target-arm/translate.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 88 insertions(+), 38 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/4] target-arm: Clean up trap/undef handling of SRS

[Peter Maydell]

The SRS instruction is:
 * UNDEFINED in Hyp mode
 * UNPREDICTABLE in User or System mode
 * UNPREDICTABLE if the specified mode isn't accessible
 * trapped to EL3 if EL3 is AArch64 and we are at Secure EL1

Clean up the code to handle all these cases cleanly, including
picking UNDEF as our choice of UNPREDICTABLE behaviour rather
blindly trusting the mode field passed in the instruction.
As part of this, move the check for IS_USER into gen_srs()
itself rather than having it done by the caller.

The exception is that we don't UNDEF for calls from System
mode, which need a runtime check. This will be dealt with in
the following commits.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/translate.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 5 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index cf3dc33..7bceb05 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7578,8 +7578,67 @@ static void gen_srs(DisasContext *s,
                     uint32_t mode, uint32_t amode, bool writeback)
 {
     int32_t offset;
-    TCGv_i32 addr = tcg_temp_new_i32();
-    TCGv_i32 tmp = tcg_const_i32(mode);
+    TCGv_i32 addr, tmp;
+    bool undef = false;
+
+    /* SRS is:
+     * - trapped to EL3 if EL3 is AArch64 and we are at Secure EL1
+     * - UNDEFINED in Hyp mode
+     * - UNPREDICTABLE in User or System mode
+     * - UNPREDICTABLE if the specified mode is:
+     * -- not implemented
+     * -- not a valid mode number
+     * -- a mode that's at a higher exception level
+     * -- Monitor, if we are Non-secure
+     * For the UNPREDICTABLE cases we choose to UNDEF, except that for
+     * "current mode is System" we will write a garbage SPSR.
+     * (This is because we don't have access to our current mode here
+     * and would have to do a runtime check to UNDEF for System.)
+     */
+    if (s->current_el == 1 && !s->ns) {
+        gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(), 3);
+        return;
+    }
+
+    if (s->current_el == 0 || s->current_el == 2) {
+        undef = true;
+    }
+
+    switch (mode) {
+    case ARM_CPU_MODE_USR:
+    case ARM_CPU_MODE_FIQ:
+    case ARM_CPU_MODE_IRQ:
+    case ARM_CPU_MODE_SVC:
+    case ARM_CPU_MODE_ABT:
+    case ARM_CPU_MODE_UND:
+    case ARM_CPU_MODE_SYS:
+        break;
+    case ARM_CPU_MODE_HYP:
+        if (s->current_el == 1 || !arm_dc_feature(s, ARM_FEATURE_EL2)) {
+            undef = true;
+        }
+        break;
+    case ARM_CPU_MODE_MON:
+        /* No need to check specifically for "are we non-secure" because
+         * we've already made EL0 UNDEF and handled the trap for S-EL1;
+         * so if this isn't EL3 then we must be non-secure.
+         */
+        if (s->current_el != 3) {
+            undef = true;
+        }
+        break;
+    default:
+        undef = true;
+    }
+
+    if (undef) {
+        gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(),
+                           default_exception_el(s));
+        return;
+    }
+
+    addr = tcg_temp_new_i32();
+    tmp = tcg_const_i32(mode);
     gen_helper_get_r13_banked(addr, cpu_env, tmp);
     tcg_temp_free_i32(tmp);
     switch (amode) {
@@ -7739,9 +7798,6 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
             }
         } else if ((insn & 0x0e5fffe0) == 0x084d0500) {
             /* srs */
-            if (IS_USER(s)) {
-                goto illegal_op;
-            }
             ARCH(6);
             gen_srs(s, (insn & 0x1f), (insn >> 23) & 3, insn & (1 << 21));
             return;
-- 
1.9.1

[Qemu-devel] [PATCH v2 2/4] target-arm: Move get/set_r13_banked() to op_helper.c

[Peter Maydell]

Move get/set_r13_banked() from helper.c to op_helper.c. This will
let us add exception-raising code to them, and also puts them
in the same file as get/set_user_reg(), which makes some conceptual
sense.

(The original reason for the helper.c/op_helper.c split was that
only op_helper.c had access to the CPU env pointer; this distinction
has not been true for a long time, though, and so the split is
now rather arbitrary.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/helper.c    | 33 ---------------------------------
 target-arm/op_helper.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index bb913c6..c46e3d0 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -5365,21 +5365,6 @@ void switch_mode(CPUARMState *env, int mode)
     }
 }
 
-void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
-{
-    ARMCPU *cpu = arm_env_get_cpu(env);
-
-    cpu_abort(CPU(cpu), "banked r13 write\n");
-}
-
-uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
-{
-    ARMCPU *cpu = arm_env_get_cpu(env);
-
-    cpu_abort(CPU(cpu), "banked r13 read\n");
-    return 0;
-}
-
 uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
                                  uint32_t cur_el, bool secure)
 {
@@ -7762,24 +7747,6 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cs, vaddr addr,
     return phys_addr;
 }
 
-void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
-{
-    if ((env->uncached_cpsr & CPSR_M) == mode) {
-        env->regs[13] = val;
-    } else {
-        env->banked_r13[bank_number(mode)] = val;
-    }
-}
-
-uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
-{
-    if ((env->uncached_cpsr & CPSR_M) == mode) {
-        return env->regs[13];
-    } else {
-        return env->banked_r13[bank_number(mode)];
-    }
-}
-
 uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
 {
     ARMCPU *cpu = arm_env_get_cpu(env);
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 049b521..053e9b6 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -457,6 +457,43 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
     }
 }
 
+#if defined(CONFIG_USER_ONLY)
+void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+
+    cpu_abort(CPU(cpu), "banked r13 write\n");
+}
+
+uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
+{
+    ARMCPU *cpu = arm_env_get_cpu(env);
+
+    cpu_abort(CPU(cpu), "banked r13 read\n");
+    return 0;
+}
+
+#else
+
+void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
+{
+    if ((env->uncached_cpsr & CPSR_M) == mode) {
+        env->regs[13] = val;
+    } else {
+        env->banked_r13[bank_number(mode)] = val;
+    }
+}
+
+uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
+{
+    if ((env->uncached_cpsr & CPSR_M) == mode) {
+        return env->regs[13];
+    } else {
+        return env->banked_r13[bank_number(mode)];
+    }
+}
+#endif
+
 void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
                                  uint32_t isread)
 {
-- 
1.9.1

[Qemu-devel] [PATCH v2 3/4] target-arm: Combine user-only and softmmu get/set_r13_banked()

[Peter Maydell]

The user-mode versions of get/set_r13_banked() exist just to assert
if they're ever called -- the translate time code should never
emit calls to them because SRS from user mode always UNDEF.
There's no code in the softmmu versions that can't compile in
CONFIG_USER_ONLY, and the assertion is not particularly useful,
so combine the two functions rather than having completely split
versions under ifdefs.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/op_helper.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 053e9b6..cfdbc8d 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -457,24 +457,6 @@ void HELPER(set_user_reg)(CPUARMState *env, uint32_t regno, uint32_t val)
     }
 }
 
-#if defined(CONFIG_USER_ONLY)
-void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
-{
-    ARMCPU *cpu = arm_env_get_cpu(env);
-
-    cpu_abort(CPU(cpu), "banked r13 write\n");
-}
-
-uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
-{
-    ARMCPU *cpu = arm_env_get_cpu(env);
-
-    cpu_abort(CPU(cpu), "banked r13 read\n");
-    return 0;
-}
-
-#else
-
 void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
 {
     if ((env->uncached_cpsr & CPSR_M) == mode) {
@@ -492,7 +474,6 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
         return env->banked_r13[bank_number(mode)];
     }
 }
-#endif
 
 void HELPER(access_check_cp_reg)(CPUARMState *env, void *rip, uint32_t syndrome,
                                  uint32_t isread)
-- 
1.9.1

[Qemu-devel] [PATCH v2 4/4] target-arm: UNDEF in the UNPREDICTABLE SRS-from-System case

[Peter Maydell]

Make get_r13_banked() raise an exception at runtime for the
corner case of SRS from System mode, so that we can UNDEF it;
this brings us in to line with the ARM ARM's set of permitted
CONSTRAINED UNPREDICTABLE choices.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/op_helper.c | 8 ++++++++
 target-arm/translate.c | 9 +++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index cfdbc8d..538887c 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -468,6 +468,14 @@ void HELPER(set_r13_banked)(CPUARMState *env, uint32_t mode, uint32_t val)
 
 uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
 {
+    if ((env->uncached_cpsr & CPSR_M) == ARM_CPU_MODE_SYS) {
+        /* SRS instruction is UNPREDICTABLE from System mode; we UNDEF.
+         * Other UNPREDICTABLE and UNDEF cases were caught at translate time.
+         */
+        raise_exception(env, EXCP_UDEF, syn_uncategorized(),
+                        exception_target_el(env));
+    }
+
     if ((env->uncached_cpsr & CPSR_M) == mode) {
         return env->regs[13];
     } else {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 7bceb05..e69145d 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7590,10 +7590,7 @@ static void gen_srs(DisasContext *s,
      * -- not a valid mode number
      * -- a mode that's at a higher exception level
      * -- Monitor, if we are Non-secure
-     * For the UNPREDICTABLE cases we choose to UNDEF, except that for
-     * "current mode is System" we will write a garbage SPSR.
-     * (This is because we don't have access to our current mode here
-     * and would have to do a runtime check to UNDEF for System.)
+     * For the UNPREDICTABLE cases we choose to UNDEF.
      */
     if (s->current_el == 1 && !s->ns) {
         gen_exception_insn(s, 4, EXCP_UDEF, syn_uncategorized(), 3);
@@ -7639,6 +7636,9 @@ static void gen_srs(DisasContext *s,
 
     addr = tcg_temp_new_i32();
     tmp = tcg_const_i32(mode);
+    /* get_r13_banked() will raise an exception if called from System mode */
+    gen_set_condexec(s);
+    gen_set_pc_im(s, s->pc - 4);
     gen_helper_get_r13_banked(addr, cpu_env, tmp);
     tcg_temp_free_i32(tmp);
     switch (amode) {
@@ -7688,6 +7688,7 @@ static void gen_srs(DisasContext *s,
         tcg_temp_free_i32(tmp);
     }
     tcg_temp_free_i32(addr);
+    s->is_jmp = DISAS_UPDATE;
 }
 
 static void disas_arm_insn(DisasContext *s, unsigned int insn)
-- 
1.9.1