Re: [Qemu-devel] [PATCH] target-arm: Decode SETEND correctly in Thumb

[Laurent Desnogues]

On Tue, Mar 13, 2012 at 3:45 PM, Peter Maydell <address@hidden> 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 <address@hidden>
> Reported-by: Daniel Forsgren <address@hidden>

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

Reviewed-by: Laurent Desnogues <address@hidden>

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