[Qemu-devel] [PATCH 0/4] target-arm: get IT bits right at exceptions

[Qemu-devel] [PATCH 0/4] target-arm: get IT bits right at exceptions

[Peter Maydell]

This patchset (when combined with my previous patchset "Translate
based on TB flags, not CPUState") is a fix for
https://bugs.launchpad.net/qemu/+bug/581335
where we were not getting the IT (conditional execution) bits in
the CPSR right when we took an unexpected exception in Thumb mode.

The linux-user patch fixes an issue exposed by fixing this, where we
weren't clearing the IT bits before entering the signal handler, so
that if we took the signal inside an IT block the first part of the
signal handler wouldn't be executed.

The first two patches in the series and the long comment in patch 4
are aimed at making it a bit clearer how we handle the IT bits; it
took me quite a long time to figure out exactly what the existing
code was doing...

Peter Maydell (4):
  target-arm: Remove redundant setting of IT bits before Thumb SWI
  target-arm: Refactor translation of exception generating instructions
  linux-user: ARM: clear the IT bits when invoking a signal handler
  target-arm: Restore IT bits when resuming after an exception

 linux-user/signal.c    |   16 +++++----
 target-arm/translate.c |   80 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 60 insertions(+), 36 deletions(-)

[Qemu-devel] [PATCH 1/4] target-arm: Remove redundant setting of IT bits before Thumb SWI

[Peter Maydell]

Remove a redundant call to gen_set_condexec() in the translation of Thumb
mode SWI. (SWI and WFI generate "exceptions" which happen after the
execution of the instruction, ie when PC and IT bits have updated.
So the condexec bits at this point are not correct. However, the code
that handles finishing the translation of the TB will write the correct
value of the condexec bits later, so the only effect was that a conditional
Thumb SWI would generate slightly worse code than necessary.)

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

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 2ce82f3..4abece1 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9014,7 +9014,6 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
 
         if (cond == 0xf) {
             /* swi */
-            gen_set_condexec(s);
             gen_set_pc_im(s->pc);
             s->is_jmp = DISAS_SWI;
             break;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/4] target-arm: Remove redundant setting of IT bits before Thumb SWI

[Aurelien Jarno]

On Mon, Jan 10, 2011 at 11:11:49PM +0000, Peter Maydell wrote:
> Remove a redundant call to gen_set_condexec() in the translation of Thumb
> mode SWI. (SWI and WFI generate "exceptions" which happen after the
> execution of the instruction, ie when PC and IT bits have updated.
> So the condexec bits at this point are not correct. However, the code
> that handles finishing the translation of the TB will write the correct
> value of the condexec bits later, so the only effect was that a conditional
> Thumb SWI would generate slightly worse code than necessary.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 2ce82f3..4abece1 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -9014,7 +9014,6 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
>  
>          if (cond == 0xf) {
>              /* swi */
> -            gen_set_condexec(s);
>              gen_set_pc_im(s->pc);
>              s->is_jmp = DISAS_SWI;
>              break;
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 2/4] target-arm: Refactor translation of exception generating instructions

[Peter Maydell]

Create a new function which does the common sequence of gen_set_condexec,
gen_set_pc_im, gen_exception, set is_jmp to DISAS_JUMP.

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

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 4abece1..0e8f7b3 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -3488,6 +3488,14 @@ gen_set_condexec (DisasContext *s)
     }
 }
 
+static void gen_exception_insn(DisasContext *s, int offset, int excp)
+{
+    gen_set_condexec(s);
+    gen_set_pc_im(s->pc - offset);
+    gen_exception(excp);
+    s->is_jmp = DISAS_JUMP;
+}
+
 static void gen_nop_hint(DisasContext *s, int val)
 {
     switch (val) {
@@ -5958,10 +5966,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
     tcg_gen_mov_i32(cpu_exclusive_test, addr);
     tcg_gen_movi_i32(cpu_exclusive_info,
                      size | (rd << 4) | (rt << 8) | (rt2 << 12));
-    gen_set_condexec(s);
-    gen_set_pc_im(s->pc - 4);
-    gen_exception(EXCP_STREX);
-    s->is_jmp = DISAS_JUMP;
+    gen_exception_insn(s, 4, EXCP_STREX);
 }
 #else
 static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
@@ -6366,10 +6371,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
                 goto illegal_op;
             }
             /* bkpt */
