[Qemu-devel] [PATCH 0/7] arm: Implement M profile exception return properly

[Qemu-devel] [PATCH 0/7] arm: Implement M profile exception return properly

[Peter Maydell]

On M profile, return from exceptions happen when privileged code
executes one of the following function call return instructions:
     * POP or LDM which loads the PC
     * LDR to PC
     * BX register
and the new PC value is 0xFFxxxxxx.
    
QEMU tries to implement this by not treating the instruction
specially but then catching the attempt to execute from the magic
address value.  This is not ideal, because:
 * there are guest visible differences from the architecturally
   specified behaviour (for instance jumping to 0xFFxxxxxx via a
   different instruction should not cause an exception return but it
   will in the QEMU implementation)
 * we have to account for it in various places (like refusing to take
   an interrupt if the PC is at a magic value, and making sure that
   the MPU doesn't deny execution at the magic value addresses)
    
Drop these hacks, and instead implement exception return the way the
architecture specifies -- by having the relevant instructions check
for the magic value and raise the 'do an exception return' QEMU
internal exception immediately.

I realised when I was looking at the MPU patches that the current
mechanism was a bit awkward and needed still more workarounds in
code that really shouldn't have to care about exception-return.
And it turns out that implementing it properly doesn't actually
have very much runtime cost at all.

NB: this is technically speaking a migration compatibility break,
if you were unlucky enough to migrate just after an exception
return when the PC value was one of the magic values, since the
new QEMU will not treat this as "now do exception return".
However we don't guarantee migration compatibility for M profile,
so that's OK.
    
The effect on the generated code is minor:
    
     bx lr, old code (and new code for unprivileged mode):
      TCG:
       mov_i32 tmp5,r14
       movi_i32 tmp6,$0xfffffffffffffffe
       and_i32 pc,tmp5,tmp6
       movi_i32 tmp6,$0x1
       and_i32 tmp5,tmp5,tmp6
       st_i32 tmp5,env,$0x218
       exit_tb $0x0
      x86_64 generated code:
       0x7f2aabe87019:  mov    %ebx,%ebp
       0x7f2aabe8701b:  and    $0xfffffffffffffffe,%ebp
       0x7f2aabe8701e:  mov    %ebp,0x3c(%r14)
       0x7f2aabe87022:  and    $0x1,%ebx
       0x7f2aabe87025:  mov    %ebx,0x218(%r14)
       0x7f2aabe8702c:  xor    %eax,%eax
       0x7f2aabe8702e:  jmpq   0x7f2aabe7c016
    
     bx lr, new code when privileged:
      TCG:
       mov_i32 tmp5,r14
       movi_i32 tmp6,$0xfffffffffffffffe
       and_i32 pc,tmp5,tmp6
       movi_i32 tmp6,$0x1
       and_i32 tmp5,tmp5,tmp6
       st_i32 tmp5,env,$0x218
       movi_i32 tmp5,$0xffffffffff000000
       brcond_i32 pc,tmp5,geu,$L1
       exit_tb $0x0
       set_label $L1
       movi_i32 tmp5,$0x8
       call exception_internal,$0x0,$0,env,tmp5
      x86_64 generated code:
       0x7fe8fa1264e3:  mov    %ebp,%ebx
       0x7fe8fa1264e5:  and    $0xfffffffffffffffe,%ebx
       0x7fe8fa1264e8:  mov    %ebx,0x3c(%r14)
       0x7fe8fa1264ec:  and    $0x1,%ebp
       0x7fe8fa1264ef:  mov    %ebp,0x218(%r14)
       0x7fe8fa1264f6:  cmp    $0xff000000,%ebx
       0x7fe8fa1264fc:  jae    0x7fe8fa126509
       0x7fe8fa126502:  xor    %eax,%eax
       0x7fe8fa126504:  jmpq   0x7fe8fa122016
       0x7fe8fa126509:  mov    %r14,%rdi
       0x7fe8fa12650c:  mov    $0x8,%esi
       0x7fe8fa126511:  mov    $0x56095dbeccf5,%r10
       0x7fe8fa12651b:  callq  *%r10
    
which is a difference of one cmp/branch-not-taken. This will
be lost in the noise of having to exit generated code and
look up the next TB anyway.
    
Patches 1 and 2 are minor bug fixes which remove code paths that
incorrectly called gen_bx(), so we don't have to consider them when
looking at which gen_bx() calls need to be changed to call
gen_bx_excret().  Patches 3 to 5 are minor preliminary refactorings;
patch 6 is the meat of the series; finally patch 7 removes the old
mechanism.

thanks
-- PMM

Peter Maydell (7):
  arm: Don't implement BXJ on M-profile CPUs
  arm: Thumb shift operations should not permit interworking branches
  arm: Factor out "generate right kind of step exception"
  arm: Move gen_set_condexec() and gen_set_pc_im() up in the file
  arm: Move condition-failed codepath generation out of if()
  arm: Implement M profile exception return properly
  arm: Remove workarounds for old M-profile exception return
    implementation

 target/arm/translate.h |   4 ++
 target/arm/cpu.c       |  43 +------------
 target/arm/translate.c | 163 ++++++++++++++++++++++++++++++++-----------------
 3 files changed, 113 insertions(+), 97 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/7] arm: Don't implement BXJ on M-profile CPUs

[Peter Maydell]

For M-profile CPUs, the BXJ instruction does not exist at all, and
the encoding should always UNDEF. We were accidentally implementing
it to behave like A-profile BXJ; correct the error.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index e32e38c..fe3f442 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -10485,7 +10485,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                         }
                         break;
                     case 4: /* bxj */
-                        /* Trivial implementation equivalent to bx.  */
+                        /* Trivial implementation equivalent to bx.
+                         * This instruction doesn't exist at all for M-profile.
+                         */
+                        if (arm_dc_feature(s, ARM_FEATURE_M)) {
+                            goto illegal_op;
+                        }
                         tmp = load_reg(s, rn);
                         gen_bx(s, tmp);
                         break;
-- 
2.7.4

[Qemu-devel] [PATCH 2/7] arm: Thumb shift operations should not permit interworking branches

[Peter Maydell]

In Thumb mode, the only instructions which can cause an interworking
branch by writing the PC are BLX, BX, BXJ, LDR, POP and LDM. Unlike
ARM mode, data processing instructions which target the PC do not
cause interworking branches.

