Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
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
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
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
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 = &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':
Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
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
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
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
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
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
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 = &tricore_cpu->env; > +#elif defined(TARGET_S390X) > +S390CPU *s390_cpu = S390_CPU(cpu); > +CPUS390XState *env = &s390_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' ] } > + > +## > +# @CpuInfoS390: > +# > +# Additional inform
Re: [Qemu-devel] [PATCH] S390: Expose s390-specific CPU info
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
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 = &tricore_cpu->env; >> +#elif defined(TARGET_S390X) >> +S390CPU *s390_cpu = S390_CPU(cpu); >> +CPUS390XState *env = &s390_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
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 = &tricore_cpu->env; > +#elif defined(TARGET_S390X) > +S390CPU *s390_cpu = S390_CPU(cpu); > +CPUS390XState *env = &s390_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 s390_cpu_has_work(CPUState *cs) > S390CP