1. Patchew
  2. QEMU
  3. View series

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

Fabiano Rosas posted 1 patch 2 months, 1 week ago 
target/ppc/translate/vsx-impl.c.inc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] target/ppc: Fix lxvx/stxvx facility check
Posted by Fabiano Rosas 2 months, 1 week ago 
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);
-- 
2.35.3
Re: [PATCH] target/ppc: Fix lxvx/stxvx facility check
Posted by Claudio Fontana 2 months, 1 week ago 
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>
Re: [PATCH] target/ppc: Fix lxvx/stxvx facility check
Posted by Claudio Fontana 2 months ago 
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>
>
Re: [PATCH] target/ppc: Fix lxvx/stxvx facility check
Posted by Claudio Fontana 2 months ago 
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>
>>
> 
>
Re: [PATCH] target/ppc: Fix lxvx/stxvx facility check
Posted by Ilya Leoshkevich 2 months ago 
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.
Re: [PATCH] target/ppc: Fix lxvx/stxvx facility check
Posted by Richard Henderson 2 months ago 
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~

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.