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

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

[Christian Borntraeger]

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.

  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=4444,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 <logfile> -d guest_errors, to
  specify the logfile and log item. The results might be,

  >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
  Guest crashed
  S390 crash parameters: (0x2000180000000 0x10ddc6)
  S390 crash reason: disabled wait
  >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>

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
+++ b/qapi/run-state.json
@@ -320,10 +320,12 @@
 #
 # An enumeration of the guest panic information types
 #
+# @s390: s390 guest panic information type (Since: 2.11)
+#
 # Since: 2.9
 ##
 { 'enum': 'GuestPanicInformationType',
-  'data': [ 'hyper-v'] }
+  'data': [ 'hyper-v', 's390' ] }
 
 ##
 # @GuestPanicInformation:
@@ -335,7 +337,8 @@
 {'union': 'GuestPanicInformation',
  'base': {'type': 'GuestPanicInformationType'},
  'discriminator': 'type',
- 'data': { 'hyper-v': 'GuestPanicInformationHyperV' } }
+ 'data': { 'hyper-v': 'GuestPanicInformationHyperV',
+           's390': 'GuestPanicInformationS390' } }
 
 ##
 # @GuestPanicInformationHyperV:
@@ -350,3 +353,15 @@
            'arg3': 'uint64',
            'arg4': 'uint64',
            'arg5': 'uint64' } }
+
+##
+# @GuestPanicInformationS390:
+#
+# S390 specific guest panic information (PSW)
+#
+# Since: 2.11
+##
+{'struct': 'GuestPanicInformationS390',
+ 'data': { 'psw-mask': 'uint64',
+           'psw-addr': 'uint64',
+           'reason': 'str' } }
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 74b3e4f..5b835fe 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"
 #ifndef CONFIG_USER_ONLY
 #include "hw/hw.h"
@@ -276,6 +278,58 @@ static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
     cpu->id = value;
 }
 
+static GuestPanicInformation *s390x_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;
+    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;
+    }
+
+    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");
+        return;
+    }
+
+    panic_info = s390x_cpu_get_crash_info(cs);
+
+    visit_type_GuestPanicInformation(v, "crash-information", &panic_info,
+                                     errp);
+    qapi_free_GuestPanicInformation(panic_info);
+}
+
 static void s390_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -291,6 +345,8 @@ static void s390_cpu_initfn(Object *obj)
     cs->exception_index = EXCP_HLT;
     object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
                         s390x_cpu_set_id, NULL, NULL, NULL);
+    object_property_add(obj, "crash-information", "GuestPanicInformation",
+                        s390x_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
     s390_cpu_model_register_props(obj);
 #if !defined(CONFIG_USER_ONLY)
     qemu_get_timedate(&tm, 0);
@@ -517,6 +573,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
     cc->do_interrupt = s390_cpu_do_interrupt;
 #endif
     cc->dump_state = s390_cpu_dump_state;
+    cc->get_crash_info = s390x_cpu_get_crash_info;
     cc->set_pc = s390_cpu_set_pc;
     cc->gdb_read_register = s390_cpu_gdb_read_register;
     cc->gdb_write_register = s390_cpu_gdb_write_register;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 0bd97a5..dba32db 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -396,6 +396,12 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
 #define EXCP_IO  7 /* I/O interrupt */
 #define EXCP_MCHK 8 /* machine check */
 
+/* Crash cases. */
+#define EXCP_CRASH_PGM 9
+#define EXCP_CRASH_EXT 10
+#define EXCP_CRASH_WAITPSW 11
+#define EXCP_CRASH_OPEREXC 12
+
 #define INTERRUPT_EXT        (1 << 0)
 #define INTERRUPT_TOD        (1 << 1)
 #define INTERRUPT_CPUTIMER   (1 << 2)
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index d07763f..77a5862 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1941,15 +1941,31 @@ static bool is_special_wait_psw(CPUState *cs)
     return cs->kvm_run->psw_addr == 0xfffUL;
 }
 
-static void unmanageable_intercept(S390CPU *cpu, const char *str, int pswoffset)
+static void unmanageable_intercept(S390CPU *cpu, int32_t reason, int pswoffset)
 {
     CPUState *cs = CPU(cpu);
+    const char *str;
 
+    switch (reason) {
+    case EXCP_CRASH_PGM:
+        str = "program interrupt loop";
+        break;
+    case EXCP_CRASH_EXT:
+        str = "external interrupt loop";
+        break;
+    case EXCP_CRASH_OPEREXC:
+        str = "operation exception loop";
+        break;
+    default:
+        str = "unknown crash reason";
+        break;
+    }
     error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx",
                  str, cs->cpu_index, ldq_phys(cs->as, cpu->env.psa + pswoffset),
                  ldq_phys(cs->as, cpu->env.psa + pswoffset + 8));
     s390_cpu_halt(cpu);
