[2/3] target-arm: implement LDA/STL instructions

This adds support for the ARMv8 load acquire/store release instructions.
Since qemu does nothing special for memory barriers, these can be
emulated like their non-acquire/release counterparts.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 target-arm/translate.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 85 insertions(+), 6 deletions(-)

On 7 June 2013 13:06, Mans Rullgard <mans@mansr.com> wrote:
> This adds support for the ARMv8 load acquire/store release instructions.
> Since qemu does nothing special for memory barriers, these can be
> emulated like their non-acquire/release counterparts.

A brief comment to this effect in the appropriate parts of the
code as well as the commit message would be good.

>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  target-arm/translate.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 96ac5bc..f529257 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7274,14 +7274,54 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
>                      rd = (insn >> 12) & 0xf;
>                      if (insn & (1 << 23)) {
>                          /* load/store exclusive */
> +                        int excl = (insn >> 9) & 1;
>                          op1 = (insn >> 21) & 0x3;
> -                        if (op1)
> +                        if (!excl)
> +                            ARCH(8);
> +                        else if (op1)
>                              ARCH(6K);
>                          else
>                              ARCH(6);

This if statement needs braces on all its arms. (QEMU coding
style mandates braces, though some existing code is not in
line with this; our general policy is that where patches touch
such existing code they update it accordingly.)

You can use scripts/checkpatch.pl to check for this.

This is also continuing to underdecode this space: some
of the new exclusive insns are ARCH(8) only but will be
allowed on ARCH(6) or 6K by this check. I know we've historically
underdecoded, but if we're touching this bit of the decoder
I'd prefer it if we tightened it up in the process.

>                          addr = tcg_temp_local_new_i32();
>                          load_reg_var(s, addr, rn);
> -                        if (insn & (1 << 20)) {
> +                        if (!excl) {
> +                            if (op1 == 1)
> +                                goto illegal_op;

This check needs to happen earlier to avoid leaking a tcg temp.

> +                            tmp = tcg_temp_new_i32();
> +                            if (insn & (1 << 20)) {
> +                                switch (op1) {
> +                                case 0: /* lda */
> +                                    tcg_gen_qemu_ld32u(tmp, addr, IS_USER(s));
> +                                    break;
> +                                case 2: /* ldab */
> +                                    tcg_gen_qemu_ld8u(tmp, addr, IS_USER(s));
> +                                    break;
> +                                case 3: /* ldah */
> +                                    tcg_gen_qemu_ld16u(tmp, addr, IS_USER(s));
> +                                    break;
> +                                default:
> +                                    abort();
> +                                }
> +                                store_reg(s, rd, tmp);
> +                            } else {
> +                                rm = insn & 0xf;
> +                                tmp = load_reg(s, rm);
> +                                switch (op1) {
> +                                case 0: /* stl */
> +                                    tcg_gen_qemu_st32(tmp, addr, IS_USER(s));
> +                                    break;
> +                                case 2: /* stlb */
> +                                    tcg_gen_qemu_st8(tmp, addr, IS_USER(s));
> +                                    break;
> +                                case 3: /* stlh */
> +                                    tcg_gen_qemu_st16(tmp, addr, IS_USER(s));
> +                                    break;
> +                                default:
> +                                    abort();
> +                                }
> +                                tcg_temp_free_i32(tmp);
> +                            }
> +                        } else if (insn & (1 << 20)) {
>                              switch (op1) {
>                              case 0: /* ldrex */
>                                  gen_load_exclusive(s, rd, 15, addr, 2);
> @@ -8126,7 +8166,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                      gen_store_exclusive(s, rd, rs, 15, addr, 2);
>                  }
>                  tcg_temp_free_i32(addr);
> -            } else if ((insn & (1 << 6)) == 0) {
> +            } else if ((insn & (3 << 6)) == 0) {

While we're fiddling with the decode here let's actually narrow
it down completely, by making this condition
   ((insn & (7 << 5)) == 0)

...

>                  /* Table Branch.  */
>                  if (rn == 15) {
>                      addr = tcg_temp_new_i32();
> @@ -8153,14 +8193,53 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
>                  store_reg(s, 15, tmp);
>              } else {
>                  /* Load/store exclusive byte/halfword/doubleword.  */
> -                ARCH(7);
> +                if (((insn >> 6) & 3) != 1)
> +                    ARCH(8);
> +                else
> +                    ARCH(7);

...and then here do something like:
    switch ((insn >> 6) & 3) {
        case 0:
            goto illegal_op;
        case 1:
            /* Load/store exclusive byte/halfword/doubleword */
            ARCH(7);
            break;
        case 2:
        case 3:
            /* Load-acquire/store-release */
            ARCH(8);
            break;
    }

>                  op = (insn >> 4) & 0x3;
> -                if (op == 2) {
> +                if (((insn >> 7) & 1) == 0 && op == 2) {
>                      goto illegal_op;
>                  }

It might be better to just pull out the op3 field
and then do tests on it rather than combinations of
testing op (op3's lower 2 bits) and bits directly from
insn.

>                  addr = tcg_temp_local_new_i32();
>                  load_reg_var(s, addr, rn);
> -                if (insn & (1 << 20)) {
> +                if ((insn & (1 << 6)) == 0) {
> +                    if (op == 3)
> +                        goto illegal_op;

This check needs to go earlier, otherwise we leak a tcg temp
if we take the illegal_op path. You might be able to roll all
the illegal-op checks and ARCH checks into a single switch on
the whole op3 field.

> +                    tmp = tcg_temp_new_i32();
> +                    if (insn & (1 << 20)) {
> +                        switch (op) {
> +                        case 0: /* ldab */
> +                            tcg_gen_qemu_ld8u(tmp, addr, IS_USER(s));
> +                            break;
> +                        case 1: /* ldah */
> +                            tcg_gen_qemu_ld16u(tmp, addr, IS_USER(s));
> +                            break;
> +                        case 2: /* lda */
> +                            tcg_gen_qemu_ld32u(tmp, addr, IS_USER(s));
> +                            break;
> +                        default:
> +                            abort();
> +                        }
> +                        store_reg(s, rs, tmp);
> +                    } else {
> +                        tmp = load_reg(s, rs);
> +                        switch (op) {
> +                        case 0: /* stlb */
> +                            tcg_gen_qemu_st8(tmp, addr, IS_USER(s));
> +                            break;
> +                        case 1: /* stlh */
> +                            tcg_gen_qemu_st16(tmp, addr, IS_USER(s));
> +                            break;
> +                        case 2: /* stl */
> +                            tcg_gen_qemu_st32(tmp, addr, IS_USER(s));
> +                            break;
> +                        default:
> +                            abort();
> +                        }
> +                        tcg_temp_free_i32(tmp);
> +                    }
> +                } else if (insn & (1 << 20)) {
>                      gen_load_exclusive(s, rs, rd, addr, op);
>                  } else {
>                      gen_store_exclusive(s, rm, rs, rd, addr, op);

Have you tested these with risu yet?

thanks
-- PMM