[Qemu-devel] [PATCH 0/2] target/arm: Fix aarch64 illegal opcode exceptions

[Qemu-devel] [PATCH 0/2] target/arm: Fix aarch64 illegal opcode exceptions

[Richard Henderson]

As Laurent pointed out, gen_exception_insn is very different
between aarch64 and aarch32, so we shouldn't accidentally
redirect one to the other.


r~


Richard Henderson (2):
  Revert "target/arm: Use unallocated_encoding for aarch32"
  target/arm: Factor out unallocated_encoding for aarch32

 target/arm/translate-a64.h | 2 ++
 target/arm/translate.h     | 2 --
 target/arm/translate-a64.c | 7 +++++++
 target/arm/translate.c     | 2 +-
 4 files changed, 10 insertions(+), 3 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] Revert "target/arm: Use unallocated_encoding for aarch32"

[Richard Henderson]

This reverts commit 3cb36637157088892e9e33ddb1034bffd1251d3b.

Despite the fact that the text for the call to gen_exception_insn
is identical for aarch64 and aarch32, the implementation inside
gen_exception_insn is totally different.

This fixes exceptions raised from aarch64.

Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 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, 21 insertions(+), 15 deletions(-)

diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
index 12ad8ac6ed..9cd2b3d238 100644
--- a/target/arm/translate-a64.h
+++ b/target/arm/translate-a64.h
@@ -18,6 +18,8 @@
 #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 92ef790be9..64304c957e 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -99,8 +99,6 @@ 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 6fd0b779d3..9183f89ba3 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -338,6 +338,13 @@ 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 3e8ea80493..5065d4524c 100644
--- a/target/arm/translate-vfp.inc.c
+++ b/target/arm/translate-vfp.inc.c
@@ -108,7 +108,8 @@ 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));
-        unallocated_encoding(s);
+        gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+                           default_exception_el(s));
         return false;
     }
 
diff --git a/target/arm/translate.c b/target/arm/translate.c
index cbe19b7a62..2aac9aae68 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1231,13 +1231,6 @@ 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));
-}
-
 /* Force a TB lookup after an instruction that changes the CPU state.  */
 static inline void gen_lookup_tb(DisasContext *s)
 {
@@ -1268,7 +1261,8 @@ static inline void gen_hlt(DisasContext *s, int imm)
         return;
     }
 
-    unallocated_encoding(s);
+    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+                       default_exception_el(s));
 }
 
 static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
@@ -7580,7 +7574,8 @@ static void gen_srs(DisasContext *s,
     }
 
     if (undef) {
-        unallocated_encoding(s);
+        gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+                           default_exception_el(s));
         return;
     }
 
@@ -9201,7 +9196,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
             break;
         default:
         illegal_op:
-            unallocated_encoding(s);
+            gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+                               default_exception_el(s));
             break;
         }
     }
@@ -10886,7 +10882,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
     }
     return;
 illegal_op:
-    unallocated_encoding(s);
+    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+                       default_exception_el(s));
 }
 
 static void disas_thumb_insn(DisasContext *s, uint32_t insn)
@@ -11709,7 +11706,8 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
     return;
 illegal_op:
 undef:
-    unallocated_encoding(s);
+    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
+                       default_exception_el(s));
 }
 
 static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
-- 
2.17.1

[Qemu-devel] [PATCH 2/2] target/arm: Factor out unallocated_encoding for aarch32

[Richard Henderson]

Make this a static function private to translate.c.
Thus we can use the same idiom between aarch64 and aarch32
without actually sharing function implementations.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-vfp.inc.c |  3 +--
 target/arm/translate.c         | 22 ++++++++++++----------
 2 files changed, 13 insertions(+), 12 deletions(-)

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 2aac9aae68..66311580c0 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -1231,6 +1231,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
     s->base.is_jmp = DISAS_NORETURN;
 }
 
+static 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));
+}
+
 /* Force a TB lookup after an instruction that changes the CPU state.  */
 static inline void gen_lookup_tb(DisasContext *s)
 {
@@ -1261,8 +1268,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,
@@ -7574,8 +7580,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;
     }
 
@@ -9196,8 +9201,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;
         }
     }
@@ -10882,8 +10886,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)
@@ -11706,8 +11709,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

Re: [Qemu-devel] [PATCH 1/2] Revert "target/arm: Use unallocated_encoding for aarch32"

[Laurent Desnogues]

Hi,

