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

[Luiz Capitulino]

On Thu, 8 Feb 2018 18:02:07 +0100
Viktor Mihajlovski  wrote:

> On 08.02.2018 17:22, Luiz Capitulino wrote:
> > On Thu, 8 Feb 2018 16:52:28 +0100
> > Viktor Mihajlovski  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.

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

[Viktor Mihajlovski]

On 08.02.2018 17:22, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:52:28 +0100
> Viktor Mihajlovski  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.

-- 
Regards,
 Viktor Mihajlovski

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

[Luiz Capitulino]

On Thu, 8 Feb 2018 16:52:28 +0100
Viktor Mihajlovski  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.

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

[Viktor Mihajlovski]

On 08.02.2018 16:30, Luiz Capitulino wrote:
> On Thu, 8 Feb 2018 16:21:26 +0100
> Cornelia Huck  wrote:
> 
>> On Thu, 8 Feb 2018 09:09:04 -0500
>> Luiz Capitulino  wrote:
>>
>>> On Thu,  8 Feb 2018 10:48:08 +0100
>>> 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.
>>>
>>> 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 
---
 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 = _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': 

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

[Luiz Capitulino]

On Thu, 8 Feb 2018 16:21:26 +0100
Cornelia Huck  wrote:

> On Thu, 8 Feb 2018 09:09:04 -0500
> Luiz Capitulino  wrote:
> 
> > On Thu,  8 Feb 2018 10:48:08 +0100
> > 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.
> > 
> > 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.

> 
> > 
> >  2. If you plan to keep adding s390 state/registers to QMP commands,
> > then you could consider adding a query-s390-cpu-state or add
> > a query-cpu-state command that accepts the arch name as a parameter  
> 
> Personally, I don't see a need for more fields. But maybe I'm just
> unimaginative.
> 
> > 
> >  3. If you end up needing to expose state that actually needs an
> > ioctl, then we should consider porting info registers to QMP
> >   
> > > 
> > > 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   
> 

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

[Viktor Mihajlovski]

On 08.02.2018 16:19, Eric Blake wrote:
> 
> Missing a documentation line that mentions when the enum grew. Also, has
> a conflict with this other proposed addition, which demonstrates what
> the documentation should look like (should be easy to resolve, though):
> https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html
> 
Good pointer, thanks. So the enum conflict would be resolved on a
first-to-ack base?
> 
>>   ##
>> +# @CpuInfoS390State:
>> +#
>> +# An enumeration of cpu states that can be assumed by a virtual
>> +# S390 CPU
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'enum': 'CpuInfoS390State',
>> +  'data': [ 'uninitialized', 'stopped', 'check_stop', 'operating',
>> 'load' ] }
>> +
> 
> Is there a consistency reason for naming this 'check_stop', or can we go
> with our preference for using dash 'check-stop'?
No specific reason, I've based that on the definitions previously in
target/s390x/cpu.h, same thing for cpu-state. Will update.
> 
>> +##
>> +# @CpuInfoS390:
>> +#
>> +# Additional information about a virtual S390 CPU
>> +#
>> +# @cpu_state: the CPUs state
>> +#
>> +# Since: 2.12
>> +##
>> +{ 'struct': 'CpuInfoS390', 'data': { 'cpu_state': 'CpuInfoS390State' } }
> 
> Likewise for 'cpu-state'
> 


-- 
Regards,
 Viktor Mihajlovski

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

[Eric Blake]

On 02/08/2018 04:37 AM, Cornelia Huck wrote:


@@ -373,7 +373,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(CPU_INFOS390_STATE_OPERATING, ipl_cpu);


Exposing the state as a QAPI enum has the unfortunate side effect of
that new name. It feels slightly awkward to me, as it is a state for
real decisions and not just for info statements...


I asked Viktor to use the qapi enum instead of having two sets of defines that
we need to keep in sync. (in fact 3, as the kernel kvm mpstate definition is 
also
there).


Agreed, using the QAPI enum makes sense.



But yes, the INFO in that name is somewhat strange. No good idea though.


Can we call the enum CpuS390State instead of CpuInfoS390State (while
keeping the CpuInfoS390 name)? Or does that violate any QAPI rules?


The name of the enum is not important to introspection; and what's more, 
you can set the 'prefix':'...' key in QAPI to pick an enum naming in the 
C code that is saner than what the generator would automatically produce 
from the enum name itself (see qapi/crypto.json for some examples).


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

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

[Cornelia Huck]

On Thu, 8 Feb 2018 09:09:04 -0500
Luiz Capitulino  wrote:

> On Thu,  8 Feb 2018 10:48:08 +0100
> 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.  
> 
> 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.

> 
>  2. If you plan to keep adding s390 state/registers to QMP commands,
> then you could consider adding a query-s390-cpu-state or add
> a query-cpu-state command that accepts the arch name as a parameter

Personally, I don't see a need for more fields. But maybe I'm just
unimaginative.

