[RFC PATCH-for-9.1 00/21] qapi: Make @query-cpu-definitions command target-agnostic

Philippe Mathieu-Daudé posted 21 patches 1 month, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20240315130910.15750-1-philmd@linaro.org
Maintainers: Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Markus Armbruster <armbru@redhat.com>, Michael Roth <michael.roth@amd.com>, Eric Blake <eblake@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Song Gao <gaosong@loongson.cn>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Huacai Chen <chenhuacai@kernel.org>, Nicholas Piggin <npiggin@gmail.com>, Daniel Henrique Barboza <danielhb413@gmail.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liwei1518@gmail.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, Artyom Tarasenko <atar4qemu@gmail.com>, Laurent Vivier <lvivier@redhat.com>
MAINTAINERS                           |   3 +-
qapi/machine-common.json              |  21 ----
qapi/machine-target.json              | 167 +-------------------------
qapi/machine.json                     | 166 ++++++++++++++++++++++++-
qapi/qapi-schema.json                 |   1 -
include/hw/core/cpu.h                 |   7 +-
include/hw/core/sysemu-cpu-ops.h      |  14 +++
include/sysemu/arch_init.h            |  71 +++++++----
target/i386/cpu-qom.h                 |  16 ++-
target/mips/cpu-qom.h                 |  13 +-
target/ppc/cpu-models.h               |   4 +
target/riscv/cpu.h                    |   2 +
target/s390x/cpu.h                    |   2 +-
target/sparc/cpu-qom.h                |   9 +-
system/cpu-qmp-cmds.c                 |  71 +++++++++++
system/cpu-qom-helpers.c              |  58 +++++++++
target/arm/arm-qmp-cmds.c             |  27 -----
target/i386/cpu.c                     |  77 ++++++------
target/loongarch/loongarch-qmp-cmds.c |  25 ----
target/mips/cpu.c                     |  34 ++++--
target/mips/sysemu/mips-qmp-cmds.c    |  31 -----
target/ppc/cpu_init.c                 |  53 ++++----
target/ppc/ppc-qmp-cmds.c             |  31 +----
target/riscv/cpu.c                    |   1 +
target/riscv/riscv-qmp-cmds.c         |  13 +-
target/sparc/cpu.c                    |  35 ++++--
tests/qtest/cpu-plug-test.c           |   2 +-
qapi/meson.build                      |   1 -
system/meson.build                    |   2 +
29 files changed, 515 insertions(+), 442 deletions(-)
delete mode 100644 qapi/machine-common.json
create mode 100644 system/cpu-qmp-cmds.c
create mode 100644 system/cpu-qom-helpers.c
[RFC PATCH-for-9.1 00/21] qapi: Make @query-cpu-definitions command target-agnostic
Posted by Philippe Mathieu-Daudé 1 month, 2 weeks ago
Hi Alex, Markus,

Markus mentioned QAPI problems with the heterogeneous emulation
binary. My understanding is, while QAPI can use host-specific
conditional (OS, library available, configure option), it
shouldn't use target-specific ones.

This series is an example on how to remove target specific
bits from the @query-cpu-definitions command. Target specific
code is registered as CPUClass handlers, then a generic method
is used, iterating over all targets built in.

The first set of patches were already posted / reviewed last
year.

The PPC and S390X targets still need work (help welcomed),
however the code is useful enough to be tested and see if this
is a good approach.

The only drawback is a change in QAPI introspection, because
targets not implementing @query-cpu-definitions were returning
"CommandNotFound". My view is this was an incomplete
implementation, rather than a feature.

Regards,

Phil.

Philippe Mathieu-Daudé (21):
  target/i386: Declare CPU QOM types using DEFINE_TYPES() macro
  target/mips: Declare CPU QOM types using DEFINE_TYPES() macro
  target/ppc: Declare CPU QOM types using DEFINE_TYPES() macro
  target/sparc: Declare CPU QOM types using DEFINE_TYPES() macro
  cpus: Open code OBJECT_DECLARE_TYPE() in OBJECT_DECLARE_CPU_TYPE()
  target/i386: Make X86_CPU common to new I386_CPU / X86_64_CPU types
  target/mips: Make MIPS_CPU common to new MIPS32_CPU / MIPS64_CPU types
  target/sparc: Make SPARC_CPU common to new SPARC32_CPU/SPARC64_CPU
    types
  qapi: Merge machine-common.json with qapi/machine.json
  qapi: Make CpuModel* definitions target agnostic
  qapi: Make CpuDefinitionInfo target agnostic
  system: Introduce QemuArchBit enum
  system: Introduce cpu_typename_by_arch_bit()
  system: Introduce QMP generic_query_cpu_definitions()
  target/arm: Use QMP generic_query_cpu_definitions()
  target/loongarch: Use QMP generic_query_cpu_definitions()
  target/riscv: Use QMP generic_query_cpu_definitions()
  target/i386: Use QMP generic_query_cpu_definitions()
  target/ppc: Factor ppc_add_alias_definitions() out
  target/ppc: Use QMP generic_query_cpu_definitions()
  qapi: Make @query-cpu-definitions target-agnostic

 MAINTAINERS                           |   3 +-
 qapi/machine-common.json              |  21 ----
 qapi/machine-target.json              | 167 +-------------------------
 qapi/machine.json                     | 166 ++++++++++++++++++++++++-
 qapi/qapi-schema.json                 |   1 -
 include/hw/core/cpu.h                 |   7 +-
 include/hw/core/sysemu-cpu-ops.h      |  14 +++
 include/sysemu/arch_init.h            |  71 +++++++----
 target/i386/cpu-qom.h                 |  16 ++-
 target/mips/cpu-qom.h                 |  13 +-
 target/ppc/cpu-models.h               |   4 +
 target/riscv/cpu.h                    |   2 +
 target/s390x/cpu.h                    |   2 +-
 target/sparc/cpu-qom.h                |   9 +-
 system/cpu-qmp-cmds.c                 |  71 +++++++++++
 system/cpu-qom-helpers.c              |  58 +++++++++
 target/arm/arm-qmp-cmds.c             |  27 -----
 target/i386/cpu.c                     |  77 ++++++------
 target/loongarch/loongarch-qmp-cmds.c |  25 ----
 target/mips/cpu.c                     |  34 ++++--
 target/mips/sysemu/mips-qmp-cmds.c    |  31 -----
 target/ppc/cpu_init.c                 |  53 ++++----
 target/ppc/ppc-qmp-cmds.c             |  31 +----
 target/riscv/cpu.c                    |   1 +
 target/riscv/riscv-qmp-cmds.c         |  13 +-
 target/sparc/cpu.c                    |  35 ++++--
 tests/qtest/cpu-plug-test.c           |   2 +-
 qapi/meson.build                      |   1 -
 system/meson.build                    |   2 +
 29 files changed, 515 insertions(+), 442 deletions(-)
 delete mode 100644 qapi/machine-common.json
 create mode 100644 system/cpu-qmp-cmds.c
 create mode 100644 system/cpu-qom-helpers.c

