[Qemu-devel] [PATCHv4 0/4] add query-cpu-fast and related s390 changes

[Qemu-devel] [PATCHv4 0/4] add query-cpu-fast and related s390 changes

[Viktor Mihajlovski]

tl;dr v4: Drop HMP "info cpus_fast" and use query-cpus-fast in
      	  HMP "info cpus" instead.  Requires HMP maintainer re-review
	  of Patch 2/4.

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/4:
  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/4:
  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/4:
  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.

Patch 4/4 (NEW):
  Starts the deprecation of query-cpus.

Series v3 -> v4:
Overall: Instead of adding a new HMP 'info cpus_fast', changed
	 'info cpus' to use query-cpus-fast directly.

Patch 1/4:
  o Don't report s390-specific data in HMP 'info cpus'

Patch 2/4:
  o Change HMP 'info cpus' to use query-cpus-fast and
    to return only basic information (no arch-specific data)
  o Drop HMP 'info cpus_fast'
  o Fixed typo in commit message

Patch 3/4:
  o Drop HMP-related changes

Patch 4/4:
  o Drop HMP-related changes

Series v2 -> v3:
Overall: Added r-b's and a-b's.

Patch 2/4:
  o Fixed commit message with respect to the halted field
    disposition.
  o Fixed grammar in qapi-schema documentation.

Patch 3/4:
  o Use CpuInfoS390 type for both query-cpus and query-cpus-fast per
    Eric Blake's comment.
  o Dropped 'duplication blurb' from commit message as it doesn't
    provide relevant information other than query-cpus should be
    deprecated, which is done in the next patch now.

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 (3):
  qmp: expose s390-specific CPU info
  qmp: add architecture specific cpu data for query-cpus-fast
  qemu-doc: deprecate query-cpus

 cpus.c                     |  54 +++++++++++++++++++++
 hmp.c                      |  46 +++---------------
 hw/intc/s390_flic.c        |   4 +-
 hw/s390x/s390-virtio-ccw.c |   2 +-
 qapi-schema.json           | 115 ++++++++++++++++++++++++++++++++++++++++++++-
 qemu-doc.texi              |   4 ++
 target/s390x/cpu.c         |  24 +++++-----
 target/s390x/cpu.h         |   7 +--
 target/s390x/kvm.c         |   8 ++--
 target/s390x/sigp.c        |  38 +++++++--------
 10 files changed, 217 insertions(+), 85 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCHv4 1/4] 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>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 cpus.c                     |  6 ++++++
 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 +++++++++++++++++++-------------------
 8 files changed, 73 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/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] [PATCHv4 2/4] 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 Drop "halted" field as it can not be retrieved in a fast
   way on most architectures

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

 o Rename some fields for better clarification & proper naming
   standard

Further, the HMP "info cpus" implementation is changed to
use the new QMP interface to avoid the side effects caused
by query-cpus. This means that only a reduced subset of cpu
information will be reported, which is acceptable as
the content of "info cpus" is not documented or guaranteed
to be stable.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Eric Blake <eblake@redhat.com>
---
 cpus.c           | 38 ++++++++++++++++++++++++++++++
 hmp.c            | 46 +++++--------------------------------
 qapi-schema.json | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+), 40 deletions(-)

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.c b/hmp.c
index 7870d6a..28b1070 100644
--- a/hmp.c
+++ b/hmp.c
@@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict)
 
 void hmp_info_cpus(Monitor *mon, const QDict *qdict)
 {
-    CpuInfoList *cpu_list, *cpu;
+    CpuInfoFastList *head, *cpu;
 
-    cpu_list = qmp_query_cpus(NULL);
+    head = qmp_query_cpus_fast(NULL);
 
-    for (cpu = cpu_list; cpu; cpu = cpu->next) {
-        int active = ' ';
-
-        if (cpu->value->CPU == monitor_get_cpu_index()) {
-            active = '*';
-        }
-
-        monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU);
-
-        switch (cpu->value->arch) {
-        case CPU_INFO_ARCH_X86:
-            monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc);
-            break;
-        case CPU_INFO_ARCH_PPC:
-            monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->u.ppc.nip);
-            break;
-        case CPU_INFO_ARCH_SPARC:
-            monitor_printf(mon, " pc=0x%016" PRIx64,
-                           cpu->value->u.q_sparc.pc);
-            monitor_printf(mon, " npc=0x%016" PRIx64,
-                           cpu->value->u.q_sparc.npc);
-            break;
-        case CPU_INFO_ARCH_MIPS:
-            monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.q_mips.PC);
-            break;
-        case CPU_INFO_ARCH_TRICORE:
-            monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.tricore.PC);
-            break;
-        default:
-            break;
-        }
-
-        if (cpu->value->halted) {
-            monitor_printf(mon, " (halted)");
-        }
-
-        monitor_printf(mon, " thread_id=%" PRId64 "\n", cpu->value->thread_id);
+    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_CpuInfoList(cpu_list);
+    qapi_free_CpuInfoFastList(head);
 }
 
 static void print_block_info(Monitor *mon, BlockInfo *info,
diff --git a/qapi-schema.json b/qapi-schema.json
index 94d560e..815f072 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
+# a small interruption to 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] [PATCHv4 3/4] 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}
]

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Acked-by: Eric Blake <eblake@redhat.com>
---
 cpus.c           | 10 ++++++++++
 qapi-schema.json | 25 ++++++++++++++++++-------
 2 files changed, 28 insertions(+), 7 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/qapi-schema.json b/qapi-schema.json
