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

[Richard Henderson <richard.henderson@linaro.org>]

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~