| Message ID | 20230216030854.1212208-2-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement FEAT_LSE2 | expand |
On Thu, 16 Feb 2023 at 03:09, Richard Henderson <richard.henderson@linaro.org> wrote: > > We currently treat cpu_exclusive_high as containing the > second word of LDXP, even though that word is not "high" > in big-endian mode. Swap things around so that it is. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/translate-a64.c | 54 ++++++++++++++++++++------------------ > 1 file changed, 29 insertions(+), 25 deletions(-) This code change looks OK as far as it goes, but the bad news is that we migrate the env.exclusive_val and env.exclusive_high values in the machine state. So a migration from a QEMU before this change to a QEMU with this change on a BE host will get confused... thanks -- PMM
On 2/23/23 05:14, Peter Maydell wrote: > On Thu, 16 Feb 2023 at 03:09, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> We currently treat cpu_exclusive_high as containing the >> second word of LDXP, even though that word is not "high" >> in big-endian mode. Swap things around so that it is. >> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> >> --- >> target/arm/translate-a64.c | 54 ++++++++++++++++++++------------------ >> 1 file changed, 29 insertions(+), 25 deletions(-) > > This code change looks OK as far as it goes, but the bad > news is that we migrate the env.exclusive_val and > env.exclusive_high values in the machine state. So a > migration from a QEMU before this change to a QEMU with > this change on a BE host will get confused... Oof. Ok, I didn't *really* need this, it just seemed to make sense. I'll add some commentary about "high" only meaning "high" for little-endian... r~
On Thu, 23 Feb 2023 at 16:12, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/23/23 05:14, Peter Maydell wrote: > > On Thu, 16 Feb 2023 at 03:09, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> We currently treat cpu_exclusive_high as containing the > >> second word of LDXP, even though that word is not "high" > >> in big-endian mode. Swap things around so that it is. > >> > >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > >> --- > >> target/arm/translate-a64.c | 54 ++++++++++++++++++++------------------ > >> 1 file changed, 29 insertions(+), 25 deletions(-) > > > > This code change looks OK as far as it goes, but the bad > > news is that we migrate the env.exclusive_val and > > env.exclusive_high values in the machine state. So a > > migration from a QEMU before this change to a QEMU with > > this change on a BE host will get confused... > > Oof. Ok, I didn't *really* need this, it just seemed to make sense. I'll add some > commentary about "high" only meaning "high" for little-endian... The current state of affairs is arguably broken, because it means you can't migrate a guest from a BE host to an LE host, because the migration stream contains host-endian-dependent data. But if it's easy to leave this sleeping dog alone then it will save us figuring out what we would need to do in a post-load function :-) -- PMM
On Thu, 23 Feb 2023 at 16:51, Peter Maydell <peter.maydell@linaro.org> wrote: > The current state of affairs is arguably broken, because it > means you can't migrate a guest from a BE host to an LE host, > because the migration stream contains host-endian-dependent > data. ...thinking it through, this isn't right. The current code has cpu_exclusive_high always be the 64-bit word from the higher of the two addresses. The change proposes making it be the high part of the guest 128-bit value (which is different if the guest is in BE mode). Neither of those definitions depend on the host endianness: they're just different and we would have to figure out how to convert between them on migration. -- PMM
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index da9f877476..78a2141224 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -2547,17 +2547,28 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2, } else { /* The pair must be single-copy atomic for *each* doubleword, not the entire quadword, however it must be quadword aligned. */ + TCGv_i64 t0 = tcg_temp_new_i64(); + TCGv_i64 t1 = tcg_temp_new_i64(); + memop |= MO_64; - tcg_gen_qemu_ld_i64(cpu_exclusive_val, addr, idx, - memop | MO_ALIGN_16); + tcg_gen_qemu_ld_i64(t0, addr, idx, memop | MO_ALIGN_16); - TCGv_i64 addr2 = tcg_temp_new_i64(); - tcg_gen_addi_i64(addr2, addr, 8); - tcg_gen_qemu_ld_i64(cpu_exclusive_high, addr2, idx, memop); - tcg_temp_free_i64(addr2); + tcg_gen_addi_i64(t1, addr, 8); + tcg_gen_qemu_ld_i64(t1, t1, idx, memop); - tcg_gen_mov_i64(cpu_reg(s, rt), cpu_exclusive_val); - tcg_gen_mov_i64(cpu_reg(s, rt2), cpu_exclusive_high); + if (s->be_data == MO_LE) { + tcg_gen_mov_i64(cpu_exclusive_val, t0); + tcg_gen_mov_i64(cpu_exclusive_high, t1); + } else { + tcg_gen_mov_i64(cpu_exclusive_high, t0); + tcg_gen_mov_i64(cpu_exclusive_val, t1); + } + + tcg_gen_mov_i64(cpu_reg(s, rt), t0); + tcg_gen_mov_i64(cpu_reg(s, rt2), t1); + + tcg_temp_free_i64(t0); + tcg_temp_free_i64(t1); } } else { memop |= size | MO_ALIGN; @@ -2604,36 +2615,29 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2, } else { TCGv_i128 t16 = tcg_temp_new_i128(); TCGv_i128 c16 = tcg_temp_new_i128(); - TCGv_i64 a, b; + TCGv_i64 lo, hi; if (s->be_data == MO_LE) { tcg_gen_concat_i64_i128(t16, cpu_reg(s, rt), cpu_reg(s, rt2)); - tcg_gen_concat_i64_i128(c16, cpu_exclusive_val, - cpu_exclusive_high); } else { tcg_gen_concat_i64_i128(t16, cpu_reg(s, rt2), cpu_reg(s, rt)); - tcg_gen_concat_i64_i128(c16, cpu_exclusive_high, - cpu_exclusive_val); } + tcg_gen_concat_i64_i128(c16, cpu_exclusive_val, cpu_exclusive_high); tcg_gen_atomic_cmpxchg_i128(t16, cpu_exclusive_addr, c16, t16, get_mem_index(s), MO_128 | MO_ALIGN | s->be_data); tcg_temp_free_i128(c16); - a = tcg_temp_new_i64(); - b = tcg_temp_new_i64(); - if (s->be_data == MO_LE) { - tcg_gen_extr_i128_i64(a, b, t16); - } else { - tcg_gen_extr_i128_i64(b, a, t16); - } + lo = tcg_temp_new_i64(); + hi = tcg_temp_new_i64(); + tcg_gen_extr_i128_i64(lo, hi, t16); - tcg_gen_xor_i64(a, a, cpu_exclusive_val); - tcg_gen_xor_i64(b, b, cpu_exclusive_high); - tcg_gen_or_i64(tmp, a, b); - tcg_temp_free_i64(a); - tcg_temp_free_i64(b); + tcg_gen_xor_i64(lo, lo, cpu_exclusive_val); + tcg_gen_xor_i64(hi, hi, cpu_exclusive_high); + tcg_gen_or_i64(tmp, lo, hi); + tcg_temp_free_i64(lo); + tcg_temp_free_i64(hi); tcg_temp_free_i128(t16); tcg_gen_setcondi_i64(TCG_COND_NE, tmp, tmp, 0);
We currently treat cpu_exclusive_high as containing the second word of LDXP, even though that word is not "high" in big-endian mode. Swap things around so that it is. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/translate-a64.c | 54 ++++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 25 deletions(-)