[PATCH] target/ppc: Fix rlwinm on ppc64

2020-03-09 Thread Vitaly Chikunov
rlwinm cannot just AND with Mask if shift value is zero on ppc64 when
Mask Begin is greater than Mask End and high bits are set to 1.

Note that PowerISA 3.0B says that for `rlwinm' ROTL32 is used, and
ROTL32 is defined (in 3.3.14) so that rotated value should have two
copies of lower word of the source value.

This seems to be another incarnation of the fix from 820724d170
("target-ppc: Fix rlwimi, rlwinm, rlwnm again"), except I leave
optimization when Mask value is less than 32 bits.

Fixes: 7b4d326f47 ("target-ppc: Use the new deposit and extract ops")
Cc: qemu-sta...@nongnu.org
Signed-off-by: Vitaly Chikunov 
---
 target/ppc/translate.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 36fa27367c..127c82a24e 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1938,15 +1938,17 @@ static void gen_rlwinm(DisasContext *ctx)
 me += 32;
 #endif
 mask = MASK(mb, me);
-if (sh == 0) {
-tcg_gen_andi_tl(t_ra, t_rs, mask);
-} else if (mask <= 0xu) {
-TCGv_i32 t0 = tcg_temp_new_i32();
-tcg_gen_trunc_tl_i32(t0, t_rs);
-tcg_gen_rotli_i32(t0, t0, sh);
-tcg_gen_andi_i32(t0, t0, mask);
-tcg_gen_extu_i32_tl(t_ra, t0);
-tcg_temp_free_i32(t0);
+if (mask <= 0xu) {
+if (sh == 0) {
+tcg_gen_andi_tl(t_ra, t_rs, mask);
+} else {
+TCGv_i32 t0 = tcg_temp_new_i32();
+tcg_gen_trunc_tl_i32(t0, t_rs);
+tcg_gen_rotli_i32(t0, t0, sh);
+tcg_gen_andi_i32(t0, t0, mask);
+tcg_gen_extu_i32_tl(t_ra, t0);
+tcg_temp_free_i32(t0);
+}
 } else {
 #if defined(TARGET_PPC64)
 tcg_gen_deposit_i64(t_ra, t_rs, t_rs, 32, 32);
-- 
2.11.0




Re: [PATCH] target/ppc: Fix rlwinm on ppc64

2020-03-11 Thread Vitaly Chikunov
David,

On Wed, Mar 11, 2020 at 10:15:03AM +1100, David Gibson wrote:
> On Mon, Mar 09, 2020 at 11:45:57PM +0300, Vitaly Chikunov wrote:
> > rlwinm cannot just AND with Mask if shift value is zero on ppc64 when
> > Mask Begin is greater than Mask End and high bits are set to 1.
> > 
> > Note that PowerISA 3.0B says that for `rlwinm' ROTL32 is used, and
> > ROTL32 is defined (in 3.3.14) so that rotated value should have two
> > copies of lower word of the source value.
> > 
> > This seems to be another incarnation of the fix from 820724d170
> > ("target-ppc: Fix rlwimi, rlwinm, rlwnm again"), except I leave
> > optimization when Mask value is less than 32 bits.
> > 
> > Fixes: 7b4d326f47 ("target-ppc: Use the new deposit and extract ops")
> > Cc: qemu-sta...@nongnu.org
> > Signed-off-by: Vitaly Chikunov 
> 
> Applied to ppc-for-5.0.

Thanks! FYI, there is at least one real case of this bug:
  https://github.com/iovisor/bcc/issues/2771
so this is not theoretical, and, probably, should go to the stable
too.

Thanks,


> 
> > ---
> >  target/ppc/translate.c | 20 +++-
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 36fa27367c..127c82a24e 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -1938,15 +1938,17 @@ static void gen_rlwinm(DisasContext *ctx)
> >  me += 32;
> >  #endif
> >  mask = MASK(mb, me);
> > -if (sh == 0) {
> > -tcg_gen_andi_tl(t_ra, t_rs, mask);
> > -} else if (mask <= 0xu) {
> > -TCGv_i32 t0 = tcg_temp_new_i32();
> > -tcg_gen_trunc_tl_i32(t0, t_rs);
> > -tcg_gen_rotli_i32(t0, t0, sh);
> > -tcg_gen_andi_i32(t0, t0, mask);
> > -tcg_gen_extu_i32_tl(t_ra, t0);
> > -tcg_temp_free_i32(t0);
> > +if (mask <= 0xu) {
> > +if (sh == 0) {
> > +tcg_gen_andi_tl(t_ra, t_rs, mask);
> > +} else {
> > +TCGv_i32 t0 = tcg_temp_new_i32();
> > +tcg_gen_trunc_tl_i32(t0, t_rs);
> > +tcg_gen_rotli_i32(t0, t0, sh);
> > +tcg_gen_andi_i32(t0, t0, mask);
> > +tcg_gen_extu_i32_tl(t_ra, t0);
> > +tcg_temp_free_i32(t0);
> > +}
> >  } else {
> >  #if defined(TARGET_PPC64)
> >  tcg_gen_deposit_i64(t_ra, t_rs, t_rs, 32, 32);
> 
> -- 
> 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