-- 
2.41.0


Re: [RFC PATCH-for-9.1 00/21] qapi: Make @query-cpu-definitions command target-agnostic
Posted by Markus Armbruster 1 month ago
The general approach looks sane enough to me.
Re: [RFC PATCH-for-9.1 00/21] qapi: Make @query-cpu-definitions command target-agnostic
Posted by Markus Armbruster 1 month ago
Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Hi Alex, Markus,
>
> Markus mentioned QAPI problems with the heterogeneous emulation
> binary. My understanding is, while QAPI can use host-specific
> conditional (OS, library available, configure option), it
> shouldn't use target-specific ones.

"Shouldn't" is too strong in the current state of things.  It can be
awkward, though.

Target-specific macros may be used only in target-specific code,
i.e. code that is compiled per target.  To catch uses in
target-independent code, we poison them there; see exec/poison.h.

QAPI-generated .c are not normally compiled per target.  To enable use
of target-specific macros in conditionals, we compile .c generated QAPI
submodules named *-target.json per target.  This is a bit of a hack.

Since the same conditionals will also appear in the .h generated from
them, these headers can only be used in target-specific code.

Sometimes, these headers "infect" target-independent code: we need to
compile per target just for the headers.  Awkward.  I can dig out an
example if there's interest.

But what about possible future state of things?

The QAPI schema is compile-time static by design.  QAPI conditionals
permit adjusting the schema for build configuration.  Target-specific
conditionals adjust it for the build configuration of the
target-specific part.  Each QEMU binary contains just one target's
target-specific part.

However, a single QEMU binary will contain several target-specific
parts, one per target it supports.  The targets' QAPI schema adjustments
may conflict.

For a single binary, we need to resolve these conflicts.

Special case: QAPI definitions that exist only for some targets.

Example: command query-sev exists only for TARGET_I386.  It's actually a
stub when CONFIG_SEV is off.

Obvious solution: make it exist if it needs to exist for any target
compiled into the binary.  Requires command stubs for the other targets.

Example: query-sev now exists when the single binary supports x86.  It's
a stub when CONFIG_SEV is off.  It behaves like a stub when CONFIG_SEV
is on, but the machine isn't x86.

Drawback: introspection with query-qmp-schema becomes less precise.
When the binary supports just one target, introspection can answer
target-specific questions like "does this target support SEV?"  With a
single binary, that's no longer possible.

Harmless enough for SEV, but consider query-cpu-model-expansion.  The
command may support expansion types "full" and "static".  Currently,
s390x supports both, ARM supports only "full", Loongarch only "static",
RISC-V only "full", and x86 supports both.

(I think.  The documentation is incomplete:

    # Some architectures may not support all expansion types.  s390x
    # supports "full" and "static". Arm only supports "full".
)

A management application may want to find out which expansion type is
supported.  Right now, it can't.  But we could improve the schema so it
can find out via introspection: define the expansion types only when
they're actually supported.

With a single binary, that's no longer possible: we have to define an
expansion type when *any* target supports it.

Such loss of introspection power is not a show-stopper, just something
we need to keep in mind.

> This series is an example on how to remove target specific
> bits from the @query-cpu-definitions command.

This is an instance of the special case with an additional twist: each
target defines its own QMP command handler.

>                                               Target specific
> code is registered as CPUClass handlers, then a generic method
> is used, iterating over all targets built in.
>
> The first set of patches were already posted / reviewed last
> year.
>
> The PPC and S390X targets still need work (help welcomed),
> however the code is useful enough to be tested and see if this
> is a good approach.
>
> The only drawback is a change in QAPI introspection, because
> targets not implementing @query-cpu-definitions were returning
> "CommandNotFound".

The change is introspection is actually something else.  Before the
series, query-qmp-schema returns a description of command
query-cpu-definitions exactly when the (single) target supports it.
Afterwards, it always returns one (I think).

The CommandNotFound change is a change in behavior when you try to
execute query-cpu-definitions, but the target doesn't support it.
Before, the command doesn't exist, and the QMP core duly replies
CommandNotFound.  Afterwards, it does exist, but the target doesn't
implement the call back, so the command fails.  I guess it fails with
GenericError.  We could make it fail with CommandNotFound if we care.

>                    My view is this was an incomplete
> implementation, rather than a feature.

Feels fair (but I'm not an expert in this area).