-    qemu_system_guest_panicked(NULL);
+    cs->exception_index = reason;
+    qemu_system_guest_panicked(cpu_get_crash_info(cs));
 }
 
 /* try to detect pgm check loops */
@@ -1979,7 +1995,7 @@ static int handle_oper_loop(S390CPU *cpu, struct kvm_run *run)
         !(oldpsw.mask & PSW_MASK_PSTATE) &&
         (newpsw.mask & PSW_MASK_ASC) == (oldpsw.mask & PSW_MASK_ASC) &&
         (newpsw.mask & PSW_MASK_DAT) == (oldpsw.mask & PSW_MASK_DAT)) {
-        unmanageable_intercept(cpu, "operation exception loop",
+        unmanageable_intercept(cpu, EXCP_CRASH_OPEREXC,
                                offsetof(LowCore, program_new_psw));
         return EXCP_HALTED;
     }
@@ -2000,12 +2016,12 @@ static int handle_intercept(S390CPU *cpu)
             r = handle_instruction(cpu, run);
             break;
         case ICPT_PROGRAM:
-            unmanageable_intercept(cpu, "program interrupt",
+            unmanageable_intercept(cpu, EXCP_CRASH_PGM,
                                    offsetof(LowCore, program_new_psw));
             r = EXCP_HALTED;
             break;
         case ICPT_EXT_INT:
-            unmanageable_intercept(cpu, "external interrupt",
+            unmanageable_intercept(cpu, EXCP_CRASH_EXT,
                                    offsetof(LowCore, external_new_psw));
             r = EXCP_HALTED;
             break;
@@ -2016,7 +2032,8 @@ static int handle_intercept(S390CPU *cpu)
                 if (is_special_wait_psw(cs)) {
                     qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
                 } else {
-                    qemu_system_guest_panicked(NULL);
+                    cs->exception_index = EXCP_CRASH_WAITPSW;
+                    qemu_system_guest_panicked(cpu_get_crash_info(cs));
                 }
             }
             r = EXCP_HALTED;
diff --git a/vl.c b/vl.c
index 9e62e92..e6b4534 100644
--- a/vl.c
+++ b/vl.c
@@ -1825,6 +1825,12 @@ void qemu_system_guest_panicked(GuestPanicInformation *info)
                           info->u.hyper_v.arg3,
                           info->u.hyper_v.arg4,
                           info->u.hyper_v.arg5);
+        } else if (info->type == GUEST_PANIC_INFORMATION_TYPE_S390) {
+            qemu_log_mask(LOG_GUEST_ERROR, "S390 crash parameters: (%#"
+                          PRIx64" %#"PRIx64")\nS390 crash reason: %s\n",
+                          info->u.s390.psw_mask,
+                          info->u.s390.psw_addr,
+                          info->u.s390.reason);
         }
         qapi_free_GuestPanicInformation(info);
     }
-- 
2.9.4

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

[Eric Blake]

[-- 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 --]

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

[David Hildenbrand]

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

Wonder if we should rather use an enum for reason.

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 74b3e4f..5b835fe 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"
>  #ifndef CONFIG_USER_ONLY
>  #include "hw/hw.h"
> @@ -276,6 +278,58 @@ static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
>      cpu->id = value;
>  }
>  
[...]
> +static void unmanageable_intercept(S390CPU *cpu, int32_t reason, int pswoffset)
>  {
>      CPUState *cs = CPU(cpu);
> +    const char *str;
>  
> +    switch (reason) {
> +    case EXCP_CRASH_PGM:
> +        str = "program interrupt loop";
> +        break;
> +    case EXCP_CRASH_EXT:
> +        str = "external interrupt loop";
> +        break;
> +    case EXCP_CRASH_OPEREXC:
> +        str = "operation exception loop";
> +        break;
> +    default:
> +        str = "unknown crash reason";
> +        break;
> +    }
>      error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx",
>                   str, cs->cpu_index, ldq_phys(cs->as, cpu->env.psa + pswoffset),
>                   ldq_phys(cs->as, cpu->env.psa + pswoffset + 8));
>      s390_cpu_halt(cpu);
> -    qemu_system_guest_panicked(NULL);
> +    cs->exception_index = reason;

Hmmm, this might work for KVM but most probably in the current form not
for TCG. cpu_handle_exception() will most probably just clear it and
then exit to the main loop.