When we added support for doing interworking branches on writes to
PC from data processing instructions in commit 21aeb3430ce7ba, we
accidentally changed a Thumb instruction to have interworking
branch behaviour for writes to PC. (MOV, MOVS register-shifted
register, encoding T2; this is the standard encoding for
LSL/LSR/ASR/ROR (register).)

For this encoding, behaviour with Rd == R15 is specified as
UNPREDICTABLE, so allowing an interworking branch is within
spec, but it's confusing and differs from our handling of this
class of UNPREDICTABLE for other Thumb ALU operations. Make
it perform a simple (non-interworking) branch like the others.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index fe3f442..ddc62b6 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9959,7 +9959,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
             gen_arm_shift_reg(tmp, op, tmp2, logic_cc);
             if (logic_cc)
                 gen_logic_CC(tmp);
-            store_reg_bx(s, rd, tmp);
+            store_reg(s, rd, tmp);
             break;
         case 1: /* Sign/zero extend.  */
             op = (insn >> 20) & 7;
-- 
2.7.4

[Qemu-devel] [PATCH 3/7] arm: Factor out "generate right kind of step exception"

[Peter Maydell]

We currently have two places that do:
            if (dc->ss_active) {
                gen_step_complete_exception(dc);
            } else {
                gen_exception_internal(EXCP_DEBUG);
            }

Factor this out into its own function, as we're about to add
a third place that needs the same logic.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index ddc62b6..870e320 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -296,6 +296,19 @@ static void gen_step_complete_exception(DisasContext *s)
     s->is_jmp = DISAS_EXC;
 }
 
+static void gen_singlestep_exception(DisasContext *s)
+{
+    /* Generate the right kind of exception for singlestep, which is
+     * either the architectural singlestep or EXCP_DEBUG for QEMU's
+     * gdb singlestepping.
+     */
+    if (s->ss_active) {
+        gen_step_complete_exception(s);
+    } else {
+        gen_exception_internal(EXCP_DEBUG);
+    }
+}
+
 static void gen_smul_dual(TCGv_i32 a, TCGv_i32 b)
 {
     TCGv_i32 tmp1 = tcg_temp_new_i32();
@@ -11998,24 +12011,15 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             gen_set_pc_im(dc, dc->pc);
             /* fall through */
         default:
-            if (dc->ss_active) {
-                gen_step_complete_exception(dc);
-            } else {
-                /* FIXME: Single stepping a WFI insn will not halt
-                   the CPU.  */
-                gen_exception_internal(EXCP_DEBUG);
-            }
+            /* FIXME: Single stepping a WFI insn will not halt the CPU. */
+            gen_singlestep_exception(dc);
         }
         if (dc->condjmp) {
             /* "Condition failed" instruction codepath. */
             gen_set_label(dc->condlabel);
             gen_set_condexec(dc);
             gen_set_pc_im(dc, dc->pc);
-            if (dc->ss_active) {
-                gen_step_complete_exception(dc);
-            } else {
-                gen_exception_internal(EXCP_DEBUG);
-            }
+            gen_singlestep_exception(dc);
         }
     } else {
         /* While branches must always occur at the end of an IT block,
-- 
2.7.4

[Qemu-devel] [PATCH 4/7] arm: Move gen_set_condexec() and gen_set_pc_im() up in the file

[Peter Maydell]

Move the utility routines gen_set_condexec() and gen_set_pc_im()
up in the file, as we will want to use them from a function
placed earlier in the file than their current location.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 870e320..a1a0e73 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -893,6 +893,21 @@ static const uint8_t table_logic_cc[16] = {
     1, /* mvn */
 };
 
