diff mbox

[4/4] target-arm: Fix decoding of Thumb preload and hint space

Message ID 1296762205-3043-5-git-send-email-peter.maydell@linaro.org
State Accepted
Commit a2fdc8907bee6eb572fea992be7126e1b616d5bf
Headers show

Commit Message

Peter Maydell Feb. 3, 2011, 7:43 p.m. UTC 
Refine the decoding of the Thumb preload and hint space, so we
UNDEF on the patterns that are supposed to UNDEF rather than NOP.
We also move the tests for this space earlier, so we don't emit
harmless but unnecessary address generation code for preload
hints (which by their nature are likely to be in hot code paths).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   70 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 48 insertions(+), 22 deletions(-)

Comments

Peter Maydell Feb. 3, 2011, 8:33 p.m. UTC | #1 
On 3 February 2011 19:43, Peter Maydell <peter.maydell@linaro.org> wrote:
> @@ -8326,9 +8362,8 @@ static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
>                 imm = insn & 0xfff;
>                 tcg_gen_addi_i32(addr, addr, imm);
>             } else {
> -                op = (insn >> 8) & 7;
>                 imm = insn & 0xff;
> -                switch (op) {
> +                switch ((insn >> 8) & 7) {
>                 case 0: case 8: /* Shifted Register.  */
>                     shift = (insn >> 4) & 0xf;
>                     if (shift > 3)

It's pretty obvious from the combination of "switch (something & 7)" with
"case 8:" that the load/store address decode logic here is bogus, but
since that's unrelated to the preload/hint space I'm going to address
it in a separate patchset later.

-- PMM
diff mbox

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index b742394..b445355 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -8308,6 +8308,42 @@  static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
                 goto illegal_op;
             break;
         }
+        op = ((insn >> 21) & 3) | ((insn >> 22) & 4);
+        if (rs == 15) {
+            if (!(insn & (1 << 20))) {
+                goto illegal_op;
+            }
+            if (op != 2) {
+                /* Byte or halfword load space with dest == r15 : memory hints.
+                 * Catch them early so we don't emit pointless addressing code.
+                 * This space is a mix of:
+                 *  PLD/PLDW/PLI,  which we implement as NOPs (note that unlike
+                 *     the ARM encodings, PLDW space doesn't UNDEF for non-v7MP
+                 *     cores)
+                 *  unallocated hints, which must be treated as NOPs
+                 *  UNPREDICTABLE space, which we NOP or UNDEF depending on
+                 *     which is easiest for the decoding logic
+                 *  Some space which must UNDEF
+                 */
+                int op1 = (insn >> 23) & 3;
+                int op2 = (insn >> 6) & 0x3f;
+                if (op & 2) {
+                    goto illegal_op;
+                }
+                if (rn == 15) {
+                    /* UNPREDICTABLE or unallocated hint */
+                    return 0;
+                }
+                if (op1 & 1) {
+                    return 0; /* PLD* or unallocated hint */
+                }
+                if ((op2 == 0) || ((op2 & 0x3c) == 0x30)) {
+                    return 0; /* PLD* or unallocated hint */
+                }
+                /* UNDEF space, or an UNPREDICTABLE */
+                return 1;
+            }
+        }
         user = IS_USER(s);
         if (rn == 15) {
             addr = new_tmp();
@@ -8326,9 +8362,8 @@  static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
                 imm = insn & 0xfff;
                 tcg_gen_addi_i32(addr, addr, imm);
             } else {
-                op = (insn >> 8) & 7;
                 imm = insn & 0xff;
-                switch (op) {
+                switch ((insn >> 8) & 7) {
                 case 0: case 8: /* Shifted Register.  */
                     shift = (insn >> 4) & 0xf;
                     if (shift > 3)
@@ -8365,32 +8400,23 @@  static int disas_thumb2_insn(CPUState *env, DisasContext *s, uint16_t insn_hw1)
                 }
             }
         }
-        op = ((insn >> 21) & 3) | ((insn >> 22) & 4);
         if (insn & (1 << 20)) {
             /* Load.  */
-            if (rs == 15 && op != 2) {
-                if (op & 2)
-                    goto illegal_op;
-                /* Memory hint.  Implemented as NOP.  */
+            switch (op) {
+            case 0: tmp = gen_ld8u(addr, user); break;
+            case 4: tmp = gen_ld8s(addr, user); break;
+            case 1: tmp = gen_ld16u(addr, user); break;
+            case 5: tmp = gen_ld16s(addr, user); break;
+            case 2: tmp = gen_ld32(addr, user); break;
+            default: goto illegal_op;
+            }
+            if (rs == 15) {
+                gen_bx(s, tmp);
             } else {
-                switch (op) {
-                case 0: tmp = gen_ld8u(addr, user); break;
-                case 4: tmp = gen_ld8s(addr, user); break;
-                case 1: tmp = gen_ld16u(addr, user); break;
-                case 5: tmp = gen_ld16s(addr, user); break;
-                case 2: tmp = gen_ld32(addr, user); break;
-                default: goto illegal_op;
-                }
-                if (rs == 15) {
-                    gen_bx(s, tmp);
-                } else {
-                    store_reg(s, rs, tmp);
-                }
+                store_reg(s, rs, tmp);
             }
         } else {
             /* Store.  */
-            if (rs == 15)
-                goto illegal_op;
             tmp = load_reg(s, rs);
             switch (op) {
             case 0: gen_st8(tmp, addr, user); break;