* [Qemu-devel] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names
2016-02-05 14:37 [Qemu-devel] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps Peter Maydell
@ 2016-02-05 14:37 ` Peter Maydell
2016-02-06 18:25 ` [Qemu-devel] [Qemu-arm] " Sergey Fedorov
2016-02-05 14:37 ` [Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps Peter Maydell
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2016-02-05 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, patches
In syndrome register values, the IL bit indicates the instruction
length, and is 1 for 4-byte instructions and 0 for 2-byte
instructions. All A64 and A32 instructions are 4-byte, but
Thumb instructions may be either 2 or 4 bytes long. Unfortunately
we named the parameter to the syn_* functions for constructing
syndromes "is_thumb", which falsely implies that it should be
set for all Thumb instructions, rather than only the 16-bit ones.
Fix the functions to name the parameter 'is_16bit' instead.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target-arm/internals.h | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/target-arm/internals.h b/target-arm/internals.h
index d226bbe..a648c1e 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -270,10 +270,10 @@ static inline uint32_t syn_aa64_smc(uint32_t imm16)
return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
}
-static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
+static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_16bit)
{
return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
- | (is_thumb ? 0 : ARM_EL_IL);
+ | (is_16bit ? 0 : ARM_EL_IL);
}
static inline uint32_t syn_aa32_hvc(uint32_t imm16)
@@ -291,10 +291,10 @@ static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
}
-static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_thumb)
+static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_16bit)
{
return (EC_AA32_BKPT << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
- | (is_thumb ? 0 : ARM_EL_IL);
+ | (is_16bit ? 0 : ARM_EL_IL);
}
static inline uint32_t syn_aa64_sysregtrap(int op0, int op1, int op2,
@@ -308,48 +308,48 @@ static inline uint32_t syn_aa64_sysregtrap(int op0, int op1, int op2,
static inline uint32_t syn_cp14_rt_trap(int cv, int cond, int opc1, int opc2,
int crn, int crm, int rt, int isread,
- bool is_thumb)
+ bool is_16bit)
{
return (EC_CP14RTTRAP << ARM_EL_EC_SHIFT)
- | (is_thumb ? 0 : ARM_EL_IL)
+ | (is_16bit ? 0 : ARM_EL_IL)
| (cv << 24) | (cond << 20) | (opc2 << 17) | (opc1 << 14)
| (crn << 10) | (rt << 5) | (crm << 1) | isread;
}
static inline uint32_t syn_cp15_rt_trap(int cv, int cond, int opc1, int opc2,
int crn, int crm, int rt, int isread,
- bool is_thumb)
+ bool is_16bit)
{
return (EC_CP15RTTRAP << ARM_EL_EC_SHIFT)
- | (is_thumb ? 0 : ARM_EL_IL)
+ | (is_16bit ? 0 : ARM_EL_IL)
| (cv << 24) | (cond << 20) | (opc2 << 17) | (opc1 << 14)
| (crn << 10) | (rt << 5) | (crm << 1) | isread;
}
static inline uint32_t syn_cp14_rrt_trap(int cv, int cond, int opc1, int crm,
int rt, int rt2, int isread,
- bool is_thumb)
+ bool is_16bit)
{
return (EC_CP14RRTTRAP << ARM_EL_EC_SHIFT)
- | (is_thumb ? 0 : ARM_EL_IL)
+ | (is_16bit ? 0 : ARM_EL_IL)
| (cv << 24) | (cond << 20) | (opc1 << 16)
| (rt2 << 10) | (rt << 5) | (crm << 1) | isread;
}
static inline uint32_t syn_cp15_rrt_trap(int cv, int cond, int opc1, int crm,
int rt, int rt2, int isread,
- bool is_thumb)
+ bool is_16bit)
{
return (EC_CP15RRTTRAP << ARM_EL_EC_SHIFT)
- | (is_thumb ? 0 : ARM_EL_IL)
+ | (is_16bit ? 0 : ARM_EL_IL)
| (cv << 24) | (cond << 20) | (opc1 << 16)
| (rt2 << 10) | (rt << 5) | (crm << 1) | isread;
}
-static inline uint32_t syn_fp_access_trap(int cv, int cond, bool is_thumb)
+static inline uint32_t syn_fp_access_trap(int cv, int cond, bool is_16bit)
{
return (EC_ADVSIMDFPACCESSTRAP << ARM_EL_EC_SHIFT)
- | (is_thumb ? 0 : ARM_EL_IL)
+ | (is_16bit ? 0 : ARM_EL_IL)
| (cv << 24) | (cond << 20);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names
2016-02-05 14:37 ` [Qemu-devel] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names Peter Maydell
@ 2016-02-06 18:25 ` Sergey Fedorov
0 siblings, 0 replies; 8+ messages in thread
From: Sergey Fedorov @ 2016-02-06 18:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: qemu-arm, patches
On 05.02.2016 17:37, Peter Maydell wrote:
> In syndrome register values, the IL bit indicates the instruction
> length, and is 1 for 4-byte instructions and 0 for 2-byte
> instructions. All A64 and A32 instructions are 4-byte, but
> Thumb instructions may be either 2 or 4 bytes long. Unfortunately
> we named the parameter to the syn_* functions for constructing
> syndromes "is_thumb", which falsely implies that it should be
> set for all Thumb instructions, rather than only the 16-bit ones.
> Fix the functions to name the parameter 'is_16bit' instead.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
> target-arm/internals.h | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index d226bbe..a648c1e 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -270,10 +270,10 @@ static inline uint32_t syn_aa64_smc(uint32_t imm16)
> return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> }
>
> -static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
> +static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_16bit)
> {
> return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> - | (is_thumb ? 0 : ARM_EL_IL);
> + | (is_16bit ? 0 : ARM_EL_IL);
> }
>
> static inline uint32_t syn_aa32_hvc(uint32_t imm16)
> @@ -291,10 +291,10 @@ static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
> return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> }
>
> -static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_thumb)
> +static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_16bit)
> {
> return (EC_AA32_BKPT << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
> - | (is_thumb ? 0 : ARM_EL_IL);
> + | (is_16bit ? 0 : ARM_EL_IL);
> }
>
> static inline uint32_t syn_aa64_sysregtrap(int op0, int op1, int op2,
> @@ -308,48 +308,48 @@ static inline uint32_t syn_aa64_sysregtrap(int op0, int op1, int op2,
>
> static inline uint32_t syn_cp14_rt_trap(int cv, int cond, int opc1, int opc2,
> int crn, int crm, int rt, int isread,
> - bool is_thumb)
> + bool is_16bit)
> {
> return (EC_CP14RTTRAP << ARM_EL_EC_SHIFT)
> - | (is_thumb ? 0 : ARM_EL_IL)
> + | (is_16bit ? 0 : ARM_EL_IL)
> | (cv << 24) | (cond << 20) | (opc2 << 17) | (opc1 << 14)
> | (crn << 10) | (rt << 5) | (crm << 1) | isread;
> }
>
> static inline uint32_t syn_cp15_rt_trap(int cv, int cond, int opc1, int opc2,
> int crn, int crm, int rt, int isread,
> - bool is_thumb)
> + bool is_16bit)
> {
> return (EC_CP15RTTRAP << ARM_EL_EC_SHIFT)
> - | (is_thumb ? 0 : ARM_EL_IL)
> + | (is_16bit ? 0 : ARM_EL_IL)
> | (cv << 24) | (cond << 20) | (opc2 << 17) | (opc1 << 14)
> | (crn << 10) | (rt << 5) | (crm << 1) | isread;
> }
>
> static inline uint32_t syn_cp14_rrt_trap(int cv, int cond, int opc1, int crm,
> int rt, int rt2, int isread,
> - bool is_thumb)
> + bool is_16bit)
> {
> return (EC_CP14RRTTRAP << ARM_EL_EC_SHIFT)
> - | (is_thumb ? 0 : ARM_EL_IL)
> + | (is_16bit ? 0 : ARM_EL_IL)
> | (cv << 24) | (cond << 20) | (opc1 << 16)
> | (rt2 << 10) | (rt << 5) | (crm << 1) | isread;
> }
>
> static inline uint32_t syn_cp15_rrt_trap(int cv, int cond, int opc1, int crm,
> int rt, int rt2, int isread,
> - bool is_thumb)
> + bool is_16bit)
> {
> return (EC_CP15RRTTRAP << ARM_EL_EC_SHIFT)
> - | (is_thumb ? 0 : ARM_EL_IL)
> + | (is_16bit ? 0 : ARM_EL_IL)
> | (cv << 24) | (cond << 20) | (opc1 << 16)
> | (rt2 << 10) | (rt << 5) | (crm << 1) | isread;
> }
>
> -static inline uint32_t syn_fp_access_trap(int cv, int cond, bool is_thumb)
> +static inline uint32_t syn_fp_access_trap(int cv, int cond, bool is_16bit)
> {
> return (EC_ADVSIMDFPACCESSTRAP << ARM_EL_EC_SHIFT)
> - | (is_thumb ? 0 : ARM_EL_IL)
> + | (is_16bit ? 0 : ARM_EL_IL)
> | (cv << 24) | (cond << 20);
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps
2016-02-05 14:37 [Qemu-devel] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps Peter Maydell
2016-02-05 14:37 ` [Qemu-devel] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names Peter Maydell
@ 2016-02-05 14:37 ` Peter Maydell
2016-02-06 18:24 ` Sergey Fedorov
2016-02-05 14:37 ` [Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps Peter Maydell
2016-02-08 13:17 ` [Qemu-devel] [Qemu-arm] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps Peter Maydell
3 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2016-02-05 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, patches
All Thumb coprocessor instructions are 32 bits, so the IL
bit in the syndrome register should be set. Pass false to the
syn_* function's is_16bit argument rather than s->thumb
so we report the correct IL bit.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target-arm/translate.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 3ec758a..10792e8 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -7184,19 +7184,19 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
case 14:
if (is64) {
syndrome = syn_cp14_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
- isread, s->thumb);
+ isread, false);
} else {
syndrome = syn_cp14_rt_trap(1, 0xe, opc1, opc2, crn, crm,
- rt, isread, s->thumb);
+ rt, isread, false);
}
break;
case 15:
if (is64) {
syndrome = syn_cp15_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
- isread, s->thumb);
+ isread, false);
} else {
syndrome = syn_cp15_rt_trap(1, 0xe, opc1, opc2, crn, crm,
- rt, isread, s->thumb);
+ rt, isread, false);
}
break;
default:
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps
2016-02-05 14:37 ` [Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps Peter Maydell
@ 2016-02-06 18:24 ` Sergey Fedorov
0 siblings, 0 replies; 8+ messages in thread
From: Sergey Fedorov @ 2016-02-06 18:24 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: qemu-arm, patches
On 05.02.2016 17:37, Peter Maydell wrote:
> All Thumb coprocessor instructions are 32 bits, so the IL
> bit in the syndrome register should be set. Pass false to the
> syn_* function's is_16bit argument rather than s->thumb
> so we report the correct IL bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
> target-arm/translate.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 3ec758a..10792e8 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7184,19 +7184,19 @@ static int disas_coproc_insn(DisasContext *s, uint32_t insn)
> case 14:
> if (is64) {
> syndrome = syn_cp14_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
> - isread, s->thumb);
> + isread, false);
> } else {
> syndrome = syn_cp14_rt_trap(1, 0xe, opc1, opc2, crn, crm,
> - rt, isread, s->thumb);
> + rt, isread, false);
> }
> break;
> case 15:
> if (is64) {
> syndrome = syn_cp15_rrt_trap(1, 0xe, opc1, crm, rt, rt2,
> - isread, s->thumb);
> + isread, false);
> } else {
> syndrome = syn_cp15_rt_trap(1, 0xe, opc1, opc2, crn, crm,
> - rt, isread, s->thumb);
> + rt, isread, false);
> }
> break;
> default:
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps
2016-02-05 14:37 [Qemu-devel] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps Peter Maydell
2016-02-05 14:37 ` [Qemu-devel] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names Peter Maydell
2016-02-05 14:37 ` [Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps Peter Maydell
@ 2016-02-05 14:37 ` Peter Maydell
2016-02-06 18:25 ` Sergey Fedorov
2016-02-08 13:17 ` [Qemu-devel] [Qemu-arm] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps Peter Maydell
3 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2016-02-05 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, patches
All Thumb Neon and VFP instructions are 32 bits, so the IL
bit in the syndrome register should be set. Pass false to the
syn_* function's is_16bit argument rather than s->thumb
so we report the correct IL bit.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
target-arm/translate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 10792e8..fa8e22c 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -3077,7 +3077,7 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
*/
if (s->fp_excp_el) {
gen_exception_insn(s, 4, EXCP_UDEF,
- syn_fp_access_trap(1, 0xe, s->thumb), s->fp_excp_el);
+ syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -4399,7 +4399,7 @@ static int disas_neon_ls_insn(DisasContext *s, uint32_t insn)
*/
if (s->fp_excp_el) {
gen_exception_insn(s, 4, EXCP_UDEF,
- syn_fp_access_trap(1, 0xe, s->thumb), s->fp_excp_el);
+ syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
@@ -5137,7 +5137,7 @@ static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
*/
if (s->fp_excp_el) {
gen_exception_insn(s, 4, EXCP_UDEF,
- syn_fp_access_trap(1, 0xe, s->thumb), s->fp_excp_el);
+ syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps
2016-02-05 14:37 ` [Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps Peter Maydell
@ 2016-02-06 18:25 ` Sergey Fedorov
0 siblings, 0 replies; 8+ messages in thread
From: Sergey Fedorov @ 2016-02-06 18:25 UTC (permalink / raw)
To: Peter Maydell, qemu-devel; +Cc: qemu-arm, patches
On 05.02.2016 17:37, Peter Maydell wrote:
> All Thumb Neon and VFP instructions are 32 bits, so the IL
> bit in the syndrome register should be set. Pass false to the
> syn_* function's is_16bit argument rather than s->thumb
> so we report the correct IL bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
> target-arm/translate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 10792e8..fa8e22c 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -3077,7 +3077,7 @@ static int disas_vfp_insn(DisasContext *s, uint32_t insn)
> */
> if (s->fp_excp_el) {
> gen_exception_insn(s, 4, EXCP_UDEF,
> - syn_fp_access_trap(1, 0xe, s->thumb), s->fp_excp_el);
> + syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
> return 0;
> }
>
> @@ -4399,7 +4399,7 @@ static int disas_neon_ls_insn(DisasContext *s, uint32_t insn)
> */
> if (s->fp_excp_el) {
> gen_exception_insn(s, 4, EXCP_UDEF,
> - syn_fp_access_trap(1, 0xe, s->thumb), s->fp_excp_el);
> + syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
> return 0;
> }
>
> @@ -5137,7 +5137,7 @@ static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
> */
> if (s->fp_excp_el) {
> gen_exception_insn(s, 4, EXCP_UDEF,
> - syn_fp_access_trap(1, 0xe, s->thumb), s->fp_excp_el);
> + syn_fp_access_trap(1, 0xe, false), s->fp_excp_el);
> return 0;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [Qemu-arm] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps
2016-02-05 14:37 [Qemu-devel] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps Peter Maydell
` (2 preceding siblings ...)
2016-02-05 14:37 ` [Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps Peter Maydell
@ 2016-02-08 13:17 ` Peter Maydell
3 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2016-02-08 13:17 UTC (permalink / raw)
To: QEMU Developers; +Cc: qemu-arm, Patch Tracking
On 5 February 2016 at 14:37, Peter Maydell <peter.maydell@linaro.org> wrote:
> This series corrects a bug I noticed while reading the code.
>
> In syndrome register values, the IL bit indicates the instruction
> length, and is 1 for 4-byte instructions and 0 for 2-byte
> instructions. All A64 and A32 instructions are 4-byte, but Thumb
> instructions may be either 2 or 4 bytes long. Unfortunately we named
> the parameter to the syn_* functions for constructing syndromes
> "is_thumb", which falsely implies that it should be set for all Thumb
> instructions, rather than only the 16-bit ones.
>
> Fix the parameter names to a less confusing "is_16bit", and
> correct the places where we should be passing in 'false' rather
> than 's->thumb' for syndrome construction, which are the
> coprocessor, VFP and Neon instruction traps (all these are always
> 32-bit for Thumb).
>
> The calls to syn_aa32_svc() and syn_aa32_bkpt() correctly still
> use s->thumb, because for SVC and BKPT the Thumb encoding is 16
> bits but the ARM encoding is 32 bits.
Applied to target-arm.next, thanks.
-- PMM
^ permalink raw reply [flat|nested] 8+ messages in thread