Re: [Qemu-devel] [PATCH v2 13/17] target-arm: Use uint16_t in syndrome generators with 16bit imms

[Greg Bellows]

Called out possibly missing fix below.  Beside it, I'm convinced this change is beneficial, other than maybe for readability.   

The change does not account for uses of the affected calls in target-arm/translate.c.  As well, the change has somewhat a ripple effect as certain code expects to receive or return a 32-bit value.

Unless there is a functional benefit, my suggestion would be to omit this change or submit it separately as it is not really related or required for your  EL2/EL3 changes.  

Greg

On 9 June 2014 10:04, Edgar E. Iglesias <address@hidden> wrote:

From: "Edgar E. Iglesias" <address@hidden>

Avoids the explicit 16bit mask. No functional change.

Signed-off-by: Edgar E. Iglesias <address@hidden>
---
 target-arm/internals.h     | 14 +++++++-------
 target-arm/translate-a64.c |  2 +-
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index 08fa697..707643e 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -199,23 +199,23 @@ static inline uint32_t syn_uncategorized(void)
     return (EC_UNCATEGORIZED << ARM_EL_EC_SHIFT) | ARM_EL_IL;
 }

-static inline uint32_t syn_aa64_svc(uint32_t imm16)
+static inline uint32_t syn_aa64_svc(uint16_t imm16)
 {
-    return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+    return (EC_AA64_SVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | imm16;
 }

-static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb)
+static inline uint32_t syn_aa32_svc(uint16_t imm16, bool is_thumb)
 {
-    return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
+    return (EC_AA32_SVC << ARM_EL_EC_SHIFT) | imm16
         | (is_thumb ? 0 : ARM_EL_IL);
 }

-static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
+static inline uint32_t syn_aa64_bkpt(uint16_t imm16)
 {
-    return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
+    return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | imm16;
 }

-static inline uint32_t syn_aa32_bkpt(uint32_t imm16, bool is_thumb)
+static inline uint32_t syn_aa32_bkpt(uint16_t imm16, bool is_thumb)
 {
     return (EC_AA32_BKPT << ARM_EL_EC_SHIFT) | (imm16 & 0xffff)
         | (is_thumb ? 0 : ARM_EL_IL);

Looks like you forgot to remove the mask on this one.

 

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index a9c4633..3589898 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1433,7 +1433,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 {
     int opc = extract32(insn, 21, 3);
     int op2_ll = extract32(insn, 0, 5);
-    int imm16 = extract32(insn, 5, 16);
+    uint16_t imm16 = extract32(insn, 5, 16);

     switch (opc) {
     case 0:
--
1.8.3.2