diff mbox

target-arm: Decode SETEND correctly in Thumb

Message ID 1331649908-5418-1-git-send-email-peter.maydell@linaro.org
State Accepted
Commit d9e028c1d9c2a8d2ad42f2aeb502b71086a52d4d
Headers show

Commit Message

Peter Maydell March 13, 2012, 2:45 p.m. UTC
Decode the SETEND instruction correctly in Thumb mode,
rather than accidentally treating it like CPS. We don't
support BE8 mode, but this change brings the Thumb mode
in to line with behaviour in ARM mode: 'SETEND BE' is
not supported and will provoke an UNDEF exception, but
'SETEND LE' is correctly handled as a no-op.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reported-by: Daniel Forsgren <daniel.forsgren@enea.com>
---
This is one of those patches where I wasn't sure whether to try
to split it into a whitespace-only part and a significant-change
part. Most of this is (a) indenting existing code another notch
for the extra switch statement and (b) adding braces to placate
checkpatch.

 target-arm/translate.c |   63 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 40 insertions(+), 23 deletions(-)

Comments

Laurent Desnogues March 13, 2012, 4:20 p.m. UTC | #1
On Tue, Mar 13, 2012 at 3:45 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Decode the SETEND instruction correctly in Thumb mode,
> rather than accidentally treating it like CPS. We don't
> support BE8 mode, but this change brings the Thumb mode
> in to line with behaviour in ARM mode: 'SETEND BE' is
> not supported and will provoke an UNDEF exception, but
> 'SETEND LE' is correctly handled as a no-op.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Daniel Forsgren <daniel.forsgren@enea.com>

The patch looks good to me;  I guess this would qualify for a:

Reviewed-by: Laurent Desnogues <laurent.desnogues@gmail.com>

The only point I dislike isn't directly related to this patch:
the use of illegal_op that behaves exactly as UNDEF
looks odd.


Laurent

