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

[QingFeng Hao]

在 2017/9/19 21:04, Eric Blake 写道:
> On 09/19/2017 02:43 AM, Christian Borntraeger wrote:
> > From: Jing Liu <address@hidden>
> >
> > 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 <address@hidden>
> > Reviewed-by: Christian Borntraeger <address@hidden>
> > Signed-off-by: Christian Borntraeger <address@hidden>
> > [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