| Message ID | 20230408070547.3609447-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [for-8.0,v2] target/ppc: Fix temp usage in gen_op_arith_modw | expand |
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.
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
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. >
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. >> >
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~
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); > } > } >
"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/
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); } }
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(-)