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

["Ashish Bijlani" <ashish.bijlani@gmail.com>]

Hello,

Emulation of SRS/RFE instruction is indeed incorrect. I faced the same
issues running okl4 on omap2420 (arm1136jfs) based. I could fix the
problem by applying the patch submitted by Hyeonsung. Did anybody else
face this problem?

Thanks,
Ashish

On Thu, Aug 7, 2008 at 6:34 PM, Hyeonseung Jang <hsjang@ok-labs.com> wrote:
> When can I expect this patch to be applied to the mainline ?
> This patch is quite clear but there is no feedback at all.
> The maintainer may be busy but I just want him or her not to forget this.
> (I have an experience where the submitted
> patch(http://lists.gnu.org/archive/html/qemu-devel/2008-02/msg00351.html)
> was totally ignored for 6 months and then applied recently after the other
> person submitted the same thing)
>
>
> On 22/07/2008, at 5:12 PM, Hans(Hyeonseung) Jang wrote:
>
>> (Sorry for sending again because of white space problem in the previous
>> mail)
>>
>> 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;
>>
>
>
>
>