-            gen_set_condexec(s);
-            gen_set_pc_im(s->pc - 4);
-            gen_exception(EXCP_BKPT);
-            s->is_jmp = DISAS_JUMP;
+            gen_exception_insn(s, 4, EXCP_BKPT);
             break;
         case 0x8: /* signed multiply */
         case 0xa:
@@ -7268,10 +7270,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
             break;
         default:
         illegal_op:
-            gen_set_condexec(s);
-            gen_set_pc_im(s->pc - 4);
-            gen_exception(EXCP_UDEF);
-            s->is_jmp = DISAS_JUMP;
+            gen_exception_insn(s, 4, EXCP_UDEF);
             break;
         }
     }
@@ -8925,10 +8924,7 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
             break;
 
         case 0xe: /* bkpt */
-            gen_set_condexec(s);
-            gen_set_pc_im(s->pc - 2);
-            gen_exception(EXCP_BKPT);
-            s->is_jmp = DISAS_JUMP;
+            gen_exception_insn(s, 2, EXCP_BKPT);
             break;
 
         case 0xa: /* rev */
@@ -9050,17 +9046,11 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
     }
     return;
 undef32:
-    gen_set_condexec(s);
-    gen_set_pc_im(s->pc - 4);
-    gen_exception(EXCP_UDEF);
-    s->is_jmp = DISAS_JUMP;
+    gen_exception_insn(s, 4, EXCP_UDEF);
     return;
 illegal_op:
 undef:
-    gen_set_condexec(s);
-    gen_set_pc_im(s->pc - 2);
-    gen_exception(EXCP_UDEF);
-    s->is_jmp = DISAS_JUMP;
+    gen_exception_insn(s, 2, EXCP_UDEF);
 }
 
 /* generate intermediate code in gen_opc_buf and gen_opparam_buf for
@@ -9149,10 +9139,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
         if (unlikely(!QTAILQ_EMPTY(&env->breakpoints))) {
             QTAILQ_FOREACH(bp, &env->breakpoints, entry) {
                 if (bp->pc == dc->pc) {
-                    gen_set_condexec(dc);
-                    gen_set_pc_im(dc->pc);
-                    gen_exception(EXCP_DEBUG);
-                    dc->is_jmp = DISAS_JUMP;
+                    gen_exception_insn(dc, 0, EXCP_DEBUG);
                     /* Advance PC so that clearing the breakpoint will
                        invalidate this TB.  */
                     dc->pc += 2;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/4] target-arm: Refactor translation of exception generating instructions

[Aurelien Jarno]