+static inline void gen_set_condexec(DisasContext *s)
+{
+    if (s->condexec_mask) {
+        uint32_t val = (s->condexec_cond << 4) | (s->condexec_mask >> 1);
+        TCGv_i32 tmp = tcg_temp_new_i32();
+        tcg_gen_movi_i32(tmp, val);
+        store_cpu_field(tmp, condexec_bits);
+    }
+}
+
+static inline void gen_set_pc_im(DisasContext *s, target_ulong val)
+{
+    tcg_gen_movi_i32(cpu_R[15], val);
+}
+
 /* Set PC and Thumb state from an immediate address.  */
 static inline void gen_bx_im(DisasContext *s, uint32_t addr)
 {
@@ -1069,11 +1084,6 @@ DO_GEN_ST(8, MO_UB)
 DO_GEN_ST(16, MO_UW)
 DO_GEN_ST(32, MO_UL)
 
-static inline void gen_set_pc_im(DisasContext *s, target_ulong val)
-{
-    tcg_gen_movi_i32(cpu_R[15], val);
-}
-
 static inline void gen_hvc(DisasContext *s, int imm16)
 {
     /* The pre HVC helper handles cases when HVC gets trapped
@@ -1107,17 +1117,6 @@ static inline void gen_smc(DisasContext *s)
     s->is_jmp = DISAS_SMC;
 }
 
-static inline void
-gen_set_condexec (DisasContext *s)
-{
-    if (s->condexec_mask) {
-        uint32_t val = (s->condexec_cond << 4) | (s->condexec_mask >> 1);
-        TCGv_i32 tmp = tcg_temp_new_i32();
-        tcg_gen_movi_i32(tmp, val);
-        store_cpu_field(tmp, condexec_bits);
-    }
-}
-
 static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
 {
     gen_set_condexec(s);
-- 
2.7.4

[Qemu-devel] [PATCH 5/7] arm: Move condition-failed codepath generation out of if()

[Peter Maydell]

Move the code to generate the "condition failed" instruction
codepath out of the if (singlestepping) {} else {}. This
will allow adding support for handling a new is_jmp type
which can't be neatly split into "singlestepping case"
versus "not singlestepping case".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index a1a0e73..87fd702 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11988,9 +11988,9 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
     /* At this stage dc->condjmp will only be set when the skipped
        instruction was a conditional branch or trap, and the PC has
        already been written.  */
+    gen_set_condexec(dc);
     if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
         /* Unconditional and "condition passed" instruction codepath. */
-        gen_set_condexec(dc);
         switch (dc->is_jmp) {
         case DISAS_SWI:
             gen_ss_advance(dc);
@@ -12013,13 +12013,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             /* FIXME: Single stepping a WFI insn will not halt the CPU. */
             gen_singlestep_exception(dc);
         }
-        if (dc->condjmp) {
-            /* "Condition failed" instruction codepath. */
-            gen_set_label(dc->condlabel);
-            gen_set_condexec(dc);
-            gen_set_pc_im(dc, dc->pc);
-            gen_singlestep_exception(dc);
-        }
     } else {
         /* While branches must always occur at the end of an IT block,
            there are a few other things that can cause us to terminate
@@ -12029,7 +12022,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             - Hardware watchpoints.
            Hardware breakpoints have already been handled and skip this code.
          */
-        gen_set_condexec(dc);
         switch(dc->is_jmp) {
         case DISAS_NEXT:
             gen_goto_tb(dc, 1, dc->pc);
@@ -12069,11 +12061,17 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
             break;
         }
-        if (dc->condjmp) {
-            gen_set_label(dc->condlabel);
-            gen_set_condexec(dc);
+    }
+
+    if (dc->condjmp) {
+        /* "Condition failed" instruction codepath for the branch/trap insn */
+        gen_set_label(dc->condlabel);
+        gen_set_condexec(dc);
+        if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
+            gen_set_pc_im(dc, dc->pc);
+            gen_singlestep_exception(dc);
+        } else {
             gen_goto_tb(dc, 1, dc->pc);
-            dc->condjmp = 0;
         }
     }
 
-- 
2.7.4

[Qemu-devel] [PATCH 6/7] arm: Implement M profile exception return properly

[Peter Maydell]

On M profile, return from exceptions happen when privileged code
executes one of the following function call return instructions:
 * POP or LDM which loads the PC
 * LDR to PC
 * BX register
and the new PC value is 0xFFxxxxxx.

QEMU tries to implement this by not treating the instruction
specially but then catching the attempt to execute from the magic
address value.  This is not ideal, because:
 * there are guest visible differences from the architecturally
   specified behaviour (for instance jumping to 0xFFxxxxxx via a
   different instruction should not cause an exception return but it
   will in the QEMU implementation)
 * we have to account for it in various places (like refusing to take
   an interrupt if the PC is at a magic value, and making sure that
   the MPU doesn't deny execution at the magic value addresses)

Drop these hacks, and instead implement exception return the way the
architecture specifies -- by having the relevant instructions check
for the magic value and raise the 'do an exception return' QEMU
internal exception immediately.

The effect on the generated code is minor:

 bx lr, old code (and new code for unprivileged mode):
  TCG:
   mov_i32 tmp5,r14
   movi_i32 tmp6,$0xfffffffffffffffe
   and_i32 pc,tmp5,tmp6
   movi_i32 tmp6,$0x1
   and_i32 tmp5,tmp5,tmp6
   st_i32 tmp5,env,$0x218
   exit_tb $0x0
   set_label $L0
   exit_tb $0x7f2aabd61993
  x86_64 generated code:
   0x7f2aabe87019:  mov    %ebx,%ebp
   0x7f2aabe8701b:  and    $0xfffffffffffffffe,%ebp
   0x7f2aabe8701e:  mov    %ebp,0x3c(%r14)
   0x7f2aabe87022:  and    $0x1,%ebx
   0x7f2aabe87025:  mov    %ebx,0x218(%r14)
   0x7f2aabe8702c:  xor    %eax,%eax
   0x7f2aabe8702e:  jmpq   0x7f2aabe7c016

 bx lr, new code when privileged:
  TCG:
   mov_i32 tmp5,r14
   movi_i32 tmp6,$0xfffffffffffffffe
   and_i32 pc,tmp5,tmp6
   movi_i32 tmp6,$0x1
   and_i32 tmp5,tmp5,tmp6
   st_i32 tmp5,env,$0x218
   movi_i32 tmp5,$0xffffffffff000000
   brcond_i32 pc,tmp5,geu,$L1
   exit_tb $0x0
   set_label $L1
   movi_i32 tmp5,$0x8
   call exception_internal,$0x0,$0,env,tmp5
  x86_64 generated code:
   0x7fe8fa1264e3:  mov    %ebp,%ebx
   0x7fe8fa1264e5:  and    $0xfffffffffffffffe,%ebx
   0x7fe8fa1264e8:  mov    %ebx,0x3c(%r14)
   0x7fe8fa1264ec:  and    $0x1,%ebp
   0x7fe8fa1264ef:  mov    %ebp,0x218(%r14)
   0x7fe8fa1264f6:  cmp    $0xff000000,%ebx
   0x7fe8fa1264fc:  jae    0x7fe8fa126509
   0x7fe8fa126502:  xor    %eax,%eax
   0x7fe8fa126504:  jmpq   0x7fe8fa122016
   0x7fe8fa126509:  mov    %r14,%rdi
   0x7fe8fa12650c:  mov    $0x8,%esi
   0x7fe8fa126511:  mov    $0x56095dbeccf5,%r10
   0x7fe8fa12651b:  callq  *%r10

which is a difference of one cmp/branch-not-taken. This will
be lost in the noise of having to exit generated code and
look up the next TB anyway.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/translate.h |  4 ++++
 target/arm/translate.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/target/arm/translate.h b/target/arm/translate.h
index abb0760..c2a5451 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -134,6 +134,10 @@ static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
 #define DISAS_HVC 8
 #define DISAS_SMC 9
 #define DISAS_YIELD 10
+/* M profile branch which might be an exception return (and so needs
+ * custom end-of-TB code)
+ */
+#define DISAS_BX_EXCRET 11
 
 #ifdef TARGET_AARCH64
 void a64_translate_init(void);
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 87fd702..156ab46 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -932,6 +932,51 @@ static inline void gen_bx(DisasContext *s, TCGv_i32 var)
     store_cpu_field(var, thumb);
 }
 
+/* Set PC and Thumb state from var. var is marked as dead.
+ * For M-profile CPUs, include logic to detect exception-return
+ * branches and handle them.
+ * This is needed for Thumb POP/LDM to PC, LDR to PC, and BX reg, and no others.
+ */
+static inline void gen_bx_excret(DisasContext *s, TCGv_i32 var)
+{
+    /* Generate the same code here as for a simple bx, but flag via
+     * s->is_jmp that we need to do the rest of the work later.
+     */
+    gen_bx(s, var);
+    if (!IS_USER(s) && arm_dc_feature(s, ARM_FEATURE_M)) {
+        s->is_jmp = DISAS_BX_EXCRET;
+    }
+}
+
+static inline void gen_bx_excret_final_code(DisasContext *s)
+{
+    /* Generate the code to finish possible exception return and end the TB */
+    TCGLabel *excret_label = gen_new_label();
+
+    /* Is the new PC value in the magic range indicating exception return? */
+    tcg_gen_brcondi_i32(TCG_COND_GEU, cpu_R[15], 0xff000000, excret_label);
+    /* No: end the TB as we would for a DISAS_JMP */
+    if (s->singlestep_enabled || s->ss_active) {
+        gen_singlestep_exception(s);
+    } else {
+        tcg_gen_exit_tb(0);
+    }
+    gen_set_label(excret_label);
+    /* Yes: this is an exception return.
+     * At this point in runtime env->regs[15] and env->thumb will hold
+     * the exception-return magic number, which do_v7m_exception_exit()
+     * will read. Nothing else will be able to see those values because
+     * the cpu-exec main loop guarantees that we will always go straight
+     * from raising the exception to the exception-handling code.
+     *
+     * gen_ss_advance(s) does nothing on M profile currently but
+     * calling it is conceptually the right thing as we have executed
+     * this instruction (compare SWI, HVC, SMC handling).
+     */
+    gen_ss_advance(s);
+    gen_exception_internal(EXCP_EXCEPTION_EXIT);
+}
+
 /* Variant of store_reg which uses branch&exchange logic when storing
    to r15 in ARM architecture v7 and above. The source must be a temporary
    and will be marked as dead. */
@@ -951,7 +996,7 @@ static inline void store_reg_bx(DisasContext *s, int reg, TCGv_i32 var)
 static inline void store_reg_from_load(DisasContext *s, int reg, TCGv_i32 var)
 {
     if (reg == 15 && ENABLE_ARCH_5) {
-        gen_bx(s, var);
+        gen_bx_excret(s, var);
     } else {
         store_reg(s, reg, var);
     }
@@ -9870,7 +9915,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                         tmp = tcg_temp_new_i32();
                         gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
                         if (i == 15) {
-                            gen_bx(s, tmp);
+                            gen_bx_excret(s, tmp);
                         } else if (i == rn) {
                             loaded_var = tmp;
                             loaded_base = 1;
@@ -10902,7 +10947,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
                 goto illegal_op;
             }
             if (rs == 15) {
-                gen_bx(s, tmp);
+                gen_bx_excret(s, tmp);
             } else {
                 store_reg(s, rs, tmp);
             }
@@ -11092,9 +11137,10 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
                     tmp2 = tcg_temp_new_i32();
                     tcg_gen_movi_i32(tmp2, val);
                     store_reg(s, 14, tmp2);
+                    gen_bx(s, tmp);
+                } else {
+                    gen_bx_excret(s, tmp);
                 }
-                /* already thumb, no need to check */
-                gen_bx(s, tmp);
                 break;
             }
             break;
