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

[David Hildenbrand]

On 12.02.2018 13:14, 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 
> Acked-by: Eric Blake 
> ---
>  cpus.c |  6 ++
>  hmp.c  |  4 
>  hw/intc/s390_flic.c|  4 ++--
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json   | 28 +++-
>  target/s390x/cpu.c | 24 
>  target/s390x/cpu.h |  7 ++-
>  target/s390x/kvm.c |  8 
>  target/s390x/sigp.c| 38 +++---
>  9 files changed, 77 insertions(+), 44 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index f298b65..6006931 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>  TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>  CPUTriCoreState *env = _cpu->env;
> +#elif defined(TARGET_S390X)
> +S390CPU *s390_cpu = S390_CPU(cpu);
> +CPUS390XState *env = _cpu->env;
>  #endif
>  
>  cpu_synchronize_state(cpu);
> @@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>  info->value->arch = CPU_INFO_ARCH_TRICORE;
>  info->value->u.tricore.PC = env->PC;
> +#elif defined(TARGET_S390X)
> +info->value->arch = CPU_INFO_ARCH_S390;
> +info->value->u.s390.cpu_state = env->cpu_state;
>  #else
>  info->value->arch = CPU_INFO_ARCH_OTHER;
>  #endif
> diff --git a/hmp.c b/hmp.c
> index 7870d6a..a6b94b7 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>  case CPU_INFO_ARCH_TRICORE:
>  monitor_printf(mon, " PC=0x%016" PRIx64, 
> cpu->value->u.tricore.PC);
>  break;
> +case CPU_INFO_ARCH_S390:
> +monitor_printf(mon, " state=%s",
> +   CpuS390State_str(cpu->value->u.s390.cpu_state));
> +break;
>  default:
>  break;
>  }
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index a85a149..5f8168f 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
>  cs->interrupt_request |= CPU_INTERRUPT_HARD;
>  
>  /* ignore CPUs that are not sleeping */
> -if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
> -s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
> +if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
> +s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
>  continue;
>  }
>  
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..4d0c3de 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -368,7 +368,7 @@ static void s390_machine_reset(void)
>  
>  /* all cpus are stopped - configure and start the ipl cpu only */
>  s390_ipl_prepare_cpu(ipl_cpu);
> -s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
> +s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>  }
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745..66e0927 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': 

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

[Luiz Capitulino]

On Mon, 12 Feb 2018 13:14:30 +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.
> 
> 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}
>]

We're adding the same information to query-cpus-fast. Why do we
need to duplicate this into query-cpus? Do you plan to keep using
query-cpus? If yes, why?

Libvirt for one, should move away from it. We don't want to run
into the risk of having the same issue we had with x86 in other
archs.

