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

[BALATON Zoltan]

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

> +            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?

> +    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?

Regards,
BALATON Zoltan

>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -602,7 +608,7 @@ static void powerpc_excp_6xx(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");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -763,7 +769,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
>     switch (excp) {
>     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");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -936,7 +942,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
>     switch (excp) {
>     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");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -1119,7 +1125,7 @@ static void powerpc_excp_booke(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");
>         }
>         /* machine check exceptions don't have ME set */
>         new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -1424,7 +1430,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>     switch (excp) {
>     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 (env->msr_mask & MSR_HVB) {
>             /*