Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info

[Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>]

On 08.02.2018 16:30, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:21:26 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 8 Feb 2018 09:09:04 -0500
>> Luiz Capitulino <lcapitulino@redhat.com> wrote:
>>
>>> On Thu,  8 Feb 2018 10:48:08 +0100
>>> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote:
>>>   
>>>> Presently s390x is the only architecture not exposing specific
>>>> CPU information via QMP query-cpus. Upstream discussion has shown
>>>> that it could make sense to report the architecture specific CPU
>>>> state, e.g. to detect that a CPU has been stopped.    
>>>
>>> I'd very strongly advise against extending query-cpus. Note that the
>>> latency problems with query-cpus exists in all archs, it's just a
>>> matter of time for it to pop up for s390 use-cases too.
>>>
>>> I think there's three options for this change:
>>>
>>>  1. If this doesn't require interrupting vCPU threads, then you
>>>     could rebase this on top of query-cpus-fast  
>>
>> From my perspective, rebasing on top of query-cpus-fast looks like a
>> good idea. This would imply that we need architecture-specific fields
>> for the new interface as well, though.
> 
> That's not a problem. I mean, to be honest I think I'd slightly prefer
> to keep things separate and add a new command for each arch that needs
> its specific information, but that's just personal preference. The only
> strong requirement for query-cpus-fast is that it doesn't interrupt
> vCPU threads.
> 
While it's a reasonable idea to deprecate query-cpus it will not go away
in a while, if ever. Reason is that there's a significant number of
libvirt release out there using it to probe the VCPUs of a domain.
It would be more than strange if the fast-but-slim version of query-cpus
would report a superset of the slow-but-thorough version.
Therefore, while query-cpus is available, it has to have all the
cpu info.

I'm going to spin a new version of the patch with the changes suggested
by Eric. Additionaly, see the patch below, which can be applied on top
Luiz' and my patch to provide the s390 cpu state with both query types.


[PATCH] S390: Add architecture specific cpu data for query-cpus-fast

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

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

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
---
 cpus.c           | 13 +++++++++++++
 hmp.c            | 11 +++++++++++
 qapi-schema.json | 22 +++++++++++++++++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/cpus.c b/cpus.c
index cb04b2c..a972378 100644
--- a/cpus.c
+++ b/cpus.c
@@ -2099,6 +2099,10 @@ CpuInfo2List *qmp_query_cpus_fast(Error **errp)
     MachineClass *mc = MACHINE_GET_CLASS(ms);
     CpuInfo2List *head = NULL, *cur_item = NULL;
     CPUState *cpu;
+#if defined(TARGET_S390X)
+    S390CPU *s390_cpu;
+    CPUS390XState *env;
+#endif
 
     CPU_FOREACH(cpu) {
         CpuInfo2List *info = g_malloc0(sizeof(*info));
@@ -2122,6 +2126,15 @@ CpuInfo2List *qmp_query_cpus_fast(Error **errp)
             info->value->halted = cpu->halted;
         }
 
+        /* add architecture specific data if available */
+#if defined(TARGET_S390X)
+        s390_cpu = S390_CPU(cpu);
+        env = &s390_cpu->env;
+        info->value->has_archdata = true;
+        info->value->archdata = g_malloc0(sizeof(*info->value->archdata));
+        info->value->archdata->arch = CPU_INFO_ARCH_S390;
+        info->value->archdata->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 0c36864..bfd1038 100644
--- a/hmp.c
+++ b/hmp.c
@@ -427,6 +427,17 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict)
         }
         monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path);
         monitor_printf(mon, "\n");
+        if (cpu->value->has_archdata) {
+            switch (cpu->value->archdata->arch) {
+            case CPU_INFO_ARCH_S390:
+                monitor_printf(mon, " state=%s\n",
+                               CpuInfoS390State_str(cpu->value->archdata->
+                                                    u.s390.cpu_state));
+                break;
+            default:
+                break;
+            }
+        }
     }
 
     qapi_free_CpuInfo2List(head);
diff --git a/qapi-schema.json b/qapi-schema.json
index 12c7dc8..0b36860 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -607,7 +607,27 @@
 ##
 { 'struct': 'CpuInfo2',
   'data': {'cpu-index': 'int', '*halted': 'bool', 'qom-path': 'str',
-           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+           'thread-id': 'int', '*props': 'CpuInstanceProperties',
+           '*archdata': 'CpuInfoArchData' } }
+
+##
+# @CpuInfoArchData:
+#
+# Architecure specific information about a virtual CPU
+#
+# Since: 2.12
+#
+##
+{ 'union': 'CpuInfoArchData',
+  'base': { 'arch': 'CpuInfoArch' },
+  'discriminator': 'arch',
+  'data': { 'x86': 'CpuInfoOther',
+            'sparc': 'CpuInfoOther',
+            'ppc': 'CpuInfoOther',
+            'mips': 'CpuInfoOther',
+            'tricore': 'CpuInfoOther',
+            's390': 'CpuInfoS390',
+            'other': 'CpuInfoOther' } }
 
 ##
 # @query-cpus-fast:
-- 
1.9.1