Re: [Qemu-devel] [PATCH v12 24/36] cpu: Convert CpuInfo into flat union

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> The CpuInfo struct is used only by the 'query-cpus' output
> command, so we are free to modify it by adding fields (clients
> are already supposed to ignore unknown output fields), or by
> changing optional members to mandatory, while still keeping
> QMP wire compatibility with older versions of qemu.
>
> 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 new 'arch' 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, since
> that is generally a poor interface practice.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v12: s/2.5/2.6/, typo fix
> v11: also fix qmp-commands.hx, improve commit message. If this
> misses 2.5 (likely), it will need updates to call out 2.6
> v10: new patch
> ---
>  cpus.c           |  31 ++++++++------
>  hmp.c            |  30 +++++++++-----
>  qapi-schema.json | 120 
> ++++++++++++++++++++++++++++++++++++++++++++++---------
>  qmp-commands.hx  |   4 ++
>  4 files changed, 143 insertions(+), 42 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 877bd70..77ba15b 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1556,22 +1556,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);

CpuInfoSparc.

> +        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 1b402c4..c2b2c16 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -311,17 +311,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 8b1a423..67e80a5 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -744,43 +744,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.6
> +##
> +{ '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.6)
> +#
>  # 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.6
> +##
> +{ 'struct': 'CpuInfoX86', 'data': { 'pc': 'int' } }
> +
> +##
> +# @CpuInfoSparc:

CpuInfoSPARC would be slightly prettier, actually.

> +#
> +# 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.6
> +##
> +{ 'struct': 'CpuInfoSparc', 'data': { 'pc': 'int', 'npc': 'int' } }
> +
> +##
> +# @CpuInfoPpc:

CpuInfoPPC would definitely be prettier.

> +#
> +# Additional information about a virtual PPC CPU
> +#
> +# @nip: the instruction pointer
> +#
> +# Since 2.6
> +##
> +{ 'struct': 'CpuInfoPpc', 'data': { 'nip': 'int' } }
> +
> +##
> +# @CpuInfoMips:

CpuInfoMIPS?

> +#
> +# Additional information about a virtual MIPS CPU
> +#
> +# @PC: the instruction pointer
> +#
> +# Since 2.6
> +##
> +{ 'struct': 'CpuInfoMips', 'data': { 'PC': 'int' } }
> +
> +##
> +# @CpuInfoTricore:
> +#
> +# Additional information about a virtual Tricore CPU
> +#
> +# @PC: the instruction pointer
> +#
> +# Since 2.6
> +##
> +{ 'struct': 'CpuInfoTricore', 'data': { 'PC': 'int' } }
> +
> +##
> +# @CpuInfoOther:
> +#
> +# No additional information is available about the virtual CPU
> +#
> +# Since 2.6
> +#
> +##
> +{ 'struct': 'CpuInfoOther', 'data': { } }
>
>  ##
>  # @query-cpus:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 9d8b42f..1b5ecb3 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2765,6 +2765,8 @@ Return a json-array. Each CPU is represented by a 
> json-object, which contains:
>  - "current": true if this is the current CPU, false otherwise (json-bool)
>  - "halted": true if the cpu is halted, false otherwise (json-bool)
>  - "qom_path": path to the CPU object in the QOM tree (json-str)
> +- "arch": architecture of the cpu, which determines what additional
> +          keys will be present (json-str)
>  - Current program counter. The key's name depends on the architecture:
>       "pc": i386/x86_64 (json-int)
>       "nip": PPC (json-int)
> @@ -2782,6 +2784,7 @@ Example:
>              "current":true,
>              "halted":false,
>              "qom_path":"/machine/unattached/device[0]",
> +            "arch":"x86",
>              "pc":3227107138,
>              "thread_id":3134
>           },
> @@ -2790,6 +2793,7 @@ Example:
>              "current":false,
>              "halted":true,
>              "qom_path":"/machine/unattached/device[2]",
> +            "arch":"x86",
>              "pc":7108165,
>              "thread_id":3135
>           }