[Qemu-devel] [PATCH 0/3] add query-cpu-fast and related s390 changes

[Qemu-devel] [PATCH 0/3] add query-cpu-fast and related s390 changes

[Viktor Mihajlovski]

This series consolidates patches around a performance issue
caused by the usage of QMP query-cpus.

A performance issue was found in an OpenStack environment, where
ceilometer was collecting domain statistics with libvirt. The domain
statistics reported by libvirt include the vCPU halted state, which 
in turn is retrieved with QMP query-cpus.

This causes two issues:
1. Performance: on most architectures query-cpus needs to issue a KVM ioctl
   to find out whether a vCPU was halted. This is not the case for s390
   but query-cpus is always causing the vCPU to exit the VM.

2. Semantics: on x86 and other architectures, halted is a highly transient
   state, which is likely to have already changed shortly after the state
   information has been retrieved. This is not the case for s390, where
   halted is an indication that the vCPU is stopped, meaning its not
   available to the guest operating system until it has been restarted.

The following patches help to alleviate the issues:

Patch 1/3:
  Adds architecture specific data to the QMP CpuInfo type, exposing
  the existing s390 cpu-state in QMP. The cpu-state is a representation
  more adequate than the ambiguous 'halted' condition.

  Changes since original v2:
  - fixed cpu-state usage in hw/intc/s390_flic.c, necessary because
    master was updated in the meantime
  - removed superfluous newline while printing cpu-state

Patch 2/3:
  Adds a new QMP function query-cpus-fast, which will only retrieve
  vCPU information that can be obtained without interrupting the
  vCPUs of a running VM. It introduces a new return type CpuInfoFast
  with the subset of fields meeting this condition. Specifically, the
  halted state is not part of CpuInfoFast. QMP clients like libvirt
  are encouraged to switch to the new API for vCPU information.

  Changes since original v2:
  - dropped optional halted state from CpuInfoFast

Patch 3/3:
  Adds the s390-specific cpu state to CpuInfoFast, allowing management
  apps to find out whether a vCPU is in the stopped state. This extension
  leads to a partial duplication of field definitions from CpuInfo
  to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and
  eventually removed.

Luiz Capitulino (1):
  qmp: add query-cpus-fast

Viktor Mihajlovski (2):
  qmp: expose s390-specific CPU info
  qmp: add architecture specific cpu data for query-cpus-fast

 cpus.c                     |  54 ++++++++++++++++++++
 hmp-commands-info.hx       |  14 ++++++
 hmp.c                      |  33 +++++++++++++
 hmp.h                      |   1 +
 hw/intc/s390_flic.c        |   4 +-
 hw/s390x/s390-virtio-ccw.c |   2 +-
 qapi-schema.json           | 121 ++++++++++++++++++++++++++++++++++++++++++++-
 target/s390x/cpu.c         |  24 ++++-----
 target/s390x/cpu.h         |   7 +--
 target/s390x/kvm.c         |   8 +--
 target/s390x/sigp.c        |  38 +++++++-------
 11 files changed, 262 insertions(+), 44 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info

[Viktor Mihajlovski]

Presently s390x is the only architecture not exposing specific
CPU information via QMP query-cpus. Upstream discussion has shown
that it could make sense to report the architecture specific CPU
state, e.g. to detect that a CPU has been stopped.

With this change the output of query-cpus will look like this on
s390:

   [
     {"arch": "s390", "current": true,
      "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
      "qom_path": "/machine/unattached/device[0]",
      "halted": false, "thread_id": 63115},
     {"arch": "s390", "current": false,
      "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
      "qom_path": "/machine/unattached/device[1]",
      "halted": true, "thread_id": 63116}
   ]

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Acked-by: Eric Blake <eblake@redhat.com>
---
 cpus.c                     |  6 ++++++
 hmp.c                      |  4 ++++
 hw/intc/s390_flic.c        |  4 ++--
 hw/s390x/s390-virtio-ccw.c |  2 +-
 qapi-schema.json           | 28 +++++++++++++++++++++++++++-
 target/s390x/cpu.c         | 24 ++++++++++++------------
 target/s390x/cpu.h         |  7 ++-----
 target/s390x/kvm.c         |  8 ++++----
 target/s390x/sigp.c        | 38 +++++++++++++++++++-------------------
 9 files changed, 77 insertions(+), 44 deletions(-)

diff --git a/cpus.c b/cpus.c
index f298b65..6006931 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 #elif defined(TARGET_TRICORE)
         TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
         CPUTriCoreState *env = &tricore_cpu->env;
+#elif defined(TARGET_S390X)
+        S390CPU *s390_cpu = S390_CPU(cpu);
+        CPUS390XState *env = &s390_cpu->env;
 #endif
 
         cpu_synchronize_state(cpu);
@@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
 #elif defined(TARGET_TRICORE)
         info->value->arch = CPU_INFO_ARCH_TRICORE;
         info->value->u.tricore.PC = env->PC;
+#elif defined(TARGET_S390X)
+        info->value->arch = CPU_INFO_ARCH_S390;
+        info->value->u.s390.cpu_state = env->cpu_state;
 #else
         info->value->arch = CPU_INFO_ARCH_OTHER;
 #endif
diff --git a/hmp.c b/hmp.c
index 7870d6a..a6b94b7 100644
--- a/hmp.c
+++ b/hmp.c
@@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
         case CPU_INFO_ARCH_TRICORE:
             monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
             break;
+        case CPU_INFO_ARCH_S390:
+            monitor_printf(mon, " state=%s",
+                           CpuS390State_str(cpu->value->u.s390.cpu_state));
+            break;
         default:
             break;
         }
diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
index a85a149..5f8168f 100644
--- a/hw/intc/s390_flic.c
+++ b/hw/intc/s390_flic.c
@@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
         cs->interrupt_request |= CPU_INTERRUPT_HARD;
 
         /* ignore CPUs that are not sleeping */
-        if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
-            s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
+        if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
+            s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
             continue;
         }
 
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4abbe89..4d0c3de 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -368,7 +368,7 @@ static void s390_machine_reset(void)
 
     /* all cpus are stopped - configure and start the ipl cpu only */
     s390_ipl_prepare_cpu(ipl_cpu);
-    s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
+    s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
 }
 
 static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c06745..66e0927 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -410,10 +410,12 @@
 # An enumeration of cpu types that enable additional information during
 # @query-cpus.
 #
+# @s390: since 2.12
+#
 # Since: 2.6
 ##
 { 'enum': 'CpuInfoArch',
-  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
 
 ##
 # @CpuInfo:
@@ -452,6 +454,7 @@
             'ppc': 'CpuInfoPPC',
             'mips': 'CpuInfoMIPS',
             'tricore': 'CpuInfoTricore',
+            's390': 'CpuInfoS390',
             'other': 'CpuInfoOther' } }
 
 ##
@@ -522,6 +525,29 @@
 { 'struct': 'CpuInfoOther', 'data': { } }
 
 ##
+# @CpuS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+##
+{ 'enum': 'CpuS390State',
+  'prefix': 'S390_CPU_STATE',
+  'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU
+#
+# @cpu-state: the virtual CPU's state
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
 # @query-cpus:
 #
 # Returns a list of information about each virtual CPU.
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index da7cb9c..52b74ed 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -58,8 +58,8 @@ static bool s390_cpu_has_work(CPUState *cs)
     S390CPU *cpu = S390_CPU(cs);
 
     /* STOPPED cpus can never wake up */
-    if (s390_cpu_get_state(cpu) != CPU_STATE_LOAD &&
-        s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
+    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD &&
+        s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
         return false;
     }
 
@@ -77,7 +77,7 @@ static void s390_cpu_load_normal(CPUState *s)
     S390CPU *cpu = S390_CPU(s);
     cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
     cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
-    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
+    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
 #endif
 
@@ -92,7 +92,7 @@ static void s390_cpu_reset(CPUState *s)
     env->bpbc = false;
     scc->parent_reset(s);
     cpu->env.sigp_order = 0;
-    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
+    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 }
 
 /* S390CPUClass::initial_reset() */
@@ -135,7 +135,7 @@ static void s390_cpu_full_reset(CPUState *s)
 
     scc->parent_reset(s);
     cpu->env.sigp_order = 0;
-    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
+    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 
     memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
 
@@ -247,7 +247,7 @@ static void s390_cpu_initfn(Object *obj)
     env->tod_basetime = 0;
     env->tod_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
     env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
-    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
+    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
 #endif
 }
 
