On Tue, May 18, 2021 at 09:30:38AM +0200, Greg Kurz wrote: > On Tue, 18 May 2021 08:40:36 +0200 > Giuseppe Musacchio <thatle...@gmail.com> wrote: > > > The ISA [1] specifies the load order to be the target one, hence > > the use of MO_TEQ in my patch (in both lxvwsx and lxvdsx). > > > > I believe the error is hidden in some of the .mak files: I could not > > reproduce this problem with Qemu's user-mode emulation in either > > BE nor LE mode, this lead me to discover that ppc64-softmmu.mak is > > always defining TARGET_WORDS_BIGENDIAN=y. The user-mode targets are > > correctly split into ppc64 and ppc64le, where only the former is > > declared as BE. > > > > Yes. In system-mode emulation, modern POWER CPUs are expected to > be able to switch from BE to LE and vice-versa at runtime. Older > PowerPC CPUs are BE. The qemu-system-ppc64 binary is thus built > with TARGET_WORDS_BIGENDIAN=y and every place where runtime > endianness matters need to do a check and only byteswap if needed.
Right. With modern POWER (and ARM and several others), "target endian" isn't really a well-defined concept, since the processor can switch at runtime. > > Mark's suggestion in another mail of this thread is the way to go. > > > The presence of that define is unconditionally making MO_TE an alias > > for MO_BE, that's why Paul's patch seems to fix the problem. > > > > I didn't catch this problem earlier as pretty much of our testing is > > done using the Linux user-mode emulation. > > > > Cheers, > > G.M. > > > > [1] https://ibm.ent.box.com/s/1hzcwkwf8rbju5h9iyf44wm94amnlcrv > > > > On 18/05/21 03:34, David Gibson wrote: > > > > > > On Mon, May 17, 2021 at 04:40:32PM -0500, Paul A. Clarke wrote: > > >> `lxvdsx` is byte-swapping the data it loads, which it should not > > >> do. Fix it. > > >> > > >> Fixes #212. > > >> > > >> Fixes: bcb0b7b1a1c05707304f80ca6f523d557816f85c > > >> Signed-off-by: Paul A. Clarke <p...@us.ibm.com > > > nit, missing '>' ...^ > > > > > > I'm having a hard time convincing myself this is correct in all cases. > > > Have you tested it with all combinations of BE/LE host and BE/LE guest > > > code? > > > > > > The description in the ISA is pretty inscrutable, since it's in terms > > > of the confusing numbering if different element types in BE vs LE > > > mode. > > > > > > It looks to me like before bcb0b7b1a1c0 this originally resolved to > > > MO_Q modified by ctx->default_tcg_memop_mask, which appears to depend > > > on the current guest endian mode. That's pretty hard to trace through > > > the various layers of macros, but for reference, before bcb0b7b1a1c0 > > > this used gen_qemu_ld64_i64(), which appears to be constructed by the > > > line GEN_QEMU_LOAD_64(ld64, DEF_MEMOP(MO_Q)) in translate.c. > > > > > > Richard or Giuseppe, care to weigh in? > > > > > >> --- > > >> target/ppc/translate/vsx-impl.c.inc | 2 +- > > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > >> > > >> diff --git a/target/ppc/translate/vsx-impl.c.inc > > >> b/target/ppc/translate/vsx-impl.c.inc > > >> index b817d31260bb..46f97c029ca8 100644 > > >> --- a/target/ppc/translate/vsx-impl.c.inc > > >> +++ b/target/ppc/translate/vsx-impl.c.inc > > >> @@ -162,7 +162,7 @@ static void gen_lxvdsx(DisasContext *ctx) > > >> gen_addr_reg_index(ctx, EA); > > >> > > >> data = tcg_temp_new_i64(); > > >> - tcg_gen_qemu_ld_i64(data, EA, ctx->mem_idx, MO_TEQ); > > >> + tcg_gen_qemu_ld_i64(data, EA, ctx->mem_idx, MO_LEQ); > > >> tcg_gen_gvec_dup_i64(MO_Q, vsr_full_offset(xT(ctx->opcode)), 16, > > >> 16, data); > > >> > > >> tcg_temp_free(EA); > > > > > > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature