Re: [PATCH] target/ppc: Fix lxvx/stxvx facility check

[Richard Henderson]

On 9/20/24 11:24, Ilya Leoshkevich wrote:
> On Thu, 2024-09-19 at 13:36 +0200, Claudio Fontana wrote:
> > ping, adding Richard.
> >
> > We will need to include this downstream because of the breakage its
> > lack causes.
> > It is already reviewed by me, but some TCG maintainer indicating it
> > will be included in some queue would help,
> >
> > Thanks,
> >
> > Claudio
> >
> > On 9/18/24 17:11, Claudio Fontana wrote:
> > > Adding Ilya FYI.
> > >
> > > Ciao,
> > >
> > > Claudio
> > >
> > > On 9/11/24 18:19, Claudio Fontana wrote:
> > > > On 9/11/24 16:16, Fabiano Rosas wrote:
> > > > > The XT check for the lxvx/stxvx instructions is currently
> > > > > inverted. This was introduced during the move to decodetree.
> > > > >
> > > > >  From the ISA:
> > > > >    Chapter 7. Vector-Scalar Extension Facility
> > > > >    Load VSX Vector Indexed X-form
> > > > >
> > > > >    lxvx XT,RA,RB
> > > > >    if TX=0 & MSR.VSX=0 then VSX_Unavailable()
> > > > >    if TX=1 & MSR.VEC=0 then Vector_Unavailable()
> > > > >    ...
> > > > >    Let XT be the value 32×TX + T.
> > > > >
> > > > > The code currently does the opposite:
> > > > >
> > > > >      if (paired || a->rt >= 32) {
> > > > >          REQUIRE_VSX(ctx);
> > > > >      } else {
> > > > >          REQUIRE_VECTOR(ctx);
> > > > >      }
> > > > >
> > > > > This was already fixed for lxv/stxv at commit "2cc0e449d1
> > > > > (target/ppc:
> > > > > Fix lxv/stxv MSR facility check)", but the indexed forms were
> > > > > missed.
> > > > >
> > > > > Cc: qemu-stable@nongnu.org
> > > > > Fixes: 70426b5bb7 ("target/ppc: moved stxvx and lxvx from
> > > > > legacy to decodtree")
> > > > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > > > > ---
> > > > >   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 40a87ddc4a..a869f30e86 100644
> > > > > --- a/target/ppc/translate/vsx-impl.c.inc
> > > > > +++ b/target/ppc/translate/vsx-impl.c.inc
> > > > > @@ -2244,7 +2244,7 @@ static bool do_lstxv_PLS_D(DisasContext
> > > > > *ctx, arg_PLS_D *a,
> > > > >   
> > > > >   static bool do_lstxv_X(DisasContext *ctx, arg_X *a, bool
> > > > > store, bool paired)
> > > > >   {
> > > > > -    if (paired || a->rt >= 32) {
> > > > > +    if (paired || a->rt < 32) {
> > > > >           REQUIRE_VSX(ctx);
> > > > >       } else {
> > > > >           REQUIRE_VECTOR(ctx);
> > > >
> > > > Reviewed-by: Claudio Fontana <cfontana@suse.de>
>
> FWIW
>
> Acked-by: Ilya Leoshkevich <iii@linux.ibm.com>
>
> But I'm not a maintainer, I guess Richard will need to pick it up.

Queued, thanks.

r~