On Fri Jun 23, 2023 at 9:51 PM AEST, BALATON Zoltan wrote:
> On Fri, 23 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>
> > ---
> > target/ppc/excp_helper.c | 35 +++++++++++++++++++++--------------
> > 1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 4bfcfc3c3d..51e83d7f07 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"
> > @@ -165,6 +166,24 @@ static void ppc_excp_debug_sw_tlb(CPUPPCState *env,
> > int excp)
> > env->error_code);
> > }
> >
> > +static void powerpc_checkstop(PowerPCCPU *cpu, const char *reason)
> > +{
> > + CPUState *cs = CPU(cpu);
> > +
> > + 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()) {
> > + FILE *logfile = qemu_log_trylock();
> > + if (logfile) {
> > + fprintf(logfile, "Entering checkstop state: %s\n", reason);
> > + cpu_dump_state(cs, logfile, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> > + qemu_log_unlock(logfile);
> > + }
> > + }
> > +}
> > +
> > #if defined(TARGET_PPC64)
> > static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
> > target_ulong *msr)
> > @@ -406,21 +425,9 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu,
> > target_ulong vector,
> >
> > static void powerpc_mcheck_test_and_checkstop(CPUPPCState *env)
> > {
> > - CPUState *cs = env_cpu(env);
> > -
> > - if (FIELD_EX64(env->msr, MSR, ME)) {
> > - return;
> > - }
> > -
> > - /* Machine check exception is not enabled. Enter checkstop state. */
> > - fprintf(stderr, "Machine check while not allowed. "
> > - "Entering checkstop state\n");
> > - if (qemu_log_separate()) {
> > - qemu_log("Machine check while not allowed. "
> > - "Entering checkstop state\n");
> > + if (!FIELD_EX64(env->msr, MSR, ME)) {
> > + powerpc_checkstop(env_archcpu(env), "machine check with
> > MSR[ME]=0");
>
> I don't mind you twaeking the patch and renaming the function but now this
> has become another one line function which just clutters code. Either keep
> this together in one function or inline the if at callers, otherwise this
> will start to look like Forth where every simple operation gets a new
> name. :-)
Yeah good point. I did want to have a powerpc_checkstop function with a
reason because other places might start to also call it in future.
As far as the machine check ME test goes... we could re-inline that I
suppose.
Thanks,
Nick
