On 12/05/2016 03:09 AM, Alex Bennée wrote: > > Alex Bennée <alex.ben...@linaro.org> writes: > >> While testing rth's latest TCG patches with risu I found ldaxp was >> broken. Investigating further I found it was broken by 1dd089d0 when >> the cmpxchg atomic work was merged. > > CC'ing Paolo/Richard > > Do you guys have any final TCG fixes planned for 2.8 that can take this > fix for the ldaxp regression? > >> As part of that change the code >> attempted to be clever by doing a single 64 bit load and then shuffle >> the data around to set the two 32 bit registers. >> >> As I couldn't quite follow the endian magic I've simply partially >> reverted the change to the original code gen_load_exclusive code. This >> doesn't affect the cmpxchg functionality as that is all done on in >> gen_store_exclusive part which is untouched. >> >> I've also restored the comment that was removed (with a slight tweak >> to mention cmpxchg). >> >> Signed-off-by: Alex Bennée <alex.ben...@linaro.org> >> --- >> target-arm/translate-a64.c | 42 +++++++++++++++++++----------------------- >> 1 file changed, 19 insertions(+), 23 deletions(-) >> >> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c >> index de48747..6dc27a6 100644 >> --- a/target-arm/translate-a64.c >> +++ b/target-arm/translate-a64.c >> @@ -1839,41 +1839,37 @@ static void disas_b_exc_sys(DisasContext *s, >> uint32_t insn) >> } >> } >> >> +/* >> + * Load/Store exclusive instructions are implemented by remembering >> + * the value/address loaded, and seeing if these are the same >> + * when the store is performed. This is not actually the architecturally >> + * mandated semantics, but it works for typical guest code sequences >> + * and avoids having to monitor regular stores. >> + * >> + * The store exclusive uses the atomic cmpxchg primitives to avoid >> + * races in multi-threaded linux-user and when MTTCG softmmu is >> + * enabled. >> + */ >> static void gen_load_exclusive(DisasContext *s, int rt, int rt2, >> TCGv_i64 addr, int size, bool is_pair) >> { >> TCGv_i64 tmp = tcg_temp_new_i64(); >> - TCGMemOp be = s->be_data; >> + TCGMemOp memop = s->be_data + size; >> >> g_assert(size <= 3); >> + tcg_gen_qemu_ld_i64(tmp, addr, get_mem_index(s), memop); >> + >> if (is_pair) { >> + TCGv_i64 addr2 = tcg_temp_new_i64(); >> TCGv_i64 hitmp = tcg_temp_new_i64(); >> >> - if (size == 3) { >> - TCGv_i64 addr2 = tcg_temp_new_i64(); >> - >> - tcg_gen_qemu_ld_i64(tmp, addr, get_mem_index(s), >> - MO_64 | MO_ALIGN_16 | be); >> - tcg_gen_addi_i64(addr2, addr, 8); >> - tcg_gen_qemu_ld_i64(hitmp, addr2, get_mem_index(s), >> - MO_64 | MO_ALIGN | be); >> - tcg_temp_free_i64(addr2); >> - } else { >> - g_assert(size == 2); >> - tcg_gen_qemu_ld_i64(tmp, addr, get_mem_index(s), >> - MO_64 | MO_ALIGN | be); >> - if (be == MO_LE) { >> - tcg_gen_extr32_i64(tmp, hitmp, tmp); >> - } else { >> - tcg_gen_extr32_i64(hitmp, tmp, tmp); >> - } >> - } >> - >> + g_assert(size >= 2); >> + tcg_gen_addi_i64(addr2, addr, 1 << size); >> + tcg_gen_qemu_ld_i64(hitmp, addr2, get_mem_index(s), memop); >> + tcg_temp_free_i64(addr2); >> tcg_gen_mov_i64(cpu_exclusive_high, hitmp); >> tcg_gen_mov_i64(cpu_reg(s, rt2), hitmp); >> tcg_temp_free_i64(hitmp); >> - } else { >> - tcg_gen_qemu_ld_i64(tmp, addr, get_mem_index(s), size | MO_ALIGN | >> be); >> } >> >> tcg_gen_mov_i64(cpu_exclusive_val, tmp);
Acked-by: Richard Henderson <r...@twiddle.net> r~