| Message ID | 1518437672-7724-4-git-send-email-mihajlov@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 12 Feb 2018 13:14:32 +0100 Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > The s390 CPU state can be retrieved without interrupting the > VM execution. Extendend the CpuInfoFast union with architecture > specific data and an implementation for s390. > > Return data looks like this: > [ > {"thread-id":64301,"props":{"core-id":0}, > "arch":"s390","cpu-state":"operating", > "qom-path":"/machine/unattached/device[0]","cpu-index":0}, > {"thread-id":64302,"props":{"core-id":1}, > "arch":"s390","cpu-state":"operating", > "qom-path":"/machine/unattached/device[1]","cpu-index":1} > ] > > Currently there's a certain amount of duplication between > the definitions of CpuInfo and CpuInfoFast, both in the > base and variable areas, since there are data fields common > to the slow and fast variants. > > A suggestion was made on the mailing list to enhance the QAPI > code generation to support two layers of unions. This would > allow to specify the common fields once and avoid the duplication > in the leaf unions. > > On the other hand, the slow query-cpus should be deprecated > along with the slow CpuInfo type and eventually be removed. > Assuming that new architectures will not be added at high > rates, we could live with the duplication for the time being. > > Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> > --- > cpus.c | 10 ++++++++++ > hmp.c | 8 ++++++++ > qapi-schema.json | 35 +++++++++++++++++++++++++++++------ > 3 files changed, 47 insertions(+), 6 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 6df6660..af67826 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) > MachineClass *mc = MACHINE_GET_CLASS(ms); > CpuInfoFastList *head = NULL, *cur_item = NULL; > CPUState *cpu; > +#if defined(TARGET_S390X) > + S390CPU *s390_cpu; > + CPUS390XState *env; > +#endif > > CPU_FOREACH(cpu) { > CpuInfoFastList *info = g_malloc0(sizeof(*info)); > @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) > info->value->props = props; > } > > +#if defined(TARGET_S390X) > + s390_cpu = S390_CPU(cpu); > + env = &s390_cpu->env; You should be able to omit the s390_cpu variable by using env = &S390_CPU(cpu)->env; > + info->value->arch = CPU_INFO_ARCH_S390; > + info->value->u.s390.cpu_state = env->cpu_state; > +#endif > if (!cur_item) { > head = cur_item = info; > } else { As you mentioned in the patch description, the duplication is a bit awkward. I'll let the QAPI experts judge that; otherwise, this looks fine to me.
On Mon, 12 Feb 2018 13:14:32 +0100 Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > -{ 'struct': 'CpuInfoFast', > - 'data': {'cpu-index': 'int', 'qom-path': 'str', > - 'thread-id': 'int', '*props': 'CpuInstanceProperties' } } > +{ 'union': 'CpuInfoFast', > + 'base': {'cpu-index': 'int', 'qom-path': 'str', > + 'thread-id': 'int', '*props': 'CpuInstanceProperties', > + 'arch': 'CpuInfoArch' }, > + 'discriminator': 'arch', > + 'data': { 'x86': 'CpuInfoOther', > + 'sparc': 'CpuInfoOther', > + 'ppc': 'CpuInfoOther', > + 'mips': 'CpuInfoOther', > + 'tricore': 'CpuInfoOther', > + 's390': 'CpuInfoS390Fast', > + 'other': 'CpuInfoOther' } } Consider this a minor comment (and QMP maintainers can give a much better advice than me), but I think this arch list has problems. For one thing, it's incomplete. And the second problem is the 'other' field. What happens when QEMU starts supporting a new arch? 'other' becomes the new arch. Is this compatible? I don't know. I don't know if this would work with the QAPI, but you could have a '*arch-data' field in the CpuInfoFast definition, like: 'data': { ..., '*arch-data': 'CpuInfoFastArchData' } where 'CpuInfoFastArchData' is defined by each arch that supports the field. An arch supporting the field could also export a query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast().
On 12.02.2018 19:15, Luiz Capitulino wrote: > On Mon, 12 Feb 2018 13:14:32 +0100 > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > >> -{ 'struct': 'CpuInfoFast', >> - 'data': {'cpu-index': 'int', 'qom-path': 'str', >> - 'thread-id': 'int', '*props': 'CpuInstanceProperties' } } >> +{ 'union': 'CpuInfoFast', >> + 'base': {'cpu-index': 'int', 'qom-path': 'str', >> + 'thread-id': 'int', '*props': 'CpuInstanceProperties', >> + 'arch': 'CpuInfoArch' }, >> + 'discriminator': 'arch', >> + 'data': { 'x86': 'CpuInfoOther', >> + 'sparc': 'CpuInfoOther', >> + 'ppc': 'CpuInfoOther', >> + 'mips': 'CpuInfoOther', >> + 'tricore': 'CpuInfoOther', >> + 's390': 'CpuInfoS390Fast', >> + 'other': 'CpuInfoOther' } } > > Consider this a minor comment (and QMP maintainers can give a much > better advice than me), but I think this arch list has problems. For > one thing, it's incomplete. And the second problem is the 'other' > field. What happens when QEMU starts supporting a new arch? 'other' > becomes the new arch. Is this compatible? I don't know. This seems to be the customary approach, see https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html > > I don't know if this would work with the QAPI, but you could have > a '*arch-data' field in the CpuInfoFast definition, like: > > 'data': { ..., '*arch-data': 'CpuInfoFastArchData' } > > where 'CpuInfoFastArchData' is defined by each arch that supports > the field. An arch supporting the field could also export a > query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast(). > I had it like this in my first reply to your initial patch. However, that adds an additional hierarchy level in the return data. This prevents clients like libvirt to reuse the data extraction code when they switch over to using query-cpus-fast.
On Tue, 13 Feb 2018 13:30:02 +0100 Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > On 12.02.2018 19:15, Luiz Capitulino wrote: > > On Mon, 12 Feb 2018 13:14:32 +0100 > > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > > > >> -{ 'struct': 'CpuInfoFast', > >> - 'data': {'cpu-index': 'int', 'qom-path': 'str', > >> - 'thread-id': 'int', '*props': 'CpuInstanceProperties' } } > >> +{ 'union': 'CpuInfoFast', > >> + 'base': {'cpu-index': 'int', 'qom-path': 'str', > >> + 'thread-id': 'int', '*props': 'CpuInstanceProperties', > >> + 'arch': 'CpuInfoArch' }, > >> + 'discriminator': 'arch', > >> + 'data': { 'x86': 'CpuInfoOther', > >> + 'sparc': 'CpuInfoOther', > >> + 'ppc': 'CpuInfoOther', > >> + 'mips': 'CpuInfoOther', > >> + 'tricore': 'CpuInfoOther', > >> + 's390': 'CpuInfoS390Fast', > >> + 'other': 'CpuInfoOther' } } > > > > Consider this a minor comment (and QMP maintainers can give a much > > better advice than me), but I think this arch list has problems. For > > one thing, it's incomplete. And the second problem is the 'other' > > field. What happens when QEMU starts supporting a new arch? 'other' > > becomes the new arch. Is this compatible? I don't know. > This seems to be the customary approach, see > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg01986.html > > > > I don't know if this would work with the QAPI, but you could have > > a '*arch-data' field in the CpuInfoFast definition, like: > > > > 'data': { ..., '*arch-data': 'CpuInfoFastArchData' } > > > > where 'CpuInfoFastArchData' is defined by each arch that supports > > the field. An arch supporting the field could also export a > > query_cpus_fast_arch() function, which is called by qmp_query_cpus_fast(). > > > I had it like this in my first reply to your initial patch. However, > that adds an additional hierarchy level in the return data. This > prevents clients like libvirt to reuse the data extraction code when > they switch over to using query-cpus-fast. OK, fair enough.
On 12.02.2018 17:23, Cornelia Huck wrote: [...] >> diff --git a/cpus.c b/cpus.c >> index 6df6660..af67826 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) >> MachineClass *mc = MACHINE_GET_CLASS(ms); >> CpuInfoFastList *head = NULL, *cur_item = NULL; >> CPUState *cpu; >> +#if defined(TARGET_S390X) >> + S390CPU *s390_cpu; >> + CPUS390XState *env; >> +#endif >> >> CPU_FOREACH(cpu) { >> CpuInfoFastList *info = g_malloc0(sizeof(*info)); >> @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) >> info->value->props = props; >> } >> >> +#if defined(TARGET_S390X) >> + s390_cpu = S390_CPU(cpu); >> + env = &s390_cpu->env; > > You should be able to omit the s390_cpu variable by using > > env = &S390_CPU(cpu)->env; > True, but I wanted to stay in style with the code in qmp_query_cpus. >> + info->value->arch = CPU_INFO_ARCH_S390; >> + info->value->u.s390.cpu_state = env->cpu_state; >> +#endif >> if (!cur_item) { >> head = cur_item = info; >> } else { > > As you mentioned in the patch description, the duplication is a bit > awkward. I'll let the QAPI experts judge that; otherwise, this looks > fine to me. >
On Tue, 13 Feb 2018 17:12:50 +0100 Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> wrote: > On 12.02.2018 17:23, Cornelia Huck wrote: > [...] > >> diff --git a/cpus.c b/cpus.c > >> index 6df6660..af67826 100644 > >> --- a/cpus.c > >> +++ b/cpus.c > >> @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) > >> MachineClass *mc = MACHINE_GET_CLASS(ms); > >> CpuInfoFastList *head = NULL, *cur_item = NULL; > >> CPUState *cpu; > >> +#if defined(TARGET_S390X) > >> + S390CPU *s390_cpu; > >> + CPUS390XState *env; > >> +#endif > >> > >> CPU_FOREACH(cpu) { > >> CpuInfoFastList *info = g_malloc0(sizeof(*info)); > >> @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) > >> info->value->props = props; > >> } > >> > >> +#if defined(TARGET_S390X) > >> + s390_cpu = S390_CPU(cpu); > >> + env = &s390_cpu->env; > > > > You should be able to omit the s390_cpu variable by using > > > > env = &S390_CPU(cpu)->env; > > > True, but I wanted to stay in style with the code in qmp_query_cpus. TBH, I don't care too much one way or the other :) > >> + info->value->arch = CPU_INFO_ARCH_S390; > >> + info->value->u.s390.cpu_state = env->cpu_state; > >> +#endif > >> if (!cur_item) { > >> head = cur_item = info; > >> } else { > > > > As you mentioned in the patch description, the duplication is a bit > > awkward. I'll let the QAPI experts judge that; otherwise, this looks > > fine to me. > > >
diff --git a/cpus.c b/cpus.c index 6df6660..af67826 100644 --- a/cpus.c +++ b/cpus.c @@ -2166,6 +2166,10 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) MachineClass *mc = MACHINE_GET_CLASS(ms); CpuInfoFastList *head = NULL, *cur_item = NULL; CPUState *cpu; +#if defined(TARGET_S390X) + S390CPU *s390_cpu; + CPUS390XState *env; +#endif CPU_FOREACH(cpu) { CpuInfoFastList *info = g_malloc0(sizeof(*info)); @@ -2183,6 +2187,12 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) info->value->props = props; } +#if defined(TARGET_S390X) + s390_cpu = S390_CPU(cpu); + env = &s390_cpu->env; + info->value->arch = CPU_INFO_ARCH_S390; + info->value->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 598bfe5..94bfc7d 100644 --- a/hmp.c +++ b/hmp.c @@ -425,6 +425,14 @@ void hmp_info_cpus_fast(Monitor *mon, const QDict *qdict) monitor_printf(mon, "CPU%" PRId64 "\n", cpu->value->cpu_index); monitor_printf(mon, " thread-id=%" PRId64 "\n", cpu->value->thread_id); monitor_printf(mon, " qom-path=%s\n", cpu->value->qom_path); + switch (cpu->value->arch) { + case CPU_INFO_ARCH_S390: + monitor_printf(mon, " state=%s\n", + CpuS390State_str(cpu->value->u.s390.cpu_state)); + break; + default: + break; + } monitor_printf(mon, "\n"); } diff --git a/qapi-schema.json b/qapi-schema.json index 2a614af..a681d83 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -537,15 +537,26 @@ 'data': [ 'uninitialized', 'stopped', 'check-stop', 'operating', 'load' ] } ## -# @CpuInfoS390: +# @CpuInfoS390Fast: # -# Additional information about a virtual S390 CPU +# Additional information about a virtual S390 CPU which can be +# obtained without a performance penalty for a running VM # # @cpu-state: the virtual CPU's state # # Since: 2.12 ## -{ 'struct': 'CpuInfoS390', 'data': { 'cpu-state': 'CpuS390State' } } +{ 'struct': 'CpuInfoS390Fast', 'data': { 'cpu-state': 'CpuS390State' } } + +## +# @CpuInfoS390: +# +# Additional information about a virtual S390 CPU, potentially expensive +# to obtain +# +# Since: 2.12 +## +{ 'struct': 'CpuInfoS390', 'base': 'CpuInfoS390Fast', 'data': { } } ## # @query-cpus: @@ -604,12 +615,24 @@ # @props: properties describing to which node/socket/core/thread # virtual CPU belongs to, provided if supported by board # +# @arch: architecture of the cpu, which determines which additional fields +# will be listed +# # Since: 2.12 # ## -{ 'struct': 'CpuInfoFast', - 'data': {'cpu-index': 'int', 'qom-path': 'str', - 'thread-id': 'int', '*props': 'CpuInstanceProperties' } } +{ 'union': 'CpuInfoFast', + 'base': {'cpu-index': 'int', 'qom-path': 'str', + 'thread-id': 'int', '*props': 'CpuInstanceProperties', + 'arch': 'CpuInfoArch' }, + 'discriminator': 'arch', + 'data': { 'x86': 'CpuInfoOther', + 'sparc': 'CpuInfoOther', + 'ppc': 'CpuInfoOther', + 'mips': 'CpuInfoOther', + 'tricore': 'CpuInfoOther', + 's390': 'CpuInfoS390Fast', + 'other': 'CpuInfoOther' } } ## # @query-cpus-fast:
The s390 CPU state can be retrieved without interrupting the VM execution. Extendend the CpuInfoFast union with architecture specific data and an implementation for s390. Return data looks like this: [ {"thread-id":64301,"props":{"core-id":0}, "arch":"s390","cpu-state":"operating", "qom-path":"/machine/unattached/device[0]","cpu-index":0}, {"thread-id":64302,"props":{"core-id":1}, "arch":"s390","cpu-state":"operating", "qom-path":"/machine/unattached/device[1]","cpu-index":1} ] Currently there's a certain amount of duplication between the definitions of CpuInfo and CpuInfoFast, both in the base and variable areas, since there are data fields common to the slow and fast variants. A suggestion was made on the mailing list to enhance the QAPI code generation to support two layers of unions. This would allow to specify the common fields once and avoid the duplication in the leaf unions. On the other hand, the slow query-cpus should be deprecated along with the slow CpuInfo type and eventually be removed. Assuming that new architectures will not be added at high rates, we could live with the duplication for the time being. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- cpus.c | 10 ++++++++++ hmp.c | 8 ++++++++ qapi-schema.json | 35 +++++++++++++++++++++++++++++------ 3 files changed, 47 insertions(+), 6 deletions(-)