On Mon, Aug 26, 2019 at 5:15 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This reverts commit 3cb36637157088892e9e33ddb1034bffd1251d3b.
>
> Despite the fact that the text for the call to gen_exception_insn
> is identical for aarch64 and aarch32, the implementation inside
> gen_exception_insn is totally different.
>
> This fixes exceptions raised from aarch64.
>
> Reported-by: Laurent Desnogues <laurent.desnogues@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  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, 21 insertions(+), 15 deletions(-)
>
> diff --git a/target/arm/translate-a64.h b/target/arm/translate-a64.h
> index 12ad8ac6ed..9cd2b3d238 100644
> --- a/target/arm/translate-a64.h
> +++ b/target/arm/translate-a64.h
> @@ -18,6 +18,8 @@
>  #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 92ef790be9..64304c957e 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -99,8 +99,6 @@ 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 6fd0b779d3..9183f89ba3 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -338,6 +338,13 @@ 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 3e8ea80493..5065d4524c 100644
> --- a/target/arm/translate-vfp.inc.c
> +++ b/target/arm/translate-vfp.inc.c
> @@ -108,7 +108,8 @@ 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));
> -        unallocated_encoding(s);
> +        gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                           default_exception_el(s));
>          return false;
>      }
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index cbe19b7a62..2aac9aae68 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1231,13 +1231,6 @@ 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));
> -}
> -
>  /* Force a TB lookup after an instruction that changes the CPU state.  */
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
> @@ -1268,7 +1261,8 @@ static inline void gen_hlt(DisasContext *s, int imm)
>          return;
>      }
>
> -    unallocated_encoding(s);
> +    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                       default_exception_el(s));
>  }
>
>  static inline void gen_add_data_offset(DisasContext *s, unsigned int insn,
> @@ -7580,7 +7574,8 @@ static void gen_srs(DisasContext *s,
>      }
>
>      if (undef) {
> -        unallocated_encoding(s);
> +        gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                           default_exception_el(s));
>          return;
>      }
>
> @@ -9201,7 +9196,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)
>              break;
>          default:
>          illegal_op:
> -            unallocated_encoding(s);
> +            gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                               default_exception_el(s));
>              break;
>          }
>      }
> @@ -10886,7 +10882,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
>      }
>      return;
>  illegal_op:
> -    unallocated_encoding(s);
> +    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                       default_exception_el(s));
>  }
>
>  static void disas_thumb_insn(DisasContext *s, uint32_t insn)
> @@ -11709,7 +11706,8 @@ static void disas_thumb_insn(DisasContext *s, uint32_t insn)
>      return;
>  illegal_op:
>  undef:
> -    unallocated_encoding(s);
> +    gen_exception_insn(s, s->pc_curr, EXCP_UDEF, syn_uncategorized(),
> +                       default_exception_el(s));
>  }
>
>  static bool insn_crosses_page(CPUARMState *env, DisasContext *s)
> --
> 2.17.1
>

Re: [Qemu-devel] [PATCH 2/2] target/arm: Factor out unallocated_encoding for aarch32

[Laurent Desnogues]

On Mon, Aug 26, 2019 at 5:15 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Make this a static function private to translate.c.
> Thus we can use the same idiom between aarch64 and aarch32
> without actually sharing function implementations.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

Thanks,

Laurent

> ---
>  target/arm/translate-vfp.inc.c |  3 +--
>  target/arm/translate.c         | 22 ++++++++++++----------
>  2 files changed, 13 insertions(+), 12 deletions(-)
>
> 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 2aac9aae68..66311580c0 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -1231,6 +1231,13 @@ static void gen_exception_bkpt_insn(DisasContext *s, uint32_t syn)
>      s->base.is_jmp = DISAS_NORETURN;
>  }
>
> +static 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));
> +}
> +
>  /* Force a TB lookup after an instruction that changes the CPU state.  */
>  static inline void gen_lookup_tb(DisasContext *s)
>  {
> @@ -1261,8 +1268,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,
> @@ -7574,8 +7580,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;
>      }
>
> @@ -9196,8 +9201,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;
>          }
>      }
> @@ -10882,8 +10886,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)
> @@ -11706,8 +11709,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
>

Re: [Qemu-devel] [PATCH 0/2] target/arm: Fix aarch64 illegal opcode exceptions

[Peter Maydell]

On Mon, 26 Aug 2019 at 16:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> As Laurent pointed out, gen_exception_insn is very different
> between aarch64 and aarch32, so we shouldn't accidentally
> redirect one to the other.
>
>
> r~
>
>
> Richard Henderson (2):
>   Revert "target/arm: Use unallocated_encoding for aarch32"
>   target/arm: Factor out unallocated_encoding for aarch32



Applied to target-arm.next, thanks.

-- PMM