Re: [Qemu-devel] [PATCH v3 1/1] s390x/cpu: expose the guest crash information
On Mon, 5 Feb 2018 14:44:36 +0100
Christian Borntraeger 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='0x', psw-addr='0x0002',
> 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.
Re: [Qemu-devel] [PATCH v3 1/1] s390x/cpu: expose the guest crash information
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='0x', psw-addr='0x0002', crash
reason: operation exception loop
anyway in the log file.
That would also address your concern from above.
Re: [Qemu-devel] [PATCH v3 1/1] s390x/cpu: expose the guest crash information
On Fri, 2 Feb 2018 14:37:46 +
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;
>
> 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=,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 -d guest_errors, to
> specify the logfile and log item. The results might be,
>
> Guest crashed
> S390 crash parameters: (0x1000 0x0006)
> S390 crash reason: operation exception loop
>
> Co-authored-by: Jing Liu
> Signed-off-by: Christian Borntraeger
> ---
> 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");
> +ret
Re: [Qemu-devel] [PATCH v3 1/1] s390x/cpu: expose the guest crash information
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
>> Signed-off-by: Christian Borntraeger
>> ---
>> 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.
>
Re: [Qemu-devel] [PATCH v3 1/1] s390x/cpu: expose the guest crash information
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
> Signed-off-by: Christian Borntraeger
> ---
> 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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
