QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Maydell <peter.maydell@linaro.org>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: QEMU Developers <qemu-devel@nongnu.org>
Subject: Re: [PATCH 14/19] target/arm: Move the vfp decodetree calls next to the base isa
Date: Thu, 20 Feb 2020 17:16:21 +0000
Message-ID: <CAFEAcA9OMmVoULYPKvj4T-6w7ZL5t8L+HT2XjV_Zo2jXvZXKUw@mail.gmail.com> (raw)
In-Reply-To: <20200214181547.21408-15-richard.henderson@linaro.org>

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


  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-14 18:15 [PATCH 00/19] target/arm: vfp feature and decodetree cleanup Richard Henderson
2020-02-14 18:15 ` [PATCH 01/19] target/arm: Fix field extract from MVFR[0-2] Richard Henderson
2020-02-14 18:29   ` Philippe Mathieu-Daudé
2020-02-14 18:35   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 02/19] target/arm: Rename isar_feature_aa32_simd_r32 Richard Henderson
2020-02-14 18:50   ` Philippe Mathieu-Daudé
2020-02-21 15:58   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 03/19] target/arm: Use isar_feature_aa32_simd_r32 more places Richard Henderson
2020-02-21 16:02   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 04/19] target/arm: Set MVFR0.FPSP for ARMv5 cpus Richard Henderson
2020-02-21 16:02   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 05/19] target/arm: Add isar_feature_aa32_simd_r16 Richard Henderson
2020-02-21 16:01   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 06/19] target/arm: Rename isar_feature_aa32_fpdp_v2 Richard Henderson
2020-02-14 18:51   ` Philippe Mathieu-Daudé
2020-02-14 18:15 ` [PATCH 07/19] target/arm: Add isar_feature_aa32_{fpsp_v2, fpsp_v3, fpdp_v3} Richard Henderson
2020-02-21 16:03   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 08/19] target/arm: Perform fpdp_v2 check first Richard Henderson
2020-02-21 16:04   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 09/19] target/arm: Replace ARM_FEATURE_VFP3 checks with fp{sp, dp}_v3 Richard Henderson
2020-02-21 16:05   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 10/19] target/arm: Add missing checks for fpsp_v2 Richard Henderson
2020-02-21 16:05   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 11/19] target/arm: Replace ARM_FEATURE_VFP4 with isar_feature_aa32_simdfmac Richard Henderson
2020-02-20 16:37   ` Peter Maydell
2020-02-20 16:41     ` Peter Maydell
2020-02-20 17:55     ` Richard Henderson
2020-02-14 18:15 ` [PATCH 12/19] target/arm: Remove ARM_FEATURE_VFP check from disas_vfp_insn Richard Henderson
2020-02-20 17:19   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 13/19] target/arm: Move VLLDM and VLSTM to vfp.decode Richard Henderson
2020-02-20 17:02   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 14/19] target/arm: Move the vfp decodetree calls next to the base isa Richard Henderson
2020-02-20 17:16   ` Peter Maydell [this message]
2020-02-14 18:15 ` [PATCH 15/19] linux-user/arm: Replace ARM_FEATURE_VFP* tests for HWCAP Richard Henderson
2020-02-20 17:32   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 16/19] target/arm: Remove ARM_FEATURE_VFP* Richard Henderson
2020-02-20 17:33   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 17/19] target/arm: Add formats for some vfp 2 and 3-register insns Richard Henderson
2020-02-20 17:40   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 18/19] target/arm: Split VFM decode Richard Henderson
2020-02-20 17:45   ` Peter Maydell
2020-02-14 18:15 ` [PATCH 19/19] target/arm: Split VMINMAXNM decode Richard Henderson
2020-02-20 17:49   ` Peter Maydell
2020-02-14 20:11 ` [PATCH 00/19] target/arm: vfp feature and decodetree cleanup no-reply
2020-02-20 17:52 ` Peter Maydell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFEAcA9OMmVoULYPKvj4T-6w7ZL5t8L+HT2XjV_Zo2jXvZXKUw@mail.gmail.com \
    --to=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git