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 <address@hidden>
> ---
>  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 <address@hidden>
Reviewed-by: Aurelien Jarno <address@hidden>

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