Re: [Qemu-devel] Fwd: [RFC] Adding multithreads to LDREX/STREX.

[Paolo Bonzini]

On 03/03/2015 17:47, Mark Burton wrote:
> +inline void arm_exclusive_lock(void)
> +{
> +    if (!cpu_have_exclusive_lock) {
> +        qemu_mutex_lock(&cpu_exclusive_lock);
> +        cpu_have_exclusive_lock = true;
> +    }
> +}
> +
> +inline void arm_exclusive_unlock(void)
> +{
> +    if (cpu_have_exclusive_lock) {
> +        cpu_have_exclusive_lock = false;
> +        qemu_mutex_unlock(&cpu_exclusive_lock);
> +    }
> +}
> +

This is almost but not quite a recursive mutex.  What about changing it
like this:

- arm_exclusive_lock just takes the lock and sets the flag; no "if"

- arm_exclusive_unlock does the opposite, again no "if"

- raise_exception checks the flag and skips "arm_exclusive_lock()" if
already set

The only other question I have is this:

> +    gen_helper_atomic_check(success, cpu_env, addr);
> +    tcg_gen_brcondi_i32(TCG_COND_EQ, success, 0, done_label);

Are you setting cpu_R[rd] correctly in this case?  That is, should you
be jumping to fail_label instead?  That could case a failure to be
reported as a success.

Paolo

> +    tcg_temp_free_i32(success);
> +
> +    /* Store shoudl be OK lets check the value */
> +    tmp = load_reg(s, rt);
> +    TCGv_i64 val64=tcg_temp_new_i64();
>     switch (size) {
>     case 0:
>         gen_aa32_ld8u(tmp, addr, get_mem_index(s));
> @@ -7450,21 +7447,19 @@ static void gen_store_exclusive(DisasContext *s,
> int rd, int rt, int rt2,
>         abort();
>     }
> 
> -    val64 = tcg_temp_new_i64();
>     if (size == 3) {
>         TCGv_i32 tmp2 = tcg_temp_new_i32();
>         TCGv_i32 tmp3 = tcg_temp_new_i32();
> +
>         tcg_gen_addi_i32(tmp2, addr, 4);
>         gen_aa32_ld32u(tmp3, tmp2, get_mem_index(s));
> -        tcg_temp_free_i32(tmp2);
>         tcg_gen_concat_i32_i64(val64, tmp, tmp3);
> -        tcg_temp_free_i32(tmp3);
> +        tcg_temp_free_i32(tmp2);
>     } else {
> -        tcg_gen_extu_i32_i64(val64, tmp);
> +      tcg_gen_extu_i32_i64(val64, tmp);
>     }
>     tcg_temp_free_i32(tmp);
> -
> -    tcg_gen_brcond_i64(TCG_COND_NE, val64, cpu_exclusive_val, fail_label);
> +    tcg_gen_brcond_i64(TCG_COND_NE, cpu_exclusive_val, val64, fail_label);
>     tcg_temp_free_i64(val64);
> 
>     tmp = load_reg(s, rt);
> @@ -7489,12 +7484,16 @@ static void gen_store_exclusive(DisasContext *s,
> int rd, int rt, int rt2,
>         gen_aa32_st32(tmp, addr, get_mem_index(s));
>         tcg_temp_free_i32(tmp);
>     }
> +
> +    gen_helper_atomic_release(cpu_env);
>     tcg_gen_movi_i32(cpu_R[rd], 0);
>     tcg_gen_br(done_label);
> +
>     gen_set_label(fail_label);
> +    gen_helper_atomic_release(cpu_env);
>     tcg_gen_movi_i32(cpu_R[rd], 1);
> +
>     gen_set_label(done_label);
> -    tcg_gen_movi_i64(cpu_exclusive_addr, -1);