Message ID | 20230703120340.45349-1-npiggin@gmail.com |
---|---|
State | New |
Headers | show |
Series | [v3] target/ppc: Make checkstop actually stop the system | expand |
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) >
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) >>
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
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(-)
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
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
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
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)
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(-)