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

[David Gibson]

On Tue, May 18, 2021 at 09:30:38AM +0200, Greg Kurz wrote:
> 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.

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

-- 
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