| Message ID | 20230821153051.93658-1-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Fix LQ, STQ register-pair order for big-endian | expand |
On 8/21/23 08:30, Nicholas Piggin wrote: > LQ, STQ have the same register-pair ordering as LQARX/STQARX., which is > the even (lower) register contains the most significant bits. This is > not implemented correctly for big-endian. > > do_ldst_quad() has variables low_addr_gpr and high_addr_gpr which is > confusing because they are low and high addresses, whereas LQARX/STQARX. > and most such things use the low and high values for lo/hi variables. > The conversion to native 128-bit memory access functions missed this > strangeness. > > Fix this by changing the if condition, and change the variable names to > hi/lo to match convention. > > Cc:qemu-stable@nongnu.org > Reported-by: Ivan Warren<ivan@vmfacility.fr> > Fixes: 57b38ffd0c6f ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ") > Resolves:https://gitlab.com/qemu-project/qemu/-/issues/1836 > Signed-off-by: Nicholas Piggin<npiggin@gmail.com> > --- > Hi Ivan, > > Thanks for your report. This gets AIX7.2 booting for me again with TCG, > if you would be able to confirm that it works there, it would be great. > > Thanks, > Nick > > target/ppc/translate/fixedpoint-impl.c.inc | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) Thanks for the catch. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc index f47f1a50e8..b423c09c26 100644 --- a/target/ppc/translate/fixedpoint-impl.c.inc +++ b/target/ppc/translate/fixedpoint-impl.c.inc @@ -71,7 +71,7 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool store, bool prefixed) { #if defined(TARGET_PPC64) TCGv ea; - TCGv_i64 low_addr_gpr, high_addr_gpr; + TCGv_i64 lo, hi; TCGv_i128 t16; REQUIRE_INSNS_FLAGS(ctx, 64BX); @@ -94,21 +94,21 @@ static bool do_ldst_quad(DisasContext *ctx, arg_D *a, bool store, bool prefixed) gen_set_access_type(ctx, ACCESS_INT); ea = do_ea_calc(ctx, a->ra, tcg_constant_tl(a->si)); - if (prefixed || !ctx->le_mode) { - low_addr_gpr = cpu_gpr[a->rt]; - high_addr_gpr = cpu_gpr[a->rt + 1]; + if (ctx->le_mode && prefixed) { + lo = cpu_gpr[a->rt]; + hi = cpu_gpr[a->rt + 1]; } else { - low_addr_gpr = cpu_gpr[a->rt + 1]; - high_addr_gpr = cpu_gpr[a->rt]; + lo = cpu_gpr[a->rt + 1]; + hi = cpu_gpr[a->rt]; } t16 = tcg_temp_new_i128(); if (store) { - tcg_gen_concat_i64_i128(t16, low_addr_gpr, high_addr_gpr); + tcg_gen_concat_i64_i128(t16, lo, hi); tcg_gen_qemu_st_i128(t16, ea, ctx->mem_idx, DEF_MEMOP(MO_128)); } else { tcg_gen_qemu_ld_i128(t16, ea, ctx->mem_idx, DEF_MEMOP(MO_128)); - tcg_gen_extr_i128_i64(low_addr_gpr, high_addr_gpr, t16); + tcg_gen_extr_i128_i64(lo, hi, t16); } #else qemu_build_not_reached();
LQ, STQ have the same register-pair ordering as LQARX/STQARX., which is the even (lower) register contains the most significant bits. This is not implemented correctly for big-endian. do_ldst_quad() has variables low_addr_gpr and high_addr_gpr which is confusing because they are low and high addresses, whereas LQARX/STQARX. and most such things use the low and high values for lo/hi variables. The conversion to native 128-bit memory access functions missed this strangeness. Fix this by changing the if condition, and change the variable names to hi/lo to match convention. Cc: qemu-stable@nongnu.org Reported-by: Ivan Warren <ivan@vmfacility.fr> Fixes: 57b38ffd0c6f ("target/ppc: Use tcg_gen_qemu_{ld,st}_i128 for LQARX, LQ, STQ") Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1836 Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Hi Ivan, Thanks for your report. This gets AIX7.2 booting for me again with TCG, if you would be able to confirm that it works there, it would be great. Thanks, Nick target/ppc/translate/fixedpoint-impl.c.inc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)