Re: [PATCH] Fix `lxvdsx` (issue #212)

[Greg Kurz <groug@kaod.org>]

On Tue, 18 May 2021 08:40:36 +0200
Giuseppe Musacchio <thatlemon@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.

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 <pc@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);
> > 
>