[PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Richard Henderson]

Fix a crash writing to 't3', which is now a constant.
Instead, write the result of the remu to 't1'.

Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
Reported-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Anton Johansson <anjo@rev.ng>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

v2: Use a temp of the correct type, for ppc64.
    It's what I get for rushing things this afternoon.

r~

---
 target/ppc/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 9d05357d03..f603f1a939 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1807,8 +1807,8 @@ static inline void gen_op_arith_modw(DisasContext *ctx, TCGv ret, TCGv arg1,
         TCGv_i32 t2 = tcg_constant_i32(1);
         TCGv_i32 t3 = tcg_constant_i32(0);
         tcg_gen_movcond_i32(TCG_COND_EQ, t1, t1, t3, t2, t1);
-        tcg_gen_remu_i32(t3, t0, t1);
-        tcg_gen_extu_i32_tl(ret, t3);
+        tcg_gen_remu_i32(t0, t0, t1);
+        tcg_gen_extu_i32_tl(ret, t0);
     }
 }
 
-- 
2.34.1

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Cédric Le Goater]

On 4/8/23 09:05, Richard Henderson wrote:
> Fix a crash writing to 't3', which is now a constant.
> Instead, write the result of the remu to 't1'.
> 
> Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Anton Johansson <anjo@rev.ng>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Looks good:

   https://gitlab.com/legoater/qemu/-/pipelines/831847446

I have a PR ready for this same branch. If you want to me send,
just tell.

I don't think we have tcg tests for the prefix or mma instructions
introduced in P10. That would be good to have.

C.

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Nicholas Piggin]

On Sun Apr 9, 2023 at 7:24 AM AEST, Cédric Le Goater wrote:
> On 4/8/23 09:05, Richard Henderson wrote:
> > Fix a crash writing to 't3', which is now a constant.
> > Instead, write the result of the remu to 't1'.
> > 
> > Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
> > Reported-by: Nicholas Piggin <npiggin@gmail.com>
> > Reviewed-by: Anton Johansson <anjo@rev.ng>
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>
> Looks good:
>
>    https://gitlab.com/legoater/qemu/-/pipelines/831847446
>
> I have a PR ready for this same branch. If you want to me send,
> just tell.

Thanks Richard and Cedric. LGTM.

> I don't think we have tcg tests for the prefix or mma instructions
> introduced in P10. That would be good to have.

I agree, we need to do a bit better on ppc.

I'm trying to get a handle on all the tests we have for these things,
I haven't looked too closely before. kvm-unit-tests actually works
well for TCG and I did find some (system level) prefix issues with it.
I don't know if that's the right place to focus on instruction level
testing though. QEMU's tcg tests sounds like a better place for it,
but is it only for userspace tests? There are also some verification
tests people are using for verifying hardware cores.

Seems like a common upstream project that others can use might be
useful.

Thanks,
Nick

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Richard Henderson]

On 4/8/23 14:24, Cédric Le Goater wrote:
> On 4/8/23 09:05, Richard Henderson wrote:
>> Fix a crash writing to 't3', which is now a constant.
>> Instead, write the result of the remu to 't1'.
>>
>> Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
>> Reported-by: Nicholas Piggin <npiggin@gmail.com>
>> Reviewed-by: Anton Johansson <anjo@rev.ng>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Looks good:
> 
>    https://gitlab.com/legoater/qemu/-/pipelines/831847446
> 
> I have a PR ready for this same branch. If you want to me send,
> just tell.

Yes, please.  Also, the comment above needs s/t1/t0/.  :-P


r~

> 
> I don't think we have tcg tests for the prefix or mma instructions
> introduced in P10. That would be good to have.
> 
> C.
> 

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Cédric Le Goater]

On 4/9/23 18:21, Richard Henderson wrote:
> On 4/8/23 14:24, Cédric Le Goater wrote:
>> On 4/8/23 09:05, Richard Henderson wrote:
>>> Fix a crash writing to 't3', which is now a constant.
>>> Instead, write the result of the remu to 't1'.
>>>
>>> Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
>>> Reported-by: Nicholas Piggin <npiggin@gmail.com>
>>> Reviewed-by: Anton Johansson <anjo@rev.ng>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Looks good:
>>
>>    https://gitlab.com/legoater/qemu/-/pipelines/831847446
>>
>> I have a PR ready for this same branch. If you want to me send,
>> just tell.
> 
> Yes, please.  Also, the comment above needs s/t1/t0/.  :-P

