Re: [Qemu-devel] ARMv6: Fix SRS/RFE instruction

[Filip Navara]

On Tue, Jul 22, 2008 at 4:20 AM, Hans Jang<address@hidden> wrote:
> We are in the process of implementing a KZM (i.MX31) ARMv6 machine port in
> order to run the OKL4 kernel. We found the new CPS/SRS/RFE instructions were
> broken.
> Vincent Palatin released a patch recently which fixes the CPS problem.
> Attached is a patch to fix the SRS/RFE bugs. Could this patch please be
> applied to the main trunk.
> Thanks
> - Hyeonsung Jang.
>
>
> - The encoding of 'IA' condition must be '01' instead of '02'.
> - SRS instruction must store banked SPSR instead of CPSR at the
> specific address.
> - 'return' statements are missing
>
>
> Index: target-arm/translate.c
> ===================================================================
> --- target-arm/translate.c      (revision 4921)
> +++ target-arm/translate.c      (working copy)
> @@ -5702,7 +5702,7 @@
>            }
>        } else if ((insn & 0x0e5fffe0) == 0x084d0500) {
>            /* srs */
> -            uint32_t offset;
> +            int32_t offset;
>            if (IS_USER(s))
>                goto illegal_op;
>            ARCH(6);
> @@ -5716,8 +5716,8 @@
>            i = (insn >> 23) & 3;
>            switch (i) {
>            case 0: offset = -4; break; /* DA */
> -            case 1: offset = -8; break; /* DB */
> -            case 2: offset = 0; break; /* IA */
> +            case 1: offset = 0; break; /* IA */
> +            case 2: offset = -8; break; /* DB */
>            case 3: offset = 4; break; /* IB */
>            default: abort();
>            }
> @@ -5725,32 +5725,33 @@
>                tcg_gen_addi_i32(addr, addr, offset);
>            tmp = load_reg(s, 14);
>            gen_st32(tmp, addr, 0);
> -            tmp = new_tmp();
> -            gen_helper_cpsr_read(tmp);
> +            tmp = load_cpu_field(spsr);
>            tcg_gen_addi_i32(addr, addr, 4);
>            gen_st32(tmp, addr, 0);
>            if (insn & (1 << 21)) {
>                /* Base writeback.  */
>                switch (i) {
>                case 0: offset = -8; break;
> -                case 1: offset = -4; break;
> -                case 2: offset = 4; break;
> +                case 1: offset = 4; break;
> +                case 2: offset = -4; break;
>                case 3: offset = 0; break;
>                default: abort();
>                }
>                if (offset)
> -                    tcg_gen_addi_i32(addr, tmp, offset);
> +                    tcg_gen_addi_i32(addr, addr, offset);
>                if (op1 == (env->uncached_cpsr & CPSR_M)) {
> -                    gen_movl_reg_T1(s, 13);
> +                    store_reg(s, 13, addr);
>                } else {
> -                    gen_helper_set_r13_banked(cpu_env, tcg_const_i32(op1),
> cpu_T[1]);
> +                    gen_helper_set_r13_banked(cpu_env, tcg_const_i32(op1),
> addr);
> +                    dead_tmp(addr);
>                }
>            } else {
>                dead_tmp(addr);
>            }
> +            return;
>        } else if ((insn & 0x0e5fffe0) == 0x081d0a00) {
>            /* rfe */
> -            uint32_t offset;
> +            int32_t offset;
>            if (IS_USER(s))
>                goto illegal_op;
>            ARCH(6);
> @@ -5759,8 +5760,8 @@
>            i = (insn >> 23) & 3;
>            switch (i) {
>            case 0: offset = -4; break; /* DA */
> -            case 1: offset = -8; break; /* DB */
> -            case 2: offset = 0; break; /* IA */
> +            case 1: offset = 0; break; /* IA */
> +            case 2: offset = -8; break; /* DB */
>            case 3: offset = 4; break; /* IB */
>            default: abort();
>            }
> @@ -5774,8 +5775,8 @@
>                /* Base writeback.  */
>                switch (i) {
>                case 0: offset = -8; break;
> -                case 1: offset = -4; break;
> -                case 2: offset = 4; break;
> +                case 1: offset = 4; break;
> +                case 2: offset = -4; break;
>                case 3: offset = 0; break;
>                default: abort();
>                }
> @@ -5786,6 +5787,7 @@
>                dead_tmp(addr);
>            }
>            gen_rfe(s, tmp, tmp2);
> +            return;
>        } else if ((insn & 0x0e000000) == 0x0a000000) {
>            /* branch link and change to thumb (blx <offset>) */
>            int32_t offset;
>
>
>
>

Can this patch be applied please? The code as it is now is wrong to
the point of being unusable, it references registers that are not
filled with any values...

Best regards,
Filip Navara