* [Qemu-devel] [PATCH v6 1/4] target-arm: Move call to disas_vfp_insn out of disas_coproc_insn.
@ 2013-11-28 17:07 Will Newton
2013-11-29 15:26 ` Peter Maydell
0 siblings, 1 reply; 3+ messages in thread
From: Will Newton @ 2013-11-28 17:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Patch Tracking
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
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. */
+ 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;
+ 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/4] target-arm: Move call to disas_vfp_insn out of disas_coproc_insn.
2013-11-28 17:07 [Qemu-devel] [PATCH v6 1/4] target-arm: Move call to disas_vfp_insn out of disas_coproc_insn Will Newton
@ 2013-11-29 15:26 ` Peter Maydell
2013-11-29 15:31 ` Will Newton
0 siblings, 1 reply; 3+ messages in thread
From: Peter Maydell @ 2013-11-29 15:26 UTC (permalink / raw)
To: Will Newton; +Cc: QEMU Developers, Patch Tracking
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/4] target-arm: Move call to disas_vfp_insn out of disas_coproc_insn.
2013-11-29 15:26 ` Peter Maydell
@ 2013-11-29 15:31 ` Will Newton
0 siblings, 0 replies; 3+ messages in thread
From: Will Newton @ 2013-11-29 15:31 UTC (permalink / raw)
To: Peter Maydell; +Cc: QEMU Developers, Patch Tracking
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!
--
Will Newton
Toolchain Working Group, Linaro
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-11-29 15:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-11-28 17:07 [Qemu-devel] [PATCH v6 1/4] target-arm: Move call to disas_vfp_insn out of disas_coproc_insn Will Newton
2013-11-29 15:26 ` Peter Maydell
2013-11-29 15:31 ` Will Newton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).