@@ -11989,7 +12035,14 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
        instruction was a conditional branch or trap, and the PC has
        already been written.  */
     gen_set_condexec(dc);
-    if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
+    if (dc->is_jmp == DISAS_BX_EXCRET) {
+        /* Exception return branches need some special case code at the
+         * end of the TB, which is complex enough that it has to
+         * handle the single-step vs not and the condition-failed
+         * insn codepath itself.
+         */
+        gen_bx_excret_final_code(dc);
+    } else if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
         /* Unconditional and "condition passed" instruction codepath. */
         switch (dc->is_jmp) {
         case DISAS_SWI:
-- 
2.7.4

[Qemu-devel] [PATCH 7/7] arm: Remove workarounds for old M-profile exception return implementation

[Peter Maydell]

Now that we've rewritten M-profile exception return so that the magic
PC values are not visible to other parts of QEMU, we can delete the
special casing of them elsewhere.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c       | 43 ++-----------------------------------------
 target/arm/translate.c |  8 --------
 2 files changed, 2 insertions(+), 49 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 04b062c..b357aee 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -304,33 +304,6 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 }
 
 #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
-static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
-                                      bool is_write, bool is_exec, int opaque,
-                                      unsigned size)
-{
-    ARMCPU *arm = ARM_CPU(cpu);
-    CPUARMState *env = &arm->env;
-
-    /* ARMv7-M interrupt return works by loading a magic value into the PC.
-     * On real hardware the load causes the return to occur.  The qemu
-     * implementation performs the jump normally, then does the exception
-     * return by throwing a special exception when when the CPU tries to
-     * execute code at the magic address.
-     */
-    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && is_exec) {
-        cpu->exception_index = EXCP_EXCEPTION_EXIT;
-        cpu_loop_exit(cpu);
-    }
-
-    /* In real hardware an attempt to access parts of the address space
-     * with nothing there will usually cause an external abort.
-     * However our QEMU board models are often missing device models where
-     * the guest can boot anyway with the default read-as-zero/writes-ignored
-     * behaviour that you get without a QEMU unassigned_access hook.
-     * So just return here to retain that default behaviour.
-     */
-}
-
 static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
@@ -338,17 +311,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     CPUARMState *env = &cpu->env;
     bool ret = false;
 
-    /* ARMv7-M interrupt return works by loading a magic value
-     * into the PC.  On real hardware the load causes the
-     * return to occur.  The qemu implementation performs the
-     * jump normally, then does the exception return when the
-     * CPU tries to execute code at the magic address.
-     * This will cause the magic PC value to be pushed to
-     * the stack if an interrupt occurred at the wrong time.
-     * We avoid this by disabling interrupts when
-     * pc contains a magic address.
-     *
-     * ARMv7-M interrupt masking works differently than -A or -R.
+    /* ARMv7-M interrupt masking works differently than -A or -R.
      * There is no FIQ/IRQ distinction. Instead of I and F bits
      * masking FIQ and IRQ interrupts, an exception is taken only
      * if it is higher priority than the current execution priority
@@ -356,8 +319,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
      * currently active exception).
      */
     if (interrupt_request & CPU_INTERRUPT_HARD
-        && (armv7m_nvic_can_take_pending_exception(env->nvic))
-        && (env->regs[15] < 0xfffffff0)) {
+        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
         cs->exception_index = EXCP_IRQ;
         cc->do_interrupt(cs);
         ret = true;
@@ -1091,7 +1053,6 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = arm_v7m_cpu_do_interrupt;
 #endif
 
-    cc->do_unassigned_access = arm_v7m_unassigned_access;
     cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
 }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 156ab46..c85bc6c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11914,14 +11914,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             dc->is_jmp = DISAS_EXC;
             break;
         }
-#else
-        if (arm_dc_feature(dc, ARM_FEATURE_M)) {
-            /* Branches to the magic exception-return addresses should
-             * already have been caught via the arm_v7m_unassigned_access hook,
-             * and never get here.
-             */
-            assert(dc->pc < 0xfffffff0);
-        }
 #endif
 
         if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 3/7] arm: Factor out "generate right kind of step exception"