index 815f072..e6ca63f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -408,7 +408,7 @@
 # @CpuInfoArch:
 #
 # An enumeration of cpu types that enable additional information during
-# @query-cpus.
+# @query-cpus and @query-cpus-fast.
 #
 # @s390: since 2.12
 #
@@ -604,12 +604,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': 'CpuInfoS390',
+            'other': 'CpuInfoOther' } }
 
 ##
 # @query-cpus-fast:
@@ -620,9 +632,6 @@
 #
 # 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:
@@ -637,6 +646,7 @@
 #                 "socket-id": 0
 #             },
 #             "qom-path": "/machine/unattached/device[0]",
+#             "arch":"x86",
 #             "cpu-index": 0
 #         },
 #         {
@@ -647,6 +657,7 @@
 #                 "socket-id": 1
 #             },
 #             "qom-path": "/machine/unattached/device[2]",
+#             "arch":"x86",
 #             "cpu-index": 1
 #         }
 #     ]
-- 
1.9.1

[Qemu-devel] [PATCHv4 4/4] qemu-doc: deprecate query-cpus

[Viktor Mihajlovski]

Start the deprecation period for QAPI query-cpus (replaced by
query-cpus-fast) beginning with 2.12.0.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 qapi-schema.json | 4 ++++
 qemu-doc.texi    | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index e6ca63f..cd98a94 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -587,6 +587,10 @@
 #       ]
 #    }
 #
+# Notes: This interface is deprecated (since 2.12.0), and it is strongly
+#        recommended that you avoid using it. Use @query-cpus-fast to
+#        obtain information about virtual CPUs.
+#
 ##
 { 'command': 'query-cpus', 'returns': ['CpuInfo'] }
 
diff --git a/qemu-doc.texi b/qemu-doc.texi
index 137f581..221f565 100644
--- a/qemu-doc.texi
+++ b/qemu-doc.texi
@@ -2764,6 +2764,10 @@ by the ``convert -l snapshot_param'' argument instead.
 "autoload" parameter is now ignored. All bitmaps are automatically loaded
 from qcow2 images.
 
+@subsection query-cpus (since 2.12.0)
+
+The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
+
 @section System emulator human monitor commands
 
 @subsection host_net_add (since 2.10.0)
-- 
1.9.1

Re: [Qemu-devel] [PATCHv4 1/4] qmp: expose s390-specific CPU info

[Eric Blake]

On 02/16/2018 06:04 AM, 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}
>     ]

Maybe worth adding the comment that:

HMP "info cpus" is intentionally not modified, as a later patch will be 
pruning it down anyways.

> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Acked-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

> ---
>   cpus.c                     |  6 ++++++
>   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 +++++++++++++++++++-------------------
>   8 files changed, 73 insertions(+), 44 deletions(-)
> 

I've now read closely through the entire patch; you can upgrade my 
Acked-by into

Reviewed-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] [PATCHv4 2/4] qmp: add query-cpus-fast

[Eric Blake]

On 02/16/2018 06:04 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 Drop "halted" field as it can not be retrieved in a fast
>     way on most architectures
> 
>   o Drop irrelevant fields such as "current", "pc" and "arch"
> 
>   o Rename some fields for better clarification & proper naming
>     standard
> 
> Further, the HMP "info cpus" implementation is changed to
> use the new QMP interface to avoid the side effects caused
> by query-cpus. This means that only a reduced subset of cpu
> information will be reported, which is acceptable as
> the content of "info cpus" is not documented or guaranteed
> to be stable.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Acked-by: Eric Blake <eblake@redhat.com>

