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

[Daniel Henrique Barboza]

On 7/3/23 09:26, BALATON Zoltan wrote:
> On Mon, 3 Jul 2023, Nicholas Piggin wrote:
> > checkstop state does not halt the system, interrupts continue to be
> > serviced, and other CPUs run. Stop the machine with
> > qemu_system_guest_panicked.
> >
> > Change the logging not to print separately to stderr because a
> > checkstop is a guest error (or perhaps a simulated machine error)
> > rather than a QEMU error. CPU registers are dumped.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >
> > Since v1:
> > - Fix loop exit so it stops on the checkstop-causing instruction, rather than
> >  after it.
> >
> > Since v2:
> > - Rebase on ppc-next.
>
> Is this really based on ppc-next or on my series or another patch from you? I 
> think the patch from my series that introduces the checksrop function that this 
> patch is changing is not yet in ppc-next so this may not apply there. I think 
> you've posted an alternative to the patch moving checkstop handling to a 
> function and the sc patch which may clash with the not yet merged parts in my 
> series but i could not follow all these patches. I'm not sure Daniel could so 
> maybe you could send it as a series to include all patches you want to add or 
> state what it's based on.

What patch are we talking about?

If the patch is built on top of something that isn't on master or ppc-next you 
can add
a note in the cover-letter mentioning it and I'll grab both. Thanks,


Daniel


> Regards,
> BALATON Zoltan
>
> > - Use qemu_system_guest_panicked rather than vm_stop (Richard)
> > - Move away from printing to stderr (Zoltan)
> > - Reduce changes to log messages.
> > - Split out from larger series since it's independent (will skip attn
> >  instruction for now).
> > ---
> > target/ppc/excp_helper.c | 26 ++++++++++++++++++--------
> > 1 file changed, 18 insertions(+), 8 deletions(-)
> >
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index e49e13a30d..a588285ef1 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"
> > @@ -427,20 +428,29 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, 
> > target_ulong vector,
> > static void powerpc_mcheck_checkstop(CPUPPCState *env)
> > {
> >     CPUState *cs = env_cpu(env);
> > +    FILE *f;
> >
> >     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");
> > +    /*
> > +     * This stops the machine and logs CPU state without killing QEMU
> > +     * (like cpu_abort()) so the machine can still be debugged (because
> > +     * it is often a guest error).
> > +     */
> > +
> > +    f = qemu_log_trylock();
> > +    if (f) {
> > +        fprintf(f, "Machine check while not allowed. "
> > +                "Entering checkstop state.\n");
> > +        cpu_dump_state(cs, f, CPU_DUMP_FPU | CPU_DUMP_CCOP);
> > +        qemu_log_unlock(f);
> >     }
> > -    cs->halted = 1;
> > -    cpu_interrupt_exittb(cs);
> > +
> > +    qemu_system_guest_panicked(NULL);
> > +
> > +    cpu_loop_exit_noexc(cs);
> > }
> >
> > static void powerpc_excp_40x(PowerPCCPU *cpu, int excp)