All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly
@ 2017-04-10 17:13 Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 1/9] arm: Don't implement BXJ on M-profile CPUs Peter Maydell
                   ` (9 more replies)
  0 siblings, 10 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

On M profile, return from exceptions happen when code in Handler mode
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 Thread 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 in Handler 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
       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 6 are minor preliminary refactorings;
patch 7 adds the 'in handler mode' bit to the TB flags; patch 8
is the meat of the series; finally patch 9 removes the old mechanism.

Changes v1->v2:
 * Abstract out "are we singlestepping" test to utility function
   (new patch 6)
 * track Handler state in TB flags and use it to determine when
   to allow exception-return, not the incorrect IS_USER test I had
   in v1
 * add a brief comment about bx vs blx in patch 8

thanks
-- PMM

Peter Maydell (9):
  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: Abstract out "are we singlestepping" test to utility function
  arm: Track M profile handler mode state in TB flags
  arm: Implement M profile exception return properly
  arm: Remove workarounds for old M-profile exception return
    implementation

 target/arm/cpu.h       |   9 +++
 target/arm/translate.h |   5 ++
 target/arm/cpu.c       |  43 +-----------
 target/arm/translate.c | 181 +++++++++++++++++++++++++++++++++----------------
 4 files changed, 138 insertions(+), 100 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 1/9] arm: Don't implement BXJ on M-profile CPUs
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 2/9] arm: Thumb shift operations should not permit interworking branches Peter Maydell
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 2/9] arm: Thumb shift operations should not permit interworking branches
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 1/9] arm: Don't implement BXJ on M-profile CPUs Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 3/9] arm: Factor out "generate right kind of step exception" Peter Maydell
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 3/9] arm: Factor out "generate right kind of step exception"
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 1/9] arm: Don't implement BXJ on M-profile CPUs Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 2/9] arm: Thumb shift operations should not permit interworking branches Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 4/9] arm: Move gen_set_condexec() and gen_set_pc_im() up in the file Peter Maydell
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

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,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 4/9] arm: Move gen_set_condexec() and gen_set_pc_im() up in the file
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (2 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 3/9] arm: Factor out "generate right kind of step exception" Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 5/9] arm: Move condition-failed codepath generation out of if() Peter Maydell
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 5/9] arm: Move condition-failed codepath generation out of if()
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (3 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 4/9] arm: Move gen_set_condexec() and gen_set_pc_im() up in the file Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 6/9] arm: Abstract out "are we singlestepping" test to utility function Peter Maydell
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 6/9] arm: Abstract out "are we singlestepping" test to utility function
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (4 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 5/9] arm: Move condition-failed codepath generation out of if() Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 18:54   ` Philippe Mathieu-Daudé
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags Peter Maydell
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

We now test for "are we singlestepping" in several places and
it's not a trivial check because we need to care about both
architectural singlestep and QEMU gdbstub singlestep. We're
also about to add another place that needs to make this check,
so pull the condition out into a function.

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

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 87fd702..f28c4ca 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -309,6 +309,17 @@ static void gen_singlestep_exception(DisasContext *s)
     }
 }
 