[Philippe Mathieu-Daudé]

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> We currently have two places that do:
>             if (dc->ss_active) {
>                 gen_step_complete_exception(dc);
>             } else {
>                 gen_exception_internal(EXCP_DEBUG);
>             }
>
> Factor this out into its own function, as we're about to add
> a third place that needs the same logic.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/translate.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index ddc62b6..870e320 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -296,6 +296,19 @@ static void gen_step_complete_exception(DisasContext *s)
>      s->is_jmp = DISAS_EXC;
>  }
>
> +static void gen_singlestep_exception(DisasContext *s)
> +{
> +    /* Generate the right kind of exception for singlestep, which is
> +     * either the architectural singlestep or EXCP_DEBUG for QEMU's
> +     * gdb singlestepping.
> +     */
> +    if (s->ss_active) {
> +        gen_step_complete_exception(s);
> +    } else {
> +        gen_exception_internal(EXCP_DEBUG);
> +    }
> +}
> +
>  static void gen_smul_dual(TCGv_i32 a, TCGv_i32 b)
>  {
>      TCGv_i32 tmp1 = tcg_temp_new_i32();
> @@ -11998,24 +12011,15 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              gen_set_pc_im(dc, dc->pc);
>              /* fall through */
>          default:
> -            if (dc->ss_active) {
> -                gen_step_complete_exception(dc);
> -            } else {
> -                /* FIXME: Single stepping a WFI insn will not halt
> -                   the CPU.  */
> -                gen_exception_internal(EXCP_DEBUG);
> -            }
> +            /* FIXME: Single stepping a WFI insn will not halt the CPU. */
> +            gen_singlestep_exception(dc);
>          }
>          if (dc->condjmp) {
>              /* "Condition failed" instruction codepath. */
>              gen_set_label(dc->condlabel);
>              gen_set_condexec(dc);
>              gen_set_pc_im(dc, dc->pc);
> -            if (dc->ss_active) {
> -                gen_step_complete_exception(dc);
> -            } else {
> -                gen_exception_internal(EXCP_DEBUG);
> -            }
> +            gen_singlestep_exception(dc);
>          }
>      } else {
>          /* While branches must always occur at the end of an IT block,
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/7] arm: Don't implement BXJ on M-profile CPUs

[Philippe Mathieu-Daudé]

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> For M-profile CPUs, the BXJ instruction does not exist at all, and
> the encoding should always UNDEF. We were accidentally implementing
> it to behave like A-profile BXJ; correct the error.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/translate.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index e32e38c..fe3f442 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -10485,7 +10485,12 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                          }
>                          break;
>                      case 4: /* bxj */
> -                        /* Trivial implementation equivalent to bx.  */
> +                        /* Trivial implementation equivalent to bx.
> +                         * This instruction doesn't exist at all for M-profile.
> +                         */
> +                        if (arm_dc_feature(s, ARM_FEATURE_M)) {
> +                            goto illegal_op;
> +                        }
>                          tmp = load_reg(s, rn);
>                          gen_bx(s, tmp);
>                          break;
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 4/7] arm: Move gen_set_condexec() and gen_set_pc_im() up in the file