> 
> Signed-off-by: Viktor Mihajlovski 
> Acked-by: Eric Blake 
> ---
>  cpus.c |  6 ++
>  hmp.c  |  4 
>  hw/intc/s390_flic.c|  4 ++--
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json   | 28 +++-
>  target/s390x/cpu.c | 24 
>  target/s390x/cpu.h |  7 ++-
>  target/s390x/kvm.c |  8 
>  target/s390x/sigp.c| 38 +++---
>  9 files changed, 77 insertions(+), 44 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index f298b65..6006931 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -2100,6 +2100,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>  TriCoreCPU *tricore_cpu = TRICORE_CPU(cpu);
>  CPUTriCoreState *env = _cpu->env;
> +#elif defined(TARGET_S390X)
> +S390CPU *s390_cpu = S390_CPU(cpu);
> +CPUS390XState *env = _cpu->env;
>  #endif
>  
>  cpu_synchronize_state(cpu);
> @@ -2127,6 +2130,9 @@ CpuInfoList *qmp_query_cpus(Error **errp)
>  #elif defined(TARGET_TRICORE)
>  info->value->arch = CPU_INFO_ARCH_TRICORE;
>  info->value->u.tricore.PC = env->PC;
> +#elif defined(TARGET_S390X)
> +info->value->arch = CPU_INFO_ARCH_S390;
> +info->value->u.s390.cpu_state = env->cpu_state;
>  #else
>  info->value->arch = CPU_INFO_ARCH_OTHER;
>  #endif
> diff --git a/hmp.c b/hmp.c
> index 7870d6a..a6b94b7 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -392,6 +392,10 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>  case CPU_INFO_ARCH_TRICORE:
>  monitor_printf(mon, " PC=0x%016" PRIx64, 
> cpu->value->u.tricore.PC);
>  break;
> +case CPU_INFO_ARCH_S390:
> +monitor_printf(mon, " state=%s",
> +   CpuS390State_str(cpu->value->u.s390.cpu_state));
> +break;
>  default:
>  break;
>  }
> diff --git a/hw/intc/s390_flic.c b/hw/intc/s390_flic.c
> index a85a149..5f8168f 100644
> --- a/hw/intc/s390_flic.c
> +++ b/hw/intc/s390_flic.c
> @@ -192,8 +192,8 @@ static void qemu_s390_flic_notify(uint32_t type)
>  cs->interrupt_request |= CPU_INTERRUPT_HARD;
>  
>  /* ignore CPUs that are not sleeping */
> -if (s390_cpu_get_state(cpu) != CPU_STATE_OPERATING &&
> -s390_cpu_get_state(cpu) != CPU_STATE_LOAD) {
> +if (s390_cpu_get_state(cpu) != S390_CPU_STATE_OPERATING &&
> +s390_cpu_get_state(cpu) != S390_CPU_STATE_LOAD) {
>  continue;
>  }
>  
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4abbe89..4d0c3de 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -368,7 +368,7 @@ static void s390_machine_reset(void)
>  
>  /* all cpus are stopped - configure and start the ipl cpu only */
>  s390_ipl_prepare_cpu(ipl_cpu);
> -s390_cpu_set_state(CPU_STATE_OPERATING, ipl_cpu);
> +s390_cpu_set_state(S390_CPU_STATE_OPERATING, ipl_cpu);
>  }
>  
>  static void s390_machine_device_plug(HotplugHandler *hotplug_dev,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c06745..66e0927 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': 

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

[Viktor Mihajlovski]

On 12.02.2018 16:52, Cornelia Huck wrote:
> On Mon, 12 Feb 2018 13:14:30 +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.
>>
>> 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 
>> Acked-by: Eric Blake 
>> ---
>>  cpus.c |  6 ++
>>  hmp.c  |  4 
>>  hw/intc/s390_flic.c|  4 ++--
>>  hw/s390x/s390-virtio-ccw.c |  2 +-
>>  qapi-schema.json   | 28 +++-
>>  target/s390x/cpu.c | 24 
>>  target/s390x/cpu.h |  7 ++-
>>  target/s390x/kvm.c |  8 
>>  target/s390x/sigp.c| 38 +++---
>>  9 files changed, 77 insertions(+), 44 deletions(-)
> 
> Patch looks good to me. I presume this should go through the s390 tree?
> Or do we want someone to pick up the whole series?
> 
The main reason for adding this patch to the series is to ensure
everything is applied in proper order. This patch can stand for itself,
but it must be applied before 3/3.
Valid orders would be 1 - 2 - 3 or 2 - 1 - 3.
As long as this is observed, I'm fine with either way.

-- 
Regards,
 Viktor Mihajlovski

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

[Cornelia Huck]

On Mon, 12 Feb 2018 13:14:30 +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.
> 
> 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 
> Acked-by: Eric Blake 
> ---
>  cpus.c |  6 ++
>  hmp.c  |  4 
>  hw/intc/s390_flic.c|  4 ++--
>  hw/s390x/s390-virtio-ccw.c |  2 +-
>  qapi-schema.json   | 28 +++-
>  target/s390x/cpu.c | 24 
>  target/s390x/cpu.h |  7 ++-
>  target/s390x/kvm.c |  8 
>  target/s390x/sigp.c| 38 +++---
>  9 files changed, 77 insertions(+), 44 deletions(-)

Patch looks good to me. I presume this should go through the s390 tree?
Or do we want someone to pick up the whole series?