| Message ID | 20180424214550.32549-2-lersek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/24/2018 04:45 PM, Laszlo Ersek wrote: > Commit ca230ff33f89 added added the @arch field to @CpuInfoFast, but it s/added added/added/ > failed to set the new field in qmp_query_cpus_fast(), when TARGET_S390X > was not defined. The updated @query-cpus-fast example in > "qapi-schema.json" showed "arch":"x86" only because qmp_query_cpus_fast() > calls g_malloc0() to allocate CpuInfoFast, and the CPU_INFO_ARCH_X86 enum > constant is generated with value 0. > > All @arch values other than @s390 implied the @CpuInfoOther sub-struct for > @CpuInfoFast -- at the time of writing the patch --, thus no fields other > than @arch needed to be set when TARGET_S390X was not defined. Set @arch > now, by copying the corresponding assignments from qmp_query_cpus(). Perhaps worth mentioning that the riscv architecture shows up as 'other' in this patch? (But that gets cleaned up in the next one, so no big deal) > > Cc: Eric Blake <eblake@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: qemu-stable@nongnu.org > Fixes: ca230ff33f89bf7102cbfbc2328716da6750aaed > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > Reviewed-by: Eric Blake <eblake@redhat.com>
Laszlo Ersek <lersek@redhat.com> writes: > Commit ca230ff33f89 added added the @arch field to @CpuInfoFast, but it > failed to set the new field in qmp_query_cpus_fast(), when TARGET_S390X > was not defined. The updated @query-cpus-fast example in > "qapi-schema.json" showed "arch":"x86" only because qmp_query_cpus_fast() > calls g_malloc0() to allocate CpuInfoFast, and the CPU_INFO_ARCH_X86 enum > constant is generated with value 0. > > All @arch values other than @s390 implied the @CpuInfoOther sub-struct for > @CpuInfoFast -- at the time of writing the patch --, thus no fields other > than @arch needed to be set when TARGET_S390X was not defined. Set @arch > now, by copying the corresponding assignments from qmp_query_cpus(). Now I'm confused. In the schema, @arch "riscv" implies CpuInfoRISCV: { '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': 'CpuInfoS390', 'riscv': 'CpuInfoRISCV', 'other': 'CpuInfoOther' } } In qmp_query_cpus_fast(), it can't imply anything, because can't occur. That's a bug, and this patch fixes it. Except it sets @arch to "other" instead of "riscv" when defined(TARGET_RISCV). Why? Oh, I see, that gets fixed in the next patch. Please explain that in your commit message, or squash the two patches. The latter feels simpler, so that's what I'd do. > > Cc: Eric Blake <eblake@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: qemu-stable@nongnu.org > Fixes: ca230ff33f89bf7102cbfbc2328716da6750aaed > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
On Tue, 24 Apr 2018 23:45:45 +0200 Laszlo Ersek <lersek@redhat.com> wrote: > Commit ca230ff33f89 added added the @arch field to @CpuInfoFast, but it > failed to set the new field in qmp_query_cpus_fast(), when TARGET_S390X > was not defined. The updated @query-cpus-fast example in > "qapi-schema.json" showed "arch":"x86" only because qmp_query_cpus_fast() > calls g_malloc0() to allocate CpuInfoFast, and the CPU_INFO_ARCH_X86 enum > constant is generated with value 0. > > All @arch values other than @s390 implied the @CpuInfoOther sub-struct for > @CpuInfoFast -- at the time of writing the patch --, thus no fields other > than @arch needed to be set when TARGET_S390X was not defined. Set @arch > now, by copying the corresponding assignments from qmp_query_cpus(). I agree with others that this looks a bit odd for riscv, and merging patch 2 would be an option. But this is fine as well. > > Cc: Eric Blake <eblake@redhat.com> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com> > Cc: Richard Henderson <rth@twiddle.net> > Cc: qemu-stable@nongnu.org > Fixes: ca230ff33f89bf7102cbfbc2328716da6750aaed > Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 04/25/18 08:39, Markus Armbruster wrote: > Laszlo Ersek <lersek@redhat.com> writes: > >> Commit ca230ff33f89 added added the @arch field to @CpuInfoFast, but it >> failed to set the new field in qmp_query_cpus_fast(), when TARGET_S390X >> was not defined. The updated @query-cpus-fast example in >> "qapi-schema.json" showed "arch":"x86" only because qmp_query_cpus_fast() >> calls g_malloc0() to allocate CpuInfoFast, and the CPU_INFO_ARCH_X86 enum >> constant is generated with value 0. >> >> All @arch values other than @s390 implied the @CpuInfoOther sub-struct for >> @CpuInfoFast -- at the time of writing the patch --, thus no fields other >> than @arch needed to be set when TARGET_S390X was not defined. Set @arch >> now, by copying the corresponding assignments from qmp_query_cpus(). > > Now I'm confused. > > In the schema, @arch "riscv" implies CpuInfoRISCV: > > { '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': 'CpuInfoS390', > 'riscv': 'CpuInfoRISCV', > 'other': 'CpuInfoOther' } } > > In qmp_query_cpus_fast(), it can't imply anything, because can't occur. > That's a bug, and this patch fixes it. Except it sets @arch to "other" > instead of "riscv" when defined(TARGET_RISCV). Why? Oh, I see, that > gets fixed in the next patch. Please explain that in your commit > message, or squash the two patches. The latter feels simpler, so that's > what I'd do. I figured I'd keep one "Fixes:" tag per patch, but I see this separation confused at least three reviewers, so I'll squash #1 and #2, and will list two "Fixes:" tags. Thanks! Laszlo
On 04/25/18 00:30, Eric Blake wrote: > On 04/24/2018 04:45 PM, Laszlo Ersek wrote: >> Commit ca230ff33f89 added added the @arch field to @CpuInfoFast, but it > > s/added added/added/ The more I edit commit messages, the more I mess them up :) Thanks! Laszlo
diff --git a/cpus.c b/cpus.c index 38eba8bff334..1a9a2edee1f2 100644 --- a/cpus.c +++ b/cpus.c @@ -2210,27 +2210,39 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) info->value->qom_path = object_get_canonical_path(OBJECT(cpu)); info->value->thread_id = cpu->thread_id; info->value->has_props = !!mc->cpu_index_to_instance_props; if (info->value->has_props) { CpuInstanceProperties *props; props = g_malloc0(sizeof(*props)); *props = mc->cpu_index_to_instance_props(ms, cpu->cpu_index); info->value->props = props; } -#if defined(TARGET_S390X) +#if defined(TARGET_I386) + info->value->arch = CPU_INFO_ARCH_X86; +#elif defined(TARGET_PPC) + info->value->arch = CPU_INFO_ARCH_PPC; +#elif defined(TARGET_SPARC) + info->value->arch = CPU_INFO_ARCH_SPARC; +#elif defined(TARGET_MIPS) + info->value->arch = CPU_INFO_ARCH_MIPS; +#elif defined(TARGET_TRICORE) + info->value->arch = CPU_INFO_ARCH_TRICORE; +#elif 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; +#else + info->value->arch = CPU_INFO_ARCH_OTHER; #endif if (!cur_item) { head = cur_item = info; } else { cur_item->next = info; cur_item = info; } } return head; }
Commit ca230ff33f89 added added the @arch field to @CpuInfoFast, but it failed to set the new field in qmp_query_cpus_fast(), when TARGET_S390X was not defined. The updated @query-cpus-fast example in "qapi-schema.json" showed "arch":"x86" only because qmp_query_cpus_fast() calls g_malloc0() to allocate CpuInfoFast, and the CPU_INFO_ARCH_X86 enum constant is generated with value 0. All @arch values other than @s390 implied the @CpuInfoOther sub-struct for @CpuInfoFast -- at the time of writing the patch --, thus no fields other than @arch needed to be set when TARGET_S390X was not defined. Set @arch now, by copying the corresponding assignments from qmp_query_cpus(). Cc: Eric Blake <eblake@redhat.com> Cc: Markus Armbruster <armbru@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com> Cc: Richard Henderson <rth@twiddle.net> Cc: qemu-stable@nongnu.org Fixes: ca230ff33f89bf7102cbfbc2328716da6750aaed Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: PATCHv1: - new patch cpus.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)