[Philippe Mathieu-Daudé]

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> Move the utility routines gen_set_condexec() and gen_set_pc_im()
> up in the file, as we will want to use them from a function
> placed earlier in the file than their current location.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/translate.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 870e320..a1a0e73 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -893,6 +893,21 @@ static const uint8_t table_logic_cc[16] = {
>      1, /* mvn */
>  };
>
> +static inline void gen_set_condexec(DisasContext *s)
> +{
> +    if (s->condexec_mask) {
> +        uint32_t val = (s->condexec_cond << 4) | (s->condexec_mask >> 1);
> +        TCGv_i32 tmp = tcg_temp_new_i32();
> +        tcg_gen_movi_i32(tmp, val);
> +        store_cpu_field(tmp, condexec_bits);
> +    }
> +}
> +
> +static inline void gen_set_pc_im(DisasContext *s, target_ulong val)
> +{
> +    tcg_gen_movi_i32(cpu_R[15], val);
> +}
> +
>  /* Set PC and Thumb state from an immediate address.  */
>  static inline void gen_bx_im(DisasContext *s, uint32_t addr)
>  {
> @@ -1069,11 +1084,6 @@ DO_GEN_ST(8, MO_UB)
>  DO_GEN_ST(16, MO_UW)
>  DO_GEN_ST(32, MO_UL)
>
> -static inline void gen_set_pc_im(DisasContext *s, target_ulong val)
> -{
> -    tcg_gen_movi_i32(cpu_R[15], val);
> -}
> -
>  static inline void gen_hvc(DisasContext *s, int imm16)
>  {
>      /* The pre HVC helper handles cases when HVC gets trapped
> @@ -1107,17 +1117,6 @@ static inline void gen_smc(DisasContext *s)
>      s->is_jmp = DISAS_SMC;
>  }
>
> -static inline void
> -gen_set_condexec (DisasContext *s)
> -{
> -    if (s->condexec_mask) {
> -        uint32_t val = (s->condexec_cond << 4) | (s->condexec_mask >> 1);
> -        TCGv_i32 tmp = tcg_temp_new_i32();
> -        tcg_gen_movi_i32(tmp, val);
> -        store_cpu_field(tmp, condexec_bits);
> -    }
> -}
> -
>  static void gen_exception_internal_insn(DisasContext *s, int offset, int excp)
>  {
>      gen_set_condexec(s);
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/7] arm: Move condition-failed codepath generation out of if()

[Philippe Mathieu-Daudé]

Hi Peter,

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> Move the code to generate the "condition failed" instruction
> codepath out of the if (singlestepping) {} else {}. This
> will allow adding support for handling a new is_jmp type
> which can't be neatly split into "singlestepping case"
> versus "not singlestepping case".

This makes also the code more readable :)

>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index a1a0e73..87fd702 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11988,9 +11988,9 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>      /* At this stage dc->condjmp will only be set when the skipped
>         instruction was a conditional branch or trap, and the PC has
>         already been written.  */
> +    gen_set_condexec(dc);
>      if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
>          /* Unconditional and "condition passed" instruction codepath. */
> -        gen_set_condexec(dc);
>          switch (dc->is_jmp) {
>          case DISAS_SWI:
>              gen_ss_advance(dc);
> @@ -12013,13 +12013,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              /* FIXME: Single stepping a WFI insn will not halt the CPU. */
>              gen_singlestep_exception(dc);
>          }
> -        if (dc->condjmp) {
> -            /* "Condition failed" instruction codepath. */
> -            gen_set_label(dc->condlabel);
> -            gen_set_condexec(dc);
> -            gen_set_pc_im(dc, dc->pc);
> -            gen_singlestep_exception(dc);
> -        }
>      } else {
>          /* While branches must always occur at the end of an IT block,
>             there are a few other things that can cause us to terminate
> @@ -12029,7 +12022,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              - Hardware watchpoints.
>             Hardware breakpoints have already been handled and skip this code.
>           */
> -        gen_set_condexec(dc);
>          switch(dc->is_jmp) {
>          case DISAS_NEXT:
>              gen_goto_tb(dc, 1, dc->pc);
> @@ -12069,11 +12061,17 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              gen_exception(EXCP_SMC, syn_aa32_smc(), 3);
>              break;
>          }
> -        if (dc->condjmp) {
> -            gen_set_label(dc->condlabel);
> -            gen_set_condexec(dc);
> +    }
> +
> +    if (dc->condjmp) {
> +        /* "Condition failed" instruction codepath for the branch/trap insn */
> +        gen_set_label(dc->condlabel);
> +        gen_set_condexec(dc);
> +        if (unlikely(cs->singlestep_enabled || dc->ss_active)) {

I'm custom to think "let's change that and think how to protect the code 
to help the next one who will modify it" and wonder if it isn't safer to 
define:

const bool singlestepping;

singlestepping = cs->singlestep_enabled || dc->ss_active;

Then use:

if unlikely(singlestepping)

At line 11993 and here, what do you think?

Either way:

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

> +            gen_set_pc_im(dc, dc->pc);
> +            gen_singlestep_exception(dc);
> +        } else {
>              gen_goto_tb(dc, 1, dc->pc);
> -            dc->condjmp = 0;
>          }
>      }
>
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] arm: Implement M profile exception return properly

[Philippe Mathieu-Daudé]

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> On M profile, return from exceptions happen when privileged code
> executes one of the following function call return instructions:
>  * POP or LDM which loads the PC
>  * LDR to PC
>  * BX register
> and the new PC value is 0xFFxxxxxx.
>
> QEMU tries to implement this by not treating the instruction
> specially but then catching the attempt to execute from the magic
> address value.  This is not ideal, because:
>  * there are guest visible differences from the architecturally
>    specified behaviour (for instance jumping to 0xFFxxxxxx via a
>    different instruction should not cause an exception return but it
>    will in the QEMU implementation)
>  * we have to account for it in various places (like refusing to take
>    an interrupt if the PC is at a magic value, and making sure that
>    the MPU doesn't deny execution at the magic value addresses)
>
> Drop these hacks, and instead implement exception return the way the
> architecture specifies -- by having the relevant instructions check
> for the magic value and raise the 'do an exception return' QEMU
> internal exception immediately.
>
> The effect on the generated code is minor:
>
>  bx lr, old code (and new code for unprivileged mode):
>   TCG:
>    mov_i32 tmp5,r14
>    movi_i32 tmp6,$0xfffffffffffffffe
>    and_i32 pc,tmp5,tmp6
>    movi_i32 tmp6,$0x1
>    and_i32 tmp5,tmp5,tmp6
>    st_i32 tmp5,env,$0x218
>    exit_tb $0x0
>    set_label $L0
>    exit_tb $0x7f2aabd61993
>   x86_64 generated code:
>    0x7f2aabe87019:  mov    %ebx,%ebp
>    0x7f2aabe8701b:  and    $0xfffffffffffffffe,%ebp
>    0x7f2aabe8701e:  mov    %ebp,0x3c(%r14)
>    0x7f2aabe87022:  and    $0x1,%ebx
>    0x7f2aabe87025:  mov    %ebx,0x218(%r14)
>    0x7f2aabe8702c:  xor    %eax,%eax
>    0x7f2aabe8702e:  jmpq   0x7f2aabe7c016
>
>  bx lr, new code when privileged:
>   TCG:
>    mov_i32 tmp5,r14
>    movi_i32 tmp6,$0xfffffffffffffffe
>    and_i32 pc,tmp5,tmp6
>    movi_i32 tmp6,$0x1
>    and_i32 tmp5,tmp5,tmp6
>    st_i32 tmp5,env,$0x218
>    movi_i32 tmp5,$0xffffffffff000000
>    brcond_i32 pc,tmp5,geu,$L1
>    exit_tb $0x0
>    set_label $L1
>    movi_i32 tmp5,$0x8
>    call exception_internal,$0x0,$0,env,tmp5
>   x86_64 generated code:
>    0x7fe8fa1264e3:  mov    %ebp,%ebx
>    0x7fe8fa1264e5:  and    $0xfffffffffffffffe,%ebx
>    0x7fe8fa1264e8:  mov    %ebx,0x3c(%r14)
>    0x7fe8fa1264ec:  and    $0x1,%ebp
>    0x7fe8fa1264ef:  mov    %ebp,0x218(%r14)
>    0x7fe8fa1264f6:  cmp    $0xff000000,%ebx
>    0x7fe8fa1264fc:  jae    0x7fe8fa126509
>    0x7fe8fa126502:  xor    %eax,%eax
>    0x7fe8fa126504:  jmpq   0x7fe8fa122016
>    0x7fe8fa126509:  mov    %r14,%rdi
>    0x7fe8fa12650c:  mov    $0x8,%esi
>    0x7fe8fa126511:  mov    $0x56095dbeccf5,%r10
>    0x7fe8fa12651b:  callq  *%r10
>
> which is a difference of one cmp/branch-not-taken. This will
> be lost in the noise of having to exit generated code and
> look up the next TB anyway.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.h |  4 ++++
>  target/arm/translate.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 63 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index abb0760..c2a5451 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -134,6 +134,10 @@ static void disas_set_insn_syndrome(DisasContext *s, uint32_t syn)
>  #define DISAS_HVC 8
>  #define DISAS_SMC 9
>  #define DISAS_YIELD 10
> +/* M profile branch which might be an exception return (and so needs
> + * custom end-of-TB code)
> + */
> +#define DISAS_BX_EXCRET 11
>
>  #ifdef TARGET_AARCH64
>  void a64_translate_init(void);
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 87fd702..156ab46 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -932,6 +932,51 @@ static inline void gen_bx(DisasContext *s, TCGv_i32 var)
>      store_cpu_field(var, thumb);
>  }
>
> +/* Set PC and Thumb state from var. var is marked as dead.
> + * For M-profile CPUs, include logic to detect exception-return
> + * branches and handle them.
> + * This is needed for Thumb POP/LDM to PC, LDR to PC, and BX reg, and no others.
> + */
> +static inline void gen_bx_excret(DisasContext *s, TCGv_i32 var)
> +{
> +    /* Generate the same code here as for a simple bx, but flag via
> +     * s->is_jmp that we need to do the rest of the work later.
> +     */
> +    gen_bx(s, var);
> +    if (!IS_USER(s) && arm_dc_feature(s, ARM_FEATURE_M)) {
> +        s->is_jmp = DISAS_BX_EXCRET;
> +    }
> +}
> +
> +static inline void gen_bx_excret_final_code(DisasContext *s)
> +{
> +    /* Generate the code to finish possible exception return and end the TB */
> +    TCGLabel *excret_label = gen_new_label();
> +
> +    /* Is the new PC value in the magic range indicating exception return? */
> +    tcg_gen_brcondi_i32(TCG_COND_GEU, cpu_R[15], 0xff000000, excret_label);

Idea for a GSoC: branch prediction hints!

tcg_gen_brcondi_i32(TCG_COND_GEU | TCG_COND_UNLIKELY, ...

> +    /* No: end the TB as we would for a DISAS_JMP */
> +    if (s->singlestep_enabled || s->ss_active) {
> +        gen_singlestep_exception(s);
> +    } else {
> +        tcg_gen_exit_tb(0);
> +    }
> +    gen_set_label(excret_label);
> +    /* Yes: this is an exception return.
> +     * At this point in runtime env->regs[15] and env->thumb will hold
> +     * the exception-return magic number, which do_v7m_exception_exit()
> +     * will read. Nothing else will be able to see those values because
> +     * the cpu-exec main loop guarantees that we will always go straight
> +     * from raising the exception to the exception-handling code.
> +     *
> +     * gen_ss_advance(s) does nothing on M profile currently but
> +     * calling it is conceptually the right thing as we have executed
> +     * this instruction (compare SWI, HVC, SMC handling).
> +     */
> +    gen_ss_advance(s);
> +    gen_exception_internal(EXCP_EXCEPTION_EXIT);
> +}
> +
>  /* Variant of store_reg which uses branch&exchange logic when storing
>     to r15 in ARM architecture v7 and above. The source must be a temporary
>     and will be marked as dead. */
> @@ -951,7 +996,7 @@ static inline void store_reg_bx(DisasContext *s, int reg, TCGv_i32 var)
>  static inline void store_reg_from_load(DisasContext *s, int reg, TCGv_i32 var)
>  {
>      if (reg == 15 && ENABLE_ARCH_5) {
> -        gen_bx(s, var);
> +        gen_bx_excret(s, var);
>      } else {
>          store_reg(s, reg, var);
>      }
> @@ -9870,7 +9915,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                          tmp = tcg_temp_new_i32();
>                          gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
>                          if (i == 15) {
> -                            gen_bx(s, tmp);
> +                            gen_bx_excret(s, tmp);
>                          } else if (i == rn) {
>                              loaded_var = tmp;
>                              loaded_base = 1;
> @@ -10902,7 +10947,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                  goto illegal_op;
>              }
>              if (rs == 15) {
> -                gen_bx(s, tmp);
> +                gen_bx_excret(s, tmp);
>              } else {
>                  store_reg(s, rs, tmp);
>              }
> @@ -11092,9 +11137,10 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
>                      tmp2 = tcg_temp_new_i32();
>                      tcg_gen_movi_i32(tmp2, val);
>                      store_reg(s, 14, tmp2);
> +                    gen_bx(s, tmp);
> +                } else {
> +                    gen_bx_excret(s, tmp);

This change was not easy to understand, can you add a one line comment?

>                  }
> -                /* already thumb, no need to check */
> -                gen_bx(s, tmp);
>                  break;
>              }
>              break;
> @@ -11989,7 +12035,14 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>         instruction was a conditional branch or trap, and the PC has
>         already been written.  */
>      gen_set_condexec(dc);
> -    if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
> +    if (dc->is_jmp == DISAS_BX_EXCRET) {
> +        /* Exception return branches need some special case code at the
> +         * end of the TB, which is complex enough that it has to
> +         * handle the single-step vs not and the condition-failed
> +         * insn codepath itself.
> +         */
> +        gen_bx_excret_final_code(dc);
> +    } else if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
>          /* Unconditional and "condition passed" instruction codepath. */
>          switch (dc->is_jmp) {
>          case DISAS_SWI:
>

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

Re: [Qemu-devel] [Qemu-arm] [PATCH 7/7] arm: Remove workarounds for old M-profile exception return implementation

[Philippe Mathieu-Daudé]

On 04/10/2017 07:39 AM, Peter Maydell wrote:
> Now that we've rewritten M-profile exception return so that the magic
> PC values are not visible to other parts of QEMU, we can delete the
> special casing of them elsewhere.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/arm/cpu.c       | 43 ++-----------------------------------------
>  target/arm/translate.c |  8 --------
>  2 files changed, 2 insertions(+), 49 deletions(-)
>
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 04b062c..b357aee 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -304,33 +304,6 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  }
>
>  #if !defined(CONFIG_USER_ONLY) || !defined(TARGET_AARCH64)
> -static void arm_v7m_unassigned_access(CPUState *cpu, hwaddr addr,
> -                                      bool is_write, bool is_exec, int opaque,
> -                                      unsigned size)
> -{
> -    ARMCPU *arm = ARM_CPU(cpu);
> -    CPUARMState *env = &arm->env;
> -
> -    /* ARMv7-M interrupt return works by loading a magic value into the PC.
> -     * On real hardware the load causes the return to occur.  The qemu
> -     * implementation performs the jump normally, then does the exception
> -     * return by throwing a special exception when when the CPU tries to
> -     * execute code at the magic address.
> -     */
> -    if (env->v7m.exception != 0 && addr >= 0xfffffff0 && is_exec) {
> -        cpu->exception_index = EXCP_EXCEPTION_EXIT;
> -        cpu_loop_exit(cpu);
> -    }
> -
> -    /* In real hardware an attempt to access parts of the address space
> -     * with nothing there will usually cause an external abort.
> -     * However our QEMU board models are often missing device models where
> -     * the guest can boot anyway with the default read-as-zero/writes-ignored
> -     * behaviour that you get without a QEMU unassigned_access hook.
> -     * So just return here to retain that default behaviour.
> -     */
> -}
> -
>  static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cs);
> @@ -338,17 +311,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>      CPUARMState *env = &cpu->env;
>      bool ret = false;
>
> -    /* ARMv7-M interrupt return works by loading a magic value
> -     * into the PC.  On real hardware the load causes the
> -     * return to occur.  The qemu implementation performs the
> -     * jump normally, then does the exception return when the
> -     * CPU tries to execute code at the magic address.
> -     * This will cause the magic PC value to be pushed to
> -     * the stack if an interrupt occurred at the wrong time.
> -     * We avoid this by disabling interrupts when
> -     * pc contains a magic address.
> -     *
> -     * ARMv7-M interrupt masking works differently than -A or -R.
> +    /* ARMv7-M interrupt masking works differently than -A or -R.
>       * There is no FIQ/IRQ distinction. Instead of I and F bits
>       * masking FIQ and IRQ interrupts, an exception is taken only
>       * if it is higher priority than the current execution priority
> @@ -356,8 +319,7 @@ static bool arm_v7m_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>       * currently active exception).
>       */
>      if (interrupt_request & CPU_INTERRUPT_HARD
> -        && (armv7m_nvic_can_take_pending_exception(env->nvic))
> -        && (env->regs[15] < 0xfffffff0)) {
> +        && (armv7m_nvic_can_take_pending_exception(env->nvic))) {
>          cs->exception_index = EXCP_IRQ;
>          cc->do_interrupt(cs);
>          ret = true;
> @@ -1091,7 +1053,6 @@ static void arm_v7m_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = arm_v7m_cpu_do_interrupt;
>  #endif
>
> -    cc->do_unassigned_access = arm_v7m_unassigned_access;
>      cc->cpu_exec_interrupt = arm_v7m_cpu_exec_interrupt;
>  }
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 156ab46..c85bc6c 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11914,14 +11914,6 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              dc->is_jmp = DISAS_EXC;
>              break;
>          }
> -#else
> -        if (arm_dc_feature(dc, ARM_FEATURE_M)) {
> -            /* Branches to the magic exception-return addresses should
> -             * already have been caught via the arm_v7m_unassigned_access hook,
> -             * and never get here.
> -             */
> -            assert(dc->pc < 0xfffffff0);
> -        }
>  #endif
>
>          if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
>

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] arm: Implement M profile exception return properly