Most probably we would the need to reset the exception index after
returning from tcg_cpu_exec().


Apart from the TCG concerns, looks good to me.

-- 

Thanks,

David

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

[David Hildenbrand]

>      CPUState *cs = CPU(obj);
> @@ -291,6 +345,8 @@ static void s390_cpu_initfn(Object *obj)
>      cs->exception_index = EXCP_HLT;
>      object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
>                          s390x_cpu_set_id, NULL, NULL, NULL);
> +    object_property_add(obj, "crash-information", "GuestPanicInformation",
> +                        s390x_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
>      s390_cpu_model_register_props(obj);
>  #if !defined(CONFIG_USER_ONLY)
>      qemu_get_timedate(&tm, 0);
> @@ -517,6 +573,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>      cc->do_interrupt = s390_cpu_do_interrupt;
>  #endif
>      cc->dump_state = s390_cpu_dump_state;
> +    cc->get_crash_info = s390x_cpu_get_crash_info;
>      cc->set_pc = s390_cpu_set_pc;
>      cc->gdb_read_register = s390_cpu_gdb_read_register;
>      cc->gdb_write_register = s390_cpu_gdb_write_register;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 0bd97a5..dba32db 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -396,6 +396,12 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
>  #define EXCP_IO  7 /* I/O interrupt */
>  #define EXCP_MCHK 8 /* machine check */
>  
> +/* Crash cases. */
> +#define EXCP_CRASH_PGM 9
> +#define EXCP_CRASH_EXT 10
> +#define EXCP_CRASH_WAITPSW 11
> +#define EXCP_CRASH_OPEREXC 12
> +

Okay, looking at cpu_handle_exception() I am pretty sure this won't work
with TCG unless cpu->exception_index >= EXCP_INTERRUPT. Maybe define new
EXCP_CRASH or store this information somewhere else inside s390 CPU?


-- 

Thanks,

David

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

[Christian Borntraeger]

On 09/19/2017 03:06 PM, David Hildenbrand wrote:
> 
>> +##
>> +# @GuestPanicInformationS390:
>> +#
>> +# S390 specific guest panic information (PSW)
>> +#
>> +# Since: 2.11
>> +##
>> +{'struct': 'GuestPanicInformationS390',
>> + 'data': { 'psw-mask': 'uint64',
>> +           'psw-addr': 'uint64',
>> +           'reason': 'str' } }
> 
> Wonder if we should rather use an enum for reason.

Eric asked a similar question. Since this is just a human readable "info"
I would like to keep it as str. This will allow QEMU to add new things without
the need to update libvirt. Ok?

> 
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 74b3e4f..5b835fe 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"
>>  #ifndef CONFIG_USER_ONLY
>>  #include "hw/hw.h"
>> @@ -276,6 +278,58 @@ static void s390x_cpu_set_id(Object *obj, Visitor *v, const char *name,
>>      cpu->id = value;
>>  }
>>  
> [...]
>> +static void unmanageable_intercept(S390CPU *cpu, int32_t reason, int pswoffset)
>>  {
>>      CPUState *cs = CPU(cpu);
>> +    const char *str;
>>  
>> +    switch (reason) {
>> +    case EXCP_CRASH_PGM:
>> +        str = "program interrupt loop";
>> +        break;
>> +    case EXCP_CRASH_EXT:
>> +        str = "external interrupt loop";
>> +        break;
>> +    case EXCP_CRASH_OPEREXC:
>> +        str = "operation exception loop";
>> +        break;
>> +    default:
>> +        str = "unknown crash reason";
>> +        break;
>> +    }
>>      error_report("Unmanageable %s! CPU%i new PSW: 0x%016lx:%016lx",
>>                   str, cs->cpu_index, ldq_phys(cs->as, cpu->env.psa + pswoffset),
>>                   ldq_phys(cs->as, cpu->env.psa + pswoffset + 8));
>>      s390_cpu_halt(cpu);
>> -    qemu_system_guest_panicked(NULL);
>> +    cs->exception_index = reason;
> 
> Hmmm, this might work for KVM but most probably in the current form not
> for TCG. cpu_handle_exception() will most probably just clear it and
> then exit to the main loop.

I was hoping to avoid the need to add new state, but adding a new variable should
be doable.

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

[Christian Borntraeger]

On 09/19/2017 03:14 PM, David Hildenbrand wrote:
> 
>>      CPUState *cs = CPU(obj);
>> @@ -291,6 +345,8 @@ static void s390_cpu_initfn(Object *obj)
>>      cs->exception_index = EXCP_HLT;
>>      object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
>>                          s390x_cpu_set_id, NULL, NULL, NULL);
>> +    object_property_add(obj, "crash-information", "GuestPanicInformation",
>> +                        s390x_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
>>      s390_cpu_model_register_props(obj);
>>  #if !defined(CONFIG_USER_ONLY)
>>      qemu_get_timedate(&tm, 0);
>> @@ -517,6 +573,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>      cc->do_interrupt = s390_cpu_do_interrupt;
>>  #endif
>>      cc->dump_state = s390_cpu_dump_state;
>> +    cc->get_crash_info = s390x_cpu_get_crash_info;
>>      cc->set_pc = s390_cpu_set_pc;
>>      cc->gdb_read_register = s390_cpu_gdb_read_register;
>>      cc->gdb_write_register = s390_cpu_gdb_write_register;
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 0bd97a5..dba32db 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -396,6 +396,12 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
>>  #define EXCP_IO  7 /* I/O interrupt */
>>  #define EXCP_MCHK 8 /* machine check */
>>  
>> +/* Crash cases. */
>> +#define EXCP_CRASH_PGM 9
>> +#define EXCP_CRASH_EXT 10
>> +#define EXCP_CRASH_WAITPSW 11
>> +#define EXCP_CRASH_OPEREXC 12
>> +
> 
> Okay, looking at cpu_handle_exception() I am pretty sure this won't work
> with TCG unless cpu->exception_index >= EXCP_INTERRUPT. Maybe define new
> EXCP_CRASH or store this information somewhere else inside s390 CPU?

Just asking: 
Would it be ok to define EXCP_CRASH as 0x20000 or so?

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

[David Hildenbrand]

On 19.09.2017 15:56, Christian Borntraeger wrote:
> 
> 
> On 09/19/2017 03:14 PM, David Hildenbrand wrote:
>>
>>>      CPUState *cs = CPU(obj);
>>> @@ -291,6 +345,8 @@ static void s390_cpu_initfn(Object *obj)
>>>      cs->exception_index = EXCP_HLT;
>>>      object_property_add(OBJECT(cpu), "id", "int64_t", s390x_cpu_get_id,
>>>                          s390x_cpu_set_id, NULL, NULL, NULL);
>>> +    object_property_add(obj, "crash-information", "GuestPanicInformation",
>>> +                        s390x_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL);
>>>      s390_cpu_model_register_props(obj);
>>>  #if !defined(CONFIG_USER_ONLY)
>>>      qemu_get_timedate(&tm, 0);
>>> @@ -517,6 +573,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data)
>>>      cc->do_interrupt = s390_cpu_do_interrupt;
>>>  #endif
>>>      cc->dump_state = s390_cpu_dump_state;
>>> +    cc->get_crash_info = s390x_cpu_get_crash_info;
>>>      cc->set_pc = s390_cpu_set_pc;
>>>      cc->gdb_read_register = s390_cpu_gdb_read_register;
>>>      cc->gdb_write_register = s390_cpu_gdb_write_register;
>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>> index 0bd97a5..dba32db 100644
>>> --- a/target/s390x/cpu.h
>>> +++ b/target/s390x/cpu.h
>>> @@ -396,6 +396,12 @@ static inline void cpu_get_tb_cpu_state(CPUS390XState* env, target_ulong *pc,
>>>  #define EXCP_IO  7 /* I/O interrupt */
>>>  #define EXCP_MCHK 8 /* machine check */
>>>  
>>> +/* Crash cases. */
>>> +#define EXCP_CRASH_PGM 9
>>> +#define EXCP_CRASH_EXT 10
>>> +#define EXCP_CRASH_WAITPSW 11
>>> +#define EXCP_CRASH_OPEREXC 12
>>> +
>>
>> Okay, looking at cpu_handle_exception() I am pretty sure this won't work
>> with TCG unless cpu->exception_index >= EXCP_INTERRUPT. Maybe define new
>> EXCP_CRASH or store this information somewhere else inside s390 CPU?
> 
> Just asking: 
> Would it be ok to define EXCP_CRASH as 0x20000 or so?
> 

I guess the main problem with EXCP_CRASH is that it will get lost during
migration (if I am not wrong). So cpu->exception_index should most
probably really only be used for "temporary" things, e.g. to exit the
main loop.

Apart from that, I guess introducing it next to the other common code
EXCP_ defines sould be fine!

-- 

Thanks,

David

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

[Cornelia Huck]

On Tue, 19 Sep 2017 09:43:14 +0200
Christian Borntraeger <borntraeger@de.ibm.com> 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.

s/the these/these/

> 
>   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=4444,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"
>           }
>       }
>   }

