Re: [PATCH] target/arm: fix IL bit for data abort exceptions

[Richard Henderson]

On 12/17/19 11:02 AM, Jeff Kubascik wrote:
> diff --git a/target/arm/tlb_helper.c b/target/arm/tlb_helper.c
> index 5feb312941..e63f8bda29 100644
> --- a/target/arm/tlb_helper.c
> +++ b/target/arm/tlb_helper.c
> @@ -44,7 +44,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t 
> template_syn,
>          syn = syn_data_abort_with_iss(same_el,
>                                        0, 0, 0, 0, 0,
>                                        ea, 0, s1ptw, is_write, fsc,
> -                                      false);
> +                                      true);
>          /* Merge the runtime syndrome with the template syndrome.  */
>          syn |= template_syn;

This doesn't look correct.  Surely the IL bit should come from template_syn?

> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d4bebbe629..a3c618fdd9 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -14045,6 +14045,7 @@ static void disas_a64_insn(CPUARMState *env, 
> DisasContext *s)
>      s->pc_curr = s->base.pc_next;
>      insn = arm_ldl_code(env, s->base.pc_next, s->sctlr_b);
>      s->insn = insn;
> +    s->is_16bit = false;
>      s->base.pc_next += 4;

Should not be necessary, as the field is not read along any a64 path.  (Also,
while it's not yet in master, there's a patch on list that zero initializes the
entire structure.)

> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 2b6c1f91bf..300480f1b7 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -8555,7 +8555,7 @@ static ISSInfo make_issinfo(DisasContext *s, int rd, 
> bool p, bool w)
>  
>      /* ISS not valid if writeback */
>      if (p && !w) {
> -        ret = rd;
> +        ret = rd | (s->is_16bit ? ISSIs16Bit : 0);
>      } else {
>          ret = ISSInvalid;
>      }
> @@ -11057,6 +11057,7 @@ static void arm_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>      dc->pc_curr = dc->base.pc_next;
>      insn = arm_ldl_code(env, dc->base.pc_next, dc->sctlr_b);
>      dc->insn = insn;
> +    dc->is_16bit = false;
>      dc->base.pc_next += 4;
>      disas_arm_insn(dc, insn);
>  
> @@ -11126,6 +11127,7 @@ static void thumb_tr_translate_insn(DisasContextBase 
> *dcbase, CPUState *cpu)
>      dc->pc_curr = dc->base.pc_next;
>      insn = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
>      is_16bit = thumb_insn_is_16bit(dc, dc->base.pc_next, insn);
> +    dc->is_16bit = is_16bit;
>      dc->base.pc_next += 2;
>      if (!is_16bit) {
>          uint32_t insn2 = arm_lduw_code(env, dc->base.pc_next, dc->sctlr_b);
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index b837b7fcbf..c16f434477 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -14,6 +14,8 @@ typedef struct DisasContext {
>      target_ulong pc_curr;
>      target_ulong page_start;
>      uint32_t insn;
> +    /* 16-bit instruction flag */
> +    bool is_16bit;
>      /* Nonzero if this instruction has been conditionally skipped.  */
>      int condjmp;
>      /* The label that will be jumped to when the instruction is skipped.  */

The rest of this looks both correct and necessary.


r~