[Peter Maydell]

On 10 April 2017 at 14:52, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 04/10/2017 07:39 AM, Peter Maydell wrote:

>> +    /* Is the new PC value in the magic range indicating exception
>> return? */
>> +    tcg_gen_brcondi_i32(TCG_COND_GEU, cpu_R[15], 0xff000000,
>> excret_label);
>
>
> Idea for a GSoC: branch prediction hints!
>
> tcg_gen_brcondi_i32(TCG_COND_GEU | TCG_COND_UNLIKELY, ...

Isn't the advice for modern CPUs to just trust the branch
predictor?

>> @@ -11092,9 +11137,10 @@ static void disas_thumb_insn(CPUARMState *env,
>> DisasContext *s)
>>                      tmp2 = tcg_temp_new_i32();
>>                      tcg_gen_movi_i32(tmp2, val);
>>                      store_reg(s, 14, tmp2);
>> +                    gen_bx(s, tmp);
>> +                } else {
>> +                    gen_bx_excret(s, tmp);
>
>
> This change was not easy to understand, can you add a one line comment?

Sure, but what did you have in mind? Maybe
    /* Only bx can be an exception-return, not blx */
?

>>                  }
>> -                /* already thumb, no need to check */
>> -                gen_bx(s, tmp);
>>                  break;
>>              }
>>              break;
>> @@ -11989,7 +12035,14 @@ void gen_intermediate_code(CPUARMState *env,
>> TranslationBlock *tb)
>>         instruction was a conditional branch or trap, and the PC has
>>         already been written.  */
>>      gen_set_condexec(dc);
>> -    if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
>> +    if (dc->is_jmp == DISAS_BX_EXCRET) {
>> +        /* Exception return branches need some special case code at the
>> +         * end of the TB, which is complex enough that it has to
>> +         * handle the single-step vs not and the condition-failed
>> +         * insn codepath itself.
>> +         */
>> +        gen_bx_excret_final_code(dc);
>> +    } else if (unlikely(cs->singlestep_enabled || dc->ss_active)) {
>>          /* Unconditional and "condition passed" instruction codepath. */
>>          switch (dc->is_jmp) {
>>          case DISAS_SWI:
>>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 6/7] arm: Implement M profile exception return properly

[Peter Maydell]

On 10 April 2017 at 11:39, Peter Maydell <peter.maydell@linaro.org> wrote:
> On M profile, return from exceptions happen when privileged code
> executes one of the following function call return instructions:
>  * POP or LDM which loads the PC
>  * LDR to PC
>  * BX register
> and the new PC value is 0xFFxxxxxx.

So this isn't quite right -- the special behaviour happens only
when in Handler mode. (Handler is always privileged, but not
all privileged code is in Handler mode)...

> +static inline void gen_bx_excret(DisasContext *s, TCGv_i32 var)
> +{
> +    /* Generate the same code here as for a simple bx, but flag via
> +     * s->is_jmp that we need to do the rest of the work later.
> +     */
> +    gen_bx(s, var);
> +    if (!IS_USER(s) && arm_dc_feature(s, ARM_FEATURE_M)) {

...so we need to track "are we in Handler mode" (ie
env->v7m.exception != 0) in the TB flags and test that here
rather than testing IS_USER.

(Otherwise if you have code which executes the same 'bx' instruction
both as a legitimate exception return and as a fake exception
return while in privileged thread mode then we assert() in
do_v7m_exception_exit. I have a test case that does this but no
real code would ever do it.)

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 5/7] arm: Move condition-failed codepath generation out of if()

[Peter Maydell]

On 10 April 2017 at 14:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> I'm custom to think "let's change that and think how to protect the code to
> help the next one who will modify it" and wonder if it isn't safer to
> define:
>
> const bool singlestepping;
>
> singlestepping = cs->singlestep_enabled || dc->ss_active;
>
> Then use:
>
> if unlikely(singlestepping)
>
> At line 11993 and here, what do you think?

I think we could do this with a function (that takes the DisasContext*);
then we can use it in the new gen_bx_excret_final_code() too.

I'll add an extra patch to the set in v2 that does that.

thanks
-- PMM