Re: [Qemu-devel] [PATCH v3 23/40] target/mips: Implement emulation of nanoMIPS LLWP/SCWP pair

[Richard Henderson]

On 07/27/2018 08:29 AM, Aleksandar Markovic wrote:
> static void gen_llwp(DisasContext *ctx, uint32_t base, int16_t offset,
>                      uint32_t reg1, uint32_t reg2)
> {
>     TCGv taddr = tcg_temp_new();
>     TCGv_i64 tval = tcg_temp_new_i64();
>     TCGv tmp1 = tcg_temp_new();
>     TCGv tmp2 = tcg_temp_new();
>     TCGv tmp3 = tcg_temp_new();
>     TCGLabel *l1 = gen_new_label();
> 
>     gen_base_offset_addr(ctx, taddr, base, offset);
>     tcg_gen_andi_tl(tmp3, taddr, 0x7);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, tmp3, 0, l1);
>     tcg_temp_free(tmp3);
>     tcg_gen_st_tl(taddr, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));
>     generate_exception(ctx, EXCP_AdES);
>     gen_set_label(l1);

You shouldn't need this, as it is implied by

cpu.h:#define ALIGNED_ONLY

and will, for softmmu anyway, fault

>     tcg_gen_qemu_ld64(tval, taddr, ctx->mem_idx);

here.

(If you are testing -linux-user, there are many other missing
alignment faults and I suggest you ignore the issue entirely; it needs to be
dealt with generically.)

>     tcg_gen_extr_i64_tl(tmp1, tmp2, tval);
>     gen_store_gpr(tmp1, reg1);
>     tcg_temp_free(tmp1);
>     gen_store_gpr(tmp2, reg2);
>     tcg_temp_free(tmp2);

Has the swap of register numbers for big vs little-endian happened in the
caller?  You didn't show enough context to tell.



> static void gen_scwp(DisasContext *ctx, uint32_t base, int16_t offset,
>                      uint32_t reg1, uint32_t reg2)
> {
>     TCGv taddr = tcg_temp_new();
>     TCGv lladdr = tcg_temp_new();
>     TCGv_i64 tval = tcg_temp_new_i64();
>     TCGv_i64 llval = tcg_temp_new_i64();
>     TCGv_i64 val = tcg_temp_new_i64();
>     TCGv tmp1 = tcg_temp_new();
>     TCGv tmp2 = tcg_temp_new();
>     TCGLabel *l1 = gen_new_label();
>     TCGLabel *lab_fail = gen_new_label();
>     TCGLabel *lab_done = gen_new_label();
>  
>     gen_base_offset_addr(ctx, taddr, base, offset);
>     tcg_gen_andi_tl(tmp1, taddr, 0x7);
>     tcg_gen_brcondi_tl(TCG_COND_EQ, tmp1, 0, l1);
>     tcg_gen_st_tl(taddr, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));
>     generate_exception(ctx, EXCP_AdES);
> 
>     gen_set_label(l1);

Hmm.  You could perhaps move the alignment test to the lab_fail path, because

>     tcg_gen_ld_tl(lladdr, cpu_env, offsetof(CPUMIPSState, lladdr));
>     tcg_gen_brcond_tl(TCG_COND_NE, taddr, lladdr, lab_fail);

if we pass this test we know that taddr == lladdr, and that lladdr is aligned
because the load for LLWP did not fault.  Even if that were not the case, the
cmpxchg itself would trigger an alignment fault.

>     gen_load_gpr(tmp1, reg1);
>     gen_load_gpr(tmp2, reg2);
>     tcg_gen_concat_tl_i64(tval, tmp1, tmp2);

Again, did a the reg1/reg2 swap happen in the caller?

>     tcg_gen_ld_i64(llval, cpu_env, offsetof(CPUMIPSState, llval_wp));
>     tcg_gen_atomic_cmpxchg_i64(val, taddr, llval, tval,
>                                ctx->mem_idx, MO_64);
>     tcg_gen_setcond_i64(TCG_COND_EQ, val, val, llval);
>     tcg_gen_br(lab_done);

You failed to write back "val" to GPR[rt].

>  
>     gen_set_label(lab_fail);
>     tcg_gen_movi_tl(cpu_gpr[reg2], 0);
>  
>     gen_set_label(lab_done);
>     tcg_gen_movi_tl(lladdr, -1);
>     tcg_gen_st_tl(lladdr, cpu_env, offsetof(CPUMIPSState, lladdr));
> }
> 


r~