Re: [PATCH v1 4/5] target/microblaze: swx: Use atomic_cmpxchg

[Richard Henderson]

On 8/17/20 7:01 AM, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> 
> Use atomic_cmpxchg to implement the atomic cmpxchg sequence.
> 
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target/microblaze/translate.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
> index c58f334a0f..530c15e5ad 100644
> --- a/target/microblaze/translate.c
> +++ b/target/microblaze/translate.c
> @@ -1075,14 +1075,16 @@ static void dec_store(DisasContext *dc)
>          swx_skip = gen_new_label();
>          tcg_gen_brcond_tl(TCG_COND_NE, env_res_addr, addr, swx_skip);
>  
> -        /* Compare the value loaded at lwx with current contents of
> -           the reserved location.
> -           FIXME: This only works for system emulation where we can expect
> -           this compare and the following write to be atomic. For user
> -           emulation we need to add atomicity between threads.  */
> +        /*
> +         * Compare the value loaded at lwx with current contents of
> +         * the reserved location.
> +         */
>          tval = tcg_temp_new_i32();
> -        tcg_gen_qemu_ld_i32(tval, addr, cpu_mmu_index(&dc->cpu->env, false),
> -                            MO_TEUL);
> +
> +        tcg_gen_atomic_cmpxchg_i32(tval, addr, env_res_val,
> +                                   cpu_R[dc->rd], mem_index,
> +                                   mop);
> +
>          tcg_gen_brcond_i32(TCG_COND_NE, env_res_val, tval, swx_skip);
>          write_carryi(dc, 0);
>          tcg_temp_free_i32(tval);
> @@ -1108,7 +1110,10 @@ static void dec_store(DisasContext *dc)
>                  break;
>          }
>      }
> -    tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> +
> +    if (!ex) {
> +        tcg_gen_qemu_st_i32(cpu_R[dc->rd], addr, mem_index, mop);
> +    }
>  
>      /* Verify alignment if needed.  */
>      if (dc->cpu->cfg.unaligned_exceptions && size > 1) {
> 

This is fine as far as the actual swx instruction goes, so
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

However, some notes for dec_store,

There seems to be a large under-decode here.  E.g. rev should never be set for
swx.  But rev is accepted and partially implemented.  E.g. there is no sbx
instruction, but the ex bit is accepted for any store instruction.

Microblaze has a relatively small instruction set.  Would you be open to a
conversion to decodetree?  It should be fairly easy.


r~