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

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(-)

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. :-)

Regards,
BALATON Zoltan

>     }
> -    cs->halted = 1;
> -    cpu_interrupt_exittb(cs);
> }
>
> static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)
>

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