[Qemu-devel] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps

[Qemu-devel] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps

[Peter Maydell]

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.


Peter Maydell (3):
  target-arm: Correct misleading 'is_thumb' syn_* parameter names
  target-arm: Fix IL bit reported for Thumb coprocessor traps
  target-arm: Fix IL bit reported for Thumb VFP and Neon traps

 target-arm/internals.h | 28 ++++++++++++++--------------
 target-arm/translate.c | 14 +++++++-------
 2 files changed, 21 insertions(+), 21 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names

[Peter Maydell]

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

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/3] target-arm: Correct misleading 'is_thumb' syn_* parameter names

[Sergey Fedorov]

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);
>  }
>  

[Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 2/3] target-arm: Fix IL bit reported for Thumb coprocessor traps

[Sergey Fedorov]

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:

[Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH 3/3] target-arm: Fix IL bit reported for Thumb VFP and Neon traps

[Sergey Fedorov]

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;
>      }
>  

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/3] target-arm: Fix IL in syndromes for FP and copro traps

[Peter Maydell]

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