The HMP changes are non-trivial compared to v3, so I might have dropped 
all R-b and Acked-by to ensure they are looked at again.

In fact,...

> ---
>   cpus.c           | 38 ++++++++++++++++++++++++++++++
>   hmp.c            | 46 +++++--------------------------------
>   qapi-schema.json | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 114 insertions(+), 40 deletions(-)
> 

> +++ b/hmp.c
> @@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon, const QDict *qdict)
>   
>   void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>   {
> -    CpuInfoList *cpu_list, *cpu;
> +    CpuInfoFastList *head, *cpu;
>   
> -    cpu_list = qmp_query_cpus(NULL);
> +    head = qmp_query_cpus_fast(NULL);
>   
> -    for (cpu = cpu_list; cpu; cpu = cpu->next) {
> -        int active = ' ';
> -
> -        if (cpu->value->CPU == monitor_get_cpu_index()) {
> -            active = '*';
> -        }

The old code was doing multiple things - it was telling you the current 
HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using 
HMP, knowing which cpu is the active target for future HMP commands that 
depend on the active target, this bit of information is important), as 
well as...

> -        monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU);
> -
> -        switch (cpu->value->arch) {
> -        case CPU_INFO_ARCH_X86:
> -            monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86.pc);
> -            break;

...inundating you with the arch-specific slow stuff.  I'm in agreement 
that dropping the slow stuff is okay, but...

> +    for (cpu = head; cpu; cpu = cpu->next) {
> +        monitor_printf(mon, "  CPU #%" PRId64 ":", cpu->value->cpu_index);

...unilaterally dropping the '*' active indicator, which has no bearing 
on the QMP changes, and thus is not an appropriate change for this patch.

I've now gone through the entire patch (and not just the UI), so on the 
next revision, I'll be prepared to give a full R-b, rather than just 
Acked-by.  But I do think this means we need a v5.

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

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

[Eric Blake]

On 02/16/2018 06:04 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/
(And I mentioned this on my last review - it's no fun when new revisions 
miss picking up fixes)

> 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}
> ]
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> Acked-by: Eric Blake <eblake@redhat.com>
> ---
>   cpus.c           | 10 ++++++++++
>   qapi-schema.json | 25 ++++++++++++++++++-------
>   2 files changed, 28 insertions(+), 7 deletions(-)
> 

I've now gone through the entire patch, so you can upgrade my Acked-by into

Reviewed-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] [PATCHv4 4/4] qemu-doc: deprecate query-cpus

[Eric Blake]

On 02/16/2018 06:04 AM, Viktor Mihajlovski wrote:
> Start the deprecation period for QAPI query-cpus (replaced by
> query-cpus-fast) beginning with 2.12.0.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> ---
>   qapi-schema.json | 4 ++++
>   qemu-doc.texi    | 4 ++++
>   2 files changed, 8 insertions(+)

Reviewed-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] [PATCHv4 2/4] qmp: add query-cpus-fast

[Viktor Mihajlovski]

