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

[BALATON Zoltan]

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)