On 29 November 2013 15:26, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 28 November 2013 17:07, Will Newton <will.new...@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.new...@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!
--
Will Newton
Toolchain Working Group, Linaro
