Re: [PATCH 4/4] target/ppc: Make checkstop stop the system

[Nicholas Piggin]

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 <npiggin@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