[v6,1/4] target-arm: Move call to disas_vfp_insn out of disas_coproc_insn.

Floating point is an extension to the instruction set rather than
a coprocessor, so call it directly from the ARM and Thumb decode
functions.

Signed-off-by: Will Newton <will.newton@linaro.org>
---
 target-arm/translate.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

Changes in v6:
 - Add return after disas_vfp_insn call in disas_arm_insn

On 28 November 2013 17:07, Will Newton <will.newton@linaro.org> wrote:
>
> Floating point is an extension to the instruction set rather than
> a coprocessor, so call it directly from the ARM and Thumb decode
> functions.

Hi; thanks for these patches. A minor process note:
for future versions, can you make sure you send patches
out properly threaded under a cover letter mail? That
will help me since some tools I use to process patches
rely on patchseries coming as a properly threaded set
of mails.

> Signed-off-by: Will Newton <will.newton@linaro.org>
> ---
>  target-arm/translate.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
>
> Changes in v6:
>  - Add return after disas_vfp_insn call in disas_arm_insn
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 5f003e7..5a6c1ea 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -2636,6 +2636,13 @@ static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
>              && rn != ARM_VFP_MVFR1 && rn != ARM_VFP_MVFR0)
>              return 1;
>      }
> +
> +    if (extract32(insn, 28, 4) == 0xf) {
> +        /* Encodings with T=1 (Thumb) or unconditional (ARM):
> +           only used in v8 and above.  */

Since I'm nitpicking: I prefer the multiline comment form
     /* text text
      * line 2
      */

though I know we're not always consistent in existing code.

> +        return 1;
> +    }
> +
>      dp = ((insn & 0xf00) == 0xb00);
>      switch ((insn >> 24) & 0xf) {
>      case 0xe:
> @@ -6296,9 +6303,6 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
>             return disas_dsp_insn(env, s, insn);
>         }
>         return 1;
> -    case 10:
> -    case 11:
> -       return disas_vfp_insn (env, s, insn);
>      default:
>          break;
>      }
> @@ -6753,6 +6757,12 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>                  goto illegal_op;
>              return;
>          }
> +        if ((insn & 0x0f000e10) == 0x0e000a00) {
> +            /* VFP.  */
> +            if (disas_vfp_insn(env, s, insn))
> +                goto illegal_op;

Missing braces. (scripts/checkpatch.pl will catch this kind of nit.)

> +            return;
> +        }
>          if (((insn & 0x0f30f000) == 0x0510f000) ||
>              ((insn & 0x0f30f010) == 0x0710f000)) {
>              if ((insn & (1 << 22)) == 0) {
> @@ -8033,9 +8043,15 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>          case 0xc:
>          case 0xd:
>          case 0xe:
> -            /* Coprocessor.  */
> -            if (disas_coproc_insn(env, s, insn))
> +            if (((insn >> 8) & 0xe) == 10) {
> +                /* VFP.  */
> +                if (disas_vfp_insn(env, s, insn)) {
> +                    goto illegal_op;
> +                }
> +            } else if (disas_coproc_insn(env, s, insn)) {
> +                /* Coprocessor.  */
>                  goto illegal_op;
> +            }
>              break;
>          case 0xf:
>              /* swi */
> @@ -8765,6 +8781,10 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>              insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);
>              if (disas_neon_data_insn(env, s, insn))
>                  goto illegal_op;
> +        } else if (((insn >> 8) & 0xe) == 10) {
> +            if (disas_vfp_insn(env, s, insn)) {
> +                goto illegal_op;
> +            }
>          } else {
>              if (insn & (1 << 28))
>                  goto illegal_op;
> --
> 1.8.1.4
>

Codewise this patch looks fine though.

thanks
-- PMM

On 29 November 2013 15:26, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 28 November 2013 17:07, Will Newton <will.newton@linaro.org> wrote:
>>
>> Floating point is an extension to the instruction set rather than
>> a coprocessor, so call it directly from the ARM and Thumb decode
>> functions.
>
> Hi; thanks for these patches. A minor process note:
> for future versions, can you make sure you send patches
> out properly threaded under a cover letter mail? That
> will help me since some tools I use to process patches
> rely on patchseries coming as a properly threaded set
> of mails.
>
>> Signed-off-by: Will Newton <will.newton@linaro.org>
>> ---
>>  target-arm/translate.c | 30 +++++++++++++++++++++++++-----
>>  1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> Changes in v6:
>>  - Add return after disas_vfp_insn call in disas_arm_insn
>>
>> diff --git a/target-arm/translate.c b/target-arm/translate.c
>> index 5f003e7..5a6c1ea 100644
>> --- a/target-arm/translate.c
>> +++ b/target-arm/translate.c
>> @@ -2636,6 +2636,13 @@ static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
>>              && rn != ARM_VFP_MVFR1 && rn != ARM_VFP_MVFR0)
>>              return 1;
>>      }
>> +
>> +    if (extract32(insn, 28, 4) == 0xf) {
>> +        /* Encodings with T=1 (Thumb) or unconditional (ARM):
>> +           only used in v8 and above.  */
>
> Since I'm nitpicking: I prefer the multiline comment form
>      /* text text
>       * line 2
>       */
>
> though I know we're not always consistent in existing code.
>
>> +        return 1;
>> +    }
>> +
>>      dp = ((insn & 0xf00) == 0xb00);
>>      switch ((insn >> 24) & 0xf) {
>>      case 0xe:
>> @@ -6296,9 +6303,6 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
>>             return disas_dsp_insn(env, s, insn);
>>         }
>>         return 1;
>> -    case 10:
>> -    case 11:
>> -       return disas_vfp_insn (env, s, insn);
>>      default:
>>          break;
>>      }
>> @@ -6753,6 +6757,12 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>>                  goto illegal_op;
>>              return;
>>          }
>> +        if ((insn & 0x0f000e10) == 0x0e000a00) {
>> +            /* VFP.  */
>> +            if (disas_vfp_insn(env, s, insn))
>> +                goto illegal_op;
>
> Missing braces. (scripts/checkpatch.pl will catch this kind of nit.)

I had noticed the checkpatch warnings but as they seemed inconsistent
with the rest of the file I assumed they were a bug in checkpatch!