Eric Blake <ebl...@redhat.com> writes:
> When qapi type CpuInfo was originally created for 0.14, we had
> no notion of a flat union, and instead just listed a bunch of
> optional fields with documentation about the mutually-exclusive
> choice of which instruction pointer field(s) would be provided
> for a given architecture. But now that we have flat unions and
> introspection, it is better to segregate off which fields will
> be provided according to the actual architecture. With this in
> place, we no longer need the fields to be optional, because the
> choice of the discriminator serves that role.
>
> This has an additional benefit: the old all-in-one struct was
> the only place in the code base that had a case-sensitive
> naming of members 'pc' vs. 'PC'. Separating these spellings
> into different branches of the flat union will allow us to add
> restrictions against future case-insensitive collisions, and
> make it possible for a future qemu to decide whether to enable
> case-insensitive QMP.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v10: new patch
> ---
> cpus.c | 31 ++++++++------
> hmp.c | 30 +++++++++-----
> qapi-schema.json | 120
> ++++++++++++++++++++++++++++++++++++++++++++++---------
> 3 files changed, 139 insertions(+), 42 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index c6a5d0e..1c8078d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1526,22 +1526,29 @@ CpuInfoList *qmp_query_cpus(Error **errp)
> info->value->qom_path = object_get_canonical_path(OBJECT(cpu));
> info->value->thread_id = cpu->thread_id;
> #if defined(TARGET_I386)
> - info->value->has_pc = true;
> - info->value->pc = env->eip + env->segs[R_CS].base;
> + info->value->arch = CPU_INFO_ARCH_X86;
> + info->value->u.x86 = g_new0(CpuInfoX86, 1);
> + info->value->u.x86->pc = env->eip + env->segs[R_CS].base;
> #elif defined(TARGET_PPC)
> - info->value->has_nip = true;
> - info->value->nip = env->nip;
> + info->value->arch = CPU_INFO_ARCH_PPC;
> + info->value->u.ppc = g_new0(CpuInfoPpc, 1);
> + info->value->u.ppc->nip = env->nip;
> #elif defined(TARGET_SPARC)
> - info->value->has_pc = true;
> - info->value->pc = env->pc;
> - info->value->has_npc = true;
> - info->value->npc = env->npc;
> + info->value->arch = CPU_INFO_ARCH_SPARC;
> + info->value->u.sparc = g_new0(CpuInfoSPARC, 1);
> + info->value->u.sparc->pc = env->pc;
> + info->value->u.sparc->npc = env->npc;
> #elif defined(TARGET_MIPS)
> - info->value->has_PC = true;
> - info->value->PC = env->active_tc.PC;
> + info->value->arch = CPU_INFO_ARCH_MIPS;
> + info->value->u.mips = g_new0(CpuInfoMips, 1);
> + info->value->u.mips->PC = env->active_tc.PC;
> #elif defined(TARGET_TRICORE)
> - info->value->has_PC = true;
> - info->value->PC = env->PC;
> + info->value->arch = CPU_INFO_ARCH_TRICORE;
> + info->value->u.tricore = g_new0(CpuInfoTricore, 1);
> + info->value->u.tricore->PC = env->PC;
> +#else
> + info->value->arch = CPU_INFO_ARCH_OTHER;
> + info->value->u.other = g_new0(CpuInfoOther, 1);
> #endif
>
> /* XXX: waiting for the qapi to support GSList */
> diff --git a/hmp.c b/hmp.c
> index a15d00c..1307016 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -310,17 +310,25 @@ void hmp_info_cpus(Monitor *mon, const QDict *qdict)
>
> monitor_printf(mon, "%c CPU #%" PRId64 ":", active, cpu->value->CPU);
>
> - if (cpu->value->has_pc) {
> - monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->pc);
> - }
> - if (cpu->value->has_nip) {
> - monitor_printf(mon, " nip=0x%016" PRIx64, cpu->value->nip);
> - }
> - if (cpu->value->has_npc) {
> - monitor_printf(mon, " npc=0x%016" PRIx64, cpu->value->npc);
> - }
> - if (cpu->value->has_PC) {
> - monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->PC);
> + switch (cpu->value->arch) {
> + case CPU_INFO_ARCH_X86:
> + monitor_printf(mon, " pc=0x%016" PRIx64, cpu->value->u.x86->pc);
> + break;
> + case CPU_INFO_ARCH_PPC:
> + monitor_printf(mon, " nip=0x%016" PRIx64,
> cpu->value->u.ppc->nip);
> + break;
> + case CPU_INFO_ARCH_SPARC:
> + monitor_printf(mon, " pc=0x%016" PRIx64,
> cpu->value->u.sparc->pc);
> + monitor_printf(mon, " npc=0x%016" PRIx64,
> cpu->value->u.sparc->npc);
> + break;
> + case CPU_INFO_ARCH_MIPS:
> + monitor_printf(mon, " PC=0x%016" PRIx64, cpu->value->u.mips->PC);
> + break;
> + case CPU_INFO_ARCH_TRICORE:
> + monitor_printf(mon, " PC=0x%016" PRIx64,
> cpu->value->u.tricore->PC);
> + break;
> + default:
> + break;
> }
>
> if (cpu->value->halted) {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 702b7b5..dd497ea 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -728,43 +728,125 @@
> { 'command': 'query-mice', 'returns': ['MouseInfo'] }
>
> ##
> -# @CpuInfo:
> +# @CpuInfoArch:
> #
> -# Information about a virtual CPU
> +# An enumeration of cpu types that enable additional information during
> +# @query-cpus.
> +#
> +# Since: 2.5
> +##
> +{ 'enum': 'CpuInfoArch',
> + 'data': ['x86', 'sparc', 'ppc', 'mips', 'tricore', 'other' ] }
> +
> +##
> +# @CpuInfoBase:
> +#
> +# Common information about a virtual CPU
> #
> # @CPU: the index of the virtual CPU
> #
> -# @current: this only exists for backwards compatible and should be ignored
> +# @current: this only exists for backwards compatibility and should be
> ignored
> #
> # @halted: true if the virtual CPU is in the halt state. Halt usually refers
> # to a processor specific low power mode.
> #
> # @qom_path: path to the CPU object in the QOM tree (since 2.4)
> #
> -# @pc: #optional If the target is i386 or x86_64, this is the 64-bit
> instruction
> -# pointer.
> -# If the target is Sparc, this is the PC component of the
> -# instruction pointer.
> -#
> -# @nip: #optional If the target is PPC, the instruction pointer
> -#
> -# @npc: #optional If the target is Sparc, the NPC component of the
> instruction
> -# pointer
> -#
> -# @PC: #optional If the target is MIPS, the instruction pointer
> -#
> # @thread_id: ID of the underlying host thread
> #
> +# @arch: architecture of the cpu, which determines which additional fields
> +# will be listed (since 2.5)
> +#
> # Since: 0.14.0
> #
> # Notes: @halted is a transient state that changes frequently. By the time
> the
> # data is sent to the client, the guest may no longer be halted.
> ##
> -{ 'struct': 'CpuInfo',
> +{ 'struct': 'CpuInfoBase',
> 'data': {'CPU': 'int', 'current': 'bool', 'halted': 'bool',
> - 'qom_path': 'str',
> - '*pc': 'int', '*nip': 'int', '*npc': 'int', '*PC': 'int',
> - 'thread_id': 'int'} }
> + 'qom_path': 'str', 'thread_id': 'int', 'arch': 'CpuInfoArch' } }
> +
> +##
> +# @CpuInfo:
> +#
> +# Information about a virtual CPU
> +#
> +# Since: 0.14.0
> +##
> +{ 'union': 'CpuInfo', 'base': 'CpuInfoBase', 'discriminator': 'arch',
> + 'data': { 'x86': 'CpuInfoX86',
> + 'sparc': 'CpuInfoSparc',
> + 'ppc': 'CpuInfoPpc',
> + 'mips': 'CpuInfoMips',
> + 'tricore': 'CpuInfoTricore',
> + 'other': 'CpuInfoOther' } }
> +
> +##
> +# @CpuInfoX86:
> +#
> +# Additional information about a virtual i386 or x86_64 CPU
> +#
> +# @pc: the 64-bit instruction pointer
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'CpuInfoX86', 'data': { 'pc': 'int' } }
> +
> +##
> +# @CpuInfoSparc:
> +#
> +# Additional information about a virtual Sparc CPU
> +#
> +# @pc: the PC component of the instruction pointer
> +#
> +# @npc: the NPC component of the instruction pointer
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'CpuInfoSparc', 'data': { 'pc': 'int', 'npc': 'int' } }
> +
> +##
> +# @CpuInfoPpc:
> +#
> +# Additional information about a virtual PPC CPU
> +#
> +# @nip: the instruction pointer
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'CpuInfoPpc', 'data': { 'nip': 'int' } }
> +
> +##
> +# @CpuInfoMips:
> +#
> +# Additional information about a virtual MIPS CPU
> +#
> +# @PC: the instruction pointer
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'CpuInfoMips', 'data': { 'PC': 'int' } }
> +
> +##
> +# @CpuInfoMips:
> +#
> +# Additional information about a virtual Tricore CPU
> +#
> +# @PC: the instruction pointer
> +#
> +# Since 2.5
> +##
> +{ 'struct': 'CpuInfoTricore', 'data': { 'PC': 'int' } }
> +
> +##
> +# @CpuInfoOther:
> +#
> +# No additional information is available about the virtual CPU
> +#
> +# Since 2.5
> +#
> +##
> +{ 'struct': 'CpuInfoOther', 'data': { } }
>
> ##
> # @query-cpus:
CpuInfo is only used as return type of query-cpus.
If I understand the change correctly, the value of query-cpus is not
changed at all. Its introspection, however, is.
Do we need this to make 2.5?
Any other messy optionals that should really be (flat) unions?
