Re: [Qemu-devel] [PATCH v14 30/33] target-tilegx: Handle atomic instructions

[Richard Henderson <rth@twiddle.net>]

On 08/24/2015 09:17 AM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>   target-tilegx/translate.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
> index 210e912..2a0798a 100644
> --- a/target-tilegx/translate.c
> +++ b/target-tilegx/translate.c
> @@ -180,6 +180,19 @@ static void gen_saturate_op(TCGv tdest, TCGv tsrca, TCGv tsrcb,
>       tcg_temp_free(t0);
>   }
>
> +static void gen_atomic_excp(DisasContext *dc, unsigned dest, unsigned srca,
> +                            unsigned srcb, TileExcp excp)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    TCGv_i32 t = tcg_const_i32((dest << 16) | (srca << 8) | srcb);
> +    tcg_gen_st_i32(t, cpu_env, offsetof(CPUTLGState, excparam));
> +    tcg_temp_free_i32(t);
> +    gen_exception(dc, excp);
> +#else
> +    gen_exception(dc, TILEGX_EXCP_OPCODE_UNIMPLEMENTED);
> +#endif
> +}


This is broken.  While it does work well enough for Hello World, implementing a 
non-trap instruction with an exception is extremely dicey for TileGX.  The 
issue is that TileGX bundles operate atomically, with no RAW issues between the 
instructions of the bundle.

Consider a bundle like

    { add r0, r0, r1 ; exch r2, r0, r3 }

In Chen's implementation, the writeback to r0 would occur before the exception, 
and so the exch would happen to the wrong address.  In my implementation here, 
the exception would occur before the writeback, and so the result of the add 
would be discarded.

While retaining the current start_exclusive scheme, it would appear that we 
need to store more data here -- to wit, the contents of the registers not their 
numbers -- and delay the exception until after writeback.

Even better, of course, would be to not exit the TB at all, and use host atomic 
instructions...  I suppose that multi-threading patch set is still in limbo?


r~