diff mbox series

[14/19] target/arm: Move the vfp decodetree calls next to the base isa

Message ID 20200214181547.21408-15-richard.henderson@linaro.org
State Superseded
Headers show
Series target/arm: vfp feature and decodetree cleanup | expand

Commit Message

Richard Henderson Feb. 14, 2020, 6:15 p.m. UTC 
Have the calls adjacent as an intermediate step toward
actually merging the decodes.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/translate.c | 80 +++++++++++++-----------------------------
 1 file changed, 24 insertions(+), 56 deletions(-)

-- 
2.20.1

Comments

Peter Maydell Feb. 20, 2020, 5:16 p.m. UTC | #1 
On Fri, 14 Feb 2020 at 18:16, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Have the calls adjacent as an intermediate step toward

> actually merging the decodes.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

>  target/arm/translate.c | 80 +++++++++++++-----------------------------

>  1 file changed, 24 insertions(+), 56 deletions(-)

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index b2641b4262..5cabe6b2e9 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -2646,31 +2646,6 @@ static void gen_neon_dup_high16(TCGv_i32 var)

>      tcg_temp_free_i32(tmp);

>  }

>

> -/*

> - * Disassemble a VFP instruction.  Returns nonzero if an error occurred

> - * (ie. an undefined instruction).

> - */

> -static int disas_vfp_insn(DisasContext *s, uint32_t insn)

> -{

> -    /*

> -     * If the decodetree decoder handles this insn it will always

> -     * emit code to either execute the insn or generate an appropriate

> -     * exception; so we don't need to ever return non-zero to tell

> -     * the calling code to emit an UNDEF exception.

> -     */

> -    if (extract32(insn, 28, 4) == 0xf) {

> -        if (disas_vfp_uncond(s, insn)) {

> -            return 0;

> -        }

> -    } else {

> -        if (disas_vfp(s, insn)) {

> -            return 0;

> -        }

> -    }

> -    /* If the decodetree decoder didn't handle this insn, it must be UNDEF */

> -    return 1;

> -}


Before this change, if this was a cp10/11 insn and
neither disas_vfp_uncond() nor disas_vfp() returned true,
we would UNDEF it.

> -

>  static inline bool use_goto_tb(DisasContext *s, target_ulong dest)

>  {

>  #ifndef CONFIG_USER_ONLY

> @@ -10524,7 +10499,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>          ARCH(5);

>

>          /* Unconditional instructions.  */

> -        if (disas_a32_uncond(s, insn)) {

> +        /* TODO: Perhaps merge these into one decodetree output file.  */

> +        if (disas_a32_uncond(s, insn) ||

> +            disas_vfp_uncond(s, insn)) {

>              return;

>          }

>          /* fall back to legacy decoder */

> @@ -10551,13 +10528,6 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>              }

>              return;

>          }

> -        if ((insn & 0x0f000e10) == 0x0e000a00) {

> -            /* VFP.  */

> -            if (disas_vfp_insn(s, insn)) {

> -                goto illegal_op;

> -            }

> -            return;

> -        }