> ---
> This is one of those patches where I wasn't sure whether to try
> to split it into a whitespace-only part and a significant-change
> part. Most of this is (a) indenting existing code another notch
> for the extra switch statement and (b) adding braces to placate
> checkpatch.
>
>  target-arm/translate.c |   63 ++++++++++++++++++++++++++++++-----------------
>  1 files changed, 40 insertions(+), 23 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 280bfca..3196619 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -9704,32 +9704,49 @@ static void disas_thumb_insn(CPUState *env, DisasContext *s)
>             store_reg(s, rd, tmp);
>             break;
>
> -        case 6: /* cps */
> -            ARCH(6);
> -            if (IS_USER(s))
> +        case 6:
> +            switch ((insn >> 5) & 7) {
> +            case 2:
> +                /* setend */
> +                ARCH(6);
> +                if (insn & (1 << 3)) {
> +                    /* BE8 mode not implemented.  */
> +                    goto illegal_op;
> +                }
>                 break;
> -            if (IS_M(env)) {
> -                tmp = tcg_const_i32((insn & (1 << 4)) != 0);
> -                /* FAULTMASK */
> -                if (insn & 1) {
> -                    addr = tcg_const_i32(19);
> -                    gen_helper_v7m_msr(cpu_env, addr, tmp);
> -                    tcg_temp_free_i32(addr);
> +            case 3:
> +                /* cps */
> +                ARCH(6);
> +                if (IS_USER(s)) {
> +                    break;
>                 }
> -                /* PRIMASK */
> -                if (insn & 2) {
> -                    addr = tcg_const_i32(16);
> -                    gen_helper_v7m_msr(cpu_env, addr, tmp);
> -                    tcg_temp_free_i32(addr);
> +                if (IS_M(env)) {
> +                    tmp = tcg_const_i32((insn & (1 << 4)) != 0);
> +                    /* FAULTMASK */
> +                    if (insn & 1) {
> +                        addr = tcg_const_i32(19);
> +                        gen_helper_v7m_msr(cpu_env, addr, tmp);
> +                        tcg_temp_free_i32(addr);
> +                    }
> +                    /* PRIMASK */
> +                    if (insn & 2) {
> +                        addr = tcg_const_i32(16);
> +                        gen_helper_v7m_msr(cpu_env, addr, tmp);
> +                        tcg_temp_free_i32(addr);
> +                    }
> +                    tcg_temp_free_i32(tmp);
> +                    gen_lookup_tb(s);
> +                } else {
> +                    if (insn & (1 << 4)) {
> +                        shift = CPSR_A | CPSR_I | CPSR_F;
> +                    } else {
> +                        shift = 0;
> +                    }
> +                    gen_set_psr_im(s, ((insn & 7) << 6), 0, shift);
>                 }
> -                tcg_temp_free_i32(tmp);
> -                gen_lookup_tb(s);
> -            } else {
> -                if (insn & (1 << 4))
> -                    shift = CPSR_A | CPSR_I | CPSR_F;
> -                else
> -                    shift = 0;
> -                gen_set_psr_im(s, ((insn & 7) << 6), 0, shift);
> +                break;
> +            default:
> +                goto undef;
>             }
>             break;
>
> --
> 1.7.1
>
>
diff mbox

Patch

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 280bfca..3196619 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9704,32 +9704,49 @@  static void disas_thumb_insn(CPUState *env, DisasContext *s)
             store_reg(s, rd, tmp);
             break;
 
-        case 6: /* cps */
-            ARCH(6);
-            if (IS_USER(s))
+        case 6:
+            switch ((insn >> 5) & 7) {
+            case 2:
+                /* setend */
+                ARCH(6);
+                if (insn & (1 << 3)) {
+                    /* BE8 mode not implemented.  */
+                    goto illegal_op;
+                }
                 break;
-            if (IS_M(env)) {
-                tmp = tcg_const_i32((insn & (1 << 4)) != 0);
-                /* FAULTMASK */
-                if (insn & 1) {
-                    addr = tcg_const_i32(19);
-                    gen_helper_v7m_msr(cpu_env, addr, tmp);
-                    tcg_temp_free_i32(addr);
+            case 3:
+                /* cps */
+                ARCH(6);
+                if (IS_USER(s)) {
+                    break;
                 }
-                /* PRIMASK */
-                if (insn & 2) {
-                    addr = tcg_const_i32(16);
-                    gen_helper_v7m_msr(cpu_env, addr, tmp);
-                    tcg_temp_free_i32(addr);
+                if (IS_M(env)) {
+                    tmp = tcg_const_i32((insn & (1 << 4)) != 0);
+                    /* FAULTMASK */
+                    if (insn & 1) {
+                        addr = tcg_const_i32(19);
+                        gen_helper_v7m_msr(cpu_env, addr, tmp);
+                        tcg_temp_free_i32(addr);
+                    }
+                    /* PRIMASK */
+                    if (insn & 2) {
+                        addr = tcg_const_i32(16);
+                        gen_helper_v7m_msr(cpu_env, addr, tmp);
+                        tcg_temp_free_i32(addr);
+                    }
+                    tcg_temp_free_i32(tmp);
+                    gen_lookup_tb(s);
+                } else {
+                    if (insn & (1 << 4)) {
+                        shift = CPSR_A | CPSR_I | CPSR_F;
+                    } else {
+                        shift = 0;
+                    }
+                    gen_set_psr_im(s, ((insn & 7) << 6), 0, shift);
                 }
-                tcg_temp_free_i32(tmp);
-                gen_lookup_tb(s);
-            } else {
-                if (insn & (1 << 4))
-                    shift = CPSR_A | CPSR_I | CPSR_F;
-                else
-                    shift = 0;
-                gen_set_psr_im(s, ((insn & 7) << 6), 0, shift);
+                break;
+            default:
+                goto undef;
             }
             break;