| Message ID | 20250424183350.1798746-1-pierrick.bouvier@linaro.org |
|---|---|
| Headers | show |
| Series | single-binary: make QAPI generated files common | expand |
On 24/4/25 20:33, Pierrick Bouvier wrote: > QAPI > ==== > > QAPI generated files contain conditional clauses to define various structures, > enums, and commands only for specific targets. This forces files to be > compiled for every target. What we try to do here is to build them only once > instead. > > In the past, we identied that the best approach to solve this is to expose code > for all targets (thus removing all #if clauses), and stub missing > symbols for concerned targets. > > This series build QAPI generated code once, by removing all TARGET_{arch} and > CONFIG_KVM clauses. What it does *not* at the moment is: > - prevent target specific commands to be visible for all targets > (see TODO comment on patch 2 explaining how to address this) + # "#if TARGET_S390X && CONFIG_KVM" will become: + # "if (target_s390x() || kvm_enabled()) {" I like it. > - nothing was done to hide all this from generated documentation > > From what I understood, the only thing that matters is to limit qmp commands > visible. Exposing enums, structure, or events is not a problem, since they > won't be used/triggered for non concerned targets. Please correct me if this is > wrong, and if there are unexpected consequences for libvirt or other consumers. What about function name clashes? I.e.: 389 ## 390 # @query-cpu-definitions: 391 # 392 # Return a list of supported virtual CPU definitions 393 # 394 # Returns: a list of CpuDefinitionInfo 395 # 396 # Since: 1.2 397 ## 398 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'], 399 'if': { 'any': [ 'TARGET_PPC', 400 'TARGET_ARM', 401 'TARGET_I386', 402 'TARGET_S390X', 403 'TARGET_MIPS', 404 'TARGET_LOONGARCH64', 405 'TARGET_RISCV' ] } } $ git grep qmp.query.cpu.definitions target/arm/arm-qmp-cmds.c:238:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) target/i386/cpu.c:6418:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) target/loongarch/loongarch-qmp-cmds.c:30:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) target/mips/system/mips-qmp-cmds.c:28:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) target/ppc/ppc-qmp-cmds.c:192:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) target/riscv/riscv-qmp-cmds.c:56:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) target/s390x/cpu_models_system.c:85:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) Prepend target name to these functions and dispatch generated code?
+Marc-André, Daniel & Dave On 24/4/25 20:33, Pierrick Bouvier wrote: > Note: This RFC was posted to trigger a discussion around this topic, and it's > not expected to merge it as it is. > > Context > ======= > > Linaro is working towards heterogeneous emulation, mixing several architectures > in a single QEMU process. The first prerequisite is to be able to build such a > binary, which we commonly name "single-binary" in our various series. > An (incomplete) list of series is available here: > https://patchew.org/search?q=project%3AQEMU+single-binary > > We don't expect to change existing command line interface or any observable > behaviour, it should be identical to existing binaries. If anyone notices a > difference, it will be a bug. > > The first objective we target is to combine qemu-system-arm and > qemu-system-aarch64 in a single binary, showing that we can build and link such > a thing. While being useless from a feature point of view, it allows us to make > good progress towards the goal, and unify two "distinct" architectures, and gain > experience on problems met. > > Our current approach is to remove compilation units duplication to be able to > link all object files together. One of the concerned subsystem is QAPI. > > QAPI > ==== > > QAPI generated files contain conditional clauses to define various structures, > enums, and commands only for specific targets. This forces files to be > compiled for every target. What we try to do here is to build them only once > instead. > > In the past, we identied that the best approach to solve this is to expose code > for all targets (thus removing all #if clauses), and stub missing > symbols for concerned targets. > > This series build QAPI generated code once, by removing all TARGET_{arch} and > CONFIG_KVM clauses. What it does *not* at the moment is: > - prevent target specific commands to be visible for all targets > (see TODO comment on patch 2 explaining how to address this) > - nothing was done to hide all this from generated documentation > > From what I understood, the only thing that matters is to limit qmp commands > visible. Exposing enums, structure, or events is not a problem, since they > won't be used/triggered for non concerned targets. Please correct me if this is > wrong, and if there are unexpected consequences for libvirt or other consumers. > > Impact on code size > =================== > > There is a strong focus on keeping QEMU fast and small. Concerning performance, > there is no impact, as the only thing that would change is to conditionally > check current target to register some commands. > Concerning code size, you can find the impact on various qemu-system binaries > with optimized and stripped build. > > upstream: > 12588 ./build/qemu-system-s390x > 83992 ./build/qemu-system-x86_64 > 31884 ./build/qemu-system-aarch64 > upstream + this series: > 12644 ./build/qemu-system-s390x (+56kB, +0.004%) > 84076 ./build/qemu-system-x86_64 (+84kB, +0.001%) > 31944 ./build/qemu-system-aarch64 (+60kB, +0.001%) > > Feedback > ======== > > The goal of this series is to be spark a conversation around following topics: > > - Would you be open to such an approach? (expose all code, and restrict commands > registered at runtime only for specific targets) > > - Are there unexpected consequences for libvirt or other consumers to expose > more definitions than what we have now? > > - Would you recommend another approach instead? I experimented with having per > target generated files, but we still need to expose quite a lot in headers, so > my opinion is that it's much more complicated for zero benefit. As well, the > code size impact is more than negligible, so the simpler, the better. > > Feel free to add anyone I could have missed in CC. > > Regards, > Pierrick > > Pierrick Bouvier (3): > qapi: add weak stubs for target specific commands > qapi: always expose TARGET_* or CONFIG_KVM code > qapi: make all generated files common > > qapi/commands-weak-stubs.c | 38 ++++++++++++++++++++++++++++++++++++++ > qapi/meson.build | 5 ++++- > scripts/qapi/commands.py | 4 ++++ > scripts/qapi/common.py | 4 +++- > 4 files changed, 49 insertions(+), 2 deletions(-) > create mode 100644 qapi/commands-weak-stubs.c >
On 4/24/25 13:43, Philippe Mathieu-Daudé wrote: > What about function name clashes? I.e.: > > 389 ## > 390 # @query-cpu-definitions: > 391 # > 392 # Return a list of supported virtual CPU definitions > 393 # > 394 # Returns: a list of CpuDefinitionInfo > 395 # > 396 # Since: 1.2 > 397 ## > 398 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'], > 399 'if': { 'any': [ 'TARGET_PPC', > 400 'TARGET_ARM', > 401 'TARGET_I386', > 402 'TARGET_S390X', > 403 'TARGET_MIPS', > 404 'TARGET_LOONGARCH64', > 405 'TARGET_RISCV' ] } } > > $ git grep qmp.query.cpu.definitions > target/arm/arm-qmp-cmds.c:238:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) > target/i386/cpu.c:6418:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) > target/loongarch/loongarch-qmp-cmds.c:30:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/mips/system/mips-qmp-cmds.c:28:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/ppc/ppc-qmp-cmds.c:192:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp) > target/riscv/riscv-qmp-cmds.c:56:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error > **errp) > target/s390x/cpu_models_system.c:85:CpuDefinitionInfoList *qmp_query_cpu_definitions(Error > **errp) > > Prepend target name to these functions and dispatch generated code? I expect we'll make this function 100% generic based on the TargetInfo API. r~
On 4/24/25 13:43, Philippe Mathieu-Daudé wrote: > What about function name clashes? I.e.: > > 389 ## > 390 # @query-cpu-definitions: > 391 # > 392 # Return a list of supported virtual CPU definitions > 393 # > 394 # Returns: a list of CpuDefinitionInfo > 395 # > 396 # Since: 1.2 > 397 ## > 398 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'], > 399 'if': { 'any': [ 'TARGET_PPC', > 400 'TARGET_ARM', > 401 'TARGET_I386', > 402 'TARGET_S390X', > 403 'TARGET_MIPS', > 404 'TARGET_LOONGARCH64', > 405 'TARGET_RISCV' ] } } > > $ git grep qmp.query.cpu.definitions > target/arm/arm-qmp-cmds.c:238:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/i386/cpu.c:6418:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/loongarch/loongarch-qmp-cmds.c:30:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/mips/system/mips-qmp-cmds.c:28:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/ppc/ppc-qmp-cmds.c:192:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/riscv/riscv-qmp-cmds.c:56:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > target/s390x/cpu_models_system.c:85:CpuDefinitionInfoList > *qmp_query_cpu_definitions(Error **errp) > > Prepend target name to these functions and dispatch generated code? In general, either we'll: - unify implementations - create a dispatcher function (based on TargetInfo target_arch()) + renaming existing symbols with suffix _{arch} - we'll create a specific interface for the concerned symbol if needed In this case, given the implementations that are very similar, maybe we can unify them in a single function using target_info()->target_cpu_type(). It's not a problem at the moment, and not directly related to QAPI generated code. We'll have to deal with symbol clashes when we have deduplicated all compilation units. QAPI code generator does not have to solve this.
On Thu, Apr 24, 2025 at 11:33:47AM -0700, Pierrick Bouvier wrote: > Feedback > ======== > > The goal of this series is to be spark a conversation around following topics: > > - Would you be open to such an approach? (expose all code, and restrict commands > registered at runtime only for specific targets) QMP defines a public API between QEMU and external mgmt apps, and personally I like the idea that the API exposed is identical across all binaries and thus the API becomes independent of the impl choice of combined vs separate binaries,. > - Are there unexpected consequences for libvirt or other consumers to expose > more definitions than what we have now? QEMU used the selective hiding of commands in the QMP schema as a mechanism to allow mgmt apps to probe for supported features. We need to check usage of each QMP API feature that's behind a TARGET_* condition and identify which libvirt uses as a feature probe, then come up with a strategy for how best to handle each case in libvirt in future. We might need some additional runtime mechanism to probe for certain things, but we won't know until we look at things in more detail. > - Would you recommend another approach instead? I experimented with having per > target generated files, but we still need to expose quite a lot in headers, so > my opinion is that it's much more complicated for zero benefit. As well, the > code size impact is more than negligible, so the simpler, the better. IMHO it is unfortunate that the API we currently expose has a dependency on a specific impl choice that mgmt apps are expected to rely on for feature probing. An ideal API design is not so closely coupled to impl choice (separate vs combined binaries), and would expose enough functionality such that mgmt apps continue to work regardless of the impl choices. We thought the conditionals were a good thing when we first designed QMP this way. We ended up using two distinct classes of conditionals, one reflecting build time features and one reflecting which target binary is used. I don't think we fully contemplated the implications that the latter set of conditionals would have on our ability to change our impl approach in future. I think the proposal here is taking us in a good direction given what we now know. With regards, Daniel
Note: This RFC was posted to trigger a discussion around this topic, and it's not expected to merge it as it is. Context ======= Linaro is working towards heterogeneous emulation, mixing several architectures in a single QEMU process. The first prerequisite is to be able to build such a binary, which we commonly name "single-binary" in our various series. An (incomplete) list of series is available here: https://patchew.org/search?q=project%3AQEMU+single-binary We don't expect to change existing command line interface or any observable behaviour, it should be identical to existing binaries. If anyone notices a difference, it will be a bug. The first objective we target is to combine qemu-system-arm and qemu-system-aarch64 in a single binary, showing that we can build and link such a thing. While being useless from a feature point of view, it allows us to make good progress towards the goal, and unify two "distinct" architectures, and gain experience on problems met. Our current approach is to remove compilation units duplication to be able to link all object files together. One of the concerned subsystem is QAPI. QAPI ==== QAPI generated files contain conditional clauses to define various structures, enums, and commands only for specific targets. This forces files to be compiled for every target. What we try to do here is to build them only once instead. In the past, we identied that the best approach to solve this is to expose code for all targets (thus removing all #if clauses), and stub missing symbols for concerned targets. This series build QAPI generated code once, by removing all TARGET_{arch} and CONFIG_KVM clauses. What it does *not* at the moment is: - prevent target specific commands to be visible for all targets (see TODO comment on patch 2 explaining how to address this) - nothing was done to hide all this from generated documentation From what I understood, the only thing that matters is to limit qmp commands visible. Exposing enums, structure, or events is not a problem, since they won't be used/triggered for non concerned targets. Please correct me if this is wrong, and if there are unexpected consequences for libvirt or other consumers. Impact on code size =================== There is a strong focus on keeping QEMU fast and small. Concerning performance, there is no impact, as the only thing that would change is to conditionally check current target to register some commands. Concerning code size, you can find the impact on various qemu-system binaries with optimized and stripped build. upstream: 12588 ./build/qemu-system-s390x 83992 ./build/qemu-system-x86_64 31884 ./build/qemu-system-aarch64 upstream + this series: 12644 ./build/qemu-system-s390x (+56kB, +0.004%) 84076 ./build/qemu-system-x86_64 (+84kB, +0.001%) 31944 ./build/qemu-system-aarch64 (+60kB, +0.001%) Feedback ======== The goal of this series is to be spark a conversation around following topics: - Would you be open to such an approach? (expose all code, and restrict commands registered at runtime only for specific targets) - Are there unexpected consequences for libvirt or other consumers to expose more definitions than what we have now? - Would you recommend another approach instead? I experimented with having per target generated files, but we still need to expose quite a lot in headers, so my opinion is that it's much more complicated for zero benefit. As well, the code size impact is more than negligible, so the simpler, the better. Feel free to add anyone I could have missed in CC. Regards, Pierrick Pierrick Bouvier (3): qapi: add weak stubs for target specific commands qapi: always expose TARGET_* or CONFIG_KVM code qapi: make all generated files common qapi/commands-weak-stubs.c | 38 ++++++++++++++++++++++++++++++++++++++ qapi/meson.build | 5 ++++- scripts/qapi/commands.py | 4 ++++ scripts/qapi/common.py | 4 +++- 4 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 qapi/commands-weak-stubs.c