On Wed Jun 28, 2023 at 3:38 AM AEST, BALATON Zoltan wrote: > On Tue, 27 Jun 2023, Nicholas Piggin wrote: > > checkstop state does not halt the system, interrupts continue to be > > serviced, and other CPUs run. > > > > Stop the machine with vm_stop(), and print a register dump too. > > > > Signed-off-by: Nicholas Piggin <npig...@gmail.com> > > --- > > Since v1: > > - Fix loop exit so it stops on the attn instruction, rather than > > after it. > > > > target/ppc/excp_helper.c | 34 ++++++++++++++++++++-------------- > > 1 file changed, 20 insertions(+), 14 deletions(-) > > > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c > > index 5beda973ce..28d8a9b212 100644 > > --- a/target/ppc/excp_helper.c > > +++ b/target/ppc/excp_helper.c > > @@ -19,6 +19,7 @@ > > #include "qemu/osdep.h" > > #include "qemu/main-loop.h" > > #include "qemu/log.h" > > +#include "sysemu/runstate.h" > > #include "cpu.h" > > #include "exec/exec-all.h" > > #include "internal.h" > > @@ -186,19 +187,24 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env, > > int excp) > > env->error_code); > > } > > > > -static void powerpc_checkstop(CPUPPCState *env) > > +static void powerpc_checkstop(CPUPPCState *env, const char *reason) > > { > > CPUState *cs = env_cpu(env); > > > > - /* Machine check exception is not enabled. Enter checkstop state. */ > > - fprintf(stderr, "Machine check while not allowed. " > > - "Entering checkstop state\n"); > > + vm_stop(RUN_STATE_GUEST_PANICKED); > > + > > + fprintf(stderr, "Entering checkstop state: %s\n", reason); > > + cpu_dump_state(cs, stderr, CPU_DUMP_FPU | CPU_DUMP_CCOP); > > if (qemu_log_separate()) { > > - qemu_log("Machine check while not allowed. " > > - "Entering checkstop state\n"); > > + FILE *logfile = qemu_log_trylock(); > > + if (logfile) { > > + fprintf(logfile, "Entering checkstop state: %s\n", reason); > > I don't think you should have fprintfs here. Is this remnants of debug > code left here by mistake? The fprintf that was there before may also need > to be converted to some qemI_log or error_report but I did not know what > these are for and did not address that. But if you want to add more then > it may need to be solved first.
I just followed existing fprintf use. Changing that should be separate patch indeed. > > + cpu_dump_state(cs, logfile, CPU_DUMP_FPU | CPU_DUMP_CCOP); > > + qemu_log_unlock(logfile); > > + } > > } > > - cs->halted = 1; > > - cpu_interrupt_exittb(cs); > > + > > Excess blank line? No, it separates the logging block from function. > > > + cpu_loop_exit_noexc(cs); > > } > > > > #if defined(TARGET_PPC64) > > @@ -483,7 +489,7 @@ static void powerpc_excp_40x(PowerPCCPU *cpu, int excp) > > break; > > case POWERPC_EXCP_MCHECK: /* Machine check exception > > */ > > if (!FIELD_EX64(env->msr, MSR, ME)) { > > - powerpc_checkstop(env); > > + powerpc_checkstop(env, "machine check with MSR[ME]=0"); > > If the message is always the same why pass it from here If the only other > option not used yet would be MSR[ME]=1 then that could also be checked in > the func so no need to pass the message. So is there any other possible > reason here? To make the checkstop function more general (e.g., used by the next patch). Thanks, Nick