Re: [Qemu-devel] [Qemu-arm] [PATCH 10/11] target/arm: Use unallocated_encoding for aarch32

[Laurent Desnogues]

Hi Richard,

On Wed, Aug 7, 2019 at 6:54 AM Richard Henderson
<address@hidden> wrote:
>
> Promote this function from aarch64 to fully general use.
> Use it to unify the code sequences for generating illegal
> opcode exceptions.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/translate-a64.h     |  2 --
>  target/arm/translate.h         |  2 ++
>  target/arm/translate-a64.c     |  7 -------
>  target/arm/translate-vfp.inc.c |  3 +--
>  target/arm/translate.c         | 22 ++++++++++++----------
>  5 files changed, 15 insertions(+), 21 deletions(-)
>
> diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
> index 9cd2b3d238..12ad8ac6ed 100644
> --- a/target/arm/translate-a64.h
> +++ b/target/arm/translate-a64.h
> @@ -18,8 +18,6 @@
>  #ifndef TARGET_ARM_TRANSLATE_A64_H
>  #define TARGET_ARM_TRANSLATE_A64_H
>
> -void unallocated_encoding(DisasContext *s);
> -
>  #define unsupported_encoding(s, insn)                                    \
>      do {                                                                 \
>          qemu_log_mask(LOG_UNIMP,                                         \
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index de600073d8..6a65df0b27 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -98,6 +98,8 @@ typedef struct DisasCompare {
>      bool value_global;
>  } DisasCompare;
>
> +void unallocated_encoding(DisasContext *s);
> +
>  /* Share the TCG temporaries common between 32 and 64 bit modes.  */
>  extern TCGv_i32 cpu_NF, cpu_ZF, cpu_CF, cpu_VF;
>  extern TCGv_i64 cpu_exclusive_addr;
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index d68bfc66d3..9e1ffe9cfb 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -352,13 +352,6 @@ static inline void gen_goto_tb(DisasContext *s, int n, 
> uint64_t dest)
>      }
>  }
>
> -void unallocated_encoding(DisasContext *s)
> -{
> -    /* Unallocated and reserved encodings are uncategorized */
> -    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                       default_exception_el(s));
> -}
> -
>  static void init_tmp_a64_array(DisasContext *s)
>  {
>  #ifdef CONFIG_DEBUG_TCG
> diff --git a/target/arm/translate-vfp.inc.c b/target/arm/translate-vfp.inc.c
> index 5065d4524c..3e8ea80493 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -108,8 +108,7 @@ static bool full_vfp_access_check(DisasContext *s, bool 
> ignore_vfp_enabled)
>
>      if (!s->vfp_enabled && !ignore_vfp_enabled) {
>          assert(!arm_dc_feature(s, ARM_FEATURE_M));
> -        gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                           default_exception_el(s));
> +        unallocated_encoding(s);
>          return false;
>      }
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index d6b0ab7247..2d447d4b90 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1285,6 +1285,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, 
> uint32_t syn)
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>
> +void unallocated_encoding(DisasContext *s)
> +{
> +    /* Unallocated and reserved encodings are uncategorized */
> +    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                       default_exception_el(s));
> +}

Isn't the move of unallocated_encoding from translate-64.c to
translate.c causing issues since gen_exception_insn isn't the same?
In particular in one case the pc is 64-bit while now it's always
32-bit.  Did I miss something?

Thanks,

Laurent

> +
>  /* Force a TB lookup after an instruction that changes the CPU state.  */
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
> @@ -1315,8 +1322,7 @@ static inline void gen_hlt(DisasContext *s, int imm)
>          return;
>      }
>
> -    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                       default_exception_el(s));
> +    unallocated_encoding(s);
>  }
>
>  static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -7638,8 +7644,7 @@ static void gen_srs(DisasContext *s,
>      }
>
>      if (undef) {
> -        gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                           default_exception_el(s));
> +        unallocated_encoding(s);
>          return;
>      }
>
> @@ -9266,8 +9271,7 @@ static void disas_arm_insn(DisasContext *s, unsigned 
> int insn)
>              break;
>          default:
>          illegal_op:
> -            gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                               default_exception_el(s));
> +            unallocated_encoding(s);
>              break;
>          }
>      }
> @@ -10955,8 +10959,7 @@ static void disas_thumb2_insn(DisasContext *s, 
> uint32_t insn)
>      }
>      return;
>  illegal_op:
> -    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                       default_exception_el(s));
> +    unallocated_encoding(s);
>  }
>
>  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> @@ -11779,8 +11782,7 @@ static void disas_thumb_insn(DisasContext *s, 
> uint32_t insn)
>      return;
>  illegal_op:
>  undef:
> -    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> -                       default_exception_el(s));
> +    unallocated_encoding(s);
>  }
>
>  static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
> --
> 2.17.1
>
>