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

[Qemu-devel] [PATCHv2 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.

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.

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.

Series v1 -> v2:
Patch 2/3:
  o Changed formatting of hmp info cpus_fast to match that of
    info cpus. This makes it easier for clients to switch to
    the fast call.

Patch 3/3:
  o Same formatting change for info cpus_fast as in 2/3, only
    for s390-specific cpu state.

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                      |  28 +++++++++++
 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, 257 insertions(+), 44 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCHv2 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 0262b9f..94d560e 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] [PATCHv2 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", "arch"
   and "halted"

 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                | 14 +++++++++++
 hmp.h                |  1 +
 qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 137 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..0bd3b3a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -410,6 +410,20 @@ 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;
+
+    head = qmp_query_cpus_fast(NULL);
+
+    for (cpu = head; cpu; cpu = cpu->next) {
+        monitor_printf(mon, "  CPU #%" PRId64 ":", cpu->value->cpu_index);
+        monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
+    }
+
+    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 94d560e..dc07729 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] [PATCHv2 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            | 10 ++++++++++
 qapi-schema.json | 35 +++++++++++++++++++++++++++++------
 3 files changed, 49 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 0bd3b3a..e27433e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -418,6 +418,16 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
 
     for (cpu = head; cpu; cpu = cpu->next) {
         monitor_printf(mon, "  CPU #%" PRId64 ":", cpu->value->cpu_index);
+
+        switch (cpu->value->arch) {
+        case CPU_INFO_ARCH_S390:
+            monitor_printf(mon, " state=%s",
+                           CpuS390State_str(cpu->value->u.s390.cpu_state));
+            break;
+        default:
+            break;
+        }
+
         monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id);
     }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index dc07729..b71f8cc 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] [PATCHv2 1/3] qmp: expose s390-specific CPU info

[Christian Borntraeger]

On 02/13/2018 06:18 PM, 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>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

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

[David Hildenbrand]

