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

[Daniel Henrique Barboza <danielhb413@gmail.com>]

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