S390: Expose s390-specific CPU info

Commit Message

Viktor Mihajlovski Feb. 8, 2018, 3:52 p.m. UTC

On 08.02.2018 16:30, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:21:26 +0100
> Cornelia Huck <[e-mail]> wrote:
> 
>> On Thu, 8 Feb 2018 09:09:04 -0500
>> Luiz Capitulino <[e-mail]> wrote:
>>
>>> On Thu,  8 Feb 2018 10:48:08 +0100
>>> Viktor Mihajlovski <[e-mail]> 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 <[e-mail]>
---
 cpus.c           | 13 +++++++++++++
 hmp.c            | 11 +++++++++++
 qapi-schema.json | 22 +++++++++++++++++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

Comments

Luiz Capitulino Feb. 8, 2018, 4:22 p.m. UTC | #1

On Thu, 8 Feb 2018 16:52:28 +0100
Viktor Mihajlovski <[e-mail]> wrote:

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

I don't think you need CpuInfoArchData, you can have S390CpuState
instead and ignore the other archs. It's not like all archs data
can be returned at the same time, and also you start having to
replicate that arch string list everywhere. Lastly, the arch name
is returned by query-target, so no need to duplicate that one either.

Viktor Mihajlovski Feb. 8, 2018, 5:02 p.m. UTC | #2

On 08.02.2018 17:22, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:52:28 +0100
> Viktor Mihajlovski <[e-mail]> wrote:
> 
>> 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:
> 
> I don't think you need CpuInfoArchData, you can have S390CpuState
> instead and ignore the other archs. It's not like all archs data
> can be returned at the same time, and also you start having to
> replicate that arch string list everywhere. Lastly, the arch name
> is returned by query-target, so no need to duplicate that one either.
> 
I don't think I really understood your suggestion. Was it to assume that
only s390 will have arch-specific data?. I.e. something along the lines of
-           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
+           'thread-id': 'int', '*props': 'CpuInstanceProperties',
+           '*archdata': 'CpuInfoS390' } }

or some kind of in-line, anonymous union? I have to confess I'm pretty
QAPI-illiterate, so I may have done it overly complicated. At least it
feels that way.

Luiz Capitulino Feb. 8, 2018, 5:37 p.m. UTC | #3

On Thu, 8 Feb 2018 18:02:07 +0100
Viktor Mihajlovski <[e-mail]> wrote:

> On 08.02.2018 17:22, Luiz Capitulino wrote:
> > On Thu, 8 Feb 2018 16:52:28 +0100
> > Viktor Mihajlovski <[e-mail]> wrote:
> >   
> >> 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:  
> > 
> > I don't think you need CpuInfoArchData, you can have S390CpuState
> > instead and ignore the other archs. It's not like all archs data
> > can be returned at the same time, and also you start having to
> > replicate that arch string list everywhere. Lastly, the arch name
> > is returned by query-target, so no need to duplicate that one either.
> >   
> I don't think I really understood your suggestion. Was it to assume that
> only s390 will have arch-specific data?. I.e. something along the lines of
> -           'thread-id': 'int', '*props': 'CpuInstanceProperties' } }
> +           'thread-id': 'int', '*props': 'CpuInstanceProperties',
> +           '*archdata': 'CpuInfoS390' } }
> 
> or some kind of in-line, anonymous union? I have to confess I'm pretty
> QAPI-illiterate, so I may have done it overly complicated. At least it
> feels that way.

Yes, what you propose above is what I had in mind. Maybe the QAPI has
some better way to do it though.

Patch

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: