| Message ID | 20180202143746.204851-2-borntraeger@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | respin of s390 crash information | expand |
patchew complained, so something like this on top
diff --git a/qapi/run-state.json b/qapi/run-state.json
index a93f6fea5c..a148088252 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -328,21 +328,19 @@
##
{ 'enum': 'GuestPanicInformationType',
'data': [ 'hyper-v', 's390' ] }
##
# @GuestPanicInformation:
#
# Information about a guest panic
#
-# @hyper-v: hyper-v guest panic information
-#
-# @s390: s390 guest panic information (Since: 2.12)
+# @type: Crash type that defines the hypervisor specific information
#
# Since: 2.9
##
{'union': 'GuestPanicInformation',
'base': {'type': 'GuestPanicInformationType'},
'discriminator': 'type',
'data': { 'hyper-v': 'GuestPanicInformationHyperV',
's390': 'GuestPanicInformationS390' } }
seems to fix it.
On 02/02/2018 03:51 PM, Eric Blake wrote:
> On 02/02/2018 08:37 AM, Christian Borntraeger wrote:
>> This patch is the s390 implementation of guest crash information,
>> similar to commit d187e08dc4 ("i386/cpu: add crash-information QOM
>> property") and the related commits. We will detect several crash
>> reasons, with the "disabled wait" being the most important one, since
>> this is used by all s390 guests as a "panic like" notification.
>>
>> Demonstrate these ways with examples as follows.
>>
>> 1. crash-information QOM property;
>
>>
>> Co-authored-by: Jing Liu <liujbjl@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> qapi/run-state.json | 29 ++++++++++++++++++++++++--
>
> QAPI changes look reasonable; I'll leave the review of the
> target-specific code to those more familiar with the target.
>
On Fri, 2 Feb 2018 14:37:46 +0000 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > This patch is the s390 implementation of guest crash information, > similar to commit d187e08dc4 ("i386/cpu: add crash-information QOM > property") and the related commits. We will detect several crash > reasons, with the "disabled wait" being the most important one, since > this is used by all s390 guests as a "panic like" notification. > > Demonstrate these ways with examples as follows. > > 1. crash-information QOM property; > > Run qemu with -qmp unix:qmp-sock,server, then use utility "qmp-shell" > to execute "qom-get" command, and might get the result like, > > (QEMU) qom-get path=/machine/cpu[0]/ property=crash-information > {"return": {"psw-addr": 1105350, "psw-mask": 562956395872256, "reason": > "disabled wait", "type": "s390"}} > > 2. GUEST_PANICKED event reporting; > > Run qemu with a socket option, and telnet or nc to that, > -chardev socket,id=qmp,port=4444,host=localhost,server \ > -mon chardev=qmp,mode=control,pretty=on \ > Negotiating the mode by { "execute": "qmp_capabilities" }, and the crash > information will be reported on a guest crash event like, > > { > "timestamp": { > "seconds": 1499931739, > "microseconds": 961296 > }, > "event": "GUEST_PANICKED", > "data": { > "action": "pause", > "info": { > "psw-addr": 1105350, > "reason": "disabled wait", > "psw-mask": 562956395872256, > "type": "s390" > } > } > } > > 3. log; > > Run qemu with the parameters: -D <logfile> -d guest_errors, to > specify the logfile and log item. The results might be, > > Guest crashed > S390 crash parameters: (0x0000100000000000 0x0000000000000006) > S390 crash reason: operation exception loop > > Co-authored-by: Jing Liu <liujbjl@linux.vnet.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > qapi/run-state.json | 29 ++++++++++++++++++++++++-- > target/s390x/cpu.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++ > target/s390x/cpu.h | 10 +++++++++ > target/s390x/helper.c | 5 ++++- > target/s390x/kvm.c | 27 +++++++++++++++++++----- > vl.c | 6 ++++++ > 6 files changed, 126 insertions(+), 8 deletions(-) Generally (and with your fixup folded in), this looks sane. Just some minor nits below. (...) > +## > +# @GuestPanicInformationS390: > +# > +# S390 specific guest panic information (PSW) > +# > +# @psw-mask: control fields of guest PSW > +# @psw-addr: guest instruction address > +# @reason: guest crash reason for human reading "in human readable form"? > +# > +# Since: 2.12 > +## > +{'struct': 'GuestPanicInformationS390', > + 'data': { 'psw-mask': 'uint64', > + 'psw-addr': 'uint64', > + 'reason': 'str' } } > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index d2e6b9f5c7..ac8e963307 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -35,6 +35,8 @@ > #include "qemu/error-report.h" > #include "trace.h" > #include "qapi/visitor.h" > +#include "qapi-visit.h" > +#include "sysemu/hw_accel.h" > #include "exec/exec-all.h" > #include "hw/qdev-properties.h" > #ifndef CONFIG_USER_ONLY > @@ -237,6 +239,58 @@ out: > error_propagate(errp, err); > } > > +static GuestPanicInformation *s390x_cpu_get_crash_info(CPUState *cs) Use s390_ instead of s390x_? That seems to be the more common usage. > +{ > + GuestPanicInformation *panic_info; > + S390CPU *cpu = S390_CPU(cs); > + > + cpu_synchronize_state(cs); > + panic_info = g_malloc0(sizeof(GuestPanicInformation)); > + > + panic_info->type = GUEST_PANIC_INFORMATION_TYPE_S390; > + panic_info->u.s390.psw_mask = cpu->env.psw.mask; > + panic_info->u.s390.psw_addr = cpu->env.psw.addr; > + > + switch (cpu->env.crash_reason) { > + case CRASH_REASON_PGM: > + panic_info->u.s390.reason = g_strdup("program interrupt loop"); > + break; > + case CRASH_REASON_EXT: > + panic_info->u.s390.reason = g_strdup("external interrupt loop"); > + break; > + case CRASH_REASON_WAITPSW: > + panic_info->u.s390.reason = g_strdup("disabled wait"); > + break; > + case CRASH_REASON_OPEREXC: > + panic_info->u.s390.reason = g_strdup("operation exception loop"); > + break; > + default: > + panic_info->u.s390.reason = g_strdup("unknown crash reason"); > + break; > + } You're doing the crash_reason -> reason mapping here and also below. Maybe introduce a helper for it? > + > + return panic_info; > +} > + > +static void s390x_cpu_get_crash_info_qom(Object *obj, Visitor *v, > + const char *name, void *opaque, > + Error **errp) > +{ > + CPUState *cs = CPU(obj); > + GuestPanicInformation *panic_info; > + > + if (!cs->crash_occurred) { > + error_setg(errp, "No crash occured"); > + return; > + } > + > + panic_info = s390x_cpu_get_crash_info(cs); > + > + visit_type_GuestPanicInformation(v, "crash-information", &panic_info, > + errp); > + qapi_free_GuestPanicInformation(panic_info); > +} > + > static void s390_cpu_initfn(Object *obj) > { > CPUState *cs = CPU(obj); (...) > diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > index 8736001156..c6a23262a8 100644 > --- a/target/s390x/kvm.c > +++ b/target/s390x/kvm.c > @@ -1568,15 +1568,32 @@ static int handle_instruction(S390CPU *cpu, struct kvm_run *run) > return r; > } > > -static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset) > +static void unmanageable_intercept(S390CPU *cpu, enum crash_reasons reason, > + int pswoffset) > { > CPUState *cs = CPU(cpu); > + const char *str; > > + switch (reason) { > + case CRASH_REASON_PGM: > + str = "program interrupt loop"; > + break; > + case CRASH_REASON_EXT: > + str = "external interrupt loop"; > + break; > + case CRASH_REASON_OPEREXC: > + str = "operation exception loop"; > + break; > + default: > + str = "unknown crash reason"; > + break; > + } > error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx", "Unmanageable unknown crash reason!" looks a bit odd. In this case, "Unmanageable intercept!" would actually look a bit saner (but you would not be able to use a common converter in that case). We can also just simply keep it :) > str, cs->cpu_index, ldq_phys(cs->as, cpu->env.psa + pswoffset), > ldq_phys(cs->as, cpu->env.psa + pswoffset + 8)); > s390_cpu_halt(cpu); > - qemu_system_guest_panicked(NULL); > + cpu->env.crash_reason = reason; > + qemu_system_guest_panicked(cpu_get_crash_info(cs)); > } > > /* try to detect pgm check loops */
On 02/05/2018 01:04 PM, Cornelia Huck wrote: > You're doing the crash_reason -> reason mapping here and also below. > Maybe introduce a helper for it? > [....] >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c >> index 8736001156..c6a23262a8 100644 >> --- a/target/s390x/kvm.c >> +++ b/target/s390x/kvm.c >> @@ -1568,15 +1568,32 @@ static int handle_instruction(S390CPU *cpu, struct kvm_run *run) >> return r; >> } >> >> -static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset) >> +static void unmanageable_intercept(S390CPU *cpu, enum crash_reasons reason, >> + int pswoffset) >> { >> CPUState *cs = CPU(cpu); >> + const char *str; >> >> + switch (reason) { >> + case CRASH_REASON_PGM: >> + str = "program interrupt loop"; >> + break; >> + case CRASH_REASON_EXT: >> + str = "external interrupt loop"; >> + break; >> + case CRASH_REASON_OPEREXC: >> + str = "operation exception loop"; >> + break; >> + default: >> + str = "unknown crash reason"; >> + break; >> + } >> error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx", > > "Unmanageable unknown crash reason!" looks a bit odd. In this case, > "Unmanageable intercept!" would actually look a bit saner (but you > would not be able to use a common converter in that case). We can also > just simply keep it :) We could maybe just drop this print in kvm.c. qemu_system_guest_panicked below will trigger some logging as well (if enabled) and it will also notify libvirt about that. a future libvirt code will print something like panic s390: psw-mask='0x0000000000000000', psw-addr='0x0000000000000002', crash reason: operation exception loop anyway in the log file. That would also address your concern from above.
On Mon, 5 Feb 2018 14:44:36 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 02/05/2018 01:04 PM, Cornelia Huck wrote: > > > You're doing the crash_reason -> reason mapping here and also below. > > Maybe introduce a helper for it? > > > [....] > >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c > >> index 8736001156..c6a23262a8 100644 > >> --- a/target/s390x/kvm.c > >> +++ b/target/s390x/kvm.c > >> @@ -1568,15 +1568,32 @@ static int handle_instruction(S390CPU *cpu, struct kvm_run *run) > >> return r; > >> } > >> > >> -static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset) > >> +static void unmanageable_intercept(S390CPU *cpu, enum crash_reasons reason, > >> + int pswoffset) > >> { > >> CPUState *cs = CPU(cpu); > >> + const char *str; > >> > >> + switch (reason) { > >> + case CRASH_REASON_PGM: > >> + str = "program interrupt loop"; > >> + break; > >> + case CRASH_REASON_EXT: > >> + str = "external interrupt loop"; > >> + break; > >> + case CRASH_REASON_OPEREXC: > >> + str = "operation exception loop"; > >> + break; > >> + default: > >> + str = "unknown crash reason"; > >> + break; > >> + } > >> error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx", > > > > "Unmanageable unknown crash reason!" looks a bit odd. In this case, > > "Unmanageable intercept!" would actually look a bit saner (but you > > would not be able to use a common converter in that case). We can also > > just simply keep it :) > > We could maybe just drop this print in kvm.c. qemu_system_guest_panicked below will > trigger some logging as well (if enabled) and it will also notify libvirt about > that. a future libvirt code will print something like > panic s390: psw-mask='0x0000000000000000', psw-addr='0x0000000000000002', crash reason: operation exception loop > anyway in the log file. > > That would also address your concern from above. > Yes, that would also work if we do some program check loop detection in tcg in the future.
diff --git a/qapi/run-state.json b/qapi/run-state.json index bca46a8785..a93f6fea5c 100644 --- a/qapi/run-state.json +++ b/qapi/run-state.json @@ -320,22 +320,31 @@ # # An enumeration of the guest panic information types # +# @hyper-v: hyper-v guest panic information type +# +# @s390: s390 guest panic information type (Since: 2.12) +# # Since: 2.9 ## { 'enum': 'GuestPanicInformationType', - 'data': [ 'hyper-v'] } + 'data': [ 'hyper-v', 's390' ] } ## # @GuestPanicInformation: # # Information about a guest panic # +# @hyper-v: hyper-v guest panic information +# +# @s390: s390 guest panic information (Since: 2.12) +# # Since: 2.9 ## {'union': 'GuestPanicInformation', 'base': {'type': 'GuestPanicInformationType'}, 'discriminator': 'type', - 'data': { 'hyper-v': 'GuestPanicInformationHyperV' } } + 'data': { 'hyper-v': 'GuestPanicInformationHyperV', + 's390': 'GuestPanicInformationS390' } } ## # @GuestPanicInformationHyperV: @@ -350,3 +359,19 @@ 'arg3': 'uint64', 'arg4': 'uint64', 'arg5': 'uint64' } } + +## +# @GuestPanicInformationS390: +# +# S390 specific guest panic information (PSW) +# +# @psw-mask: control fields of guest PSW +# @psw-addr: guest instruction address +# @reason: guest crash reason for human reading +# +# Since: 2.12 +## +{'struct': 'GuestPanicInformationS390', + 'data': { 'psw-mask': 'uint64', + 'psw-addr': 'uint64', + 'reason': 'str' } } diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index d2e6b9f5c7..ac8e963307 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -35,6 +35,8 @@ #include "qemu/error-report.h" #include "trace.h" #include "qapi/visitor.h" +#include "qapi-visit.h" +#include "sysemu/hw_accel.h" #include "exec/exec-all.h" #include "hw/qdev-properties.h" #ifndef CONFIG_USER_ONLY @@ -237,6 +239,58 @@ out: error_propagate(errp, err); } +static GuestPanicInformation *s390x_cpu_get_crash_info(CPUState *cs) +{ + GuestPanicInformation *panic_info; + S390CPU *cpu = S390_CPU(cs); + + cpu_synchronize_state(cs); + panic_info = g_malloc0(sizeof(GuestPanicInformation)); + + panic_info->type = GUEST_PANIC_INFORMATION_TYPE_S390; + panic_info->u.s390.psw_mask = cpu->env.psw.mask; + panic_info->u.s390.psw_addr = cpu->env.psw.addr; + + switch (cpu->env.crash_reason) { + case CRASH_REASON_PGM: + panic_info->u.s390.reason = g_strdup("program interrupt loop"); + break; + case CRASH_REASON_EXT: + panic_info->u.s390.reason = g_strdup("external interrupt loop"); + break; + case CRASH_REASON_WAITPSW: + panic_info->u.s390.reason = g_strdup("disabled wait"); + break; + case CRASH_REASON_OPEREXC: + panic_info->u.s390.reason = g_strdup("operation exception loop"); + break; + default: + panic_info->u.s390.reason = g_strdup("unknown crash reason"); + break; + } + + return panic_info; +} + +static void s390x_cpu_get_crash_info_qom(Object *obj, Visitor *v, + const char *name, void *opaque, + Error **errp) +{ + CPUState *cs = CPU(obj); + GuestPanicInformation *panic_info; + + if (!cs->crash_occurred) { + error_setg(errp, "No crash occured"); + return; + } + + panic_info = s390x_cpu_get_crash_info(cs); + + visit_type_GuestPanicInformation(v, "crash-information", &panic_info, + errp); + qapi_free_GuestPanicInformation(panic_info); +} + static void s390_cpu_initfn(Object *obj) { CPUState *cs = CPU(obj); @@ -249,6 +303,8 @@ static void s390_cpu_initfn(Object *obj) cs->env_ptr = env; cs->halted = 1; cs->exception_index = EXCP_HLT; + object_property_add(obj, "crash-information", "GuestPanicInformation", + s390x_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL); s390_cpu_model_register_props(obj); #if !defined(CONFIG_USER_ONLY) qemu_get_timedate(&tm, 0); @@ -482,6 +538,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) cc->do_interrupt = s390_cpu_do_interrupt; #endif cc->dump_state = s390_cpu_dump_state; + cc->get_crash_info = s390x_cpu_get_crash_info; cc->set_pc = s390_cpu_set_pc; cc->gdb_read_register = s390_cpu_gdb_read_register; cc->gdb_write_register = s390_cpu_gdb_write_register; diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h index a1123ad621..71414cc324 100644 --- a/target/s390x/cpu.h +++ b/target/s390x/cpu.h @@ -77,6 +77,14 @@ typedef struct MchkQueue { uint16_t type; } MchkQueue; +/* Crash cases. */ +enum crash_reasons { + CRASH_REASON_PGM, + CRASH_REASON_EXT, + CRASH_REASON_WAITPSW, + CRASH_REASON_OPEREXC, +}; + struct CPUS390XState { uint64_t regs[16]; /* GP registers */ /* @@ -102,6 +110,8 @@ struct CPUS390XState { PSW psw; + enum crash_reasons crash_reason; + uint64_t cc_src; uint64_t cc_dst; uint64_t cc_vr; diff --git a/target/s390x/helper.c b/target/s390x/helper.c index 35d9741918..56c3f13686 100644 --- a/target/s390x/helper.c +++ b/target/s390x/helper.c @@ -84,12 +84,15 @@ static inline bool is_special_wait_psw(uint64_t psw_addr) void s390_handle_wait(S390CPU *cpu) { + CPUState *cs = CPU(cpu); + if (s390_cpu_halt(cpu) == 0) { #ifndef CONFIG_USER_ONLY if (is_special_wait_psw(cpu->env.psw.addr)) { qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN); } else { - qemu_system_guest_panicked(NULL); + cpu->env.crash_reason = CRASH_REASON_WAITPSW; + qemu_system_guest_panicked(cpu_get_crash_info(cs)); } #endif } diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c index 8736001156..c6a23262a8 100644 --- a/target/s390x/kvm.c +++ b/target/s390x/kvm.c @@ -1568,15 +1568,32 @@ static int handle_instruction(S390CPU *cpu, struct kvm_run *run) return r; } -static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset) +static void unmanageable_intercept(S390CPU *cpu, enum crash_reasons reason, + int pswoffset) { CPUState *cs = CPU(cpu); + const char *str; + switch (reason) { + case CRASH_REASON_PGM: + str = "program interrupt loop"; + break; + case CRASH_REASON_EXT: + str = "external interrupt loop"; + break; + case CRASH_REASON_OPEREXC: + str = "operation exception loop"; + break; + default: + str = "unknown crash reason"; + break; + } error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx", str, cs->cpu_index, ldq_phys(cs->as, cpu->env.psa + pswoffset), ldq_phys(cs->as, cpu->env.psa + pswoffset + 8)); s390_cpu_halt(cpu); - qemu_system_guest_panicked(NULL); + cpu->env.crash_reason = reason; + qemu_system_guest_panicked(cpu_get_crash_info(cs)); } /* try to detect pgm check loops */ @@ -1606,7 +1623,7 @@ static int handle_oper_loop(S390CPU *cpu, struct kvm_run *run) !(oldpsw.mask & PSW_MASK_PSTATE) && (newpsw.mask & PSW_MASK_ASC) == (oldpsw.mask & PSW_MASK_ASC) && (newpsw.mask & PSW_MASK_DAT) == (oldpsw.mask & PSW_MASK_DAT)) { - unmanageable_intercept(cpu, "operation exception loop", + unmanageable_intercept(cpu, CRASH_REASON_OPEREXC, offsetof(LowCore, program_new_psw)); return EXCP_HALTED; } @@ -1627,12 +1644,12 @@ static int handle_intercept(S390CPU *cpu) r = handle_instruction(cpu, run); break; case ICPT_PROGRAM: - unmanageable_intercept(cpu, "program interrupt", + unmanageable_intercept(cpu, CRASH_REASON_PGM, offsetof(LowCore, program_new_psw)); r = EXCP_HALTED; break; case ICPT_EXT_INT: - unmanageable_intercept(cpu, "external interrupt", + unmanageable_intercept(cpu, CRASH_REASON_EXT, offsetof(LowCore, external_new_psw)); r = EXCP_HALTED; break; diff --git a/vl.c b/vl.c index e517a8d995..74ea60f248 100644 --- a/vl.c +++ b/vl.c @@ -1761,6 +1761,12 @@ void qemu_system_guest_panicked(GuestPanicInformation *info) info->u.hyper_v.arg3, info->u.hyper_v.arg4, info->u.hyper_v.arg5); + } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_S390) { + qemu_log_mask(LOG_GUEST_ERROR, "S390 crash parameters: (0x%016" + PRIx64" 0x%016" PRIx64")\nS390 crash reason: %s\n", + info->u.s390.psw_mask, + info->u.s390.psw_addr, + info->u.s390.reason); } qapi_free_GuestPanicInformation(info); }
This patch is the s390 implementation of guest crash information, similar to commit d187e08dc4 ("i386/cpu: add crash-information QOM property") and the related commits. We will detect several crash reasons, with the "disabled wait" being the most important one, since this is used by all s390 guests as a "panic like" notification. Demonstrate these ways with examples as follows. 1. crash-information QOM property; Run qemu with -qmp unix:qmp-sock,server, then use utility "qmp-shell" to execute "qom-get" command, and might get the result like, (QEMU) qom-get path=/machine/cpu[0]/ property=crash-information {"return": {"psw-addr": 1105350, "psw-mask": 562956395872256, "reason": "disabled wait", "type": "s390"}} 2. GUEST_PANICKED event reporting; Run qemu with a socket option, and telnet or nc to that, -chardev socket,id=qmp,port=4444,host=localhost,server \ -mon chardev=qmp,mode=control,pretty=on \ Negotiating the mode by { "execute": "qmp_capabilities" }, and the crash information will be reported on a guest crash event like, { "timestamp": { "seconds": 1499931739, "microseconds": 961296 }, "event": "GUEST_PANICKED", "data": { "action": "pause", "info": { "psw-addr": 1105350, "reason": "disabled wait", "psw-mask": 562956395872256, "type": "s390" } } } 3. log; Run qemu with the parameters: -D <logfile> -d guest_errors, to specify the logfile and log item. The results might be, Guest crashed S390 crash parameters: (0x0000100000000000 0x0000000000000006) S390 crash reason: operation exception loop Co-authored-by: Jing Liu <liujbjl@linux.vnet.ibm.com> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- qapi/run-state.json | 29 ++++++++++++++++++++++++-- target/s390x/cpu.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++ target/s390x/cpu.h | 10 +++++++++ target/s390x/helper.c | 5 ++++- target/s390x/kvm.c | 27 +++++++++++++++++++----- vl.c | 6 ++++++ 6 files changed, 126 insertions(+), 8 deletions(-)