>          if ((insn & 0x0e000f00) == 0x0c000100) {

>              if (arm_dc_feature(s, ARM_FEATURE_IWMMXT)) {

>                  /* iWMMXt register transfer.  */

> @@ -10588,7 +10558,9 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>          arm_skip_unless(s, cond);

>      }

>

> -    if (disas_a32(s, insn)) {

> +    /* TODO: Perhaps merge these into one decodetree output file.  */

> +    if (disas_a32(s, insn) ||

> +        disas_vfp(s, insn)) {

>          return;

>      }

>      /* fall back to legacy decoder */

> @@ -10597,12 +10569,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>      case 0xc:

>      case 0xd:

>      case 0xe:

> -        if (((insn >> 8) & 0xe) == 10) {

> -            /* VFP.  */

> -            if (disas_vfp_insn(s, insn)) {

> -                goto illegal_op;

> -            }

> -        } else if (disas_coproc_insn(s, insn)) {

> +        if (((insn >> 8) & 0xe) != 10 && disas_coproc_insn(s, insn)) {

>              /* Coprocessor.  */

>              goto illegal_op;

>          }


But now if the VFP decodetree doesn't handle the insn,
we'll fall into this case here, I think, the
"(((insn >> 8) & 0xe) != 10" part of the condition will
be false, and we'll end up at a 'break' statement, which
I think means we'll end up doing a NOP rather than an UNDEF.



> @@ -10691,7 +10658,14 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>          ARCH(6T2);

>      }

>

> -    if (disas_t32(s, insn)) {

> +    /*

> +     * TODO: Perhaps merge these into one decodetree output file.

> +     * Note disas_vfp is written for a32 with cond field in the

> +     * top nibble.  The t32 encoding requires 0xe in the top nibble.

> +     */

> +    if (disas_t32(s, insn) ||

> +        disas_vfp_uncond(s, insn) ||

> +        ((insn >> 28) == 0xe && disas_vfp(s, insn))) {

>          return;

>      }

>      /* fall back to legacy decoder */

> @@ -10708,17 +10682,15 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>                  goto illegal_op; /* op0 = 0b11 : unallocated */

>              }

>

> -            if (disas_vfp_insn(s, insn)) {

> -                if (((insn >> 8) & 0xe) == 10 &&

> -                    dc_isar_feature(aa32_fpsp_v2, s)) {

> -                    /* FP, and the CPU supports it */

> -                    goto illegal_op;

> -                } else {

> -                    /* All other insns: NOCP */

> -                    gen_exception_insn(s, s->pc_curr, EXCP_NOCP,

> -                                       syn_uncategorized(),

> -                                       default_exception_el(s));

> -                }

> +            if (((insn >> 8) & 0xe) == 10 &&

> +                dc_isar_feature(aa32_fpsp_v2, s)) {

> +                /* FP, and the CPU supports it */

> +                goto illegal_op;

> +            } else {

> +                /* All other insns: NOCP */

> +                gen_exception_insn(s, s->pc_curr, EXCP_NOCP,

> +                                   syn_uncategorized(),

> +                                   default_exception_el(s));

>              }

>              break;

>          }

> @@ -10740,10 +10712,6 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>              if (disas_neon_data_insn(s, insn)) {

>                  goto illegal_op;

>              }

> -        } else if (((insn >> 8) & 0xe) == 10) {

> -            if (disas_vfp_insn(s, insn)) {

> -                goto illegal_op;

> -            }

>          } else {

>              if (insn & (1 << 28))

>                  goto illegal_op;


Here in the thumb decoder there's a similar issue I think:
now for insns which are in the VFP space but don't exist
(ie where the decodetree has returned false) we'll end up
now falling through into disas_coproc_insn() rather than
just UNDEFing them.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index b2641b4262..5cabe6b2e9 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -2646,31 +2646,6 @@  static void gen_neon_dup_high16(TCGv_i32 var)
     tcg_temp_free_i32(tmp);
 }
 
-/*
- * Disassemble a VFP instruction.  Returns nonzero if an error occurred
- * (ie. an undefined instruction).
- */
-static int disas_vfp_insn(DisasContext *s, uint32_t insn)
-{
-    /*
-     * If the decodetree decoder handles this insn it will always
-     * emit code to either execute the insn or generate an appropriate
-     * exception; so we don't need to ever return non-zero to tell
-     * the calling code to emit an UNDEF exception.
-     */
-    if (extract32(insn, 28, 4) == 0xf) {
-        if (disas_vfp_uncond(s, insn)) {
-            return 0;
-        }
-    } else {
-        if (disas_vfp(s, insn)) {
-            return 0;
-        }
-    }
-    /* If the decodetree decoder didn't handle this insn, it must be UNDEF */
-    return 1;
-}
-
 static inline bool use_goto_tb(DisasContext *s, target_ulong dest)
 {
 #ifndef CONFIG_USER_ONLY
@@ -10524,7 +10499,9 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
         ARCH(5);
 
         /* Unconditional instructions.  */
-        if (disas_a32_uncond(s, insn)) {
+        /* TODO: Perhaps merge these into one decodetree output file.  */
+        if (disas_a32_uncond(s, insn) ||
+            disas_vfp_uncond(s, insn)) {
             return;
         }
         /* fall back to legacy decoder */
@@ -10551,13 +10528,6 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
             }
             return;
         }
-        if ((insn & 0x0f000e10) == 0x0e000a00) {
-            /* VFP.  */
-            if (disas_vfp_insn(s, insn)) {
-                goto illegal_op;
-            }
-            return;
-        }
         if ((insn & 0x0e000f00) == 0x0c000100) {
             if (arm_dc_feature(s, ARM_FEATURE_IWMMXT)) {
                 /* iWMMXt register transfer.  */
@@ -10588,7 +10558,9 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
         arm_skip_unless(s, cond);
     }
 
-    if (disas_a32(s, insn)) {
+    /* TODO: Perhaps merge these into one decodetree output file.  */
+    if (disas_a32(s, insn) ||
+        disas_vfp(s, insn)) {
         return;
     }
     /* fall back to legacy decoder */
@@ -10597,12 +10569,7 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
     case 0xc:
     case 0xd:
     case 0xe:
-        if (((insn >> 8) & 0xe) == 10) {
-            /* VFP.  */
-            if (disas_vfp_insn(s, insn)) {
-                goto illegal_op;
-            }
-        } else if (disas_coproc_insn(s, insn)) {
+        if (((insn >> 8) & 0xe) != 10 && disas_coproc_insn(s, insn)) {
             /* Coprocessor.  */
             goto illegal_op;
         }
@@ -10691,7 +10658,14 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
         ARCH(6T2);
     }
 
-    if (disas_t32(s, insn)) {
+    /*
+     * TODO: Perhaps merge these into one decodetree output file.
+     * Note disas_vfp is written for a32 with cond field in the 
+     * top nibble.  The t32 encoding requires 0xe in the top nibble.
+     */
+    if (disas_t32(s, insn) ||
+        disas_vfp_uncond(s, insn) ||
+        ((insn >> 28) == 0xe && disas_vfp(s, insn))) {
         return;
     }
     /* fall back to legacy decoder */
@@ -10708,17 +10682,15 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 goto illegal_op; /* op0 = 0b11 : unallocated */
             }
 
-            if (disas_vfp_insn(s, insn)) {
-                if (((insn >> 8) & 0xe) == 10 &&
-                    dc_isar_feature(aa32_fpsp_v2, s)) {
-                    /* FP, and the CPU supports it */
-                    goto illegal_op;
-                } else {
-                    /* All other insns: NOCP */
-                    gen_exception_insn(s, s->pc_curr, EXCP_NOCP,
-                                       syn_uncategorized(),
-                                       default_exception_el(s));
-                }
+            if (((insn >> 8) & 0xe) == 10 &&
+                dc_isar_feature(aa32_fpsp_v2, s)) {
+                /* FP, and the CPU supports it */
+                goto illegal_op;
+            } else {
+                /* All other insns: NOCP */
+                gen_exception_insn(s, s->pc_curr, EXCP_NOCP,
+                                   syn_uncategorized(),
+                                   default_exception_el(s));
             }
             break;
         }
@@ -10740,10 +10712,6 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
             if (disas_neon_data_insn(s, insn)) {
                 goto illegal_op;
             }
-        } else if (((insn >> 8) & 0xe) == 10) {
-            if (disas_vfp_insn(s, insn)) {
-                goto illegal_op;
-            }
         } else {
             if (insn & (1 << 28))
                 goto illegal_op;