Out of scope for this patch, but is there a way to print the values as
hex in the monitor?

> 
>   3. log;
> 
>   Run qemu with the parameters: -D <logfile> -d guest_errors, to
>   specify the logfile and log item. The results might be,
> 
>   >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>  
>   Guest crashed
>   S390 crash parameters: (0x2000180000000 0x10ddc6)

Would it make sense to pad with zeroes, for readability?

>   S390 crash reason: disabled wait
>   >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>  
> 
> 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(-)

No further comments over what others have already said.

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

[Christian Borntraeger]

On 09/20/2017 11:14 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 09:43:14 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> 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.
> 
> s/the these/these/
> 
>>
>>   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=4444,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"
>>           }
>>       }
>>   }
> 
> Out of scope for this patch, but is there a way to print the values as
> hex in the monitor?

This is qmp, so I guess not?
> 
>>
>>   3. log;
>>
>>   Run qemu with the parameters: -D <logfile> -d guest_errors, to
>>   specify the logfile and log item. The results might be,
>>
>>   >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>  
>>   Guest crashed
>>   S390 crash parameters: (0x2000180000000 0x10ddc6)
> 
> Would it make sense to pad with zeroes, for readability?

Yes, makes sense. 
> 
>>   S390 crash reason: disabled wait
>>   >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>  
>>
>> 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(-)
> 
> No further comments over what others have already said.
> 

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