On 16.02.2018 15:03, Eric Blake wrote:
> On 02/16/2018 06:04 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 Drop "halted" field as it can not be retrieved in a fast
>>     way on most architectures
>>
>>   o Drop irrelevant fields such as "current", "pc" and "arch"
>>
>>   o Rename some fields for better clarification & proper naming
>>     standard
>>
>> Further, the HMP "info cpus" implementation is changed to
>> use the new QMP interface to avoid the side effects caused
>> by query-cpus. This means that only a reduced subset of cpu
>> information will be reported, which is acceptable as
>> the content of "info cpus" is not documented or guaranteed
>> to be stable.
>>
>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> Acked-by: Eric Blake <eblake@redhat.com>
> 
> The HMP changes are non-trivial compared to v3, so I might have dropped
> all R-b and Acked-by to ensure they are looked at again.
> 
> In fact,...
> 
>> ---
>>   cpus.c           | 38 ++++++++++++++++++++++++++++++
>>   hmp.c            | 46 +++++--------------------------------
>>   qapi-schema.json | 70
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 114 insertions(+), 40 deletions(-)
>>
> 
>> +++ b/hmp.c
>> @@ -360,50 +360,16 @@ void hmp_info_migrate_cache_size(Monitor *mon,
>> const QDict *qdict)
>>     void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>>   {
>> -    CpuInfoList *cpu_list, *cpu;
>> +    CpuInfoFastList *head, *cpu;
>>   -    cpu_list = qmp_query_cpus(NULL);
>> +    head = qmp_query_cpus_fast(NULL);
>>   -    for (cpu = cpu_list; cpu; cpu = cpu->next) {
>> -        int active = ' ';
>> -
>> -        if (cpu->value->CPU == monitor_get_cpu_index()) {
>> -            active = '*';
>> -        }
> 
> The old code was doing multiple things - it was telling you the current
> HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using
> HMP, knowing which cpu is the active target for future HMP commands that
> depend on the active target, this bit of information is important), as
> well as...
> 
>> -        monitor_printf(mon, "%c CPU #%" PRId64 ":", active,
>> cpu->value->CPU);
>> -
>> -        switch (cpu->value->arch) {
>> -        case CPU_INFO_ARCH_X86:
>> -            monitor_printf(mon, " pc=0x%016" PRIx64,
>> cpu->value->u.x86.pc);
>> -            break;
> 
> ...inundating you with the arch-specific slow stuff.  I'm in agreement
> that dropping the slow stuff is okay, but...
> 
>> +    for (cpu = head; cpu; cpu = cpu->next) {
>> +        monitor_printf(mon, "  CPU #%" PRId64 ":",
>> cpu->value->cpu_index);
> 
> ...unilaterally dropping the '*' active indicator, which has no bearing
> on the QMP changes, and thus is not an appropriate change for this patch>
> I've now gone through the entire patch (and not just the UI), so on the
> next revision, I'll be prepared to give a full R-b, rather than just
> Acked-by.  But I do think this means we need a v5.
> 
Thanks for the patience. I'll respin with the r/a-b's on patch 2/4
removed, but want to verify first that I can get the active cpu without
triggering cpu_synchronize_state via monitor_get_cpu_index...

-- 
Regards,
 Viktor Mihajlovski

Re: [Qemu-devel] [PATCHv4 2/4] qmp: add query-cpus-fast

[Eric Blake]

On 02/16/2018 08:49 AM, Viktor Mihajlovski wrote:

>>
>> The HMP changes are non-trivial compared to v3, so I might have dropped
>> all R-b and Acked-by to ensure they are looked at again.
>>
>> In fact,...
>>

>>> -
>>> -        if (cpu->value->CPU == monitor_get_cpu_index()) {
>>> -            active = '*';
>>> -        }
>>
>> The old code was doing multiple things - it was telling you the current
>> HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using
>> HMP, knowing which cpu is the active target for future HMP commands that
>> depend on the active target, this bit of information is important),

>>
> Thanks for the patience. I'll respin with the r/a-b's on patch 2/4
> removed, but want to verify first that I can get the active cpu without
> triggering cpu_synchronize_state via monitor_get_cpu_index...

What does it matter?  HMP is not the time-critical interface like QMP, 
so even if it has to synchronize things, you're no worse off than you 
were before when using HMP.

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

Re: [Qemu-devel] [PATCHv4 2/4] qmp: add query-cpus-fast

[Dr. David Alan Gilbert]

* Eric Blake (eblake@redhat.com) wrote:
> On 02/16/2018 08:49 AM, Viktor Mihajlovski wrote:
> 
> > > 
> > > The HMP changes are non-trivial compared to v3, so I might have dropped
> > > all R-b and Acked-by to ensure they are looked at again.
> > > 
> > > In fact,...
> > > 
> 
> > > > -
> > > > -        if (cpu->value->CPU == monitor_get_cpu_index()) {
> > > > -            active = '*';
> > > > -        }
> > > 
> > > The old code was doing multiple things - it was telling you the current
> > > HMP cpu (HMP has 'cpu' with no QMP counterpart, but if you are using
> > > HMP, knowing which cpu is the active target for future HMP commands that
> > > depend on the active target, this bit of information is important),
> 
> > > 
> > Thanks for the patience. I'll respin with the r/a-b's on patch 2/4
> > removed, but want to verify first that I can get the active cpu without
> > triggering cpu_synchronize_state via monitor_get_cpu_index...
> 
> What does it matter?  HMP is not the time-critical interface like QMP, so
> even if it has to synchronize things, you're no worse off than you were
> before when using HMP.

I guess it's not obvious that mon_get_cpu_index has that side effect;
although I can see why it's there since it guarantees correctness of all
the HMP data with one call; a comment might be nice to point it out.

But yes, I agree the performance cost there doesn't worry me for HMP.

Dave

> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK