| Message ID | 20240731100953.14950-1-iii@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: Set ctx->opcode for decode_insn32() | expand |
On 31/7/24 12:07, Ilya Leoshkevich wrote: > 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") > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- > target/ppc/translate.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 7/31/24 20:07, Ilya Leoshkevich wrote: > 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") > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> Accessing opcode from decodetree should not happen -- divd needs fixing. But I agree this is a good short-term workaround for the release, so: Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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) {
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") Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- target/ppc/translate.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)