Re: [Qemu-devel] [PATCH 8/9] target/arm: Implement CAS and CASP

[Peter Maydell]

On 27 April 2018 at 01:26, Richard Henderson
<address@hidden> wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target/arm/helper-a64.h    |   2 +
>  target/arm/helper-a64.c    |  43 ++++++++++++++++
>  target/arm/translate-a64.c | 119 
> +++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 161 insertions(+), 3 deletions(-)

> +static void gen_compare_and_swap_pair(DisasContext *s, int rs, int rt,
> +                                      int rn, int size)
> +{
> +    TCGv_i64 s1 = cpu_reg(s, rs);
> +    TCGv_i64 s2 = cpu_reg(s, rs + 1);
> +    TCGv_i64 t1 = cpu_reg(s, rt);
> +    TCGv_i64 t2 = cpu_reg(s, rt + 1);
> +    TCGv_i64 addr = cpu_reg_sp(s, rn);
> +    int memidx = get_mem_index(s);
> +
> +    if (rn == 31) {
> +        gen_check_sp_alignment(s);
> +    }
> +
> +    if (size == 2) {
> +        TCGv_i64 cmp = tcg_temp_new_i64();
> +        TCGv_i64 val = tcg_temp_new_i64();
> +
> +        if (s->be_data == MO_LE) {
> +            tcg_gen_concat32_i64(val, t1, t2);
> +            tcg_gen_concat32_i64(cmp, s1, s2);
> +        } else {
> +            tcg_gen_concat32_i64(val, t2, t1);
> +            tcg_gen_concat32_i64(cmp, s2, s1);
> +        }
> +
> +        tcg_gen_atomic_cmpxchg_i64(cmp, addr, cmp, val, memidx,
> +                                   MO_64 | MO_ALIGN | s->be_data);
> +        tcg_temp_free_i64(val);
> +
> +        if (s->be_data == MO_LE) {
> +            tcg_gen_extr32_i64(s1, s2, cmp);
> +        } else {
> +            tcg_gen_extr32_i64(s2, s1, cmp);
> +        }
> +        tcg_temp_free_i64(cmp);
> +    } else if (tb_cflags(s->base.tb) & CF_PARALLEL) {
> +        TCGv_i32 tcg_rs = tcg_const_i32(rs);
> +
> +        if (s->be_data == MO_LE) {
> +            gen_helper_casp_le_parallel(cpu_env, tcg_rs, addr, t1, t2);
> +        } else {
> +            gen_helper_casp_be_parallel(cpu_env, tcg_rs, addr, t1, t2);
> +        }
> +        tcg_temp_free_i32(tcg_rs);
> +    } else {
> +        TCGv_i64 d1 = tcg_temp_new_i64();
> +        TCGv_i64 d2 = tcg_temp_new_i64();
> +        TCGv_i64 a2 = tcg_temp_new_i64();
> +        TCGv_i64 c1 = tcg_temp_new_i64();
> +        TCGv_i64 c2 = tcg_temp_new_i64();
> +        TCGv_i64 zero = tcg_const_i64(0);
> +
> +        /* Load the two words, in memory order.  */
> +        tcg_gen_qemu_ld_i64(d1, addr, memidx,
> +                            MO_64 | MO_ALIGN_16 | s->be_data);
> +        tcg_gen_addi_i64(a2, addr, 8);
> +        tcg_gen_qemu_ld_i64(d2, addr, memidx, MO_64 | s->be_data);
> +
> +        /* Compare the two words, also in memory order.  */
> +        tcg_gen_setcond_i64(TCG_COND_EQ, c1, d1, s1);
> +        tcg_gen_setcond_i64(TCG_COND_EQ, c2, d2, s2);
> +        tcg_gen_and_i64(c2, c2, c1);
> +
> +        /* If compare equal, write back new data, else write back old data.  
> */
> +        tcg_gen_movcond_i64(TCG_COND_NE, c1, c2, zero, t1, d1);
> +        tcg_gen_movcond_i64(TCG_COND_NE, c2, c2, zero, t2, d2);
> +        tcg_gen_qemu_st_i64(c1, addr, memidx, MO_64 | s->be_data);
> +        tcg_gen_qemu_st_i64(c2, a2, memidx, MO_64 | s->be_data);

I think this has the wrong behaviour if you do a CASP-with-mismatched-value
to read-only memory -- architecturally this should fail the comparison
and return the memory value in registers, it's not allowed to do a
memory write and take a data abort because the memory isn't writable.

> +        tcg_temp_free_i64(a2);
> +        tcg_temp_free_i64(c1);
> +        tcg_temp_free_i64(c2);
> +        tcg_temp_free_i64(zero);
> +
> +        /* Write back the data from memory to Rs.  */
> +        tcg_gen_mov_i64(s1, d1);
> +        tcg_gen_mov_i64(s2, d2);
> +        tcg_temp_free_i64(d1);
> +        tcg_temp_free_i64(d2);
> +    }
> +}
> +

thanks
-- PMM