sure :)

Are you taking care of :

   https://lore.kernel.org/r/20230408154316.3812709-1-richard.henderson@linaro.org

C.


> 
> 
> r~
> 
>>
>> I don't think we have tcg tests for the prefix or mma instructions
>> introduced in P10. That would be good to have.
>>
>> C.
>>
> 

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Richard Henderson]

On 4/9/23 10:29, Cédric Le Goater wrote:
> On 4/9/23 18:21, Richard Henderson wrote:
>> On 4/8/23 14:24, Cédric Le Goater wrote:
>>> On 4/8/23 09:05, Richard Henderson wrote:
>>>> Fix a crash writing to 't3', which is now a constant.
>>>> Instead, write the result of the remu to 't1'.
>>>>
>>>> Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
>>>> Reported-by: Nicholas Piggin <npiggin@gmail.com>
>>>> Reviewed-by: Anton Johansson <anjo@rev.ng>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>> Looks good:
>>>
>>>    https://gitlab.com/legoater/qemu/-/pipelines/831847446
>>>
>>> I have a PR ready for this same branch. If you want to me send,
>>> just tell.
>>
>> Yes, please.  Also, the comment above needs s/t1/t0/.  :-P
> 
> sure :)
> 
> Are you taking care of :
> 
>    https://lore.kernel.org/r/20230408154316.3812709-1-richard.henderson@linaro.org

Yes, I'll send that with two other tcg fixes.


r~

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Daniel Henrique Barboza]

On 4/8/23 04:05, Richard Henderson wrote:
> Fix a crash writing to 't3', which is now a constant.
> Instead, write the result of the remu to 't1'.


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


And thanks Cedric for picking this up :D


Daniel

> 
> Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Reviewed-by: Anton Johansson <anjo@rev.ng>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> v2: Use a temp of the correct type, for ppc64.
>      It's what I get for rushing things this afternoon.
> 
> r~
> 
> ---
>   target/ppc/translate.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 9d05357d03..f603f1a939 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -1807,8 +1807,8 @@ static inline void gen_op_arith_modw(DisasContext *ctx, TCGv ret, TCGv arg1,
>           TCGv_i32 t2 = tcg_constant_i32(1);
>           TCGv_i32 t3 = tcg_constant_i32(0);
>           tcg_gen_movcond_i32(TCG_COND_EQ, t1, t1, t3, t2, t1);
> -        tcg_gen_remu_i32(t3, t0, t1);
> -        tcg_gen_extu_i32_tl(ret, t3);
> +        tcg_gen_remu_i32(t0, t0, t1);
> +        tcg_gen_extu_i32_tl(ret, t0);
>       }
>   }
>   

Re: [PATCH for-8.0 v2] target/ppc: Fix temp usage in gen_op_arith_modw

[Fabiano Rosas]

"Nicholas Piggin" <npiggin@gmail.com> writes:

> On Sun Apr 9, 2023 at 7:24 AM AEST, Cédric Le Goater wrote:
>> On 4/8/23 09:05, Richard Henderson wrote:
>> > Fix a crash writing to 't3', which is now a constant.
>> > Instead, write the result of the remu to 't1'.
>> > 
>> > Fixes: 7058ff5231a ("target/ppc: Avoid tcg_const_* in translate.c")
>> > Reported-by: Nicholas Piggin <npiggin@gmail.com>
>> > Reviewed-by: Anton Johansson <anjo@rev.ng>
>> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Looks good:
>>
>>    https://gitlab.com/legoater/qemu/-/pipelines/831847446
>>
>> I have a PR ready for this same branch. If you want to me send,
>> just tell.
>
> Thanks Richard and Cedric. LGTM.
>
>> I don't think we have tcg tests for the prefix or mma instructions
>> introduced in P10. That would be good to have.
>
> I agree, we need to do a bit better on ppc.
>
> I'm trying to get a handle on all the tests we have for these things,
> I haven't looked too closely before. kvm-unit-tests actually works
> well for TCG and I did find some (system level) prefix issues with it.
> I don't know if that's the right place to focus on instruction level
> testing though. QEMU's tcg tests sounds like a better place for it,
> but is it only for userspace tests?

Last time we looked at adding softmmu to the tests:

https://lore.kernel.org/all/20220324190854.156898-1-leandro.lupori@eldorado.org.br/