Re: [Qemu-devel] [PATCH 37/52] target-m68k: add cas/cas2 ops

[Richard Henderson]

On 05/04/2016 11:08 AM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  linux-user/main.c       | 193 ++++++++++++++++++++++++++++++++++++++++++++++++
>  target-m68k/cpu.h       |   9 +++
>  target-m68k/qregs.def   |   5 ++
>  target-m68k/translate.c | 175 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 382 insertions(+)
>
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 74b02c7..3c51afe 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -2994,6 +2994,194 @@ void cpu_loop(CPUMBState *env)
>
>  #ifdef TARGET_M68K
>
> +static int do_cas(CPUM68KState *env)
> +{
> +    int size, is_cas;
> +    int cmp1_reg, upd1_reg;
> +    int cmp2_reg, upd2_reg;
> +    uint32_t dest1, cmp1, addr1;
> +    uint32_t dest2, cmp2, addr2;
> +    int segv = 0;
> +    int z;
> +
> +    start_exclusive();
> +
> +    /* cas_param bits
> +     * 31    -> CAS(0) / CAS2(1)
> +     * 11:13 -> update reg 2
> +     * 8:10  -> cmp reg 2
> +     * 5:7   -> update reg 1
> +     * 2:4   -> cmp reg 1
> +     * 0:1   -> opsize
> +     */
> +
> +    is_cas = (env->cas_param & 0x80000000) == 0;
> +
> +    size = env->cas_param & 0x3;
> +
> +    cmp1_reg = (env->cas_param >> 2) & 7;
> +    upd1_reg = (env->cas_param >> 5) & 7;
> +    cmp2_reg = (env->cas_param >> 8) & 7;
> +    upd2_reg = (env->cas_param >> 11) & 7;
> +
> +    addr1 = env->cas_addr1;
> +    addr2 = env->cas_addr2;
> +
> +    switch (size) {
> +    case OS_BYTE:
> +        segv = get_user_u8(dest1, addr1);
> +        cmp1 = (uint8_t)env->dregs[cmp1_reg];
> +        break;
> +    case OS_WORD:
> +        segv = get_user_u16(dest1, addr1);
> +        cmp1 = (uint16_t)env->dregs[cmp1_reg];
> +        break;
> +    case OS_LONG:
> +    default:
> +        segv = get_user_u32(dest1, addr1);
> +        cmp1 = env->dregs[cmp1_reg];
> +        break;
> +    }
> +    if (segv) {
> +        env->mmu.ar = addr1;
> +        goto done;
> +    }
> +    env->cc_n = dest1;
> +    env->cc_v = cmp1;
> +    z = dest1 - cmp1;
> +    env->cc_op = CC_OP_CMPB + size;

Incorrect setting of CC_* before checking for all of the possible segv?

> +    dest = gen_load(s, opsize, addr, 0);
> +
> +    zero = tcg_const_i32(0);
> +    cmp = gen_extend(DREG(ext, 0), opsize, 0);
> +
> +    /* if  dest - cmp == 0 */
> +
> +    res = tcg_temp_new();
> +    tcg_gen_sub_i32(res, dest, cmp);
> +
> +    /* then dest = update1 */
> +
> +    tcg_gen_movcond_i32(TCG_COND_EQ, dest,
> +                        res, zero,
> +                        DREG(ext, 6), dest);
> +
> +    /* else cmp = dest */
> +
> +    tcg_gen_movcond_i32(TCG_COND_NE, cmp,
> +                        res, zero,
> +                        dest, cmp);

Note that you don't need movcond for cmp here.  If the condition is false, we 
know that cmp == dest anyway.  So just a plain store into cmp.

You also don't need to compute RES, since you can perform the first movcond 
based on dest and cmp directly.  Although this does require an extra temp:

  load = gen_load(...)
  cmp = gen_extend(...)
  store = tcg_temp_new();

  tcg_gen_movcond_i32(TCG_COND_EQ, store, load, cmp, DREG(ext, 6), load);
  tcg_gen_mov_i32(cmp, load);

  gen_partset_reg(..., cmp);
  gen_store(..., store);

> +    gen_logic_cc(s, res, opsize);

gen_logic_cc is wrong, afaics, and clobbers the proper CMPB + opsize that you 
set up earlier.

> +    TCGv cmp1, cmp2;
> +    TCGv dest1, dest2;
> +    TCGv res1, res2;
> +    TCGv zero;
> +    zero = tcg_const_i32(0);
> +    dest1 = gen_load(s, opsize, addr1, 0);
> +    cmp1 = gen_extend(DREG(ext1, 0), opsize, 0);
> +
> +    res1 = tcg_temp_new();
> +    tcg_gen_sub_i32(res1, dest1, cmp1);
> +    dest2 = gen_load(s, opsize, addr2, 0);
> +    cmp2 = gen_extend(DREG(ext2, 0), opsize, 0);
> +
> +    res2 = tcg_temp_new();
> +    tcg_gen_sub_i32(res2, dest2, cmp2);
> +
> +    /* if dest1 - cmp1 == 0 and dest2 - cmp2 == 0 */
> +
> +    tcg_gen_movcond_i32(TCG_COND_EQ, res1,
> +                        res1, zero,
> +                        res2, res1);

  z = (cmp1 - load1) | (cmp2 - load2)

would be a better computation here than sub+sub+movcond.

Of course, we also need to correctly compute N, C and V, so...

  N = (cmp1 == load1 ? cmp2 : cmp1);
  V = (cmp1 == load1 ? load2 : load1);
  cc_op = CC_OP_CMP;

  store1 = (N == V ? update1 : load1);
  store2 = (N == V ? update2 : load2);

  cmp1 = load1;
  cmp2 = load2;

Oh, and we need to think about delaying all flag and register updates until 
after the stores, for both CAS and CAS2.  Otherwise we don't get the right 
results at the exception handler for a write-protected page.


r~