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

[QingFeng Hao <haoqf@linux.vnet.ibm.com>]

在 2017/9/19 21:04, Eric Blake 写道:
> 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
I didn't get your point, do you mean we need to add comments
for the three fields? But I don't see the comments for Hyper-V either.
Thanks
> 'reason' is for human consumption only (presumably the case) rather than
> for machine parsing.
yes, the 'reason' is for human understanding and also investigation but 
psw-*
could be used to parse later.
>
>> +    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?
Actually we had considered to use enum type for 'reason' instead of str, 
the awkwardness
is that then qemu_system_guest_panicked has to parse the 'reason' before 
it prints it, but
qemu_system_guest_panicked is a common function and we don't want to do 
more arch
related handling there.
Thanks!
>

-- 
Regards
QingFeng Hao