@@ -275,8 +275,8 @@ static unsigned s390_count_running_cpus(void)
 
     CPU_FOREACH(cpu) {
         uint8_t state = S390_CPU(cpu)->env.cpu_state;
-        if (state == CPU_STATE_OPERATING ||
-            state == CPU_STATE_LOAD) {
+        if (state == S390_CPU_STATE_OPERATING ||
+            state == S390_CPU_STATE_LOAD) {
             if (!disabled_wait(cpu)) {
                 nr_running++;
             }
@@ -315,13 +315,13 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
     trace_cpu_set_state(CPU(cpu)->cpu_index, cpu_state);
 
     switch (cpu_state) {
-    case CPU_STATE_STOPPED:
-    case CPU_STATE_CHECK_STOP:
+    case S390_CPU_STATE_STOPPED:
+    case S390_CPU_STATE_CHECK_STOP:
         /* halt the cpu for common infrastructure */
         s390_cpu_halt(cpu);
         break;
-    case CPU_STATE_OPERATING:
-    case CPU_STATE_LOAD:
+    case S390_CPU_STATE_OPERATING:
+    case S390_CPU_STATE_LOAD:
         /*
          * Starting a CPU with a PSW WAIT bit set:
          * KVM: handles this internally and triggers another WAIT exit.
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 21ce40d..66baa7a 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -141,12 +141,9 @@ struct CPUS390XState {
      * architectures, there is a difference between a halt and a stop on s390.
      * If all cpus are either stopped (including check stop) or in the disabled
      * wait state, the vm can be shut down.
+     * The acceptable cpu_state values are defined in the CpuInfoS390State
+     * enum.
      */
-#define CPU_STATE_UNINITIALIZED        0x00
-#define CPU_STATE_STOPPED              0x01
-#define CPU_STATE_CHECK_STOP           0x02
-#define CPU_STATE_OPERATING            0x03
-#define CPU_STATE_LOAD                 0x04
     uint8_t cpu_state;
 
     /* currently processed sigp order */
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 0301e9d..45dd1c5 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -1881,16 +1881,16 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
     }
 
     switch (cpu_state) {
-    case CPU_STATE_STOPPED:
+    case S390_CPU_STATE_STOPPED:
         mp_state.mp_state = KVM_MP_STATE_STOPPED;
         break;
-    case CPU_STATE_CHECK_STOP:
+    case S390_CPU_STATE_CHECK_STOP:
         mp_state.mp_state = KVM_MP_STATE_CHECK_STOP;
         break;
-    case CPU_STATE_OPERATING:
+    case S390_CPU_STATE_OPERATING:
         mp_state.mp_state = KVM_MP_STATE_OPERATING;
         break;
-    case CPU_STATE_LOAD:
+    case S390_CPU_STATE_LOAD:
         mp_state.mp_state = KVM_MP_STATE_LOAD;
         break;
     default:
diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index ac3f8e7..5a7a9c4 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -46,13 +46,13 @@ static void sigp_sense(S390CPU *dst_cpu, SigpInfo *si)
     }
 
     /* sensing without locks is racy, but it's the same for real hw */
-    if (state != CPU_STATE_STOPPED && !ext_call) {
+    if (state != S390_CPU_STATE_STOPPED && !ext_call) {
         si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
     } else {
         if (ext_call) {
             status |= SIGP_STAT_EXT_CALL_PENDING;
         }
-        if (state == CPU_STATE_STOPPED) {
+        if (state == S390_CPU_STATE_STOPPED) {
             status |= SIGP_STAT_STOPPED;
         }
         set_sigp_status(si, status);
@@ -94,12 +94,12 @@ static void sigp_start(CPUState *cs, run_on_cpu_data arg)
     S390CPU *cpu = S390_CPU(cs);
     SigpInfo *si = arg.host_ptr;
 
-    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
+    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
         si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
         return;
     }
 
-    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
+    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
     si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
 }
 
@@ -108,14 +108,14 @@ static void sigp_stop(CPUState *cs, run_on_cpu_data arg)
     S390CPU *cpu = S390_CPU(cs);
     SigpInfo *si = arg.host_ptr;
 
-    if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
+    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
         si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
         return;
     }
 
     /* disabled wait - sleeping in user space */
     if (cs->halted) {
-        s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
+        s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
     } else {
         /* execute the stop function */
         cpu->env.sigp_order = SIGP_STOP;
@@ -130,17 +130,17 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
     SigpInfo *si = arg.host_ptr;
 
     /* disabled wait - sleeping in user space */
-    if (s390_cpu_get_state(cpu) == CPU_STATE_OPERATING && cs->halted) {
-        s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
+    if (s390_cpu_get_state(cpu) == S390_CPU_STATE_OPERATING && cs->halted) {
+        s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
     }
 
     switch (s390_cpu_get_state(cpu)) {
-    case CPU_STATE_OPERATING:
+    case S390_CPU_STATE_OPERATING:
         cpu->env.sigp_order = SIGP_STOP_STORE_STATUS;
         cpu_inject_stop(cpu);
         /* store will be performed in do_stop_interrup() */
         break;
-    case CPU_STATE_STOPPED:
+    case S390_CPU_STATE_STOPPED:
         /* already stopped, just store the status */
         cpu_synchronize_state(cs);
         s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
@@ -156,7 +156,7 @@ static void sigp_store_status_at_address(CPUState *cs, run_on_cpu_data arg)
     uint32_t address = si->param & 0x7ffffe00u;
 
     /* cpu has to be stopped */
-    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
+    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
         set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
         return;
     }
@@ -186,7 +186,7 @@ static void sigp_store_adtl_status(CPUState *cs, run_on_cpu_data arg)
     }
 
     /* cpu has to be stopped */
-    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
+    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
         set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
         return;
     }
@@ -229,17 +229,17 @@ static void sigp_restart(CPUState *cs, run_on_cpu_data arg)
     SigpInfo *si = arg.host_ptr;
 
     switch (s390_cpu_get_state(cpu)) {
-    case CPU_STATE_STOPPED:
+    case S390_CPU_STATE_STOPPED:
         /* the restart irq has to be delivered prior to any other pending irq */
         cpu_synchronize_state(cs);
         /*
          * Set OPERATING (and unhalting) before loading the restart PSW.
          * load_psw() will then properly halt the CPU again if necessary (TCG).
          */
-        s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
+        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
         do_restart_interrupt(&cpu->env);
         break;
-    case CPU_STATE_OPERATING:
+    case S390_CPU_STATE_OPERATING:
         cpu_inject_restart(cpu);
         break;
     }
@@ -285,7 +285,7 @@ static void sigp_set_prefix(CPUState *cs, run_on_cpu_data arg)
     }
 
     /* cpu has to be stopped */
-    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
+    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
         set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
         return;
     }
@@ -318,7 +318,7 @@ static void sigp_cond_emergency(S390CPU *src_cpu, S390CPU *dst_cpu,
     p_asn = dst_cpu->env.cregs[4] & 0xffff;  /* Primary ASN */
     s_asn = dst_cpu->env.cregs[3] & 0xffff;  /* Secondary ASN */
 
-    if (s390_cpu_get_state(dst_cpu) != CPU_STATE_STOPPED ||
+    if (s390_cpu_get_state(dst_cpu) != S390_CPU_STATE_STOPPED ||
         (psw_mask & psw_int_mask) != psw_int_mask ||
         (idle && psw_addr != 0) ||
         (!idle && (asn == p_asn || asn == s_asn))) {
@@ -435,7 +435,7 @@ static int sigp_set_architecture(S390CPU *cpu, uint32_t param,
         if (cur_cpu == cpu) {
             continue;
         }
-        if (s390_cpu_get_state(cur_cpu) != CPU_STATE_STOPPED) {
+        if (s390_cpu_get_state(cur_cpu) != S390_CPU_STATE_STOPPED) {
             all_stopped = false;
         }
     }
@@ -492,7 +492,7 @@ void do_stop_interrupt(CPUS390XState *env)
 {
     S390CPU *cpu = s390_env_get_cpu(env);
 
-    if (s390_cpu_set_state(CPU_STATE_STOPPED, cpu) == 0) {
+    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
         qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
     if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
-- 
1.9.1

[Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast

[Viktor Mihajlovski]

From: Luiz Capitulino <lcapitulino@redhat.com>

The query-cpus command has an extremely serious side effect:
it always interrupts all running vCPUs so that they can run
ioctl calls. This can cause a huge performance degradation for
some workloads. And most of the information retrieved by the
ioctl calls are not even used by query-cpus.

This commit introduces a replacement for query-cpus called
query-cpus-fast, which has the following features:

 o Never interrupt vCPUs threads. query-cpus-fast only returns
   vCPU information maintained by QEMU itself, which should be
   sufficient for most management software needs

 o Make "halted" field optional: we only return it if the
   halted state is maintained by QEMU. But this also gives
   the option of dropping the field in the future (see below)

 o Drop irrelevant fields such as "current", "pc" and "arch"

 o Drop field "halted" since it can't be provided fast reliably
   and is too volatile on most architectures to be really useful

 o Rename some fields for better clarification & proper naming
   standard

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 cpus.c               | 38 ++++++++++++++++++++++++++++
 hmp-commands-info.hx | 14 +++++++++++
 hmp.c                | 21 ++++++++++++++++
 hmp.h                |  1 +
 qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 144 insertions(+)

diff --git a/cpus.c b/cpus.c
index 6006931..6df6660 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
     return head;
 }
 
+/*
+ * fast means: we NEVER interrupt vCPU threads to retrieve
+ * information from KVM.
+ */
+CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
+{
+    MachineState *ms = MACHINE(qdev_get_machine());
+    MachineClass *mc = MACHINE_GET_CLASS(ms);
+    CpuInfoFastList *head = NULL, *cur_item = NULL;
+    CPUState *cpu;
+
+    CPU_FOREACH(cpu) {
+        CpuInfoFastList *info = g_malloc0(sizeof(*info));
+        info->value = g_malloc0(sizeof(*info->value));
+
+        info->value->cpu_index = cpu->cpu_index;
+        info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
+        info->value->thread_id = cpu->thread_id;
+
+        info->value->has_props = !!mc->cpu_index_to_instance_props;
+        if (info->value->has_props) {
+            CpuInstanceProperties *props;
+            props = g_malloc0(sizeof(*props));
+            *props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
+            info->value->props = props;
+        }
+
+        if (!cur_item) {
+            head = cur_item = info;
+        } else {
+            cur_item->next = info;
+            cur_item = info;
+        }
+    }
+
+    return head;
+}
+
 void qmp_memsave(int64_t addr, int64_t size, const char *filename,
                  bool has_cpu, int64_t cpu_index, Error **errp)
 {
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ad590a4..16ac602 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -160,6 +160,20 @@ Show infos for each CPU.
 ETEXI
 
     {
+        .name       = "cpus_fast",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show information for each CPU without interrupting them",
+        .cmd        = hmp_info_cpus_fast,
+    },
+
+STEXI
+@item info cpus_fast
+@findex info cpus_fast
+Show infos for each CPU without performance penalty.
+ETEXI
+
+    {
         .name       = "history",
         .args_type  = "",
         .params     = "",
diff --git a/hmp.c b/hmp.c
index a6b94b7..598bfe5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -410,6 +410,27 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
     qapi_free_CpuInfoList(cpu_list);
 }
 
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
+{
+    CpuInfoFastList *head, *cpu;
+    TargetInfo *target;
+
+    target = qmp_query_target(NULL);
+    monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch);
+    qapi_free_TargetInfo(target);
+
+    head = qmp_query_cpus_fast(NULL);
+
+    for (cpu = head; cpu; cpu = cpu->next) {
+        monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
+        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+        monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
+        monitor_printf(mon, "\n");
+    }
+
+    qapi_free_CpuInfoFastList(head);
+}
+
 static void print_block_info(Monitor *mon, BlockInfo *info,
                              BlockDeviceInfo *inserted, bool verbose)
 {
diff --git a/hmp.h b/hmp.h
index 1143db4..93fb4e4 100644
--- a/hmp.h
+++ b/hmp.h
@@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
 void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
 void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
 void hmp_info_cpus(Monitor *mon, const QDict *qdict);
+void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
 void hmp_info_block(Monitor *mon, const QDict *qdict);
 void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
 void hmp_info_vnc(Monitor *mon, const QDict *qdict);
diff --git a/qapi-schema.json b/qapi-schema.json
index 66e0927..2a614af 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -552,6 +552,12 @@
 #
 # Returns a list of information about each virtual CPU.
 #
+# This command causes vCPU threads to exit to userspace, which causes
+# an small interruption guest CPU execution. This will have a negative
+# impact on realtime guests and other latency sensitive guest workloads.
+# It is recommended to use @query-cpus-fast instead of this command to
+# avoid the vCPU interruption.
+#
 # Returns: a list of @CpuInfo for each virtual CPU
 #
 # Since: 0.14.0
@@ -585,6 +591,70 @@
 { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
 
 ##
+# @CpuInfoFast:
+#
+# Information about a virtual CPU
+#
+# @cpu-index: index of the virtual CPU
+#
+# @qom-path: path to the CPU object in the QOM tree
+#
+# @thread-id: ID of the underlying host thread
+#
+# @props: properties describing to which node/socket/core/thread
+#         virtual CPU belongs to, provided if supported by board
+#
+# Since: 2.12
+#
+##
+{ 'struct': 'CpuInfoFast',
+  'data': {'cpu-index': 'int', 'qom-path': 'str',
+           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+
+##
+# @query-cpus-fast:
+#
+# Returns information about all virtual CPUs. This command does not
+# incur a performance penalty and should be used in production
+# instead of query-cpus.
+#
+# Returns: list of @CpuInfoFast
+#
+# Notes: The CPU architecture name is not returned by query-cpus-fast.
+#        Use query-target to retrieve that information.
+#
+# Since: 2.12
+#
+# Example:
+#
+# -> { "execute": "query-cpus-fast" }
+# <- { "return": [
+#         {
+#             "thread-id": 25627,
+#             "props": {
+#                 "core-id": 0,
+#                 "thread-id": 0,
+#                 "socket-id": 0
+#             },
+#             "qom-path": "/machine/unattached/device[0]",
+#             "cpu-index": 0
+#         },
+#         {
+#             "thread-id": 25628,
+#             "props": {
+#                 "core-id": 0,
+#                 "thread-id": 0,
+#                 "socket-id": 1
+#             },
+#             "qom-path": "/machine/unattached/device[2]",
+#             "cpu-index": 1
+#         }
+#     ]
+# }
+##
+{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] }
+
+##
 # @IOThreadInfo:
 #
 # Information about an iothread
-- 
1.9.1

[Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Viktor Mihajlovski]

The s390 CPU state can be retrieved without interrupting the
VM execution. Extendend the CpuInfoFast union with architecture
specific data and an implementation for s390.

Return data looks like this:
 [
   {"thread-id":64301,"props":{"core-id":0},
    "arch":"s390","cpu-state":"operating",
    "qom-path":"/machine/unattached/device[0]","cpu-index":0},
   {"thread-id":64302,"props":{"core-id":1},
    "arch":"s390","cpu-state":"operating",
    "qom-path":"/machine/unattached/device[1]","cpu-index":1}
]

Currently there's a certain amount of duplication between
the definitions of CpuInfo and CpuInfoFast, both in the
base and variable areas, since there are data fields common
to the slow and fast variants.

A suggestion was made on the mailing list to enhance the QAPI
code generation to support two layers of unions. This would
allow to specify the common fields once and avoid the duplication
in the leaf unions.

On the other hand, the slow query-cpus should be deprecated
along with the slow CpuInfo type and eventually be removed.
Assuming that new architectures will not be added at high
rates, we could live with the duplication for the time being.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 cpus.c           | 10 ++++++++++
 hmp.c            |  8 ++++++++
 qapi-schema.json | 35 +++++++++++++++++++++++++++++------
 3 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/cpus.c b/cpus.c
index 6df6660..af67826 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     CpuInfoFastList *head = NULL, *cur_item = NULL;
     CPUState *cpu;
+#if defined(TARGET_S390X)
+    S390CPU *s390_cpu;
+    CPUS390XState *env;
+#endif
 
     CPU_FOREACH(cpu) {
         CpuInfoFastList *info = g_malloc0(sizeof(*info));
@@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
             info->value->props = props;
         }
 
+#if defined(TARGET_S390X)
+        s390_cpu = S390_CPU(cpu);
+        env = &s390_cpu->env;
+        info->value->arch = CPU_INFO_ARCH_S390;
+        info->value->u.s390.cpu_state = env->cpu_state;
+#endif
         if (!cur_item) {
             head = cur_item = info;
         } else {
diff --git a/hmp.c b/hmp.c
index 598bfe5..94bfc7d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -425,6 +425,14 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
         monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
         monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
+        switch (cpu->value->arch) {
+        case CPU_INFO_ARCH_S390:
+            monitor_printf(mon, " state=%s\n",
+                           CpuS390State_str(cpu->value->u.s390.cpu_state));
+            break;
+        default:
+            break;
+        }
         monitor_printf(mon, "\n");
     }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 2a614af..a681d83 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -537,15 +537,26 @@
   'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
 
 ##
-# @CpuInfoS390:
+# @CpuInfoS390Fast:
 #
-# Additional information about a virtual S390 CPU
+# Additional information about a virtual S390 CPU which can be
+# obtained without a performance penalty for a running VM
 #
 # @cpu-state: the virtual CPU's state
 #
 # Since: 2.12
 ##
-{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
+{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } }
+
+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU, potentially expensive
+# to obtain
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } }
 
 ##
 # @query-cpus:
@@ -604,12 +615,24 @@
 # @props: properties describing to which node/socket/core/thread
 #         virtual CPU belongs to, provided if supported by board
 #
+# @arch: architecture of the cpu, which determines which additional fields
+#        will be listed
+#
 # Since: 2.12
 #
 ##
-{ 'struct': 'CpuInfoFast',
-  'data': {'cpu-index': 'int', 'qom-path': 'str',
-           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+{ 'union': 'CpuInfoFast',
+  'base': {'cpu-index': 'int', 'qom-path': 'str',
+           'thread-id': 'int', '*props': 'CpuInstanceProperties',
+           'arch': 'CpuInfoArch' },
+  'discriminator': 'arch',
+  'data': { 'x86': 'CpuInfoOther',
+            'sparc': 'CpuInfoOther',
+            'ppc': 'CpuInfoOther',
+            'mips': 'CpuInfoOther',
+            'tricore': 'CpuInfoOther',
+            's390': 'CpuInfoS390Fast',
+            'other': 'CpuInfoOther' } }
 
 ##
 # @query-cpus-fast:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/3] add query-cpu-fast and related s390 changes

[Cornelia Huck]

On Mon, 12 Feb 2018 13:14:29 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> This series consolidates patches around a performance issue
> caused by the usage of QMP query-cpus.

Thank you for consolidating this; it was a bit hard to follow the
different discussions.

> 
> A performance issue was found in an OpenStack environment, where
> ceilometer was collecting domain statistics with libvirt. The domain
> statistics reported by libvirt include the vCPU halted state, which 
> in turn is retrieved with QMP query-cpus.
> 
> This causes two issues:
> 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl
>    to find out whether a vCPU was halted. This is not the case for s390
>    but query-cpus is always causing the vCPU to exit the VM.
> 
> 2. Semantics: on x86 and other architectures, halted is a highly transient
>    state, which is likely to have already changed shortly after the state
>    information has been retrieved. This is not the case for s390, where
>    halted is an indication that the vCPU is stopped, meaning its not
>    available to the guest operating system until it has been restarted.
> 
> The following patches help to alleviate the issues:
> 
> Patch 1/3:
>   Adds architecture specific data to the QMP CpuInfo type, exposing
>   the existing s390 cpu-state in QMP. The cpu-state is a representation
>   more adequate than the ambiguous 'halted' condition.
> 
>   Changes since original v2:
>   - fixed cpu-state usage in hw/intc/s390_flic.c, necessary because
>     master was updated in the meantime
>   - removed superfluous newline while printing cpu-state

So this is adding s390x info in query-cpus (presumably for legacy
callers?)...

> 
> Patch 2/3:
>   Adds a new QMP function query-cpus-fast, which will only retrieve
>   vCPU information that can be obtained without interrupting the
>   vCPUs of a running VM. It introduces a new return type CpuInfoFast
>   with the subset of fields meeting this condition. Specifically, the
>   halted state is not part of CpuInfoFast. QMP clients like libvirt
>   are encouraged to switch to the new API for vCPU information.
> 
>   Changes since original v2:
>   - dropped optional halted state from CpuInfoFast

...this is adding the new query-cpus-fast...

> 
> Patch 3/3:
>   Adds the s390-specific cpu state to CpuInfoFast, allowing management
>   apps to find out whether a vCPU is in the stopped state. This extension
>   leads to a partial duplication of field definitions from CpuInfo
>   to CpuInfoFast. This should be tolerable if CpuInfo is deprecated and
>   eventually removed.

...and this is adding the s390x state to query-cpus-fast, right?

> 
> Luiz Capitulino (1):
>   qmp: add query-cpus-fast
> 
> Viktor Mihajlovski (2):
>   qmp: expose s390-specific CPU info
>   qmp: add architecture specific cpu data for query-cpus-fast
> 
>  cpus.c                     |  54 ++++++++++++++++++++
>  hmp-commands-info.hx       |  14 ++++++
>  hmp.c                      |  33 +++++++++++++
>  hmp.h                      |   1 +
>  hw/intc/s390_flic.c        |   4 +-
>  hw/s390x/s390-virtio-ccw.c |   2 +-
>  qapi-schema.json           | 121 ++++++++++++++++++++++++++++++++++++++++++++-
>  target/s390x/cpu.c         |  24 ++++-----
>  target/s390x/cpu.h         |   7 +--
>  target/s390x/kvm.c         |   8 +--
>  target/s390x/sigp.c        |  38 +++++++-------
>  11 files changed, 262 insertions(+), 44 deletions(-)
> 

Re: [Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info

[Cornelia Huck]

On Mon, 12 Feb 2018 13:14:30 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> Presently s390x is the only architecture not exposing specific
> CPU information via QMP query-cpus. Upstream discussion has shown
> that it could make sense to report the architecture specific CPU
> state, e.g. to detect that a CPU has been stopped.
> 
> With this change the output of query-cpus will look like this on
> s390:
> 
>    [
>      {"arch": "s390", "current": true,
>       "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>       "qom_path": "/machine/unattached/device[0]",
>       "halted": false, "thread_id": 63115},
>      {"arch": "s390", "current": false,
>       "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>       "qom_path": "/machine/unattached/device[1]",
>       "halted": true, "thread_id": 63116}
>    ]
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Acked-by: Eric Blake <eblake@redhat.com>
> ---
>  cpus.c                     |  6 ++++++
>  hmp.c                      |  4 ++++
>  hw/intc/s390_flic.c        |  4 ++--
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json           | 28 +++++++++++++++++++++++++++-
>  target/s390x/cpu.c         | 24 ++++++++++++------------
>  target/s390x/cpu.h         |  7 ++-----
>  target/s390x/kvm.c         |  8 ++++----
>  target/s390x/sigp.c        | 38 +++++++++++++++++++-------------------
>  9 files changed, 77 insertions(+), 44 deletions(-)

Patch looks good to me. I presume this should go through the s390 tree?
Or do we want someone to pick up the whole series?

Re: [Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast

[Cornelia Huck]

On Mon, 12 Feb 2018 13:14:31 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> The query-cpus command has an extremely serious side effect:
> it always interrupts all running vCPUs so that they can run
> ioctl calls. This can cause a huge performance degradation for
> some workloads. And most of the information retrieved by the
> ioctl calls are not even used by query-cpus.
> 
> This commit introduces a replacement for query-cpus called
> query-cpus-fast, which has the following features:
> 
>  o Never interrupt vCPUs threads. query-cpus-fast only returns
>    vCPU information maintained by QEMU itself, which should be
>    sufficient for most management software needs
> 
>  o Make "halted" field optional: we only return it if the
>    halted state is maintained by QEMU. But this also gives
>    the option of dropping the field in the future (see below)
> 
>  o Drop irrelevant fields such as "current", "pc" and "arch"
> 
>  o Drop field "halted" since it can't be provided fast reliably
>    and is too volatile on most architectures to be really useful
> 
>  o Rename some fields for better clarification & proper naming
>    standard
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  cpus.c               | 38 ++++++++++++++++++++++++++++
>  hmp-commands-info.hx | 14 +++++++++++
>  hmp.c                | 21 ++++++++++++++++
>  hmp.h                |  1 +
>  qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 144 insertions(+)

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info

[Viktor Mihajlovski]

On 12.02.2018 16:52, Cornelia Huck wrote:
> On Mon, 12 Feb 2018 13:14:30 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 
>> Presently s390x is the only architecture not exposing specific
>> CPU information via QMP query-cpus. Upstream discussion has shown
>> that it could make sense to report the architecture specific CPU
>> state, e.g. to detect that a CPU has been stopped.
>>
>> With this change the output of query-cpus will look like this on
>> s390:
>>
>>    [
>>      {"arch": "s390", "current": true,
>>       "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>>       "qom_path": "/machine/unattached/device[0]",
>>       "halted": false, "thread_id": 63115},
>>      {"arch": "s390", "current": false,
>>       "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>>       "qom_path": "/machine/unattached/device[1]",
>>       "halted": true, "thread_id": 63116}
>>    ]
>>
>> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>> Acked-by: Eric Blake <eblake@redhat.com>
>> ---
>>  cpus.c                     |  6 ++++++
>>  hmp.c                      |  4 ++++
>>  hw/intc/s390_flic.c        |  4 ++--
>>  hw/s390x/s390-virtio-ccw.c |  2 +-
>>  qapi-schema.json           | 28 +++++++++++++++++++++++++++-
>>  target/s390x/cpu.c         | 24 ++++++++++++------------
>>  target/s390x/cpu.h         |  7 ++-----
>>  target/s390x/kvm.c         |  8 ++++----
>>  target/s390x/sigp.c        | 38 +++++++++++++++++++-------------------
>>  9 files changed, 77 insertions(+), 44 deletions(-)
> 
> Patch looks good to me. I presume this should go through the s390 tree?
> Or do we want someone to pick up the whole series?
> 
The main reason for adding this patch to the series is to ensure
everything is applied in proper order. This patch can stand for itself,
but it must be applied before 3/3.
Valid orders would be 1 - 2 - 3 or 2 - 1 - 3.
As long as this is observed, I'm fine with either way.

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Cornelia Huck]

On Mon, 12 Feb 2018 13:14:32 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> The s390 CPU state can be retrieved without interrupting the
> VM execution. Extendend the CpuInfoFast union with architecture
> specific data and an implementation for s390.
> 
> Return data looks like this:
>  [
>    {"thread-id":64301,"props":{"core-id":0},
>     "arch":"s390","cpu-state":"operating",
>     "qom-path":"/machine/unattached/device[0]","cpu-index":0},
>    {"thread-id":64302,"props":{"core-id":1},
>     "arch":"s390","cpu-state":"operating",
>     "qom-path":"/machine/unattached/device[1]","cpu-index":1}
> ]
> 
> Currently there's a certain amount of duplication between
> the definitions of CpuInfo and CpuInfoFast, both in the
> base and variable areas, since there are data fields common
> to the slow and fast variants.
> 
> A suggestion was made on the mailing list to enhance the QAPI
> code generation to support two layers of unions. This would
> allow to specify the common fields once and avoid the duplication
> in the leaf unions.
> 
> On the other hand, the slow query-cpus should be deprecated
> along with the slow CpuInfo type and eventually be removed.
> Assuming that new architectures will not be added at high
> rates, we could live with the duplication for the time being.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  cpus.c           | 10 ++++++++++
>  hmp.c            |  8 ++++++++
>  qapi-schema.json | 35 +++++++++++++++++++++++++++++------
>  3 files changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 6df6660..af67826 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>      MachineClass *mc = MACHINE_GET_CLASS(ms);
>      CpuInfoFastList *head = NULL, *cur_item = NULL;
>      CPUState *cpu;
> +#if defined(TARGET_S390X)
> +    S390CPU *s390_cpu;
> +    CPUS390XState *env;
> +#endif
>  
>      CPU_FOREACH(cpu) {
>          CpuInfoFastList *info = g_malloc0(sizeof(*info));
> @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>              info->value->props = props;
>          }
>  
> +#if defined(TARGET_S390X)
> +        s390_cpu = S390_CPU(cpu);
> +        env = &s390_cpu->env;

You should be able to omit the s390_cpu variable by using

           env = &S390_CPU(cpu)->env;

> +        info->value->arch = CPU_INFO_ARCH_S390;
> +        info->value->u.s390.cpu_state = env->cpu_state;
> +#endif
>          if (!cur_item) {
>              head = cur_item = info;
>          } else {

As you mentioned in the patch description, the duplication is a bit
awkward. I'll let the QAPI experts judge that; otherwise, this looks
fine to me.

Re: [Qemu-devel] [PATCH 0/3] add query-cpu-fast and related s390 changes

[Viktor Mihajlovski]

On 12.02.2018 16:38, Cornelia Huck wrote:
> On Mon, 12 Feb 2018 13:14:29 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 
>> This series consolidates patches around a performance issue
>> caused by the usage of QMP query-cpus.
> 
> Thank you for consolidating this; it was a bit hard to follow the
> different discussions.
> 
>>
>> A performance issue was found in an OpenStack environment, where
>> ceilometer was collecting domain statistics with libvirt. The domain
>> statistics reported by libvirt include the vCPU halted state, which 
>> in turn is retrieved with QMP query-cpus.
>>
>> This causes two issues:
>> 1. Performance: on most architectures query-cpus needs to issue a KVM ioctl
>>    to find out whether a vCPU was halted. This is not the case for s390
>>    but query-cpus is always causing the vCPU to exit the VM.
>>
>> 2. Semantics: on x86 and other architectures, halted is a highly transient
>>    state, which is likely to have already changed shortly after the state
>>    information has been retrieved. This is not the case for s390, where
>>    halted is an indication that the vCPU is stopped, meaning its not
>>    available to the guest operating system until it has been restarted.
>>
>> The following patches help to alleviate the issues:
>>
>> Patch 1/3:
>>   Adds architecture specific data to the QMP CpuInfo type, exposing
>>   the existing s390 cpu-state in QMP. The cpu-state is a representation
>>   more adequate than the ambiguous 'halted' condition.
>>
>>   Changes since original v2:
>>   - fixed cpu-state usage in hw/intc/s390_flic.c, necessary because
>>     master was updated in the meantime
>>   - removed superfluous newline while printing cpu-state
> 
> So this is adding s390x info in query-cpus (presumably for legacy
> callers?)...
> 
Yeah, as long as we have the slow call, the fast data should be in there
as well.
[...]
> 
> ...this is adding the new query-cpus-fast...
> 
[...]
> 
> ...and this is adding the s390x state to query-cpus-fast, right?
> 
Exactly.


-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast

[Dr. David Alan Gilbert]

* Viktor Mihajlovski (mihajlov@linux.vnet.ibm.com) wrote:
> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> The query-cpus command has an extremely serious side effect:
> it always interrupts all running vCPUs so that they can run
> ioctl calls. This can cause a huge performance degradation for
> some workloads. And most of the information retrieved by the
> ioctl calls are not even used by query-cpus.
> 
> This commit introduces a replacement for query-cpus called
> query-cpus-fast, which has the following features:
> 
>  o Never interrupt vCPUs threads. query-cpus-fast only returns
>    vCPU information maintained by QEMU itself, which should be
>    sufficient for most management software needs
> 
>  o Make "halted" field optional: we only return it if the
>    halted state is maintained by QEMU. But this also gives
>    the option of dropping the field in the future (see below)
> 
>  o Drop irrelevant fields such as "current", "pc" and "arch"
> 
>  o Drop field "halted" since it can't be provided fast reliably
>    and is too volatile on most architectures to be really useful
> 
>  o Rename some fields for better clarification & proper naming
>    standard
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>  cpus.c               | 38 ++++++++++++++++++++++++++++
>  hmp-commands-info.hx | 14 +++++++++++
>  hmp.c                | 21 ++++++++++++++++
>  hmp.h                |  1 +
>  qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 144 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 6006931..6df6660 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2156,6 +2156,44 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>      return head;
>  }
>  
> +/*
> + * fast means: we NEVER interrupt vCPU threads to retrieve
> + * information from KVM.
> + */
> +CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
> +{
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +    MachineClass *mc = MACHINE_GET_CLASS(ms);
> +    CpuInfoFastList *head = NULL, *cur_item = NULL;
> +    CPUState *cpu;
> +
> +    CPU_FOREACH(cpu) {
> +        CpuInfoFastList *info = g_malloc0(sizeof(*info));
> +        info->value = g_malloc0(sizeof(*info->value));
> +
> +        info->value->cpu_index = cpu->cpu_index;
> +        info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
> +        info->value->thread_id = cpu->thread_id;
> +
> +        info->value->has_props = !!mc->cpu_index_to_instance_props;
> +        if (info->value->has_props) {
> +            CpuInstanceProperties *props;
> +            props = g_malloc0(sizeof(*props));
> +            *props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index);
> +            info->value->props = props;
> +        }
> +
> +        if (!cur_item) {
> +            head = cur_item = info;
> +        } else {
> +            cur_item->next = info;
> +            cur_item = info;
> +        }
> +    }
> +
> +    return head;
> +}
> +
>  void qmp_memsave(int64_t addr, int64_t size, const char *filename,
>                   bool has_cpu, int64_t cpu_index, Error **errp)
>  {
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index ad590a4..16ac602 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -160,6 +160,20 @@ Show infos for each CPU.
>  ETEXI
>  
>      {
> +        .name       = "cpus_fast",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show information for each CPU without interrupting them",
> +        .cmd        = hmp_info_cpus_fast,
> +    },
> +
> +STEXI
> +@item info cpus_fast
> +@findex info cpus_fast
> +Show infos for each CPU without performance penalty.
> +ETEXI
> +
> +    {
>          .name       = "history",
>          .args_type  = "",
>          .params     = "",
> diff --git a/hmp.c b/hmp.c
> index a6b94b7..598bfe5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -410,6 +410,27 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>      qapi_free_CpuInfoList(cpu_list);
>  }
>  
> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
> +{
> +    CpuInfoFastList *head, *cpu;
> +    TargetInfo *target;
> +
> +    target = qmp_query_target(NULL);
> +    monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch);
> +    qapi_free_TargetInfo(target);
> +
> +    head = qmp_query_cpus_fast(NULL);
> +
> +    for (cpu = head; cpu; cpu = cpu->next) {
> +        monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
> +        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
> +        monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
> +        monitor_printf(mon, "\n");
> +    }
> +
> +    qapi_free_CpuInfoFastList(head);
> +}
> +
>  static void print_block_info(Monitor *mon, BlockInfo *info,
>                               BlockDeviceInfo *inserted, bool verbose)
>  {
> diff --git a/hmp.h b/hmp.h
> index 1143db4..93fb4e4 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
>  void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
>  void hmp_info_cpus(Monitor *mon, const QDict *qdict);
> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
>  void hmp_info_block(Monitor *mon, const QDict *qdict);
>  void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
>  void hmp_info_vnc(Monitor *mon, const QDict *qdict);

For HMP:

Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> diff --git a/qapi-schema.json b/qapi-schema.json
> index 66e0927..2a614af 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -552,6 +552,12 @@
>  #
>  # Returns a list of information about each virtual CPU.
>  #
> +# This command causes vCPU threads to exit to userspace, which causes
> +# an small interruption guest CPU execution. This will have a negative
> +# impact on realtime guests and other latency sensitive guest workloads.
> +# It is recommended to use @query-cpus-fast instead of this command to
> +# avoid the vCPU interruption.
> +#
>  # Returns: a list of @CpuInfo for each virtual CPU
>  #
>  # Since: 0.14.0
> @@ -585,6 +591,70 @@
>  { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
>  
>  ##
> +# @CpuInfoFast:
> +#
> +# Information about a virtual CPU
> +#
> +# @cpu-index: index of the virtual CPU
> +#
> +# @qom-path: path to the CPU object in the QOM tree
> +#
> +# @thread-id: ID of the underlying host thread
> +#
> +# @props: properties describing to which node/socket/core/thread
> +#         virtual CPU belongs to, provided if supported by board
> +#
> +# Since: 2.12
> +#
> +##
> +{ 'struct': 'CpuInfoFast',
> +  'data': {'cpu-index': 'int', 'qom-path': 'str',
> +           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> +
> +##
> +# @query-cpus-fast:
> +#
> +# Returns information about all virtual CPUs. This command does not
> +# incur a performance penalty and should be used in production
> +# instead of query-cpus.
> +#
> +# Returns: list of @CpuInfoFast
> +#
> +# Notes: The CPU architecture name is not returned by query-cpus-fast.
> +#        Use query-target to retrieve that information.
> +#
> +# Since: 2.12
> +#
> +# Example:
> +#
> +# -> { "execute": "query-cpus-fast" }
> +# <- { "return": [
> +#         {
> +#             "thread-id": 25627,
> +#             "props": {
> +#                 "core-id": 0,
> +#                 "thread-id": 0,
> +#                 "socket-id": 0
> +#             },
> +#             "qom-path": "/machine/unattached/device[0]",
> +#             "cpu-index": 0
> +#         },
> +#         {
> +#             "thread-id": 25628,
> +#             "props": {
> +#                 "core-id": 0,
> +#                 "thread-id": 0,
> +#                 "socket-id": 1
> +#             },
> +#             "qom-path": "/machine/unattached/device[2]",
> +#             "cpu-index": 1
> +#         }
> +#     ]
> +# }
> +##
> +{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] }
> +
> +##
>  # @IOThreadInfo:
>  #
>  # Information about an iothread
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info

[Luiz Capitulino]

On Mon, 12 Feb 2018 13:14:30 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> Presently s390x is the only architecture not exposing specific
> CPU information via QMP query-cpus. Upstream discussion has shown
> that it could make sense to report the architecture specific CPU
> state, e.g. to detect that a CPU has been stopped.
> 
> With this change the output of query-cpus will look like this on
> s390:
> 
>    [
>      {"arch": "s390", "current": true,
>       "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>       "qom_path": "/machine/unattached/device[0]",
>       "halted": false, "thread_id": 63115},
>      {"arch": "s390", "current": false,
>       "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>       "qom_path": "/machine/unattached/device[1]",
>       "halted": true, "thread_id": 63116}
>    ]

We're adding the same information to query-cpus-fast. Why do we
need to duplicate this into query-cpus? Do you plan to keep using
query-cpus? If yes, why?

Libvirt for one, should move away from it. We don't want to run
into the risk of having the same issue we had with x86 in other
archs.

> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Acked-by: Eric Blake <eblake@redhat.com>
> ---
>  cpus.c                     |  6 ++++++
>  hmp.c                      |  4 ++++
>  hw/intc/s390_flic.c        |  4 ++--
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json           | 28 +++++++++++++++++++++++++++-
>  target/s390x/cpu.c         | 24 ++++++++++++------------
>  target/s390x/cpu.h         |  7 ++-----
>  target/s390x/kvm.c         |  8 ++++----
>  target/s390x/sigp.c        | 38 +++++++++++++++++++-------------------
>  9 files changed, 77 insertions(+), 44 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index f298b65..6006931 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>          TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>          CPUTriCoreState *env = &tricore_cpu->env;
> +#elif defined(TARGET_S390X)
> +        S390CPU *s390_cpu = S390_CPU(cpu);
> +        CPUS390XState *env = &s390_cpu->env;
>  #endif
>  
>          cpu_synchronize_state(cpu);
> @@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>          info->value->arch = CPU_INFO_ARCH_TRICORE;
>          info->value->u.tricore.PC = env->PC;
> +#elif defined(TARGET_S390X)
> +        info->value->arch = CPU_INFO_ARCH_S390;
> +        info->value->u.s390.cpu_state = env->cpu_state;
>  #else
>          info->value->arch = CPU_INFO_ARCH_OTHER;
>  #endif
> diff --git a/hmp.c b/hmp.c
> index 7870d6a..a6b94b7 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>          case CPU_INFO_ARCH_TRICORE:
>              monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
>              break;
> +        case CPU_INFO_ARCH_S390:
> +            monitor_printf(mon, " state=%s",
> +                           CpuS390State_str(cpu->value->u.s390.cpu_state));
> +            break;
>          default:
>              break;
>          }
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index a85a149..5f8168f 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
>          cs->interrupt_request |= CPU_INTERRUPT_HARD;
>  
>          /* ignore CPUs that are not sleeping */
> -        if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
> -            s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
> +        if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
> +            s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
>              continue;
>          }
>  
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..4d0c3de 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -368,7 +368,7 @@ static void s390_machine_reset(void)
>  
>      /* all cpus are stopped - configure and start the ipl cpu only */
>      s390_ipl_prepare_cpu(ipl_cpu);
> -    s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>  }
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745..66e0927 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -410,10 +410,12 @@
>  # An enumeration of cpu types that enable additional information during
>  # @query-cpus.
>  #
> +# @s390: since 2.12
> +#
>  # Since: 2.6
>  ##
>  { 'enum': 'CpuInfoArch',
> -  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
> +  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
>  
>  ##
>  # @CpuInfo:
> @@ -452,6 +454,7 @@
>              'ppc': 'CpuInfoPPC',
>              'mips': 'CpuInfoMIPS',
>              'tricore': 'CpuInfoTricore',
> +            's390': 'CpuInfoS390',
>              'other': 'CpuInfoOther' } }
>  
>  ##
> @@ -522,6 +525,29 @@
>  { 'struct': 'CpuInfoOther', 'data': { } }
>  
>  ##
> +# @CpuS390State:
> +#
> +# An enumeration of cpu states that can be assumed by a virtual
> +# S390 CPU
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'CpuS390State',
> +  'prefix': 'S390_CPU_STATE',
> +  'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
> +
> +##
> +# @CpuInfoS390:
> +#
> +# Additional information about a virtual S390 CPU
> +#
> +# @cpu-state: the virtual CPU's state
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
> +
> +##
>  # @query-cpus:
>  #
>  # Returns a list of information about each virtual CPU.
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index da7cb9c..52b74ed 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -58,8 +58,8 @@ static bool s390_cpu_has_work(CPUState *cs)
>      S390CPU *cpu = S390_CPU(cs);
>  
>      /* STOPPED cpus can never wake up */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_LOAD &&
> -        s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD &&
> +        s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
>          return false;
>      }
>  
> @@ -77,7 +77,7 @@ static void s390_cpu_load_normal(CPUState *s)
>      S390CPU *cpu = S390_CPU(s);
>      cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
>      cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
> -    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
>  #endif
>  
> @@ -92,7 +92,7 @@ static void s390_cpu_reset(CPUState *s)
>      env->bpbc = false;
>      scc->parent_reset(s);
>      cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  }
>  
>  /* S390CPUClass::initial_reset() */
> @@ -135,7 +135,7 @@ static void s390_cpu_full_reset(CPUState *s)
>  
>      scc->parent_reset(s);
>      cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  
>      memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
>  
> @@ -247,7 +247,7 @@ static void s390_cpu_initfn(Object *obj)
>      env->tod_basetime = 0;
>      env->tod_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
>      env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
> -    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  #endif
>  }
>  
> @@ -275,8 +275,8 @@ static unsigned s390_count_running_cpus(void)
>  
>      CPU_FOREACH(cpu) {
>          uint8_t state = S390_CPU(cpu)->env.cpu_state;
> -        if (state == CPU_STATE_OPERATING ||
> -            state == CPU_STATE_LOAD) {
> +        if (state == S390_CPU_STATE_OPERATING ||
> +            state == S390_CPU_STATE_LOAD) {
>              if (!disabled_wait(cpu)) {
>                  nr_running++;
>              }
> @@ -315,13 +315,13 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>      trace_cpu_set_state(CPU(cpu)->cpu_index, cpu_state);
>  
>      switch (cpu_state) {
> -    case CPU_STATE_STOPPED:
> -    case CPU_STATE_CHECK_STOP:
> +    case S390_CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_CHECK_STOP:
>          /* halt the cpu for common infrastructure */
>          s390_cpu_halt(cpu);
>          break;
> -    case CPU_STATE_OPERATING:
> -    case CPU_STATE_LOAD:
> +    case S390_CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_LOAD:
>          /*
>           * Starting a CPU with a PSW WAIT bit set:
>           * KVM: handles this internally and triggers another WAIT exit.
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 21ce40d..66baa7a 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -141,12 +141,9 @@ struct CPUS390XState {
>       * architectures, there is a difference between a halt and a stop on s390.
>       * If all cpus are either stopped (including check stop) or in the disabled
>       * wait state, the vm can be shut down.
> +     * The acceptable cpu_state values are defined in the CpuInfoS390State
> +     * enum.
>       */
> -#define CPU_STATE_UNINITIALIZED        0x00
> -#define CPU_STATE_STOPPED              0x01
> -#define CPU_STATE_CHECK_STOP           0x02
> -#define CPU_STATE_OPERATING            0x03
> -#define CPU_STATE_LOAD                 0x04
>      uint8_t cpu_state;
>  
>      /* currently processed sigp order */
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 0301e9d..45dd1c5 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1881,16 +1881,16 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
>      }
>  
>      switch (cpu_state) {
> -    case CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_STOPPED:
>          mp_state.mp_state = KVM_MP_STATE_STOPPED;
>          break;
> -    case CPU_STATE_CHECK_STOP:
> +    case S390_CPU_STATE_CHECK_STOP:
>          mp_state.mp_state = KVM_MP_STATE_CHECK_STOP;
>          break;
> -    case CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_OPERATING:
>          mp_state.mp_state = KVM_MP_STATE_OPERATING;
>          break;
> -    case CPU_STATE_LOAD:
> +    case S390_CPU_STATE_LOAD:
>          mp_state.mp_state = KVM_MP_STATE_LOAD;
>          break;
>      default:
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index ac3f8e7..5a7a9c4 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -46,13 +46,13 @@ static void sigp_sense(S390CPU *dst_cpu, SigpInfo *si)
>      }
>  
>      /* sensing without locks is racy, but it's the same for real hw */
> -    if (state != CPU_STATE_STOPPED && !ext_call) {
> +    if (state != S390_CPU_STATE_STOPPED && !ext_call) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>      } else {
>          if (ext_call) {
>              status |= SIGP_STAT_EXT_CALL_PENDING;
>          }
> -        if (state == CPU_STATE_STOPPED) {
> +        if (state == S390_CPU_STATE_STOPPED) {
>              status |= SIGP_STAT_STOPPED;
>          }
>          set_sigp_status(si, status);
> @@ -94,12 +94,12 @@ static void sigp_start(CPUState *cs, run_on_cpu_data arg)
>      S390CPU *cpu = S390_CPU(cs);
>      SigpInfo *si = arg.host_ptr;
>  
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>          return;
>      }
>  
> -    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>      si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>  }
>  
> @@ -108,14 +108,14 @@ static void sigp_stop(CPUState *cs, run_on_cpu_data arg)
>      S390CPU *cpu = S390_CPU(cs);
>      SigpInfo *si = arg.host_ptr;
>  
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>          return;
>      }
>  
>      /* disabled wait - sleeping in user space */
>      if (cs->halted) {
> -        s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +        s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>      } else {
>          /* execute the stop function */
>          cpu->env.sigp_order = SIGP_STOP;
> @@ -130,17 +130,17 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>      SigpInfo *si = arg.host_ptr;
>  
>      /* disabled wait - sleeping in user space */
> -    if (s390_cpu_get_state(cpu) == CPU_STATE_OPERATING && cs->halted) {
> -        s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    if (s390_cpu_get_state(cpu) == S390_CPU_STATE_OPERATING && cs->halted) {
> +        s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>      }
>  
>      switch (s390_cpu_get_state(cpu)) {
> -    case CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_OPERATING:
>          cpu->env.sigp_order = SIGP_STOP_STORE_STATUS;
>          cpu_inject_stop(cpu);
>          /* store will be performed in do_stop_interrup() */
>          break;
> -    case CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_STOPPED:
>          /* already stopped, just store the status */
>          cpu_synchronize_state(cs);
>          s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
> @@ -156,7 +156,7 @@ static void sigp_store_status_at_address(CPUState *cs, run_on_cpu_data arg)
>      uint32_t address = si->param & 0x7ffffe00u;
>  
>      /* cpu has to be stopped */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
>          return;
>      }
> @@ -186,7 +186,7 @@ static void sigp_store_adtl_status(CPUState *cs, run_on_cpu_data arg)
>      }
>  
>      /* cpu has to be stopped */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
>          return;
>      }
> @@ -229,17 +229,17 @@ static void sigp_restart(CPUState *cs, run_on_cpu_data arg)
>      SigpInfo *si = arg.host_ptr;
>  
>      switch (s390_cpu_get_state(cpu)) {
> -    case CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_STOPPED:
>          /* the restart irq has to be delivered prior to any other pending irq */
>          cpu_synchronize_state(cs);
>          /*
>           * Set OPERATING (and unhalting) before loading the restart PSW.
>           * load_psw() will then properly halt the CPU again if necessary (TCG).
>           */
> -        s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>          do_restart_interrupt(&cpu->env);
>          break;
> -    case CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_OPERATING:
>          cpu_inject_restart(cpu);
>          break;
>      }
> @@ -285,7 +285,7 @@ static void sigp_set_prefix(CPUState *cs, run_on_cpu_data arg)
>      }
>  
>      /* cpu has to be stopped */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
>          return;
>      }
> @@ -318,7 +318,7 @@ static void sigp_cond_emergency(S390CPU *src_cpu, S390CPU *dst_cpu,
>      p_asn = dst_cpu->env.cregs[4] & 0xffff;  /* Primary ASN */
>      s_asn = dst_cpu->env.cregs[3] & 0xffff;  /* Secondary ASN */
>  
> -    if (s390_cpu_get_state(dst_cpu) != CPU_STATE_STOPPED ||
> +    if (s390_cpu_get_state(dst_cpu) != S390_CPU_STATE_STOPPED ||
>          (psw_mask & psw_int_mask) != psw_int_mask ||
>          (idle && psw_addr != 0) ||
>          (!idle && (asn == p_asn || asn == s_asn))) {
> @@ -435,7 +435,7 @@ static int sigp_set_architecture(S390CPU *cpu, uint32_t param,
>          if (cur_cpu == cpu) {
>              continue;
>          }
> -        if (s390_cpu_get_state(cur_cpu) != CPU_STATE_STOPPED) {
> +        if (s390_cpu_get_state(cur_cpu) != S390_CPU_STATE_STOPPED) {
>              all_stopped = false;
>          }
>      }
> @@ -492,7 +492,7 @@ void do_stop_interrupt(CPUS390XState *env)
>  {
>      S390CPU *cpu = s390_env_get_cpu(env);
>  
> -    if (s390_cpu_set_state(CPU_STATE_STOPPED, cpu) == 0) {
> +    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>          qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>      }
>      if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {

Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Luiz Capitulino]

On Mon, 12 Feb 2018 13:14:32 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> -{ 'struct': 'CpuInfoFast',
> -  'data': {'cpu-index': 'int', 'qom-path': 'str',
> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> +{ 'union': 'CpuInfoFast',
> +  'base': {'cpu-index': 'int', 'qom-path': 'str',
> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
> +           'arch': 'CpuInfoArch' },
> +  'discriminator': 'arch',
> +  'data': { 'x86': 'CpuInfoOther',
> +            'sparc': 'CpuInfoOther',
> +            'ppc': 'CpuInfoOther',
> +            'mips': 'CpuInfoOther',
> +            'tricore': 'CpuInfoOther',
> +            's390': 'CpuInfoS390Fast',
> +            'other': 'CpuInfoOther' } }

Consider this a minor comment (and QMP maintainers can give a much
better advice than me), but I think this arch list has problems. For
one thing, it's incomplete. And the second problem is the 'other'
field. What happens when QEMU starts supporting a new arch? 'other'
becomes the new arch. Is this compatible? I don't know.

I don't know if this would work with the QAPI, but you could have
a '*arch-data' field in the CpuInfoFast definition, like:

'data': { ..., '*arch-data': 'CpuInfoFastArchData' }

where 'CpuInfoFastArchData' is defined by each arch that supports
the field. An arch supporting the field could also export a
query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast().

Re: [Qemu-devel] [PATCH 1/3] qmp: expose s390-specific CPU info

[David Hildenbrand]

On 12.02.2018 13:14, Viktor Mihajlovski wrote:
> Presently s390x is the only architecture not exposing specific
> CPU information via QMP query-cpus. Upstream discussion has shown
> that it could make sense to report the architecture specific CPU
> state, e.g. to detect that a CPU has been stopped.
> 
> With this change the output of query-cpus will look like this on
> s390:
> 
>    [
>      {"arch": "s390", "current": true,
>       "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>       "qom_path": "/machine/unattached/device[0]",
>       "halted": false, "thread_id": 63115},
>      {"arch": "s390", "current": false,
>       "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>       "qom_path": "/machine/unattached/device[1]",
>       "halted": true, "thread_id": 63116}
>    ]
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Acked-by: Eric Blake <eblake@redhat.com>
> ---
>  cpus.c                     |  6 ++++++
>  hmp.c                      |  4 ++++
>  hw/intc/s390_flic.c        |  4 ++--
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json           | 28 +++++++++++++++++++++++++++-
>  target/s390x/cpu.c         | 24 ++++++++++++------------
>  target/s390x/cpu.h         |  7 ++-----
>  target/s390x/kvm.c         |  8 ++++----
>  target/s390x/sigp.c        | 38 +++++++++++++++++++-------------------
>  9 files changed, 77 insertions(+), 44 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index f298b65..6006931 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>          TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>          CPUTriCoreState *env = &tricore_cpu->env;
> +#elif defined(TARGET_S390X)
> +        S390CPU *s390_cpu = S390_CPU(cpu);
> +        CPUS390XState *env = &s390_cpu->env;
>  #endif
>  
>          cpu_synchronize_state(cpu);
> @@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>          info->value->arch = CPU_INFO_ARCH_TRICORE;
>          info->value->u.tricore.PC = env->PC;
> +#elif defined(TARGET_S390X)
> +        info->value->arch = CPU_INFO_ARCH_S390;
> +        info->value->u.s390.cpu_state = env->cpu_state;
>  #else
>          info->value->arch = CPU_INFO_ARCH_OTHER;
>  #endif
> diff --git a/hmp.c b/hmp.c
> index 7870d6a..a6b94b7 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>          case CPU_INFO_ARCH_TRICORE:
>              monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
>              break;
> +        case CPU_INFO_ARCH_S390:
> +            monitor_printf(mon, " state=%s",
> +                           CpuS390State_str(cpu->value->u.s390.cpu_state));
> +            break;
>          default:
>              break;
>          }
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index a85a149..5f8168f 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
>          cs->interrupt_request |= CPU_INTERRUPT_HARD;
>  
>          /* ignore CPUs that are not sleeping */
> -        if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
> -            s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
> +        if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
> +            s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
>              continue;
>          }
>  
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..4d0c3de 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -368,7 +368,7 @@ static void s390_machine_reset(void)
>  
>      /* all cpus are stopped - configure and start the ipl cpu only */
>      s390_ipl_prepare_cpu(ipl_cpu);
> -    s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>  }
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745..66e0927 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -410,10 +410,12 @@
>  # An enumeration of cpu types that enable additional information during
>  # @query-cpus.
>  #
> +# @s390: since 2.12
> +#
>  # Since: 2.6
>  ##
>  { 'enum': 'CpuInfoArch',
> -  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
> +  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
>  
>  ##
>  # @CpuInfo:
> @@ -452,6 +454,7 @@
>              'ppc': 'CpuInfoPPC',
>              'mips': 'CpuInfoMIPS',
>              'tricore': 'CpuInfoTricore',
> +            's390': 'CpuInfoS390',
>              'other': 'CpuInfoOther' } }
>  
>  ##
> @@ -522,6 +525,29 @@
>  { 'struct': 'CpuInfoOther', 'data': { } }
>  
>  ##
> +# @CpuS390State:
> +#
> +# An enumeration of cpu states that can be assumed by a virtual
> +# S390 CPU
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'CpuS390State',
> +  'prefix': 'S390_CPU_STATE',
> +  'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] }
> +
> +##
> +# @CpuInfoS390:
> +#
> +# Additional information about a virtual S390 CPU
> +#
> +# @cpu-state: the virtual CPU's state
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } }
> +
> +##
>  # @query-cpus:
>  #
>  # Returns a list of information about each virtual CPU.
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index da7cb9c..52b74ed 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -58,8 +58,8 @@ static bool s390_cpu_has_work(CPUState *cs)
>      S390CPU *cpu = S390_CPU(cs);
>  
>      /* STOPPED cpus can never wake up */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_LOAD &&
> -        s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD &&
> +        s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
>          return false;
>      }
>  
> @@ -77,7 +77,7 @@ static void s390_cpu_load_normal(CPUState *s)
>      S390CPU *cpu = S390_CPU(s);
>      cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
>      cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
> -    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>  }
>  #endif
>  
> @@ -92,7 +92,7 @@ static void s390_cpu_reset(CPUState *s)
>      env->bpbc = false;
>      scc->parent_reset(s);
>      cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  }
>  
>  /* S390CPUClass::initial_reset() */
> @@ -135,7 +135,7 @@ static void s390_cpu_full_reset(CPUState *s)
>  
>      scc->parent_reset(s);
>      cpu->env.sigp_order = 0;
> -    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  
>      memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
>  
> @@ -247,7 +247,7 @@ static void s390_cpu_initfn(Object *obj)
>      env->tod_basetime = 0;
>      env->tod_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
>      env->cpu_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
> -    s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>  #endif
>  }
>  
> @@ -275,8 +275,8 @@ static unsigned s390_count_running_cpus(void)
>  
>      CPU_FOREACH(cpu) {
>          uint8_t state = S390_CPU(cpu)->env.cpu_state;
> -        if (state == CPU_STATE_OPERATING ||
> -            state == CPU_STATE_LOAD) {
> +        if (state == S390_CPU_STATE_OPERATING ||
> +            state == S390_CPU_STATE_LOAD) {
>              if (!disabled_wait(cpu)) {
>                  nr_running++;
>              }
> @@ -315,13 +315,13 @@ unsigned int s390_cpu_set_state(uint8_t cpu_state, S390CPU *cpu)
>      trace_cpu_set_state(CPU(cpu)->cpu_index, cpu_state);
>  
>      switch (cpu_state) {
> -    case CPU_STATE_STOPPED:
> -    case CPU_STATE_CHECK_STOP:
> +    case S390_CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_CHECK_STOP:
>          /* halt the cpu for common infrastructure */
>          s390_cpu_halt(cpu);
>          break;
> -    case CPU_STATE_OPERATING:
> -    case CPU_STATE_LOAD:
> +    case S390_CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_LOAD:
>          /*
>           * Starting a CPU with a PSW WAIT bit set:
>           * KVM: handles this internally and triggers another WAIT exit.
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 21ce40d..66baa7a 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -141,12 +141,9 @@ struct CPUS390XState {
>       * architectures, there is a difference between a halt and a stop on s390.
>       * If all cpus are either stopped (including check stop) or in the disabled
>       * wait state, the vm can be shut down.
> +     * The acceptable cpu_state values are defined in the CpuInfoS390State
> +     * enum.
>       */
> -#define CPU_STATE_UNINITIALIZED        0x00
> -#define CPU_STATE_STOPPED              0x01
> -#define CPU_STATE_CHECK_STOP           0x02
> -#define CPU_STATE_OPERATING            0x03
> -#define CPU_STATE_LOAD                 0x04
>      uint8_t cpu_state;
>  
>      /* currently processed sigp order */
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index 0301e9d..45dd1c5 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -1881,16 +1881,16 @@ int kvm_s390_set_cpu_state(S390CPU *cpu, uint8_t cpu_state)
>      }
>  
>      switch (cpu_state) {
> -    case CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_STOPPED:
>          mp_state.mp_state = KVM_MP_STATE_STOPPED;
>          break;
> -    case CPU_STATE_CHECK_STOP:
> +    case S390_CPU_STATE_CHECK_STOP:
>          mp_state.mp_state = KVM_MP_STATE_CHECK_STOP;
>          break;
> -    case CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_OPERATING:
>          mp_state.mp_state = KVM_MP_STATE_OPERATING;
>          break;
> -    case CPU_STATE_LOAD:
> +    case S390_CPU_STATE_LOAD:
>          mp_state.mp_state = KVM_MP_STATE_LOAD;
>          break;
>      default:
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index ac3f8e7..5a7a9c4 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -46,13 +46,13 @@ static void sigp_sense(S390CPU *dst_cpu, SigpInfo *si)
>      }
>  
>      /* sensing without locks is racy, but it's the same for real hw */
> -    if (state != CPU_STATE_STOPPED && !ext_call) {
> +    if (state != S390_CPU_STATE_STOPPED && !ext_call) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>      } else {
>          if (ext_call) {
>              status |= SIGP_STAT_EXT_CALL_PENDING;
>          }
> -        if (state == CPU_STATE_STOPPED) {
> +        if (state == S390_CPU_STATE_STOPPED) {
>              status |= SIGP_STAT_STOPPED;
>          }
>          set_sigp_status(si, status);
> @@ -94,12 +94,12 @@ static void sigp_start(CPUState *cs, run_on_cpu_data arg)
>      S390CPU *cpu = S390_CPU(cs);
>      SigpInfo *si = arg.host_ptr;
>  
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>          return;
>      }
>  
> -    s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> +    s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>      si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>  }
>  
> @@ -108,14 +108,14 @@ static void sigp_stop(CPUState *cs, run_on_cpu_data arg)
>      S390CPU *cpu = S390_CPU(cs);
>      SigpInfo *si = arg.host_ptr;
>  
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>          return;
>      }
>  
>      /* disabled wait - sleeping in user space */
>      if (cs->halted) {
> -        s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +        s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>      } else {
>          /* execute the stop function */
>          cpu->env.sigp_order = SIGP_STOP;
> @@ -130,17 +130,17 @@ static void sigp_stop_and_store_status(CPUState *cs, run_on_cpu_data arg)
>      SigpInfo *si = arg.host_ptr;
>  
>      /* disabled wait - sleeping in user space */
> -    if (s390_cpu_get_state(cpu) == CPU_STATE_OPERATING && cs->halted) {
> -        s390_cpu_set_state(CPU_STATE_STOPPED, cpu);
> +    if (s390_cpu_get_state(cpu) == S390_CPU_STATE_OPERATING && cs->halted) {
> +        s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>      }
>  
>      switch (s390_cpu_get_state(cpu)) {
> -    case CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_OPERATING:
>          cpu->env.sigp_order = SIGP_STOP_STORE_STATUS;
>          cpu_inject_stop(cpu);
>          /* store will be performed in do_stop_interrup() */
>          break;
> -    case CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_STOPPED:
>          /* already stopped, just store the status */
>          cpu_synchronize_state(cs);
>          s390_store_status(cpu, S390_STORE_STATUS_DEF_ADDR, true);
> @@ -156,7 +156,7 @@ static void sigp_store_status_at_address(CPUState *cs, run_on_cpu_data arg)
>      uint32_t address = si->param & 0x7ffffe00u;
>  
>      /* cpu has to be stopped */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
>          return;
>      }
> @@ -186,7 +186,7 @@ static void sigp_store_adtl_status(CPUState *cs, run_on_cpu_data arg)
>      }
>  
>      /* cpu has to be stopped */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
>          return;
>      }
> @@ -229,17 +229,17 @@ static void sigp_restart(CPUState *cs, run_on_cpu_data arg)
>      SigpInfo *si = arg.host_ptr;
>  
>      switch (s390_cpu_get_state(cpu)) {
> -    case CPU_STATE_STOPPED:
> +    case S390_CPU_STATE_STOPPED:
>          /* the restart irq has to be delivered prior to any other pending irq */
>          cpu_synchronize_state(cs);
>          /*
>           * Set OPERATING (and unhalting) before loading the restart PSW.
>           * load_psw() will then properly halt the CPU again if necessary (TCG).
>           */
> -        s390_cpu_set_state(CPU_STATE_OPERATING, cpu);
> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>          do_restart_interrupt(&cpu->env);
>          break;
> -    case CPU_STATE_OPERATING:
> +    case S390_CPU_STATE_OPERATING:
>          cpu_inject_restart(cpu);
>          break;
>      }
> @@ -285,7 +285,7 @@ static void sigp_set_prefix(CPUState *cs, run_on_cpu_data arg)
>      }
>  
>      /* cpu has to be stopped */
> -    if (s390_cpu_get_state(cpu) != CPU_STATE_STOPPED) {
> +    if (s390_cpu_get_state(cpu) != S390_CPU_STATE_STOPPED) {
>          set_sigp_status(si, SIGP_STAT_INCORRECT_STATE);
>          return;
>      }
> @@ -318,7 +318,7 @@ static void sigp_cond_emergency(S390CPU *src_cpu, S390CPU *dst_cpu,
>      p_asn = dst_cpu->env.cregs[4] & 0xffff;  /* Primary ASN */
>      s_asn = dst_cpu->env.cregs[3] & 0xffff;  /* Secondary ASN */
>  
> -    if (s390_cpu_get_state(dst_cpu) != CPU_STATE_STOPPED ||
> +    if (s390_cpu_get_state(dst_cpu) != S390_CPU_STATE_STOPPED ||
>          (psw_mask & psw_int_mask) != psw_int_mask ||
>          (idle && psw_addr != 0) ||
>          (!idle && (asn == p_asn || asn == s_asn))) {
> @@ -435,7 +435,7 @@ static int sigp_set_architecture(S390CPU *cpu, uint32_t param,
>          if (cur_cpu == cpu) {
>              continue;
>          }
> -        if (s390_cpu_get_state(cur_cpu) != CPU_STATE_STOPPED) {
> +        if (s390_cpu_get_state(cur_cpu) != S390_CPU_STATE_STOPPED) {
>              all_stopped = false;
>          }
>      }
> @@ -492,7 +492,7 @@ void do_stop_interrupt(CPUS390XState *env)
>  {
>      S390CPU *cpu = s390_env_get_cpu(env);
>  
> -    if (s390_cpu_set_state(CPU_STATE_STOPPED, cpu) == 0) {
> +    if (s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu) == 0) {
>          qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>      }
>      if (cpu->env.sigp_order == SIGP_STOP_STORE_STATUS) {
> 

Wonder if we should convert all the uint8_t into proper enum types now.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast

[David Hildenbrand]

On 12.02.2018 13:14, Viktor Mihajlovski wrote:
> From: Luiz Capitulino <lcapitulino@redhat.com>
> 
> The query-cpus command has an extremely serious side effect:
> it always interrupts all running vCPUs so that they can run
> ioctl calls. This can cause a huge performance degradation for
> some workloads. And most of the information retrieved by the
> ioctl calls are not even used by query-cpus.
> 
> This commit introduces a replacement for query-cpus called
> query-cpus-fast, which has the following features:
> 
>  o Never interrupt vCPUs threads. query-cpus-fast only returns
>    vCPU information maintained by QEMU itself, which should be
>    sufficient for most management software needs
> 
>  o Make "halted" field optional: we only return it if the
>    halted state is maintained by QEMU. But this also gives
>    the option of dropping the field in the future (see below)
> 

If I'm not wrong, this comment is superseded by ...

>  o Drop irrelevant fields such as "current", "pc" and "arch"
> 
>  o Drop field "halted" since it can't be provided fast reliably
>    and is too volatile on most architectures to be really useful
> 

this comment :)

>  o Rename some fields for better clarification & proper naming
>    standard>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

Wondering if we could tweak the old interface with a simple flag "fast =
true".


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] qmp: expose s390-specific CPU info

[David Hildenbrand]

On 12.02.2018 19:03, Luiz Capitulino wrote:
> On Mon, 12 Feb 2018 13:14:30 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 
>> Presently s390x is the only architecture not exposing specific
>> CPU information via QMP query-cpus. Upstream discussion has shown
>> that it could make sense to report the architecture specific CPU
>> state, e.g. to detect that a CPU has been stopped.
>>
>> With this change the output of query-cpus will look like this on
>> s390:
>>
>>    [
>>      {"arch": "s390", "current": true,
>>       "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>>       "qom_path": "/machine/unattached/device[0]",
>>       "halted": false, "thread_id": 63115},
>>      {"arch": "s390", "current": false,
>>       "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>>       "qom_path": "/machine/unattached/device[1]",
>>       "halted": true, "thread_id": 63116}
>>    ]
> 
> We're adding the same information to query-cpus-fast. Why do we
> need to duplicate this into query-cpus? Do you plan to keep using
> query-cpus? If yes, why?

Wonder if we could simply pass a flag to query-cpus "fast=true", that
makes it behave differently. (either not indicate the critical values or
simply provide dummy values - e.g. simply halted=false)

> 
> Libvirt for one, should move away from it. We don't want to run
> into the risk of having the same issue we had with x86 in other
> archs.
> 
>>


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] qmp: expose s390-specific CPU info

[Viktor Mihajlovski]

On 13.02.2018 12:16, David Hildenbrand wrote:
> On 12.02.2018 19:03, Luiz Capitulino wrote:
>> On Mon, 12 Feb 2018 13:14:30 +0100
>> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
>>
>>> Presently s390x is the only architecture not exposing specific
>>> CPU information via QMP query-cpus. Upstream discussion has shown
>>> that it could make sense to report the architecture specific CPU
>>> state, e.g. to detect that a CPU has been stopped.
>>>
>>> With this change the output of query-cpus will look like this on
>>> s390:
>>>
>>>    [
>>>      {"arch": "s390", "current": true,
>>>       "props": {"core-id": 0}, "cpu-state": "operating", "CPU": 0,
>>>       "qom_path": "/machine/unattached/device[0]",
>>>       "halted": false, "thread_id": 63115},
>>>      {"arch": "s390", "current": false,
>>>       "props": {"core-id": 1}, "cpu-state": "stopped", "CPU": 1,
>>>       "qom_path": "/machine/unattached/device[1]",
>>>       "halted": true, "thread_id": 63116}
>>>    ]
>>
>> We're adding the same information to query-cpus-fast. Why do we
>> need to duplicate this into query-cpus? Do you plan to keep using
>> query-cpus? If yes, why?
> 
> Wonder if we could simply pass a flag to query-cpus "fast=true", that
> makes it behave differently. (either not indicate the critical values or
> simply provide dummy values - e.g. simply halted=false)
> 
That was one of the ideas in the earlier stages of this discussion (and
I was inclined to go that way initially). The major drawback of this
approach that the slow call is hard to deprecate (OK, one could change
the default to fast=true over time). It's easier to deprecate the entire
query-cpus function. The other issue, maybe not as bad, is that one has
to deal with fields that are suddenly optional or obsolete in way not
confusing every one.
Bottom line is that I'm convinced it's better to have both APIs and to
deprecate the slow one over time. But I have to confess I'm not familiar
with QAPI deprecation rules, maybe Eric can shed some light on this...
>>
>> Libvirt for one, should move away from it. We don't want to run
>> into the risk of having the same issue we had with x86 in other
>> archs.
>>
>>>
> 
> 


-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Viktor Mihajlovski]

On 12.02.2018 19:15, Luiz Capitulino wrote:
> On Mon, 12 Feb 2018 13:14:32 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 
>> -{ 'struct': 'CpuInfoFast',
>> -  'data': {'cpu-index': 'int', 'qom-path': 'str',
>> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
>> +{ 'union': 'CpuInfoFast',
>> +  'base': {'cpu-index': 'int', 'qom-path': 'str',
>> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
>> +           'arch': 'CpuInfoArch' },
>> +  'discriminator': 'arch',
>> +  'data': { 'x86': 'CpuInfoOther',
>> +            'sparc': 'CpuInfoOther',
>> +            'ppc': 'CpuInfoOther',
>> +            'mips': 'CpuInfoOther',
>> +            'tricore': 'CpuInfoOther',
>> +            's390': 'CpuInfoS390Fast',
>> +            'other': 'CpuInfoOther' } }
> 
> Consider this a minor comment (and QMP maintainers can give a much
> better advice than me), but I think this arch list has problems. For
> one thing, it's incomplete. And the second problem is the 'other'
> field. What happens when QEMU starts supporting a new arch? 'other'
> becomes the new arch. Is this compatible? I don't know.
This seems to be the customary approach, see
https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html
> 
> I don't know if this would work with the QAPI, but you could have
> a '*arch-data' field in the CpuInfoFast definition, like:
> 
> 'data': { ..., '*arch-data': 'CpuInfoFastArchData' }
> 
> where 'CpuInfoFastArchData' is defined by each arch that supports
> the field. An arch supporting the field could also export a
> query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast().
> 
I had it like this in my first reply to your initial patch. However,
that adds an additional hierarchy level in the return data. This
prevents clients like libvirt to reuse the data extraction code when
they switch over to using query-cpus-fast.

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Luiz Capitulino]

On Tue, 13 Feb 2018 13:30:02 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> On 12.02.2018 19:15, Luiz Capitulino wrote:
> > On Mon, 12 Feb 2018 13:14:32 +0100
> > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> >   
> >> -{ 'struct': 'CpuInfoFast',
> >> -  'data': {'cpu-index': 'int', 'qom-path': 'str',
> >> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> >> +{ 'union': 'CpuInfoFast',
> >> +  'base': {'cpu-index': 'int', 'qom-path': 'str',
> >> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
> >> +           'arch': 'CpuInfoArch' },
> >> +  'discriminator': 'arch',
> >> +  'data': { 'x86': 'CpuInfoOther',
> >> +            'sparc': 'CpuInfoOther',
> >> +            'ppc': 'CpuInfoOther',
> >> +            'mips': 'CpuInfoOther',
> >> +            'tricore': 'CpuInfoOther',
> >> +            's390': 'CpuInfoS390Fast',
> >> +            'other': 'CpuInfoOther' } }  
> > 
> > Consider this a minor comment (and QMP maintainers can give a much
> > better advice than me), but I think this arch list has problems. For
> > one thing, it's incomplete. And the second problem is the 'other'
> > field. What happens when QEMU starts supporting a new arch? 'other'
> > becomes the new arch. Is this compatible? I don't know.  
> This seems to be the customary approach, see
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html
> > 
> > I don't know if this would work with the QAPI, but you could have
> > a '*arch-data' field in the CpuInfoFast definition, like:
> > 
> > 'data': { ..., '*arch-data': 'CpuInfoFastArchData' }
> > 
> > where 'CpuInfoFastArchData' is defined by each arch that supports
> > the field. An arch supporting the field could also export a
> > query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast().
> >   
> I had it like this in my first reply to your initial patch. However,
> that adds an additional hierarchy level in the return data. This
> prevents clients like libvirt to reuse the data extraction code when
> they switch over to using query-cpus-fast.

OK, fair enough.

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] qmp: expose s390-specific CPU info

[Eric Blake]

On 02/13/2018 06:20 AM, Viktor Mihajlovski wrote:
>>> We're adding the same information to query-cpus-fast. Why do we
>>> need to duplicate this into query-cpus? Do you plan to keep using
>>> query-cpus? If yes, why?
>>
>> Wonder if we could simply pass a flag to query-cpus "fast=true", that
>> makes it behave differently. (either not indicate the critical values or
>> simply provide dummy values - e.g. simply halted=false)
>>
> That was one of the ideas in the earlier stages of this discussion (and
> I was inclined to go that way initially). The major drawback of this
> approach that the slow call is hard to deprecate (OK, one could change
> the default to fast=true over time). It's easier to deprecate the entire
> query-cpus function. The other issue, maybe not as bad, is that one has
> to deal with fields that are suddenly optional or obsolete in way not
> confusing every one.
> Bottom line is that I'm convinced it's better to have both APIs and to
> deprecate the slow one over time. But I have to confess I'm not familiar
> with QAPI deprecation rules, maybe Eric can shed some light on this...

You are correct that it is easier to have two commands if we plan for 
one to completely disappear (after a proper deprecation period, where it 
is well-documented for a couple of releases that we plan on removing the 
older command).  The alternative of adding a bool that controls whether 
the painful fields are omitted, is partially introspecitble (you can 
learn whether the new bool exists, in which case you use it for the new 
behavior), but later changing the default value of that bool over time 
is not (as we don't yet have a way to introspect default values - maybe 
we should add that someday, but we're not there yet).  But right now, it 
is easy to introspect the addition of a new command (if it exists, use 
it) and a later disappearance of a command.  So I'm in agreement with 
your approach of a new command.

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

Re: [Qemu-devel] [PATCH 2/3] qmp: add query-cpus-fast

[Viktor Mihajlovski]

On 12.02.2018 17:50, Dr. David Alan Gilbert wrote:
[...]
>> diff --git a/hmp.c b/hmp.c
>> index a6b94b7..598bfe5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -410,6 +410,27 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>>      qapi_free_CpuInfoList(cpu_list);
>>  }
>>  
>> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
>> +{
>> +    CpuInfoFastList *head, *cpu;
>> +    TargetInfo *target;
>> +
>> +    target = qmp_query_target(NULL);
>> +    monitor_printf(mon, "CPU architecture is '%s'\n\n", target->arch);
>> +    qapi_free_TargetInfo(target);
>> +
>> +    head = qmp_query_cpus_fast(NULL);
>> +
>> +    for (cpu = head; cpu; cpu = cpu->next) {
>> +        monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index);
>> +        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
>> +        monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
>> +        monitor_printf(mon, "\n");
>> +    }
I started some prototyping in libvirt and stumbled over the changed
layout of "info cpus_fast" vs. "info cpus". IMHO it would be better to
stick with the original format (one line per CPU). I'll post a v2.
>> +
>> +    qapi_free_CpuInfoFastList(head);
>> +}
>> +
>>  static void print_block_info(Monitor *mon, BlockInfo *info,
>>                               BlockDeviceInfo *inserted, bool verbose)
>>  {
>> diff --git a/hmp.h b/hmp.h
>> index 1143db4..93fb4e4 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -29,6 +29,7 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict);
>>  void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict);
>>  void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict);
>>  void hmp_info_cpus(Monitor *mon, const QDict *qdict);
>> +void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict);
>>  void hmp_info_block(Monitor *mon, const QDict *qdict);
>>  void hmp_info_blockstats(Monitor *mon, const QDict *qdict);
>>  void hmp_info_vnc(Monitor *mon, const QDict *qdict);
> 
> For HMP:
> 
> Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
[...]

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Viktor Mihajlovski]

On 12.02.2018 17:23, Cornelia Huck wrote:
[...]
>> diff --git a/cpus.c b/cpus.c
>> index 6df6660..af67826 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>>      MachineClass *mc = MACHINE_GET_CLASS(ms);
>>      CpuInfoFastList *head = NULL, *cur_item = NULL;
>>      CPUState *cpu;
>> +#if defined(TARGET_S390X)
>> +    S390CPU *s390_cpu;
>> +    CPUS390XState *env;
>> +#endif
>>  
>>      CPU_FOREACH(cpu) {
>>          CpuInfoFastList *info = g_malloc0(sizeof(*info));
>> @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
>>              info->value->props = props;
>>          }
>>  
>> +#if defined(TARGET_S390X)
>> +        s390_cpu = S390_CPU(cpu);
>> +        env = &s390_cpu->env;
> 
> You should be able to omit the s390_cpu variable by using
> 
>            env = &S390_CPU(cpu)->env;
> 
True, but I wanted to stay in style with the code in qmp_query_cpus.
>> +        info->value->arch = CPU_INFO_ARCH_S390;
>> +        info->value->u.s390.cpu_state = env->cpu_state;
>> +#endif
>>          if (!cur_item) {
>>              head = cur_item = info;
>>          } else {
> 
> As you mentioned in the patch description, the duplication is a bit
> awkward. I'll let the QAPI experts judge that; otherwise, this looks
> fine to me.
> 

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast

[Cornelia Huck]

On Tue, 13 Feb 2018 17:12:50 +0100
Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:

> On 12.02.2018 17:23, Cornelia Huck wrote:
> [...]
> >> diff --git a/cpus.c b/cpus.c
> >> index 6df6660..af67826 100644
> >> --- a/cpus.c
> >> +++ b/cpus.c
> >> @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
> >>      MachineClass *mc = MACHINE_GET_CLASS(ms);
> >>      CpuInfoFastList *head = NULL, *cur_item = NULL;
> >>      CPUState *cpu;
> >> +#if defined(TARGET_S390X)
> >> +    S390CPU *s390_cpu;
> >> +    CPUS390XState *env;
> >> +#endif
> >>  
> >>      CPU_FOREACH(cpu) {
> >>          CpuInfoFastList *info = g_malloc0(sizeof(*info));
> >> @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp)
> >>              info->value->props = props;
> >>          }
> >>  
> >> +#if defined(TARGET_S390X)
> >> +        s390_cpu = S390_CPU(cpu);
> >> +        env = &s390_cpu->env;  
> > 
> > You should be able to omit the s390_cpu variable by using
> > 
> >            env = &S390_CPU(cpu)->env;
> >   
> True, but I wanted to stay in style with the code in qmp_query_cpus.


TBH, I don't care too much one way or the other :)

> >> +        info->value->arch = CPU_INFO_ARCH_S390;
> >> +        info->value->u.s390.cpu_state = env->cpu_state;
> >> +#endif
> >>          if (!cur_item) {
> >>              head = cur_item = info;
> >>          } else {  
> > 
> > As you mentioned in the patch description, the duplication is a bit
> > awkward. I'll let the QAPI experts judge that; otherwise, this looks
> > fine to me.
> >   
>