> 
>  3. If you end up needing to expose state that actually needs an
> ioctl, then we should consider porting info registers to QMP
> 
> > 
> > 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 

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

[Eric Blake]

On 02/08/2018 03:48 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}]

Signed-off-by: Viktor Mihajlovski 
---



+++ b/qapi-schema.json
@@ -413,7 +413,7 @@
  # Since: 2.6
  ##
  { 'enum': 'CpuInfoArch',
-  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
+  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }


Missing a documentation line that mentions when the enum grew. Also, has 
a conflict with this other proposed addition, which demonstrates what 
the documentation should look like (should be easy to resolve, though):

https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html



  ##
+# @CpuInfoS390State:
+#
+# An enumeration of cpu states that can be assumed by a virtual
+# S390 CPU
+#
+# Since: 2.12
+##
+{ 'enum': 'CpuInfoS390State',
+  'data': [ 'uninitialized', 'stopped', 'check_stop', 'operating', 'load' ] }
+


Is there a consistency reason for naming this 'check_stop', or can we go 
with our preference for using dash 'check-stop'?



+##
+# @CpuInfoS390:
+#
+# Additional information about a virtual S390 CPU
+#
+# @cpu_state: the CPUs state
+#
+# Since: 2.12
+##
+{ 'struct': 'CpuInfoS390', 'data': { 'cpu_state': 'CpuInfoS390State' } }


Likewise for 'cpu-state'

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

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

[Luiz Capitulino]

On Thu,  8 Feb 2018 10:48:08 +0100
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.

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

 2. If you plan to keep adding s390 state/registers to QMP commands,
then you could consider adding a query-s390-cpu-state or add
a query-cpu-state command that accepts the arch name as a parameter

 3. If you end up needing to expose state that actually needs an
ioctl, then we should consider porting info registers to QMP

> 
> 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 
> ---
>  cpus.c |  6 ++
>  hmp.c  |  4 
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json   | 25 -
>  target/s390x/cpu.c | 24 
>  target/s390x/cpu.h |  7 ++-
>  target/s390x/kvm.c |  8 
>  target/s390x/sigp.c| 38 +++---
>  8 files changed, 72 insertions(+), 42 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 2cb0af9..39e46dd 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2033,6 +2033,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>  TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>  CPUTriCoreState *env = _cpu->env;
> +#elif defined(TARGET_S390X)
> +S390CPU *s390_cpu = S390_CPU(cpu);
> +CPUS390XState *env = _cpu->env;
>  #endif
>  
>  cpu_synchronize_state(cpu);
> @@ -2060,6 +2063,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 b3de32d..37e04c3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -390,6 +390,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",
> +   
> CpuInfoS390State_str(cpu->value->u.s390.cpu_state));
> +break;
>  default:
>  break;
>  }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3807dcb..3e6360e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -373,7 +373,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(CPU_INFOS390_STATE_OPERATING, ipl_cpu);
>  }
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745..c34880b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -413,7 +413,7 @@
>  # Since: 2.6
>  ##
>  { 'enum': 'CpuInfoArch',
> -  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
> +  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
>  
>  ##
>  # @CpuInfo:
> @@ -452,6 +452,7 @@
>  'ppc': 'CpuInfoPPC',
>  'mips': 'CpuInfoMIPS',
>  'tricore': 'CpuInfoTricore',
> +'s390': 'CpuInfoS390',
>  'other': 'CpuInfoOther' } }
>  
>  ##
> @@ -522,6 +523,28 @@
>  { 'struct': 'CpuInfoOther', 'data': { } }
>  
>  ##
> +# @CpuInfoS390State:
> +#
> +# An enumeration of cpu states that can be assumed by a virtual
> +# S390 CPU
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'CpuInfoS390State',
> +  'data': [ 'uninitialized', 'stopped', 'check_stop', 'operating', 'load' ] }
> +
> +##

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

[Cornelia Huck]

On Thu, 8 Feb 2018 11:24:48 +0100
Christian Borntraeger  wrote:

> On 02/08/2018 11:16 AM, Cornelia Huck wrote:
> > On Thu,  8 Feb 2018 10:48:08 +0100
> > Viktor Mihajlovski  wrote:

> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 3807dcb..3e6360e 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -373,7 +373,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(CPU_INFOS390_STATE_OPERATING, ipl_cpu);  
> > 
> > Exposing the state as a QAPI enum has the unfortunate side effect of
> > that new name. It feels slightly awkward to me, as it is a state for
> > real decisions and not just for info statements...  
> 
> I asked Viktor to use the qapi enum instead of having two sets of defines that
> we need to keep in sync. (in fact 3, as the kernel kvm mpstate definition is 
> also
> there).

Agreed, using the QAPI enum makes sense.

> 
> But yes, the INFO in that name is somewhat strange. No good idea though.

Can we call the enum CpuS390State instead of CpuInfoS390State (while
keeping the CpuInfoS390 name)? Or does that violate any QAPI rules?

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

[Christian Borntraeger]

