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

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

[Nicholas Piggin]

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

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

[BALATON Zoltan]

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.

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

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

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

[Nicholas Piggin]

On Mon Jul 3, 2023 at 10:26 PM AEST, 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.

It is based on ppc-next unless I've done something silly. Is this
the patch you were talking about?

https://gitlab.com/danielhb/qemu/-/commit/26d089ac20080066061ed61fb58a5411e275e191

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

Things are getting a little confusing, but I think what Daniel has
is okay. Were were talking about changing checkstop with my patch,
but no big deal to take yours first, I've reworked things.

Thanks,
Nick

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

[Philippe Mathieu-Daudé]

On 3/7/23 14:03, 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>

<here ...>

> 
> Since v1:
> - Fix loop exit so it stops on the checkstop-causing instruction, rather than
>    after it.
> 
> Since v2:
> - Rebase on ppc-next.

[*]

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

... goes this line, so the version changes are stripped out
when the patch is applied (usually they are not useful in the
git history).

>   target/ppc/excp_helper.c | 26 ++++++++++++++++++--------
>   1 file changed, 18 insertions(+), 8 deletions(-)

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

[BALATON Zoltan]

On Mon, 3 Jul 2023, Nicholas Piggin wrote:
> On Mon Jul 3, 2023 at 10:26 PM AEST, 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.
>
> It is based on ppc-next unless I've done something silly. Is this
> the patch you were talking about?
>
> https://gitlab.com/danielhb/qemu/-/commit/26d089ac20080066061ed61fb58a5411e275e191
>
>> 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.
>
> Things are getting a little confusing, but I think what Daniel has
> is okay. Were were talking about changing checkstop with my patch,
> but no big deal to take yours first, I've reworked things.

Sorry my bad. Yes it's getting confusing to me and missed that Daniel 
already merged that patch on ppc-next, I thought it was one of the patches 
not yet merged. So this can go on top then.

There was another change for fixing sc that may clash that I'm not sure 
if I need to do anything about.

Regards,
BALATON Zoltan

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

[Nicholas Piggin]

On Tue Jul 4, 2023 at 1:06 AM AEST, BALATON Zoltan wrote:
> On Mon, 3 Jul 2023, Nicholas Piggin wrote:
> > On Mon Jul 3, 2023 at 10:26 PM AEST, 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.
> >
> > It is based on ppc-next unless I've done something silly. Is this
> > the patch you were talking about?
> >
> > https://gitlab.com/danielhb/qemu/-/commit/26d089ac20080066061ed61fb58a5411e275e191
> >
> >> 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.
> >
> > Things are getting a little confusing, but I think what Daniel has
> > is okay. Were were talking about changing checkstop with my patch,
> > but no big deal to take yours first, I've reworked things.
>
> Sorry my bad. Yes it's getting confusing to me and missed that Daniel 
> already merged that patch on ppc-next, I thought it was one of the patches 
> not yet merged. So this can go on top then.
>
> There was another change for fixing sc that may clash that I'm not sure 
> if I need to do anything about.

The sc patches still had the problem of changing the syscall dump to
the instruction after syscall IIRC. So that would need to be fixed.
But that does make me a bit less happy about them too... Anyway
resend with fix and rebase if you want  them.

Thanks,
Nick

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

[BALATON Zoltan]

On Wed, 5 Jul 2023, Nicholas Piggin wrote:
> On Tue Jul 4, 2023 at 1:06 AM AEST, BALATON Zoltan wrote:
>> On Mon, 3 Jul 2023, Nicholas Piggin wrote:
>>> On Mon Jul 3, 2023 at 10:26 PM AEST, 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.
>>>
>>> It is based on ppc-next unless I've done something silly. Is this
>>> the patch you were talking about?
>>>
>>> https://gitlab.com/danielhb/qemu/-/commit/26d089ac20080066061ed61fb58a5411e275e191
>>>
>>>> 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.
>>>
>>> Things are getting a little confusing, but I think what Daniel has
>>> is okay. Were were talking about changing checkstop with my patch,
>>> but no big deal to take yours first, I've reworked things.
>>
>> Sorry my bad. Yes it's getting confusing to me and missed that Daniel
>> already merged that patch on ppc-next, I thought it was one of the patches
>> not yet merged. So this can go on top then.
>>
>> There was another change for fixing sc that may clash that I'm not sure
>> if I need to do anything about.
>
> The sc patches still had the problem of changing the syscall dump to
> the instruction after syscall IIRC. So that would need to be fixed.
> But that does make me a bit less happy about them too... Anyway
> resend with fix and rebase if you want  them.

I won't have time now to change this series so I gave on these excp_helper 
patches for now. I'll wait for your patches to go in first then I'll 
rebase the remaining ones on that in next devel cycle. What's alredy 
merged for ppc440 and pegasos2 is enough for the next release or maybe if 
the last 3 patches from the ppc440 series renaming host bridge type names 
could go in that would be all from me at the moment. Thank you.

Regards,
BALATON Zoltan