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

[Qemu-devel] [PATCH 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.

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 | 32 ++++++++++++++++++++++++
 target-arm/translate.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 94 insertions(+), 38 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 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>
---
 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 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>
---
 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 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, so combine the two functions rather than
having completely split versions under ifdefs.

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

diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 053e9b6..05f97a7 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -457,26 +457,11 @@ 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 defined(CONFIG_USER_ONLY)
+    g_assert_not_reached();
+#endif
     if ((env->uncached_cpsr & CPSR_M) == mode) {
         env->regs[13] = val;
     } else {
@@ -486,13 +471,15 @@ 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 defined(CONFIG_USER_ONLY)
+    g_assert_not_reached();
+#endif
     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 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>
---
 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 05f97a7..8183108 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -474,6 +474,14 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
 #if defined(CONFIG_USER_ONLY)
     g_assert_not_reached();
 #endif
+    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

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

[Sergey Fedorov]

On 11.02.2016 22:11, Peter Maydell wrote:
> 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>

> ---
>  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;

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

[Sergey Fedorov]

On 11.02.2016 22:11, Peter Maydell wrote:
> 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>

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

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

[Sergey Fedorov]

On 11.02.2016 22:11, Peter Maydell wrote:
> 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, so combine the two functions rather than
> having completely split versions under ifdefs.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/op_helper.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 053e9b6..05f97a7 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -457,26 +457,11 @@ 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 defined(CONFIG_USER_ONLY)
> +    g_assert_not_reached();
> +#endif
>      if ((env->uncached_cpsr & CPSR_M) == mode) {
>          env->regs[13] = val;
>      } else {
> @@ -486,13 +471,15 @@ 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 defined(CONFIG_USER_ONLY)
> +    g_assert_not_reached();
> +#endif
>      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)

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

[Sergey Fedorov]

On 11.02.2016 22:11, Peter Maydell wrote:
> 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>

It's a bit misleading that the name "get_r13_banked" says nothing about
SRS instruction but raises an SRS-specific exception. Though, it's only
used for SRS and there seems to be no other candidate to use it; so

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.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 05f97a7..8183108 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -474,6 +474,14 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
>  #if defined(CONFIG_USER_ONLY)
>      g_assert_not_reached();
>  #endif
> +    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)

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

[Edgar E. Iglesias]

On Thu, Feb 11, 2016 at 07:11:46PM +0000, Peter Maydell wrote:
> 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: 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
> 

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

[Edgar E. Iglesias]

On Thu, Feb 11, 2016 at 07:11:47PM +0000, Peter Maydell wrote:
> 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: 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
> 

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

[Edgar E. Iglesias]

On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
> 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, so combine the two functions rather than
> having completely split versions under ifdefs.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi Peter,

Do we really need the assert?
If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?

Cheers,
Edgar



> ---
>  target-arm/op_helper.c | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 053e9b6..05f97a7 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -457,26 +457,11 @@ 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 defined(CONFIG_USER_ONLY)
> +    g_assert_not_reached();
> +#endif
>      if ((env->uncached_cpsr & CPSR_M) == mode) {
>          env->regs[13] = val;
>      } else {
> @@ -486,13 +471,15 @@ 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 defined(CONFIG_USER_ONLY)
> +    g_assert_not_reached();
> +#endif
>      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
> 

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

[Peter Maydell]

On 12 February 2016 at 15:12, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
>> 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, so combine the two functions rather than
>> having completely split versions under ifdefs.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Hi Peter,
>
> Do we really need the assert?
> If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?

I would be happy to entirely drop the assert, yes.

thanks
-- PMM

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

[Edgar E. Iglesias]

On Fri, Feb 12, 2016 at 04:12:18PM +0100, Edgar E. Iglesias wrote:
> On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
> > 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, so combine the two functions rather than
> > having completely split versions under ifdefs.
> > 
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Hi Peter,
> 
> Do we really need the assert?
> If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?

e.g:

assert(arm_current_el(env) != 0);

Allthough, I'd probably vote for removing it...

Cheers,
Edgar



> 
> Cheers,
> Edgar
> 
> 
> 
> > ---
> >  target-arm/op_helper.c | 25 ++++++-------------------
> >  1 file changed, 6 insertions(+), 19 deletions(-)
> > 
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index 053e9b6..05f97a7 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -457,26 +457,11 @@ 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 defined(CONFIG_USER_ONLY)
> > +    g_assert_not_reached();
> > +#endif
> >      if ((env->uncached_cpsr & CPSR_M) == mode) {
> >          env->regs[13] = val;
> >      } else {
> > @@ -486,13 +471,15 @@ 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 defined(CONFIG_USER_ONLY)
> > +    g_assert_not_reached();
> > +#endif
> >      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
> > 

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

[Edgar E. Iglesias]

On Fri, Feb 12, 2016 at 03:15:22PM +0000, Peter Maydell wrote:
> On 12 February 2016 at 15:12, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
> >> 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, so combine the two functions rather than
> >> having completely split versions under ifdefs.
> >>
> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > Hi Peter,
> >
> > Do we really need the assert?
> > If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?
> 
> I would be happy to entirely drop the assert, yes.

OK, thanks.

With that change:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

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

[Edgar E. Iglesias]

On Thu, Feb 11, 2016 at 07:11:49PM +0000, Peter Maydell wrote:
> 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.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>


> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  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 05f97a7..8183108 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -474,6 +474,14 @@ uint32_t HELPER(get_r13_banked)(CPUARMState *env, uint32_t mode)
>  #if defined(CONFIG_USER_ONLY)
>      g_assert_not_reached();
>  #endif
> +    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
> 

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

[Sergey Fedorov]

On 12.02.2016 18:16, Edgar E. Iglesias wrote:
> On Fri, Feb 12, 2016 at 03:15:22PM +0000, Peter Maydell wrote:
>> On 12 February 2016 at 15:12, Edgar E. Iglesias
>> <edgar.iglesias@gmail.com> wrote:
>>> On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
>>>> 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, so combine the two functions rather than
>>>> having completely split versions under ifdefs.
>>>>
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> Hi Peter,
>>>
>>> Do we really need the assert?
>>> If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?
>> I would be happy to entirely drop the assert, yes.
> OK, thanks.
>
> With that change:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>

Yes, I also think it would be okay to drop that assert. Anyway:

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

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

[Peter Maydell]

On 12 February 2016 at 15:16, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Fri, Feb 12, 2016 at 03:15:22PM +0000, Peter Maydell wrote:
>> On 12 February 2016 at 15:12, Edgar E. Iglesias
>> <edgar.iglesias@gmail.com> wrote:
>> > On Thu, Feb 11, 2016 at 07:11:48PM +0000, Peter Maydell wrote:
>> >> 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, so combine the two functions rather than
>> >> having completely split versions under ifdefs.
>> >>
>> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> >
>> > Hi Peter,
>> >
>> > Do we really need the assert?
>> > If we keep it, can't we have it for both -user and -softmmu (avoiding the ifdef)?
>>
>> I would be happy to entirely drop the assert, yes.
>
> OK, thanks.

It turns out that the compiler was being clever and dropping all
the code after the g_assert_not_reached(), which meant I didn't
notice that bank_number() is only compiled in in the softmmu build.
We should just move bank_number() into being a static inline in
internals.h, I think -- it's pretty trivial. Will send that patch...

thanks
-- PMM