[Eric Blake]

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

On 09/20/2017 04:14 AM, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 09:43:14 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> 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.
>>

>>       "data": {
>>           "action": "pause",
>>           "info": {
>>               "psw-addr": 1105350,
>>               "reason": "disabled wait",
>>               "psw-mask": 562956395872256,
>>               "type": "s390"
>>           }
>>       }
>>   }
> 
> Out of scope for this patch, but is there a way to print the values as
> hex in the monitor?
> 

Sadly, no. JSON intentionally chose decimal-only as its numeric format,
so hex values can only be provided as JSON strings.  And since pure JSON
doesn't even allow comments, it's tough to argue that we could modify
the JSON pretty-printer to even list a hex representation as a comment
alongside every integer (although it's hard to know without more context
which values are most useful as hex, so it may be noisier than expected
if every JSON number has that sort of comment).

>>
>>   3. log;
>>
>>   Run qemu with the parameters: -D <logfile> -d guest_errors, to
>>   specify the logfile and log item. The results might be,
>>
>>   >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>  
>>   Guest crashed
>>   S390 crash parameters: (0x2000180000000 0x10ddc6)
> 
> Would it make sense to pad with zeroes, for readability?

Here, you're asking about post-processing the JSON output - at which
point, yes, we can make whatever changes we want to the decimal data
passed over the wire to transform it into something more useful to the
reader.

-- 
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 --]

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 <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

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

[Eric Blake]

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

On 11/07/2017 05:00 AM, QingFeng Hao wrote:

>>> +
>>> +##
>>> +# @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.

Your mailer is eating blank lines, which makes it harder to visually
distinguish where quoting ends and where new content begins (I had a
blank line between your "'reason': 'str' } }" content and my "Missing
documentation..." comment).

I mean something like:

##
# @GuestPanicInformationS390:
#
# S390 specific guest panic information (PSW)
#
# @psw-mask: <description of what this mask represents>
# @psw-addr: Address that was accessed to cause the panic
# @reason: Human-readable explanation of the panic (should not be parsed
#          by a machine)
#
# Since: 2.11
##

-- 
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 --]

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

[QingFeng Hao]

在 2017/11/8 3:35, Eric Blake 写道:
> On 11/07/2017 05:00 AM, QingFeng Hao wrote:
>
>>>> +
>>>> +##
>>>> +# @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.
> Your mailer is eating blank lines, which makes it harder to visually
> distinguish where quoting ends and where new content begins (I had a
> blank line between your "'reason': 'str' } }" content and my "Missing
> documentation..." comment).
Sorry about that I am using thunderbird and I had set some related 
configurations
but not sure what's going on. I can read it normally.
>
> I mean something like:
>
> ##
> # @GuestPanicInformationS390:
> #
> # S390 specific guest panic information (PSW)
> #
> # @psw-mask: <description of what this mask represents>
> # @psw-addr: Address that was accessed to cause the panic
> # @reason: Human-readable explanation of the panic (should not be parsed
> #          by a machine)
> #
> # Since: 2.11
> ##
Thanks and our new patch will contain this change.

-- 
Regards
QingFeng Hao