[PATCH v2 0/2] target/ppc: Make divd[u] handler method decodetree compatible

[PATCH v2 0/2] target/ppc: Make divd[u] handler method decodetree compatible

[Ilya Leoshkevich]

v1: https://lore.kernel.org/qemu-devel/20240731100953.14950-1-iii@linux.ibm.com/
v1 -> v2: Add R-bs and a targeted divd[u] patch.

Hi,

This series contains two fixes for the same issue: divd[u] touching
uninitialized ctx->opcode.

Patch 1 is a catch-all solution for all issues in this class. IMHO
it's worth having something like this until the legacy decoder is
fully eliminated.

Patch 2 is a targeted fix for divd[u] only.

Best regards,
Ilya

Ilya Leoshkevich (2):
  target/ppc: Set ctx->opcode for decode_insn32()
  target/ppc: Make divd[u] handler method decodetree compatible

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

-- 
2.45.2

[PATCH v2 1/2] target/ppc: Set ctx->opcode for decode_insn32()

[Ilya Leoshkevich]

divdu (without a dot) sometimes updates cr0, even though it shouldn't.
The reason is that gen_op_arith_divd() checks Rc(ctx->opcode), which is
not initialized. This field is initialized only for instructions that
go through decode_legacy(), and not decodetree.

There already was a similar issue fixed in commit 86e6202a57b1
("target/ppc: Make divw[u] handler method decodetree compatible.").

It's not immediately clear what else may access the uninitialized
ctx->opcode, so instead of playing whack-a-mole and changing the check
to compute_rc0, simply initialize ctx->opcode.

Cc: qemu-stable@nongnu.org
Fixes: 99082815f17f ("target/ppc: Add infrastructure for prefixed insns")
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/ppc/translate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 71513ba9646..02c810e8848 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6426,8 +6426,6 @@ static bool decode_legacy(PowerPCCPU *cpu, DisasContext *ctx, uint32_t insn)
     opc_handler_t **table, *handler;
     uint32_t inval;
 
-    ctx->opcode = insn;
-
     LOG_DISAS("translate opcode %08x (%02x %02x %02x %02x) (%s)\n",
               insn, opc1(insn), opc2(insn), opc3(insn), opc4(insn),
               ctx->le_mode ? "little" : "big");
@@ -6561,6 +6559,7 @@ static void ppc_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
     ctx->base.pc_next = pc += 4;
 
     if (!is_prefix_insn(ctx, insn)) {
+        ctx->opcode = insn;
         ok = (decode_insn32(ctx, insn) ||
               decode_legacy(cpu, ctx, insn));
     } else if ((pc & 63) == 0) {
-- 
2.45.2

[PATCH v2 2/2] target/ppc: Make divd[u] handler method decodetree compatible

[Ilya Leoshkevich]

This is like commit 86e6202a57b1 ("target/ppc: Make divw[u] handler
method decodetree compatible."), but for gen_op_arith_divd().

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/ppc/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 02c810e8848..5a352cdad1b 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1823,7 +1823,7 @@ static inline void gen_op_arith_divd(DisasContext *ctx, TCGv ret,
         tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
     }
 
-    if (unlikely(Rc(ctx->opcode) != 0)) {
+    if (unlikely(compute_rc0)) {
         gen_set_Rc0(ctx, ret);
     }
 }
-- 
2.45.2

Re: [PATCH v2 2/2] target/ppc: Make divd[u] handler method decodetree compatible

[Richard Henderson]

On 8/12/24 18:53, Ilya Leoshkevich wrote:
> This is like commit 86e6202a57b1 ("target/ppc: Make divw[u] handler
> method decodetree compatible."), but for gen_op_arith_divd().
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/ppc/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 02c810e8848..5a352cdad1b 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -1823,7 +1823,7 @@ static inline void gen_op_arith_divd(DisasContext *ctx, TCGv ret,
>           tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
>       }
>   
> -    if (unlikely(Rc(ctx->opcode) != 0)) {
> +    if (unlikely(compute_rc0)) {
>           gen_set_Rc0(ctx, ret);
>       }
>   }

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 2/2] target/ppc: Make divd[u] handler method decodetree compatible

[Philippe Mathieu-Daudé]

On 12/8/24 10:53, Ilya Leoshkevich wrote:
> This is like commit 86e6202a57b1 ("target/ppc: Make divw[u] handler
> method decodetree compatible."), but for gen_op_arith_divd().
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   target/ppc/translate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

PING: [PATCH v2 0/2] target/ppc: Make divd[u] handler method decodetree compatible

[Ilya Leoshkevich]

On Mon, 2024-08-12 at 10:53 +0200, Ilya Leoshkevich wrote:
> v1:
> https://lore.kernel.org/qemu-devel/20240731100953.14950-1-iii@linux.ibm.com/
> v1 -> v2: Add R-bs and a targeted divd[u] patch.
> 
> Hi,
> 
> This series contains two fixes for the same issue: divd[u] touching
> uninitialized ctx->opcode.
> 
> Patch 1 is a catch-all solution for all issues in this class. IMHO
> it's worth having something like this until the legacy decoder is
> fully eliminated.
> 
> Patch 2 is a targeted fix for divd[u] only.
> 
> Best regards,
> Ilya
> 
> Ilya Leoshkevich (2):
>   target/ppc: Set ctx->opcode for decode_insn32()
>   target/ppc: Make divd[u] handler method decodetree compatible
> 
>  target/ppc/translate.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

Ping.

Re: PING: [PATCH v2 0/2] target/ppc: Make divd[u] handler method decodetree compatible

[Nicholas Piggin]

On Wed Sep 18, 2024 at 7:56 PM AEST, Ilya Leoshkevich wrote:
> On Mon, 2024-08-12 at 10:53 +0200, Ilya Leoshkevich wrote:
> > v1:
> > https://lore.kernel.org/qemu-devel/20240731100953.14950-1-iii@linux.ibm.com/
> > v1 -> v2: Add R-bs and a targeted divd[u] patch.
> > 
> > Hi,
> > 
> > This series contains two fixes for the same issue: divd[u] touching
> > uninitialized ctx->opcode.
> > 
> > Patch 1 is a catch-all solution for all issues in this class. IMHO
> > it's worth having something like this until the legacy decoder is
> > fully eliminated.
> > 
> > Patch 2 is a targeted fix for divd[u] only.
> > 
> > Best regards,
> > Ilya
> > 
> > Ilya Leoshkevich (2):
> >   target/ppc: Set ctx->opcode for decode_insn32()
> >   target/ppc: Make divd[u] handler method decodetree compatible
> > 
> >  target/ppc/translate.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
>
> Ping.

Hey Ilya,

Sorry for the late reply, had some time off and taken me a while
to get back to it.

Yeah this is a good fix, thank you. I have queued it.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick