Re: [Qemu-devel] [PATCH v2] s390x/cpu: expose the guest crash information

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3053 bytes --]

On 09/19/2017 02:43 AM, Christian Borntraeger wrote:
> From: Jing Liu <liujbjl@linux.vnet.ibm.com>
> 
> 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 the these ways with examples as follows.
> 

> 
> Signed-off-by: Jing Liu <liujbjl@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> [minor fixes due to upstream feedback]
> ---
> V1->V2:
> 	- rename kvm-s390 to s390 in all places
> 	- add "loop" to the crash reasons where appropriate
> 	- use "-" instead of "_" for qapi
> 
>  qapi/run-state.json | 19 ++++++++++++++++--
>  target/s390x/cpu.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  target/s390x/cpu.h  |  6 ++++++
>  target/s390x/kvm.c  | 29 +++++++++++++++++++++------
>  vl.c                |  6 ++++++
>  5 files changed, 109 insertions(+), 8 deletions(-)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index d36ff49..4567510 100644
> --- a/qapi/run-state.json

> +
> +##
> +# @GuestPanicInformationS390:
> +#
> +# S390 specific guest panic information (PSW)
> +#
> +# Since: 2.11
> +##
> +{'struct': 'GuestPanicInformationS390',
> + 'data': { 'psw-mask': 'uint64',
> +           'psw-addr': 'uint64',
> +           'reason': 'str' } }

Missing documentation of the three fields; in particular, whether
'reason' is for human consumption only (presumably the case) rather than
for machine parsing.


> +    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 (cs->exception_index) {
> +    case EXCP_CRASH_PGM:
> +        panic_info->u.s390.reason = g_strdup("program interrupt loop");
> +        break;
> +    case EXCP_CRASH_EXT:
> +        panic_info->u.s390.reason = g_strdup("external interrupt loop");
> +        break;
> +    case EXCP_CRASH_WAITPSW:
> +        panic_info->u.s390.reason = g_strdup("disabled wait");
> +        break;
> +    case EXCP_CRASH_OPEREXC:
> +        panic_info->u.s390.reason = g_strdup("operation exception loop");
> +        break;
> +    default:
> +        panic_info->u.s390.reason = g_strdup("unknown crash reason");
> +        break;

Is it worth a QAPI enum type to expose the reason as one of a finite set
of known strings, or is that information not needed beyond the
human-only string that you are setting here?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]