On 02/08/2018 11:16 AM, Cornelia Huck wrote:
> On Thu,  8 Feb 2018 10:48:08 +0100
> Viktor Mihajlovski  wrote:
> 
> [added some cc:s]
> 
>> 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 
>> ---
>>  cpus.c |  6 ++
>>  hmp.c  |  4 
>>  hw/s390x/s390-virtio-ccw.c |  2 +-
>>  qapi-schema.json   | 25 -
>>  target/s390x/cpu.c | 24 
>>  target/s390x/cpu.h |  7 ++-
>>  target/s390x/kvm.c |  8 
>>  target/s390x/sigp.c| 38 +++---
>>  8 files changed, 72 insertions(+), 42 deletions(-)
>>
>> diff --git a/cpus.c b/cpus.c
>> index 2cb0af9..39e46dd 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -2033,6 +2033,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>>  #elif defined(TARGET_TRICORE)
>>  TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>>  CPUTriCoreState *env = _cpu->env;
>> +#elif defined(TARGET_S390X)
>> +S390CPU *s390_cpu = S390_CPU(cpu);
>> +CPUS390XState *env = _cpu->env;
>>  #endif
>>  
>>  cpu_synchronize_state(cpu);
>> @@ -2060,6 +2063,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 b3de32d..37e04c3 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -390,6 +390,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",
>> +   
>> CpuInfoS390State_str(cpu->value->u.s390.cpu_state));
>> +break;
>>  default:
>>  break;
>>  }
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 3807dcb..3e6360e 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -373,7 +373,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(CPU_INFOS390_STATE_OPERATING, ipl_cpu);
> 
> Exposing the state as a QAPI enum has the unfortunate side effect of
> that new name. It feels slightly awkward to me, as it is a state for
> real decisions and not just for info statements...

I asked Viktor to use the qapi enum instead of having two sets of defines that
we need to keep in sync. (in fact 3, as the kernel kvm mpstate definition is 
also
there).

But yes, the INFO in that name is somewhat strange. No good idea though.

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

[Cornelia Huck]

On Thu,  8 Feb 2018 10:48:08 +0100
Viktor Mihajlovski  wrote:

[added some cc:s]

> 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 
> ---
>  cpus.c |  6 ++
>  hmp.c  |  4 
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json   | 25 -
>  target/s390x/cpu.c | 24 
>  target/s390x/cpu.h |  7 ++-
>  target/s390x/kvm.c |  8 
>  target/s390x/sigp.c| 38 +++---
>  8 files changed, 72 insertions(+), 42 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 2cb0af9..39e46dd 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2033,6 +2033,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>  TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>  CPUTriCoreState *env = _cpu->env;
> +#elif defined(TARGET_S390X)
> +S390CPU *s390_cpu = S390_CPU(cpu);
> +CPUS390XState *env = _cpu->env;
>  #endif
>  
>  cpu_synchronize_state(cpu);
> @@ -2060,6 +2063,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 b3de32d..37e04c3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -390,6 +390,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",
> +   
> CpuInfoS390State_str(cpu->value->u.s390.cpu_state));
> +break;
>  default:
>  break;
>  }
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 3807dcb..3e6360e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -373,7 +373,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(CPU_INFOS390_STATE_OPERATING, ipl_cpu);

Exposing the state as a QAPI enum has the unfortunate side effect of
that new name. It feels slightly awkward to me, as it is a state for
real decisions and not just for info statements...

>  }
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745..c34880b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -413,7 +413,7 @@
>  # Since: 2.6
>  ##
>  { 'enum': 'CpuInfoArch',
> -  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
> +  'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 's390', 'other' ] }
>  
>  ##
>  # @CpuInfo:
> @@ -452,6 +452,7 @@
>  'ppc': 'CpuInfoPPC',
>  'mips': 'CpuInfoMIPS',
>  'tricore': 'CpuInfoTricore',
> +'s390': 'CpuInfoS390',
>  'other': 'CpuInfoOther' } }
>  
>  ##
> @@ -522,6 +523,28 @@
>  { 'struct': 'CpuInfoOther', 'data': { } }
>  
>  ##
> +# @CpuInfoS390State:
> +#
> +# An enumeration of cpu states that can be assumed by a virtual
> +# S390 CPU
> +#
> +# Since: 2.12
> +##
> +{ 'enum': 'CpuInfoS390State',
> +  'data': [ 'uninitialized', 'stopped', 'check_stop', 'operating', 'load' ] }
> +
> +##
> +# @CpuInfoS390:
> +#
> +# Additional information about a virtual S390 CPU
> +#
> +# @cpu_state: the CPUs state
> +#
> +# Since: 2.12
> +##
> +{ 'struct': 'CpuInfoS390', 'data': { 'cpu_state': 'CpuInfoS390State' } }
> +
> +##
>  # @query-cpus:
>  #
>  # Returns a list of information about each virtual CPU.
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index d2e6b9f..996cbc8 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -58,8 +58,8 @@ static bool