On Mon, Jan 10, 2011 at 11:11:50PM +0000, Peter Maydell wrote:
> Create a new function which does the common sequence of gen_set_condexec,
> gen_set_pc_im, gen_exception, set is_jmp to DISAS_JUMP.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate.c |   43 +++++++++++++++----------------------------
>  1 files changed, 15 insertions(+), 28 deletions(-)

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 4abece1..0e8f7b3 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -3488,6 +3488,14 @@ gen_set_condexec (DisasContext *s)
>      }
>  }
>  
> +static void gen_exception_insn(DisasContext *s, int offset, int excp)
> +{
> +    gen_set_condexec(s);
> +    gen_set_pc_im(s->pc - offset);
> +    gen_exception(excp);
> +    s->is_jmp = DISAS_JUMP;
> +}
> +
>  static void gen_nop_hint(DisasContext *s, int val)
>  {
>      switch (val) {
> @@ -5958,10 +5966,7 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
>      tcg_gen_mov_i32(cpu_exclusive_test, addr);
>      tcg_gen_movi_i32(cpu_exclusive_info,
>                       size | (rd << 4) | (rt << 8) | (rt2 << 12));
> -    gen_set_condexec(s);
> -    gen_set_pc_im(s->pc - 4);
> -    gen_exception(EXCP_STREX);
> -    s->is_jmp = DISAS_JUMP;
> +    gen_exception_insn(s, 4, EXCP_STREX);
>  }
>  #else
>  static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
> @@ -6366,10 +6371,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
>                  goto illegal_op;
>              }
>              /* bkpt */
> -            gen_set_condexec(s);
> -            gen_set_pc_im(s->pc - 4);
> -            gen_exception(EXCP_BKPT);
> -            s->is_jmp = DISAS_JUMP;
> +            gen_exception_insn(s, 4, EXCP_BKPT);
>              break;
>          case 0x8: /* signed multiply */
>          case 0xa:
> @@ -7268,10 +7270,7 @@ static void disas_arm_insn(CPUState * env, DisasContext *s)
>              break;
>          default:
>          illegal_op:
> -            gen_set_condexec(s);
> -            gen_set_pc_im(s->pc - 4);
> -            gen_exception(EXCP_UDEF);
> -            s->is_jmp = DISAS_JUMP;
> +            gen_exception_insn(s, 4, EXCP_UDEF);
>              break;
>          }
>      }
> @@ -8925,10 +8924,7 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
>              break;
>  
>          case 0xe: /* bkpt */
> -            gen_set_condexec(s);
> -            gen_set_pc_im(s->pc - 2);
> -            gen_exception(EXCP_BKPT);
> -            s->is_jmp = DISAS_JUMP;
> +            gen_exception_insn(s, 2, EXCP_BKPT);
>              break;
>  
>          case 0xa: /* rev */
> @@ -9050,17 +9046,11 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
>      }
>      return;
>  undef32:
> -    gen_set_condexec(s);
> -    gen_set_pc_im(s->pc - 4);
> -    gen_exception(EXCP_UDEF);
> -    s->is_jmp = DISAS_JUMP;
> +    gen_exception_insn(s, 4, EXCP_UDEF);
>      return;
>  illegal_op:
>  undef:
> -    gen_set_condexec(s);
> -    gen_set_pc_im(s->pc - 2);
> -    gen_exception(EXCP_UDEF);
> -    s->is_jmp = DISAS_JUMP;
> +    gen_exception_insn(s, 2, EXCP_UDEF);
>  }
>  
>  /* generate intermediate code in gen_opc_buf and gen_opparam_buf for
> @@ -9149,10 +9139,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
>          if (unlikely(!QTAILQ_EMPTY(&env->breakpoints))) {
>              QTAILQ_FOREACH(bp, &env->breakpoints, entry) {
>                  if (bp->pc == dc->pc) {
> -                    gen_set_condexec(dc);
> -                    gen_set_pc_im(dc->pc);
> -                    gen_exception(EXCP_DEBUG);
> -                    dc->is_jmp = DISAS_JUMP;
> +                    gen_exception_insn(dc, 0, EXCP_DEBUG);
>                      /* Advance PC so that clearing the breakpoint will
>                         invalidate this TB.  */
>                      dc->pc += 2;
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 3/4] linux-user: ARM: clear the IT bits when invoking a signal handler

[Peter Maydell]

When invoking a signal handler for an ARM target, make sure the IT
bits in the CPSR are cleared. (This would otherwise cause incorrect
execution if the IT state was non-zero when an exception occured.
This bug has been masked previously because we weren't getting the
IT state bits at exception entry right anyway.)

