Re: [Qemu-devel] [PATCH 1/3] arm: basic support for ARMv4/ARMv4T emulation

[Peter Maydell]

On 29 March 2011 17:58, Dmitry Eremin-Solenikov <address@hidden> wrote:

Looks good, nearly there I think.

> @@ -7172,10 +7191,11 @@ static void disas_arm_insn(CPUState * env, 
> DisasContext *s)
>             }
>             if (insn & (1 << 20)) {
>                 /* Complete the load.  */
> -                if (rd == 15)
> +                if (rd == 15 && ENABLE_ARCH_4T) {
>                     gen_bx(s, tmp);
> -                else
> +                } else {
>                     store_reg(s, rd, tmp);
> +                }
>             }
>             break;
>         case 0x08:

Shouldn't this be ENABLE_ARCH_5T ? Loads to PC are only interworking
in v5T and above.
(But see below...)

> @@ -7229,7 +7249,11 @@ static void disas_arm_insn(CPUState * env, 
> DisasContext *s)
>                             /* load */
>                             tmp = gen_ld32(addr, IS_USER(s));
>                             if (i == 15) {
> -                                gen_bx(s, tmp);
> +                                if (ENABLE_ARCH_5) {
> +                                    gen_bx(s, tmp);
> +                                } else {
> +                                    store_reg(s, i, tmp);
> +                                }
>                             } else if (user) {
>                                 tmp2 = tcg_const_i32(i);
>                                 gen_helper_set_user_reg(tmp2, tmp);


> @@ -8980,8 +9006,13 @@ static void disas_thumb_insn(CPUState *env, 
> DisasContext *s)
>             /* write back the new stack pointer */
>             store_reg(s, 13, addr);
>             /* set the new PC value */
> -            if ((insn & 0x0900) == 0x0900)
> -                gen_bx(s, tmp);
> +            if ((insn & 0x0900) == 0x0900) {
> +                if (ENABLE_ARCH_5) {
> +                    gen_bx(s, tmp);
> +                } else {
> +                    store_reg(s, 15, tmp);
> +                }
> +            }
>             break;
>
>         case 1: case 3: case 9: case 11: /* czb */

These two are right, but I think we should have a utility function
(put it next to store_reg_bx()):

/* Variant of store_reg which uses branch&exchange logic when storing
 * to r15 in ARM architecture v5T and above. This is used for storing
 * the results of a LDR/LDM/POP into r15, and corresponds to the cases
 * in the ARM ARM which use the LoadWritePC() pseudocode function.
 */
static inline void store_reg_from_load(CPUState *env, DisasContext *s,
                                       int reg, TCGv var)
{
    if (reg == 15 && ENABLE_ARCH_5TE) {
        gen_bx(s, var);
    } else {
        store_reg(s, reg, var);
    }
}

Then you can use this in the three code hunks above. (You'll want
to tweak the middle one, you can move it to
  if (user) {
    ...
  } else if (i == rn) {
    ...
  } else {
    store_reg_from_load(env, s, i, tmp);
  }

because if i==15 then user must be false, and if rn == 15 this
is UNPREDICTABLE anyway.)

These comments from last round still hold for this patch:

The CPSR Q bit needs to RAZ/WI on v4 and v4T.

For v4 you need to make sure that the core can't get into
thumb mode at all. So feature guards in gen_bx_imm() and
gen_bx(), make sure PSR masks prevent the T bit getting set,
and check helper.c for anything that sets env->thumb from
somewhere else...

-- PMM