Re: [qemu-s390x] [PATCH v7] s390x/cpu: expose the guest crash information

[Christian Borntraeger]

On 02/08/2018 03:20 PM, Eric Blake wrote:
> On 02/08/2018 03:57 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.
>>
> 
>> Co-authored-by: Jing Liu <address@hidden>
>> Signed-off-by: Christian Borntraeger <address@hidden>
>> ---
> 
>> +##
>> +# @GuestPanicInformationS390:
>> +#
>> +# S390 specific guest panic information (PSW)
>> +#
>> +# @core: core id of the CPU that crashed
>> +# @psw-mask: control fields of guest PSW
>> +# @psw-addr: guest instruction address
>> +# @reason: guest crash reason in human readable form
>> +#
>> +# Since: 2.12
>> +##
>> +{'struct': 'GuestPanicInformationS390',
>> + 'data': { 'core': 'uint32',
> 
> Should core be optional,...
> 
> 
>> +static GuestPanicInformation *s390_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;
>> +#if !defined(CONFIG_USER_ONLY)
>> +    panic_info->u.s390.core = cpu->env.core_id;
>> +#endif
> 
> ...given that it is only conditionally assigned?  If so, you'd also need to 
> set panic_info->u.s390.has_core when you have a core id to expose.

Can we simply set it to 0 here and keep it mandatory? After all for
linux-user the qapi interface really makes not a lot of sense.


> 
> 
>> +
>> +static void s390_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");
> 
> s/occured/occurred/
> 
yes