On 13.02.2018 18:18, 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 0262b9f..94d560e 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) {
> 

Looks like you dropped my r-b.

-- 

Thanks,

David / dhildenb

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

[Cornelia Huck]

On Tue, 13 Feb 2018 18:18:47 +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", "arch"
>    and "halted"

I'd suggest updating this description, as "halted" is now gone
completely... what about:

o Drop the "halted" field, even if the halted state is maintained by
QEMU. It had unclear semantics anyway.

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

> 
>  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                | 14 +++++++++++
>  hmp.h                |  1 +
>  qapi-schema.json     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 137 insertions(+)

Otherwise, looks good to me.

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

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

[Cornelia Huck]

On Tue, 13 Feb 2018 18:18:48 +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.

What would be a realistic timeframe for deprecation/removal of
query-cpus, considering the libvirt usage? Are we aware of other users?

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

Patch looks good to me.

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

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

[Cornelia Huck]

On Tue, 13 Feb 2018 18:18:45 +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.
> 
> 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.
> 
> 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.
> 
> 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.

How shall we proceed with this series? Patch 3 depends upon patch 1, so
I think it makes sense to merge this in one go.

I can give my R-b on patch 1 and Someone(tm) can merge this, or I can
take the whole series through the s390 tree (with some further
reviews/acks on patches 2/3).

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

[Viktor Mihajlovski]

On 14.02.2018 11:57, Cornelia Huck wrote:
[...]
> 
> How shall we proceed with this series? Patch 3 depends upon patch 1, so
> I think it makes sense to merge this in one go.
> 
> I can give my R-b on patch 1 and Someone(tm) can merge this, or I can
> take the whole series through the s390 tree (with some further
> reviews/acks on patches 2/3).
> 
Let's wait for the outstanding reviews (QAPI/HMP). If no major concerns
are voiced, the series could go in via the s390 tree.
As David H. pointed out, I've missed his r-b on patch 1, so you could
add that along with Christian's if no v3 is required.

-- 
Regards,
 Viktor Mihajlovski

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

[Eric Blake]

On 02/14/2018 04:15 AM, Cornelia Huck wrote:
> On Tue, 13 Feb 2018 18:18:48 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 

>> 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.
> 
> What would be a realistic timeframe for deprecation/removal of
> query-cpus, considering the libvirt usage? Are we aware of other users?

Well, if we want to start the clock ticking, this series also needs to 
touch qemu-doc.texi to start the deprecation clock, then we go at least 
two more releases with support for both old and new commands.

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

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

[Eric Blake]

On 02/14/2018 04:57 AM, Cornelia Huck wrote:
> On Tue, 13 Feb 2018 18:18:45 +0100
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> 

>> 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.
>>
>> 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.
>>
>> 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.
> 
> How shall we proceed with this series? Patch 3 depends upon patch 1, so
> I think it makes sense to merge this in one go.
> 
> I can give my R-b on patch 1 and Someone(tm) can merge this, or I can
> take the whole series through the s390 tree (with some further
> reviews/acks on patches 2/3).

I'd still like to give a thorough QMP review; it's on my list to get to 
today.

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

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

[Cornelia Huck]

On Wed, 14 Feb 2018 09:16:15 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 02/14/2018 04:57 AM, Cornelia Huck wrote:
> > On Tue, 13 Feb 2018 18:18:45 +0100
> > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> >   
> 
> >> 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.
> >>
> >> 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.
> >>
> >> 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.  
> > 
> > How shall we proceed with this series? Patch 3 depends upon patch 1, so
> > I think it makes sense to merge this in one go.
> > 
> > I can give my R-b on patch 1 and Someone(tm) can merge this, or I can
> > take the whole series through the s390 tree (with some further
> > reviews/acks on patches 2/3).  
> 
> I'd still like to give a thorough QMP review; it's on my list to get to 
> today.
> 

Sure, no need to hurry.

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

[Cornelia Huck]

On Wed, 14 Feb 2018 09:15:23 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 02/14/2018 04:15 AM, Cornelia Huck wrote:
> > On Tue, 13 Feb 2018 18:18:48 +0100
> > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
> >   
> 
> >> 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.  
> > 
> > What would be a realistic timeframe for deprecation/removal of
> > query-cpus, considering the libvirt usage? Are we aware of other users?  
> 
> Well, if we want to start the clock ticking, this series also needs to 
> touch qemu-doc.texi to start the deprecation clock, then we go at least 
> two more releases with support for both old and new commands.
> 

I'd like to know first what libvirt plans to do -- no sense in starting
deprecation if we're still stuck with it for a while.

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

[Dr. David Alan Gilbert]

* Viktor Mihajlovski (mihajlov@linux.vnet.ibm.com) wrote:
> On 14.02.2018 11:57, Cornelia Huck wrote:
> [...]
> > 
> > How shall we proceed with this series? Patch 3 depends upon patch 1, so
> > I think it makes sense to merge this in one go.
> > 
> > I can give my R-b on patch 1 and Someone(tm) can merge this, or I can
> > take the whole series through the s390 tree (with some further
> > reviews/acks on patches 2/3).
> > 
> Let's wait for the outstanding reviews (QAPI/HMP). If no major concerns
> are voiced, the series could go in via the s390 tree.
> As David H. pointed out, I've missed his r-b on patch 1, so you could
> add that along with Christian's if no v3 is required.

I'm happy with the HMP changes in teh series; they're only minor, so

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

> 
> -- 
> Regards,
>  Viktor Mihajlovski
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Viktor Mihajlovski]

On 14.02.2018 16:27, Cornelia Huck wrote:
> On Wed, 14 Feb 2018 09:15:23 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
>> On 02/14/2018 04:15 AM, Cornelia Huck wrote:
>>> On Tue, 13 Feb 2018 18:18:48 +0100
>>> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
>>>   
>>
>>>> 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.  
>>>
>>> What would be a realistic timeframe for deprecation/removal of
>>> query-cpus, considering the libvirt usage? Are we aware of other users?  
>>
>> Well, if we want to start the clock ticking, this series also needs to 
>> touch qemu-doc.texi to start the deprecation clock, then we go at least 
>> two more releases with support for both old and new commands.
>>
> 
> I'd like to know first what libvirt plans to do -- no sense in starting
> deprecation if we're still stuck with it for a while.
> 
FWIW, I'm currently preparing libvirt patches to use query-cpus-fast if
available. I wouldn't see why it would take more than one libvirt
release to get them in.

The question might rather be, which combinations of qemu and libvirt are
considered useful. E.g., I didn't upgrade libvirt in a while on my test
system but am using bleeding edge QEMU. But that doesn't necessarily
resemble a valid setup.

-- 
Regards,
 Viktor Mihajlovski

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

[Eric Blake]

On 02/14/2018 09:50 AM, Viktor Mihajlovski wrote:

>> I'd like to know first what libvirt plans to do -- no sense in starting
>> deprecation if we're still stuck with it for a while.
>>
> FWIW, I'm currently preparing libvirt patches to use query-cpus-fast if
> available. I wouldn't see why it would take more than one libvirt
> release to get them in.

And libvirt releases more frequently, so that's probably not a limiting 
factor for deprecating it now in qemu.

> 
> The question might rather be, which combinations of qemu and libvirt are
> considered useful. E.g., I didn't upgrade libvirt in a while on my test
> system but am using bleeding edge QEMU. But that doesn't necessarily
> resemble a valid setup.