Also use the proper cpsr_read()/cpsr_write() interface to update
the CPSR rather than manipulating CPUState fields directly.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index c846b8c..0664770 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -1256,6 +1256,14 @@ setup_return(CPUState *env, struct target_sigaction *ka,
 	abi_ulong handler = ka->_sa_handler;
 	abi_ulong retcode;
 	int thumb = handler & 1;
+	uint32_t cpsr = cpsr_read(env);
+
+	cpsr &= ~CPSR_IT;
+	if (thumb) {
+		cpsr |= CPSR_T;
+	} else {
+		cpsr &= ~CPSR_T;
+	}
 
 	if (ka->sa_flags & TARGET_SA_RESTORER) {
 		retcode = ka->sa_restorer;
@@ -1278,13 +1286,7 @@ setup_return(CPUState *env, struct target_sigaction *ka,
 	env->regs[13] = frame_addr;
 	env->regs[14] = retcode;
 	env->regs[15] = handler & (thumb ? ~1 : ~3);
-	env->thumb = thumb;
-
-#if 0
-#ifdef TARGET_CONFIG_CPU_32
-	env->cpsr = cpsr;
-#endif
-#endif
+	cpsr_write(env, cpsr, 0xffffffff);
 
 	return 0;
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/4] linux-user: ARM: clear the IT bits when invoking a signal handler

[Aurelien Jarno]

On Mon, Jan 10, 2011 at 11:11:51PM +0000, Peter Maydell wrote:
> When invoking a signal handler for an ARM target, make sure the IT
> bits in the CPSR are cleared. (This would otherwise cause incorrect
> execution if the IT state was non-zero when an exception occured.
> This bug has been masked previously because we weren't getting the
> IT state bits at exception entry right anyway.)
> 
> Also use the proper cpsr_read()/cpsr_write() interface to update
> the CPSR rather than manipulating CPUState fields directly.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/signal.c |   16 +++++++++-------
>  1 files changed, 9 insertions(+), 7 deletions(-)

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index c846b8c..0664770 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -1256,6 +1256,14 @@ setup_return(CPUState *env, struct target_sigaction *ka,
>  	abi_ulong handler = ka->_sa_handler;
>  	abi_ulong retcode;
>  	int thumb = handler & 1;
> +	uint32_t cpsr = cpsr_read(env);
> +
> +	cpsr &= ~CPSR_IT;
> +	if (thumb) {
> +		cpsr |= CPSR_T;
> +	} else {
> +		cpsr &= ~CPSR_T;
> +	}
>  
>  	if (ka->sa_flags & TARGET_SA_RESTORER) {
>  		retcode = ka->sa_restorer;
> @@ -1278,13 +1286,7 @@ setup_return(CPUState *env, struct target_sigaction *ka,
>  	env->regs[13] = frame_addr;
>  	env->regs[14] = retcode;
>  	env->regs[15] = handler & (thumb ? ~1 : ~3);
> -	env->thumb = thumb;
> -
> -#if 0
> -#ifdef TARGET_CONFIG_CPU_32
> -	env->cpsr = cpsr;
> -#endif
> -#endif
> +	cpsr_write(env, cpsr, 0xffffffff);
>  
>  	return 0;
>  }
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 4/4] target-arm: Restore IT bits when resuming after an exception

[Peter Maydell]

We were not correctly restoring the IT bits when resuming execution
after taking an unexpected exception in the middle of an IT block.
Fix this by tracking them along with PC changes and restoring in
gen_pc_load().

This fixes bug https://bugs.launchpad.net/qemu/+bug/581335

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

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 0e8f7b3..dd0442f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -61,6 +61,8 @@ typedef struct DisasContext {
 #endif
 } DisasContext;
 
+static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
+
 #if defined(CONFIG_USER_ONLY)
 #define IS_USER(s) 1
 #else
@@ -9108,6 +9110,38 @@ static inline void gen_intermediate_code_internal(CPUState *env,
         max_insns = CF_COUNT_MASK;
 
     gen_icount_start();
+
+    /* A note on handling of the condexec (IT) bits:
+     *
+     * We want to avoid the overhead of having to write the updated condexec
+     * bits back to the CPUState for every instruction in an IT block. So:
+     * (1) if the condexec bits are not already zero then we write
+     * zero back into the CPUState now. This avoids complications trying
+     * to do it at the end of the block. (For example if we don't do this
+     * it's hard to identify whether we can safely skip writing condexec
+     * at the end of the TB, which we definitely want to do for the case
+     * where a TB doesn't do anything with the IT state at all.)
+     * (2) if we are going to leave the TB then we call gen_set_condexec()
+     * which will write the correct value into CPUState if zero is wrong.
+     * This is done both for leaving the TB at the end, and for leaving
+     * it because of an exception we know will happen, which is done in
+     * gen_exception_insn(). The latter is necessary because we need to
+     * leave the TB with the PC/IT state just prior to execution of the
+     * instruction which caused the exception.
+     * (3) if we leave the TB unexpectedly (eg a data abort on a load)
+     * then the CPUState will be wrong and we need to reset it.
+     * This is handled in the same way as restoration of the
+     * PC in these situations: we will be called again with search_pc=1
+     * and generate a mapping of the condexec bits for each PC in
+     * gen_opc_condexec_bits[]. gen_pc_load[] then uses this to restore
+     * the condexec bits.
+     *
+     * Note that there are no instructions which can read the condexec
+     * bits, and none which can write non-static values to them, so
+     * we don't need to care about whether CPUState is correct in the
+     * middle of a TB.
+     */
+
     /* Reset the conditional execution bits immediately. This avoids
        complications trying to do it at the end of the block.  */
     if (env->condexec_bits)
@@ -9156,6 +9190,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
                     gen_opc_instr_start[lj++] = 0;
             }
             gen_opc_pc[lj] = dc->pc;
+            gen_opc_condexec_bits[lj] = (dc->condexec_cond << 4) | (dc->condexec_mask >> 1);
             gen_opc_instr_start[lj] = 1;
             gen_opc_icount[lj] = num_insns;
         }
@@ -9363,4 +9398,5 @@ void gen_pc_load(CPUState *env, TranslationBlock *tb,
                 unsigned long searched_pc, int pc_pos, void *puc)
 {
     env->regs[15] = gen_opc_pc[pc_pos];
+    env->condexec_bits = gen_opc_condexec_bits[pc_pos];
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/4] target-arm: Restore IT bits when resuming after an exception

[Aurelien Jarno]

On Mon, Jan 10, 2011 at 11:11:52PM +0000, Peter Maydell wrote:
> We were not correctly restoring the IT bits when resuming execution
> after taking an unexpected exception in the middle of an IT block.
> Fix this by tracking them along with PC changes and restoring in
> gen_pc_load().
> 
> This fixes bug https://bugs.launchpad.net/qemu/+bug/581335
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 0e8f7b3..dd0442f 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -61,6 +61,8 @@ typedef struct DisasContext {
>  #endif
>  } DisasContext;
>  
> +static uint32_t gen_opc_condexec_bits[OPC_BUF_SIZE];
> +
>  #if defined(CONFIG_USER_ONLY)
>  #define IS_USER(s) 1
>  #else
> @@ -9108,6 +9110,38 @@ static inline void gen_intermediate_code_internal(CPUState *env,
>          max_insns = CF_COUNT_MASK;
>  
>      gen_icount_start();
> +
> +    /* A note on handling of the condexec (IT) bits:
> +     *
> +     * We want to avoid the overhead of having to write the updated condexec
> +     * bits back to the CPUState for every instruction in an IT block. So:
> +     * (1) if the condexec bits are not already zero then we write
> +     * zero back into the CPUState now. This avoids complications trying
> +     * to do it at the end of the block. (For example if we don't do this
> +     * it's hard to identify whether we can safely skip writing condexec
> +     * at the end of the TB, which we definitely want to do for the case
> +     * where a TB doesn't do anything with the IT state at all.)
> +     * (2) if we are going to leave the TB then we call gen_set_condexec()
> +     * which will write the correct value into CPUState if zero is wrong.
> +     * This is done both for leaving the TB at the end, and for leaving
> +     * it because of an exception we know will happen, which is done in
> +     * gen_exception_insn(). The latter is necessary because we need to
> +     * leave the TB with the PC/IT state just prior to execution of the
> +     * instruction which caused the exception.
> +     * (3) if we leave the TB unexpectedly (eg a data abort on a load)
> +     * then the CPUState will be wrong and we need to reset it.
> +     * This is handled in the same way as restoration of the
> +     * PC in these situations: we will be called again with search_pc=1
> +     * and generate a mapping of the condexec bits for each PC in
> +     * gen_opc_condexec_bits[]. gen_pc_load[] then uses this to restore
> +     * the condexec bits.
> +     *
> +     * Note that there are no instructions which can read the condexec
> +     * bits, and none which can write non-static values to them, so
> +     * we don't need to care about whether CPUState is correct in the
> +     * middle of a TB.
> +     */
> +
>      /* Reset the conditional execution bits immediately. This avoids
>         complications trying to do it at the end of the block.  */
>      if (env->condexec_bits)
> @@ -9156,6 +9190,7 @@ static inline void gen_intermediate_code_internal(CPUState *env,
>                      gen_opc_instr_start[lj++] = 0;
>              }
>              gen_opc_pc[lj] = dc->pc;
> +            gen_opc_condexec_bits[lj] = (dc->condexec_cond << 4) | (dc->condexec_mask >> 1);
>              gen_opc_instr_start[lj] = 1;
>              gen_opc_icount[lj] = num_insns;
>          }
> @@ -9363,4 +9398,5 @@ void gen_pc_load(CPUState *env, TranslationBlock *tb,
>                  unsigned long searched_pc, int pc_pos, void *puc)
>  {
>      env->regs[15] = gen_opc_pc[pc_pos];
> +    env->condexec_bits = gen_opc_condexec_bits[pc_pos];
>  }

Everything is correct, however note that gen_pc_load() is currently only
called through tlb_fill(), and thus only for load/stores. Other
instructions which can trigger an exception should either implement the
same mechanism, or save the condexec_bit. See for example the patch I
just sent "target-sh4: implement FPU exceptions" to restore the cpu
state for exceptions generated in op_helper.c.

For what I have checked with the current code, everything is fine, that
is the bits are saved by one or other way. It's just in case some more
exceptions are added later.

Tested-by: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 0/4] target-arm: get IT bits right at exceptions

[Aurelien Jarno]

On Mon, Jan 10, 2011 at 11:11:48PM +0000, Peter Maydell wrote:
> This patchset (when combined with my previous patchset "Translate
> based on TB flags, not CPUState") is a fix for
> https://bugs.launchpad.net/qemu/+bug/581335
> where we were not getting the IT (conditional execution) bits in
> the CPSR right when we took an unexpected exception in Thumb mode.
> 
> The linux-user patch fixes an issue exposed by fixing this, where we
> weren't clearing the IT bits before entering the signal handler, so
> that if we took the signal inside an IT block the first part of the
> signal handler wouldn't be executed.
> 
> The first two patches in the series and the long comment in patch 4
> are aimed at making it a bit clearer how we handle the IT bits; it
> took me quite a long time to figure out exactly what the existing
> code was doing...
> 
> Peter Maydell (4):
>   target-arm: Remove redundant setting of IT bits before Thumb SWI
>   target-arm: Refactor translation of exception generating instructions
>   linux-user: ARM: clear the IT bits when invoking a signal handler
>   target-arm: Restore IT bits when resuming after an exception
> 
>  linux-user/signal.c    |   16 +++++----
>  target-arm/translate.c |   80 ++++++++++++++++++++++++++++++-----------------
>  2 files changed, 60 insertions(+), 36 deletions(-)
> 

Thanks, all applied.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net