Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
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.
>
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On Tue, 13 Feb 2018 17:12:50 +0100
Viktor Mihajlovski 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.
> >
>
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On Tue, 13 Feb 2018 13:30:02 +0100
Viktor Mihajlovski wrote:
> On 12.02.2018 19:15, Luiz Capitulino wrote:
> > On Mon, 12 Feb 2018 13:14:32 +0100
> > Viktor Mihajlovski 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.
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On 12.02.2018 19:15, Luiz Capitulino wrote:
> On Mon, 12 Feb 2018 13:14:32 +0100
> Viktor Mihajlovski 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.
--
Regards,
Viktor Mihajlovski
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On Mon, 12 Feb 2018 13:14:32 +0100
Viktor Mihajlovski 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().
Re: [Qemu-devel] [PATCH 3/3] qmp: add architecture specific cpu data for query-cpus-fast
On Mon, 12 Feb 2018 13:14:32 +0100
Viktor Mihajlovski 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
> ---
> 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.
