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

[BALATON Zoltan]

On Wed, 28 Jun 2023, Nicholas Piggin wrote:
> On Wed Jun 28, 2023 at 3:38 AM AEST, BALATON Zoltan wrote:
> > 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.
>
> I just followed existing fprintf use. Changing that should be separate
> patch indeed.

As this is the only printf here maybe cleaning it up should come before 
adding more.

> > > +            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?
>
> No, it separates the logging block from function.
>
> > > +    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?
>
> To make the checkstop function more general (e.g., used by the next patch).

OK, I did look where it's needed but missed that line in the next patch. 
That's not practical to check in the checkstop function then.

Regards,
BALATON Zoltan