In general, new libvirt and old qemu is supported (libvirt tries hard to 
make sure it can manage older images gracefully, for as long as older 
qemu is still supported by a distro), but old libvirt and new qemu is 
unsupported (you often get lucky where it works for a release or two, 
because qemu tries hard not to break back-compat without proper 
deprecation periods, but there may be features that you NEED a newer 
libvirt to properly drive, and this is one of those cases).

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

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

[Eric Blake]

On 02/13/2018 11:18 AM, 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)
> 
>   o Drop irrelevant fields such as "current", "pc", "arch"
>     and "halted"
> 

Others have pointed out the commit message inconsistencies; I'm focusing 
on the QMP.

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

s/an small interruption/a small interruption to/

> +# 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' } }

Comparing against the existing CpuInfo: if I'm not mistaken, you've renamed:

CPU -> cpu-index
qom_path -> qom-path
thread_id -> thread-id

and kept props unchanged.

If we REALLY cared about reducing duplication, we can do:

{ 'struct': 'CpuInfoFast',
   'data': { 'CPU': 'int', 'qom_path': 'str', 'thread_id': 'int',
             '*props': 'CpuInstanceProperties' } }
{ 'struct': 'CpuInfoBase',
   'base': 'CpuInfoFast',
   'data': { 'current': 'bool', 'halted': 'bool',
             'arch': 'CpuInfoArch' } }
{ 'union': 'CpuInfo', 'base': 'CpuInfoBase', 'discriminator': 'arch',
   'data': { ... } }

but I'm not yet convinced whether we need that.  If we're going to 
deprecate the old command, having the new command use modern spelling is 
a nice change, even though it costs some duplication in the qapi file; 
where sharing code may save qapi costs in the short run, but will burden 
us with poor naming down the road even when we delete the slow command.

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

> +{ 'command': 'query-cpus-fast', 'returns': [ 'CpuInfoFast' ] }
> +
> +##
>   # @IOThreadInfo:
>   #
>   # Information about an iothread
> 

I didn't inspect the code, so similar to 1/3, once you fix the grammar 
nit and commit message,

Acked-by: Eric Blake <eblake@redhat.com>

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

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

[Eric Blake]

On 02/13/2018 11:18 AM, Viktor Mihajlovski wrote:
> The s390 CPU state can be retrieved without interrupting the
> VM execution. Extendend the CpuInfoFast union with architecture

s/Extendend/Extend/

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

Even with the difference in spelling that I pointed out in 2/3?

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

Yes, this part is true.

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

> +++ 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': { } }

This part works - but since you aren't adding any further fields, what 
is the point in having two type names?  Why not just document that 
CpuInfoS390 is always fast, whether used in CpuInfoFast or in the slower 
CpuInfo?  It's not like we get any additional compile-time safety by 
inventing a new type.

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

Doing this leaves a stale comment in query-cpus-fast that the arch name 
is not provided; you'll need to delete that comment if this patch goes in.

> +#
>   # 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' } }

CpuInfoOther is an empty class, so it works as a placeholder for all 
other arches without having the dichotomy like the 
CpuInfoS390/CpuInfoS390Fast split.

At any rate, whether or not you merge the two CpuInfoS390 type into one, 
converting CpuInfoFast into a union (and NOT trying to inherit between 
the slow type with different field names and the new fast type) seems 
reasonable, if libvirt really wants to keep using cpu-info-fast to learn 
whether S390 vcpus are halted.

>   ##
>   # @query-cpus-fast:

So, if you fix the stale comment here, a v3 patch can have
Acked-by: Eric Blake <eblake@redhat.com>

I also think your v3 series should touch qemu-doc.texi to start the 
deprecation timeframe.

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

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

[Eric Blake]

On 02/14/2018 09:16 AM, Eric Blake wrote:
>> How shall we proceed with this series? Patch 3 depends upon patch 1, so
>> I think it makes sense to merge this in one go.
>>
>> I can give my R-b on patch 1 and Someone(tm) can merge this, or I can
>> take the whole series through the s390 tree (with some further
>> reviews/acks on patches 2/3).
> 
> I'd still like to give a thorough QMP review; it's on my list to get to 
> today.

Okay, I've added more review comments; I think there's still enough 
worth polishing that resending a v3 of the series is worthwhile, and do 
agree that we want all three patches through a single tree (s390 sound 
fine to me).  But in general, I'm okay with the QMP changes (what looks 
like duplication is caused by modernizing the naming, which means we 
DON'T want to play games with trying to inherit one type from the other, 
and the argument about deprecating the old naming seems reasonable).

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