+static inline bool is_singlestepping(DisasContext *s)
+{
+    /* Return true if we are singlestepping either because of
+     * architectural singlestep or QEMU gdbstub singlestep. This does
+     * not include the command line '-singlestep' mode which is rather
+     * misnamed as it only means "one instruction per TB" and doesn't
+     * affect the code we generate.
+     */
+    return s->singlestep_enabled || s->ss_active;
+}
+
 static void gen_smul_dual(TCGv_i32 a, TCGv_i32 b)
 {
     TCGv_i32 tmp1 = tcg_temp_new_i32();
@@ -4104,7 +4115,7 @@ static inline void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
 
 static inline void gen_jmp (DisasContext *s, uint32_t dest)
 {
-    if (unlikely(s->singlestep_enabled || s->ss_active)) {
+    if (unlikely(is_singlestepping(s))) {
         /* An indirect jump so that we still trigger the debug exception.  */
         if (s->thumb)
             dest |= 1;
@@ -11970,9 +11981,8 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
             ((dc->pc >= next_page_start - 3) && insn_crosses_page(env, dc));
 
     } while (!dc->is_jmp && !tcg_op_buf_full() &&
-             !cs->singlestep_enabled &&
+             !is_singlestepping(dc) &&
              !singlestep &&
-             !dc->ss_active &&
              !end_of_page &&
              num_insns < max_insns);
 
@@ -11989,7 +11999,7 @@ 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 (unlikely(is_singlestepping(dc))) {
         /* Unconditional and "condition passed" instruction codepath. */
         switch (dc->is_jmp) {
         case DISAS_SWI:
@@ -12067,7 +12077,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
         /* "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)) {
+        if (unlikely(is_singlestepping(dc))) {
             gen_set_pc_im(dc, dc->pc);
             gen_singlestep_exception(dc);
         } else {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (5 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 6/9] arm: Abstract out "are we singlestepping" test to utility function Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-17  4:20   ` Philippe Mathieu-Daudé
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 8/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

For M profile exception-return handling we'd like to generate different
code for some instructions depending on whether we are in Handler
mode or Thread mode. This isn't the same as "are we privileged
or user", so we need an extra bit in the TB flags to distinguish.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h       | 9 +++++++++
 target/arm/translate.h | 1 +
 target/arm/translate.c | 1 +
 3 files changed, 11 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index e6f05e2..7d26357 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2291,6 +2291,9 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
 #define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
 #define ARM_TBFLAG_BE_DATA_SHIFT    20
 #define ARM_TBFLAG_BE_DATA_MASK     (1 << ARM_TBFLAG_BE_DATA_SHIFT)
+/* For M profile only, Handler (ie not Thread) mode */
+#define ARM_TBFLAG_HANDLER_SHIFT    21
+#define ARM_TBFLAG_HANDLER_MASK     (1 << ARM_TBFLAG_HANDLER_SHIFT)
 
 /* Bit usage when in AArch64 state */
 #define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
@@ -2327,6 +2330,8 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
     (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
 #define ARM_TBFLAG_BE_DATA(F) \
     (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
+#define ARM_TBFLAG_HANDLER(F) \
+    (((F) & ARM_TBFLAG_HANDLER_MASK) >> ARM_TBFLAG_HANDLER_SHIFT)
 #define ARM_TBFLAG_TBI0(F) \
     (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
 #define ARM_TBFLAG_TBI1(F) \
@@ -2517,6 +2522,10 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
     }
     *flags |= fp_exception_el(env) << ARM_TBFLAG_FPEXC_EL_SHIFT;
 
+    if (env->v7m.exception != 0) {
+        *flags |= ARM_TBFLAG_HANDLER_MASK;
+    }
+
     *cs_base = 0;
 }
 
diff --git a/target/arm/translate.h b/target/arm/translate.h
index abb0760..f955174 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -31,6 +31,7 @@ typedef struct DisasContext {
     bool vfp_enabled; /* FP enabled via FPSCR.EN */
     int vec_len;
     int vec_stride;
+    bool handler; /* v7M Handler mode */
     /* Immediate value in AArch32 SVC insn; must be set if is_jmp == DISAS_SWI
      * so that top level loop can generate correct syndrome information.
      */
diff --git a/target/arm/translate.c b/target/arm/translate.c
index f28c4ca..f980cda 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11780,6 +11780,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
     dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
     dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
     dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags);
+    dc->handler = ARM_TBFLAG_HANDLER(tb->flags);
     dc->cp_regs = cpu->cp_regs;
     dc->features = env->features;
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 8/9] arm: Implement M profile exception return properly
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (6 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 9/9] arm: Remove workarounds for old M-profile exception return implementation Peter Maydell
  2017-04-15 12:31 ` [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Richard Henderson
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

On M profile, return from exceptions happen when code in Handler mode
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 Thread 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 in Handler 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
   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 | 66 +++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/target/arm/translate.h b/target/arm/translate.h
index f955174..0bbb95a 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -135,6 +135,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 f980cda..1e8b0a8 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -943,6 +943,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, and happens only for code 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 (s->handler && 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 (is_singlestepping(s)) {
+        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. */
@@ -962,7 +1007,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);
     }
@@ -9881,7 +9926,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;
@@ -10913,7 +10958,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);
             }
@@ -11103,9 +11148,11 @@ 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 {
+                    /* Only BX works as exception-return, not BLX */
+                    gen_bx_excret(s, tmp);
                 }
-                /* already thumb, no need to check */
-                gen_bx(s, tmp);
                 break;
             }
             break;
@@ -12000,7 +12047,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(is_singlestepping(dc))) {
+    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(is_singlestepping(dc))) {
         /* Unconditional and "condition passed" instruction codepath. */
         switch (dc->is_jmp) {
         case DISAS_SWI:
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Qemu-devel] [PATCH v2 9/9] arm: Remove workarounds for old M-profile exception return implementation
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (7 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 8/9] arm: Implement M profile exception return properly Peter Maydell
@ 2017-04-10 17:13 ` Peter Maydell
  2017-04-15 12:31 ` [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Richard Henderson
  9 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-10 17:13 UTC (permalink / raw)
  To: qemu-arm, qemu-devel
  Cc: patches, Richard Henderson, Philippe Mathieu-Daudé

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 1e8b0a8..6968f73 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -11927,14 +11927,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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 6/9] arm: Abstract out "are we singlestepping" test to utility function
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 6/9] arm: Abstract out "are we singlestepping" test to utility function Peter Maydell
@ 2017-04-10 18:54   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-10 18:54 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: patches, Richard Henderson

On 04/10/2017 02:13 PM, Peter Maydell wrote:
> We now test for "are we singlestepping" in several places and
> it's not a trivial check because we need to care about both
> architectural singlestep and QEMU gdbstub singlestep. We're
> also about to add another place that needs to make this check,
> so pull the condition out into a function.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/translate.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 87fd702..f28c4ca 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -309,6 +309,17 @@ static void gen_singlestep_exception(DisasContext *s)
>      }
>  }
>
> +static inline bool is_singlestepping(DisasContext *s)
> +{
> +    /* Return true if we are singlestepping either because of
> +     * architectural singlestep or QEMU gdbstub singlestep. This does
> +     * not include the command line '-singlestep' mode which is rather
> +     * misnamed as it only means "one instruction per TB" and doesn't
> +     * affect the code we generate.
> +     */

This comment is very welcome :)

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

> +    return s->singlestep_enabled || s->ss_active;
> +}
> +
>  static void gen_smul_dual(TCGv_i32 a, TCGv_i32 b)
>  {
>      TCGv_i32 tmp1 = tcg_temp_new_i32();
> @@ -4104,7 +4115,7 @@ static inline void gen_goto_tb(DisasContext *s, int n, target_ulong dest)
>
>  static inline void gen_jmp (DisasContext *s, uint32_t dest)
>  {
> -    if (unlikely(s->singlestep_enabled || s->ss_active)) {
> +    if (unlikely(is_singlestepping(s))) {
>          /* An indirect jump so that we still trigger the debug exception.  */
>          if (s->thumb)
>              dest |= 1;
> @@ -11970,9 +11981,8 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>              ((dc->pc >= next_page_start - 3) && insn_crosses_page(env, dc));
>
>      } while (!dc->is_jmp && !tcg_op_buf_full() &&
> -             !cs->singlestep_enabled &&
> +             !is_singlestepping(dc) &&
>               !singlestep &&
> -             !dc->ss_active &&
>               !end_of_page &&
>               num_insns < max_insns);
>
> @@ -11989,7 +11999,7 @@ 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 (unlikely(is_singlestepping(dc))) {
>          /* Unconditional and "condition passed" instruction codepath. */
>          switch (dc->is_jmp) {
>          case DISAS_SWI:
> @@ -12067,7 +12077,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>          /* "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)) {
> +        if (unlikely(is_singlestepping(dc))) {
>              gen_set_pc_im(dc, dc->pc);
>              gen_singlestep_exception(dc);
>          } else {
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly
  2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
                   ` (8 preceding siblings ...)
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 9/9] arm: Remove workarounds for old M-profile exception return implementation Peter Maydell
@ 2017-04-15 12:31 ` Richard Henderson
  2017-04-20 14:28   ` Peter Maydell
  9 siblings, 1 reply; 15+ messages in thread
From: Richard Henderson @ 2017-04-15 12:31 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: Philippe Mathieu-Daudé, patches

On 04/10/2017 10:13 AM, Peter Maydell wrote:
> Peter Maydell (9):
>   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: Abstract out "are we singlestepping" test to utility function
>   arm: Track M profile handler mode state in TB flags
>   arm: Implement M profile exception return properly
>   arm: Remove workarounds for old M-profile exception return
>     implementation
>
>  target/arm/cpu.h       |   9 +++
>  target/arm/translate.h |   5 ++
>  target/arm/cpu.c       |  43 +-----------
>  target/arm/translate.c | 181 +++++++++++++++++++++++++++++++++----------------
>  4 files changed, 138 insertions(+), 100 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags
  2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags Peter Maydell
@ 2017-04-17  4:20   ` Philippe Mathieu-Daudé
  2017-04-20 14:25     ` Peter Maydell
  0 siblings, 1 reply; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-17  4:20 UTC (permalink / raw)
  To: Peter Maydell, qemu-arm, qemu-devel; +Cc: patches, Richard Henderson

Hi Peter,

On 04/10/2017 02:13 PM, Peter Maydell wrote:
> For M profile exception-return handling we'd like to generate different
> code for some instructions depending on whether we are in Handler
> mode or Thread mode. This isn't the same as "are we privileged
> or user", so we need an extra bit in the TB flags to distinguish.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu.h       | 9 +++++++++
>  target/arm/translate.h | 1 +
>  target/arm/translate.c | 1 +
>  3 files changed, 11 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e6f05e2..7d26357 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2291,6 +2291,9 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>  #define ARM_TBFLAG_NS_MASK          (1 << ARM_TBFLAG_NS_SHIFT)
>  #define ARM_TBFLAG_BE_DATA_SHIFT    20
>  #define ARM_TBFLAG_BE_DATA_MASK     (1 << ARM_TBFLAG_BE_DATA_SHIFT)
> +/* For M profile only, Handler (ie not Thread) mode */
> +#define ARM_TBFLAG_HANDLER_SHIFT    21
> +#define ARM_TBFLAG_HANDLER_MASK     (1 << ARM_TBFLAG_HANDLER_SHIFT)
>
>  /* Bit usage when in AArch64 state */
>  #define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
> @@ -2327,6 +2330,8 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
>      (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
>  #define ARM_TBFLAG_BE_DATA(F) \
>      (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
> +#define ARM_TBFLAG_HANDLER(F) \
> +    (((F) & ARM_TBFLAG_HANDLER_MASK) >> ARM_TBFLAG_HANDLER_SHIFT)
>  #define ARM_TBFLAG_TBI0(F) \
>      (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
>  #define ARM_TBFLAG_TBI1(F) \
> @@ -2517,6 +2522,10 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
>      }
>      *flags |= fp_exception_el(env) << ARM_TBFLAG_FPEXC_EL_SHIFT;
>
> +    if (env->v7m.exception != 0) {
> +        *flags |= ARM_TBFLAG_HANDLER_MASK;
> +    }
> +
>      *cs_base = 0;
>  }
>
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index abb0760..f955174 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -31,6 +31,7 @@ typedef struct DisasContext {
>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
>      int vec_len;
>      int vec_stride;
> +    bool handler; /* v7M Handler mode */

What about a more descriptive name like 'is_v7m_handler_mode' or 
'v7m_mode_handler'? It makes your next patch easier to understand:

     if (s->handler && arm_dc_feature(s, ARM_FEATURE_M)) {

>      /* Immediate value in AArch32 SVC insn; must be set if is_jmp == DISAS_SWI
>       * so that top level loop can generate correct syndrome information.
>       */
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index f28c4ca..f980cda 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -11780,6 +11780,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
>      dc->vec_len = ARM_TBFLAG_VECLEN(tb->flags);
>      dc->vec_stride = ARM_TBFLAG_VECSTRIDE(tb->flags);
>      dc->c15_cpar = ARM_TBFLAG_XSCALE_CPAR(tb->flags);
> +    dc->handler = ARM_TBFLAG_HANDLER(tb->flags);
>      dc->cp_regs = cpu->cp_regs;
>      dc->features = env->features;
>
>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags
  2017-04-17  4:20   ` Philippe Mathieu-Daudé
@ 2017-04-20 14:25     ` Peter Maydell
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-20 14:25 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-arm, QEMU Developers, patches, Richard Henderson

On 17 April 2017 at 05:20, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 04/10/2017 02:13 PM, Peter Maydell wrote:
>> --- a/target/arm/translate.h
>> +++ b/target/arm/translate.h
>> @@ -31,6 +31,7 @@ typedef struct DisasContext {
>>      bool vfp_enabled; /* FP enabled via FPSCR.EN */
>>      int vec_len;
>>      int vec_stride;
>> +    bool handler; /* v7M Handler mode */
>
>
> What about a more descriptive name like 'is_v7m_handler_mode' or
> 'v7m_mode_handler'? It makes your next patch easier to understand:

Seems reasonable; I've made that change (but won't repost the
series as that's the only tweak).

thanks
-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly
  2017-04-15 12:31 ` [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Richard Henderson
@ 2017-04-20 14:28   ` Peter Maydell
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2017-04-20 14:28 UTC (permalink / raw)
  To: Richard Henderson
  Cc: qemu-arm, QEMU Developers, Philippe Mathieu-Daudé, patches

On 15 April 2017 at 13:31, Richard Henderson <rth@twiddle.net> wrote:
> On 04/10/2017 10:13 AM, Peter Maydell wrote:
>>
>> Peter Maydell (9):
>>   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: Abstract out "are we singlestepping" test to utility function
>>   arm: Track M profile handler mode state in TB flags
>>   arm: Implement M profile exception return properly
>>   arm: Remove workarounds for old M-profile exception return
>>     implementation
>>
>>  target/arm/cpu.h       |   9 +++
>>  target/arm/translate.h |   5 ++
>>  target/arm/cpu.c       |  43 +-----------
>>  target/arm/translate.c | 181
>> +++++++++++++++++++++++++++++++++----------------
>>  4 files changed, 138 insertions(+), 100 deletions(-)
>
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Thanks; applied to target-arm.next.

-- PMM

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2017-04-20 14:29 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 17:13 [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 1/9] arm: Don't implement BXJ on M-profile CPUs Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 2/9] arm: Thumb shift operations should not permit interworking branches Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 3/9] arm: Factor out "generate right kind of step exception" Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 4/9] arm: Move gen_set_condexec() and gen_set_pc_im() up in the file Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 5/9] arm: Move condition-failed codepath generation out of if() Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 6/9] arm: Abstract out "are we singlestepping" test to utility function Peter Maydell
2017-04-10 18:54   ` Philippe Mathieu-Daudé
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 7/9] arm: Track M profile handler mode state in TB flags Peter Maydell
2017-04-17  4:20   ` Philippe Mathieu-Daudé
2017-04-20 14:25     ` Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 8/9] arm: Implement M profile exception return properly Peter Maydell
2017-04-10 17:13 ` [Qemu-devel] [PATCH v2 9/9] arm: Remove workarounds for old M-profile exception return implementation Peter Maydell
2017-04-15 12:31 ` [Qemu-devel] [PATCH v2 0/9] arm: Implement M profile exception return properly Richard Henderson
2017-04-20 14:28   ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.