| 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
On 4/25/25 00:35, Daniel P. Berrangé wrote: > 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. > Could we consider to hide the concerned commands from introspection related commands as well? The same way we prevent those commands to be registered, we can probably prevent them from being visible for libvirt. The code would still be there, and compiled once, but based on runtime target_X() value, it would not appear in introspected schema. I'm not sure how all this is implemented from QAPI code generator, maybe it's hard to do something like this, if we build the schema at compile time for instance. >> - 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. > At this point, do we know which kind of "feature" gets probed? Is it only the list of commands available, or is there probes based on enum/struct definition? If yes, the latter seems to be a wrong way to identify a target, when it could simply use query-target. > 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. > Thanks for considering an alternative way given the new needs, that's appreciated. Would that possible to get some help from people from libvirt or QAPI developers for this? > With regards, > Daniel Thanks, Pierrick
On 4/25/25 08:38, Markus Armbruster wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> 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. > > Define "notice a difference" :) More on that below. > Given a single-binary *named* exactly like an existing qemu-system-X binary, any user or QEMU management layer should not be able to distinguish it from the real qemu-system-X binary. The new potential things will be: - introduction of an (optional) -target option, which allows to override/disambiguate default target detected. - potentially more boards/cpus/devices visible, once we start developing heterogeneous emulation. See it as a new CONFIG_{new_board} present. Out of that, once the current target is identified, based on argv[0], there should be absolutely no difference, whether in the behaviour, UI, command line, or the monitor interfaces. Maybe (i.e. probably) one day people will be interested to create a new shiny command line for heteregenous scenarios, but for now, this is *not* the goal we pursue. We just want to be able to manually define a board mixing two different cpu architectures, without reinventing all the wheels coming with that. Once everything is ready (and not before), it will be a good time to revisit the command line interface to reflect this. Definitely a small task compared to all we have left to do now. Finally, even if we decide to do those changes, I think they should be reflected on both existing binaries and the new single-binary. It would be a mistake to create "yet another" way to use QEMU, just because we have N architectures available instead of one. >> 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. > > Makes sense to me. > >> 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. > > To be precise: conditionals that use macros restricted to > target-specific code, i.e. the ones poisoned by exec/poison.h. Let's > call them target-specific QAPI conditionals. > > The QAPI generator is blissfully unaware of all this. > Indeed, the only thing QAPI generaor is aware of is that it's a compile time definition, since it implements those with #if, compared to a runtime check. > The build system treats QAPI modules qapi/*-target.json as > target-specific. The .c files generated for them are compiled per > target. See qapi/meson.build. > > Only such target-specific modules can can use target-specific QAPI > conditionals. Use in target-independent modules will generate C that > won't compile. > > Poisoned macros used in qapi/*-target.json: > > CONFIG_KVM > TARGET_ARM > TARGET_I386 > TARGET_LOONGARCH64 > TARGET_MIPS > TARGET_PPC > TARGET_RISCV > TARGET_S390X > >> What we try to do here is to build them only once >> instead. > > You're trying to eliminate target-specific QAPI conditionals. Correct? > Yes, but without impacting the list of commands exposed. Thus, it would be needed to select at runtime to expose/register commands. >> 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 affects QAPI/QMP introspection, i.e. the value of query-qmp-schema. > > Management applications can no longer use introspection to find out > whether target-specific things are available. > As asked on my previous email answering Daniel, would that be possible to build the schema dynamically, so we can decide what to expose or not introspection wise? > For instance, query-cpu-definitions is implemented for targets arm, > i386, loongarch, mips, ppc, riscv, and s390x. It initially was for > fewer targets, and more targets followed one by one. Still more may > follow in the future. Right now, management applications can use > introspection to find out whether it is available. That stops working > when you make it available for all targets, stubbed out for the ones > that don't (yet) implement it. > I will repeat, just to be clear, I don't think exposing all commands is a good idea. The current series *does not* do this, simply because I didn't want to huge work for nothing. > Management applications may have to be adjusted for this. > > This is not an attempt to shoot down your approach. I'm merely > demonstrating limitations of your promise "if anyone notices a > difference, it will be a bug." > I stick to this promise :). > Now, we could get really fancy and try to keep introspection the same by > applying conditionals dynamically somehow. I.e. have the single binary > return different introspection values depending on the actual guest's > target. > > This requires fixing the target before introspection. Unless this is > somehow completely transparent (wrapper scripts, or awful hacks based on > the binary's filename, perhaps), management applications may have to be > adjusted to actually do that. > > Applies not just to introspection. Consider query-cpu-definitions > again. It currently returns CPU definitions for *the* target. What > would a single binary's query-cpu-definitions return? The CPU > definitions for *all* its targets? Management applications then receive > CPUs that won't work, which may upset them. To avoid noticable > difference, we again have to fix the target before we look. > > Of course, "fixing the target" stops making sense once we move to > heterogeneous machines with multiple targets. > At this point, I don't have think about what should be the semantic when we'll have multiple targets running simultaneously (expose the union, restrict to the main arch, choose a third way). >> 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 > > For better or worse, generated documentation always contains everything. > Fine for me, it makes sense, as the official documentation published, which is what people will consume primarily, is for all targets. > An argument could be made for stripping out documentation for the stuff > that isn't included in this build. > >> 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. > > I'm not sure what you mean by "to limit qmp commands visible". > > QAPI/QMP introspection has all commands and events, and all types > reachable from them. query-qmp-schema returns an array, where each > array element describes one command, event, or type. When a command, > event, or type is conditional in the schema, the element is wrapped in > the #if generated for the condition. > After reading and answering to your valuable email, I definitely think the introspection schema we expose should be adapted, independently of how we build QAPI code (i.e. using #ifdef TARGET or not). Is it something technically hard to achieve? >> >> 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) > > Yes, if we can find acceptable solutions for the problems that come with > it. > >> - Are there unexpected consequences for libvirt or other consumers to expose >> more definitions than what we have now? > > Maybe. > >> - 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. > > I'm throwing in devel@lists.libvirt.org. > >> 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 > Thanks, Pierrick
On 4/25/25 14:07, Pierrick Bouvier wrote: >> QAPI/QMP introspection has all commands and events, and all types >> reachable from them. query-qmp-schema returns an array, where each >> array element describes one command, event, or type. When a command, >> event, or type is conditional in the schema, the element is wrapped in >> the #if generated for the condition. >> > > After reading and answering to your valuable email, I definitely think > the introspection schema we expose should be adapted, independently of > how we build QAPI code (i.e. using #ifdef TARGET or not). > > Is it something technically hard to achieve? > From existing json format, we could imagine to change semantic of "if" field to mean "expose in the schema, and register associated commands", instead "introduce ifdefs around this". QAPI generator would not have to know about what is inside the ifs, simply calling expression passed as value, and condition command registering and schema exposition with that.
On 25/4/25 09:35, Daniel P. Berrangé wrote: > 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,. I tried to expose all structures / unions as a first step (not yet commands) but realized even structure fields can be conditional, see @deprecated-props: ## # @CpuModelExpansionInfo: # # The result of a cpu model expansion. # # @model: the expanded CpuModelInfo. # # @deprecated-props: a list of properties that are flagged as # deprecated by the CPU vendor. The list depends on the # CpuModelExpansionType: "static" properties are a subset of the # enabled-properties for the expanded model; "full" properties are # a set of properties that are deprecated across all models for # the architecture. (since: 9.1). # # Since: 2.8 ## { 'struct': 'CpuModelExpansionInfo', 'data': { 'model': 'CpuModelInfo', 'deprecated-props' : { 'type': ['str'], 'if': 'TARGET_S390X' } }, 'if': { 'any': [ 'TARGET_S390X', 'TARGET_I386', 'TARGET_ARM', 'TARGET_LOONGARCH64', 'TARGET_RISCV' ] } }
Philippe Mathieu-Daudé <philmd@linaro.org> writes: > On 25/4/25 09:35, Daniel P. Berrangé wrote: >> 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,. > > I tried to expose all structures / unions as a first step (not yet > commands) but realized even structure fields can be conditional, Correct. See docs/devel/qapi-code-gen.rst section "Configuring the schema". > see @deprecated-props: > > ## > # @CpuModelExpansionInfo: > # > # The result of a cpu model expansion. > # > # @model: the expanded CpuModelInfo. > # > # @deprecated-props: a list of properties that are flagged as > # deprecated by the CPU vendor. The list depends on the > # CpuModelExpansionType: "static" properties are a subset of the > # enabled-properties for the expanded model; "full" properties are > # a set of properties that are deprecated across all models for > # the architecture. (since: 9.1). > # > # Since: 2.8 > ## > { 'struct': 'CpuModelExpansionInfo', > 'data': { 'model': 'CpuModelInfo', > 'deprecated-props' : { 'type': ['str'], > 'if': 'TARGET_S390X' } }, > 'if': { 'any': [ 'TARGET_S390X', > 'TARGET_I386', > 'TARGET_ARM', > 'TARGET_LOONGARCH64', > 'TARGET_RISCV' ] } }
On Fri, Apr 25, 2025 at 01:39:49PM -0700, Pierrick Bouvier wrote: > On 4/25/25 00:35, Daniel P. Berrangé wrote: > > 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. > > > > Could we consider to hide the concerned commands from introspection related > commands as well? The same way we prevent those commands to be registered, > we can probably prevent them from being visible for libvirt. > The code would still be there, and compiled once, but based on runtime > target_X() value, it would not appear in introspected schema. > > I'm not sure how all this is implemented from QAPI code generator, maybe > it's hard to do something like this, if we build the schema at compile time > for instance. > > > > - 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. > > > > At this point, do we know which kind of "feature" gets probed? Is it only > the list of commands available, or is there probes based on enum/struct > definition? In general if it is visible from QMP it is liable to get probed - any and every aspect of it is in scope. To figure this out you need to produce a list of each command/struct/field that has a 'if $TARGET' conditional, and check whether libvirt uses it or not. > > 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. > > > > Thanks for considering an alternative way given the new needs, that's > appreciated. > > Would that possible to get some help from people from libvirt or QAPI > developers for this? There challenge here is how QEMU will change this without back compat problems. Our deprecation process won't work well here. There's no nice way to flag up that we're about to change the way conditionals work. Most of the time libvirt adapts by changing the way we "probe the data", but in this case its about adapting to the way we "probe data about the data". This kind of problem is why I liked the previous idea that Phillippe was trying of introducing a "qemu-system-any" - it didn't affect the behaviour of existing qemu-system-$TARGET commands, so apps had a clean line in the sand between old & new behaviour. With regards, Daniel
On 4/28/25 1:37 AM, Daniel P. Berrangé wrote: > On Fri, Apr 25, 2025 at 01:39:49PM -0700, Pierrick Bouvier wrote: >> On 4/25/25 00:35, Daniel P. Berrangé wrote: >>> 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. >>> >> >> Could we consider to hide the concerned commands from introspection related >> commands as well? The same way we prevent those commands to be registered, >> we can probably prevent them from being visible for libvirt. >> The code would still be there, and compiled once, but based on runtime >> target_X() value, it would not appear in introspected schema. >> >> I'm not sure how all this is implemented from QAPI code generator, maybe >> it's hard to do something like this, if we build the schema at compile time >> for instance. >> >>>> - 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. >>> >> >> At this point, do we know which kind of "feature" gets probed? Is it only >> the list of commands available, or is there probes based on enum/struct >> definition? > > In general if it is visible from QMP it is liable to get probed - any > and every aspect of it is in scope. > > To figure this out you need to produce a list of each command/struct/field > that has a 'if $TARGET' conditional, and check whether libvirt uses it or > not. > At this point, it seems more natural to expose exactly the same information, instead of trying to see "potential" impact of exposing more. I just feel like we'll open a pandora box, and it will take a long time to converge anywhere. The initial motivation is just to remove per target code in QAPI generated code, and not to trigger a whole refactoring in several projects. >>> 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. >>> >> >> Thanks for considering an alternative way given the new needs, that's >> appreciated. >> >> Would that possible to get some help from people from libvirt or QAPI >> developers for this? > > There challenge here is how QEMU will change this without back compat > problems. > > Our deprecation process won't work well here. There's no nice way to flag > up that we're about to change the way conditionals work. > If we expose exactly the same schema, there is no need to change anything. The only impact is on modifying the .json, to add new *_if clauses, targetting runtime, instead of compile time. When posting my series, I was not aware of all the introspection aspect, and simply was focused on qemu monitor commands which are registered, so thanks for bringing this. After looking at schema introspection code, I think it will not be hard to expose parts based on a runtime check. > Most of the time libvirt adapts by changing the way we "probe the data", > but in this case its about adapting to the way we "probe data about the > data". > > This kind of problem is why I liked the previous idea that Phillippe > was trying of introducing a "qemu-system-any" - it didn't affect the > behaviour of existing qemu-system-$TARGET commands, so apps had a > clean line in the sand between old & new behaviour. > It's something we might talk about later for sure. On this thread, if possible, I would like to avoid starting talking about the new binary, new command line, new monitor schema, as they are a different topic, and I'm sure people will have a lot of different opinions, which won't solve the current issue of compiling QAPI generated code only once. > With regards, > Daniel
On 4/25/25 11:21 PM, Markus Armbruster wrote: > Trouble is some uses of the second kind are in QAPI conditionals. I can > see three options: > > (1) Drop these conditionals. > > (2) Replace them by run-time checks. > > (3) Have target-specific QAPI-generated code for multiple targets > coexist in the single binary. > > As far as I can tell, your RFC series is an incomplete attempt at (2). > > I gather you considered (3), but you dislike it for its bloat and > possibly other reasons. I sympathize; the QAPI-generated code is plenty > bloated as it is, in good part to early design decisions (not mine). > > Your "no noticeable differences" goal precludes (1). > > Back to (2). In C, replacing compile-time conditionals by run-time > checks means replacing #if FOO by if (foo). Such a transformation isn't > possible in the QAPI schema. To make it possible, we need to evolve the > QAPI schema language. > > docs/devel/qapi-code-gen.rst describes what we have: > > COND = STRING > | { 'all: [ COND, ... ] } > | { 'any: [ COND, ... ] } > | { 'not': COND } > > [....] > > The C code generated for the definition will then be guarded by an #if > preprocessing directive with an operand generated from that condition: > > * STRING will generate defined(STRING) > * { 'all': [COND, ...] } will generate (COND && ...) > * { 'any': [COND, ...] } will generate (COND || ...) > * { 'not': COND } will generate !COND > > So, conditions are expression trees where the leaves are preprocessor > symbols and the inner nodes are operators. > > It's not quite obvious to me how to best evolve this to support run-time > checks. > After looking at the introspection code, I don't see any major blocker. We need to keep some of existing "if", as they are based on config-host, and should apply. We can introduce a new "available_if" (or any other name), which generates a runtime check when building the schema, or when serializing a struct. This way, by modifying the .json with: - if: 'TARGET_I386' + available_if: 'target_i386()' This way, we keep the possibility to have ifdef, and we can expose at runtime based on available_if. So we can keep the exact same schema we have today per target. > Whatever we choose should support generating Rust and Go as well. Why? > Rust usage in QEMU is growing, and we'll likely need to generate some > Rust from the QAPI schema. Victor Toso has been working on Go bindings > for use in Go QMP client software. > I don't see any blocker with that. If you mention generating Rust and Go from qapi json definitions, it's already dependent on C preprocessor because of ifdef constant. So it will have to be adapted anyway. Having the same function (target_i386()) name through different languages is not something hard to achieve. >>> The build system treats QAPI modules qapi/*-target.json as >>> target-specific. The .c files generated for them are compiled per >>> target. See qapi/meson.build. >>> >>> Only such target-specific modules can can use target-specific QAPI >>> conditionals. Use in target-independent modules will generate C that >>> won't compile. >>> >>> Poisoned macros used in qapi/*-target.json: >>> >>> CONFIG_KVM >>> TARGET_ARM >>> TARGET_I386 >>> TARGET_LOONGARCH64 >>> TARGET_MIPS >>> TARGET_PPC >>> TARGET_RISCV >>> TARGET_S390X >>> >>>> What we try to do here is to build them only once >>>> instead. >>> >>> You're trying to eliminate target-specific QAPI conditionals. Correct? >>> >> >> Yes, but without impacting the list of commands exposed. Thus, it would >> be needed to select at runtime to expose/register commands. > > Conditionals affect more than just commands. > Thus, the proposal above to do the same for concerned struct members. >>>> 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 affects QAPI/QMP introspection, i.e. the value of query-qmp-schema. >>> >>> Management applications can no longer use introspection to find out >>> whether target-specific things are available. >>> >> >> As asked on my previous email answering Daniel, would that be possible >> to build the schema dynamically, so we can decide what to expose or not >> introspection wise? > > QAPI was designed to be compile-time static. Revising such fundamental > design assumptions is always fraught. I can't give you a confident > assessment now. All I can offer you is my willingness to explore > solutions. See "really fancy" below. > > Fun fact: we used to generate the value of query-qmp-schema as a single > string. We switched to the current, more bloated representation to > support conditionals (commit 7d0f982bfbb). > It's nice to have this, and this is what would allow us to conditionnally include or not various definitions/commands/fields. I was a bit worried we would have a "static string", but was glad to find a static list instead. >>> For instance, query-cpu-definitions is implemented for targets arm, >>> i386, loongarch, mips, ppc, riscv, and s390x. It initially was for >>> fewer targets, and more targets followed one by one. Still more may >>> follow in the future. Right now, management applications can use >>> introspection to find out whether it is available. That stops working >>> when you make it available for all targets, stubbed out for the ones >>> that don't (yet) implement it. >>> >> >> I will repeat, just to be clear, I don't think exposing all commands is >> a good idea. >> The current series *does not* do this, simply because I didn't want to >> huge work for nothing. > > Got it. > >>> Management applications may have to be adjusted for this. >>> >>> This is not an attempt to shoot down your approach. I'm merely >>> demonstrating limitations of your promise "if anyone notices a >>> difference, it will be a bug." >>> >> >> I stick to this promise :). >> >>> Now, we could get really fancy and try to keep introspection the same by >>> applying conditionals dynamically somehow. I.e. have the single binary >>> return different introspection values depending on the actual guest's >>> target. >>> >>> This requires fixing the target before introspection. Unless this is >>> somehow completely transparent (wrapper scripts, or awful hacks based on >>> the binary's filename, perhaps), management applications may have to be >>> adjusted to actually do that. >>> >>> Applies not just to introspection. Consider query-cpu-definitions >>> again. It currently returns CPU definitions for *the* target. What >>> would a single binary's query-cpu-definitions return? The CPU >>> definitions for *all* its targets? Management applications then receive >>> CPUs that won't work, which may upset them. To avoid noticable >>> difference, we again have to fix the target before we look. >>> >>> Of course, "fixing the target" stops making sense once we move to >>> heterogeneous machines with multiple targets. >>> >> >> At this point, I don't have think about what should be the semantic when >> we'll have multiple targets running simultaneously (expose the union, >> restrict to the main arch, choose a third way). > > We have to unless we make query-cpu-definitions fail or impossible to > send while the target is still undecided. > > Making it fail would violate the "no observable differences" goal. > > The only path to true "no observable differences" I can see is to fix > the target before the management application interacts with QEMU in any > way. This would make QMP commands (query-cpu-definitions, > query-qmp-schema, ...) impossible to send before the target is fixed. > The current target will be set at the entry of main() in QEMU, so before the monitor is created. Thus, it will be unambiguous. >>>> 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 >>> >>> For better or worse, generated documentation always contains everything. >>> >> >> Fine for me, it makes sense, as the official documentation published, >> which is what people will consume primarily, is for all targets. >> >>> An argument could be made for stripping out documentation for the stuff >>> that isn't included in this build. >>> >>>> 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. >>> >>> I'm not sure what you mean by "to limit qmp commands visible". >>> >>> QAPI/QMP introspection has all commands and events, and all types >>> reachable from them. query-qmp-schema returns an array, where each >>> array element describes one command, event, or type. When a command, >>> event, or type is conditional in the schema, the element is wrapped in >>> the #if generated for the condition. >>> >> >> After reading and answering to your valuable email, I definitely think > > Thanks! > >> the introspection schema we expose should be adapted, independently of >> how we build QAPI code (i.e. using #ifdef TARGET or not). >> >> Is it something technically hard to achieve? > > Unclear. See "fundamental design assumptions" and "need to evolve the > QAPI schema language" above. > > If you want to learn more about introspection, I'd recommend > docs/devel/qapi-code-gen.rst section "Client JSON Protocol > introspection". > I'll give a try at conditioning all this by runtime checks, so you can review which changes it would create. > [...] > Regards, Pierrick
On 4/28/25 3:25 AM, Peter Krempa wrote: > On Fri, Apr 25, 2025 at 14:07:34 -0700, Pierrick Bouvier wrote: >> On 4/25/25 08:38, Markus Armbruster wrote: >>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >>> >>>> 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. >>> >>> Define "notice a difference" :) More on that below. >>> >> >> Given a single-binary *named* exactly like an existing qemu-system-X binary, >> any user or QEMU management layer should not be able to distinguish it from >> the real qemu-system-X binary. >> >> The new potential things will be: >> - introduction of an (optional) -target option, which allows to >> override/disambiguate default target detected. >> - potentially more boards/cpus/devices visible, once we start developing >> heterogeneous emulation. See it as a new CONFIG_{new_board} present. >> >> Out of that, once the current target is identified, based on argv[0], there >> should be absolutely no difference, whether in the behaviour, UI, command >> line, or the monitor interfaces. > > Okay, so assuming that the correctly named binaries will apply whatever > necessary magic to have them behave identically as they did. > > I'll also ignore the distros that rename them assuming they do it in a > way that stays compatible. > > The question is how the new unified binary will behave when being > introspected: > > - Can the unified binary be introspected without selecting an > architecture? > (by introspection I mean starting with -M none and querying stuff via > QMP) > > if no: libvirt will have a chicken&egg problem deciding what to do > > - What will be the answer for the platform-specific stuff such as CPU > definitions? > It's a good question, to which I didn't really spend time thinking of, mainly because I prefer to stay focus on achieving a single-binary having a *single-target* available at runtime first. Once we have this, which should be a "zero effect" change, I think we'll be ready to discuss how the monitor and QEMU command line should be affected by heterogeneous emulation. I feel that several people expressed their desire to create a new and minimal command line/monitor for this new heterogeneous scenario. IMHO, I would prefer to see things going towards the direction of having a "main" target, and secondaries targets available at runtime. Basically, for a aarch64/microblaze mixed board, it would be the same as enhancing qemu-system-aarch64, with microblaze cpus available. Eventually, we can add specific options/monitor commands associated to this, but I'm not sure that refactoring the whole thing is the good way to go. That said, my experience on such usages, and on QEMU in general, is much more limited than all the people here, so I would be happy to listen and follow what they will design and implement. It's just my personal software engineer alarm which gets triggered when I hear "Let's create another new way to access this", when we are not even sure at this point of how many heterogenous boards we'll have in the next 10 years. Maybe heterogenenous scenarios will concern only a few exotic boards, and it would not be worth buying new shiny wheels for the QEMU car ahead of this. Just my two cents. In all cases, my current focus is to be able to compile and link such a binary, without which we can't do any step further. Regards, Pierrick
On 4/28/25 4:07 AM, Markus Armbruster wrote: > Peter Krempa <pkrempa@redhat.com> writes: > >> So what should libvirt do once multiple targets are supported? >> >> How do we query CPUs for each of the supported targets? >> It's kind of a similar question we have to solve now with QEMU code. What happens when a symbol is duplicated, and available only for several targets? In this case, we found various approaches to solve this: - unify this symbol for all targets (single implementation) - unify all targets to provide this symbol (multiple impl, all targets) - rename symbols adding {arch} suffix, so it's disambiguated by name - create a proper interface which an available function (multiple impl, selective targets) In the case of query-cpu-definitions, my intuition is that we want to have a single implementation, and that we return *all* the cpus, merging all architectures. In the end, we (and libvirt also) should think out of the "target" box. It's an implementation detail, based on the fact QEMU had 'targets' associated to various binaries for a long time and not a concept that should leak into all consumers. >> Will the result be the same if we query them one at a time or all at >> once? > > Pierrick's stated goal is to have no noticable differences between the > single binary and the qemu-system-<target> it covers. This is obviously > impossible if we can interact with the single binary before the target > is fixed. > Right. At this point, we can guarantee the target will be fixed before anything else, at the start of main(). It's obviously an implementation choice, but to be honest, I don't see what we would gain from having a "null" default QEMU target, unable to emulate anything. >>> This requires fixing the target before introspection. Unless this is >>> somehow completely transparent (wrapper scripts, or awful hacks based on >>> the binary's filename, perhaps), management applications may have to be >>> adjusted to actually do that. >> >> As noted filename will not work. Users can specify any filename and >> create override scripts or rename the binary. > > True. > I would prefer to not open this pandora box on this thread, but don't worry, the best will be done to support all those cases, including renaming the binary, allowing any prefix, suffix, as long as name stays unambiguous. If you rename it to qemu-ok, how can you expect anything? We can provide the possibility to have a "default" target set at compile time, for distributors creating their own specific QEMU binaries. But in the context of classical software distribution, it doesn't make any sense.
On Thu, Apr 24, 2025 at 2:35 PM Pierrick Bouvier <pierrick.bouvier@linaro.org> 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) > > - 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. Do you anticipate that Linux distributions will change how they package QEMU? For example, should they ship a single qemu-all package in addition to or as a replacement for the typical model today where qemu-system-aarch64, qemu-system-x86_64, etc are shipped as separate packages? It would be nice to hear from packager maintainers in this discussion so that there is a consensus between developers and package maintainers. Stefan
Hi Stefan, On 4/28/25 11:14 AM, Stefan Hajnoczi wrote: > On Thu, Apr 24, 2025 at 2:35 PM Pierrick Bouvier > <pierrick.bouvier@linaro.org> 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) >> >> - 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. > > Do you anticipate that Linux distributions will change how they > package QEMU? For example, should they ship a single qemu-all package > in addition to or as a replacement for the typical model today where > qemu-system-aarch64, qemu-system-x86_64, etc are shipped as separate > packages? > Different distributions will have different opinions. In case we decide one day (which is *not* short term future) to replace existing binaries with a single one, it's probably a discussion that will happen. My personal "anticipation" is that if we unify all targets in a single binary (which is not happening tomorrow), distributions can always create a qemu-system-common package, and depend on it for all targets. Thus, every qemu-system-X will simply include the expected symlink (or wrapper script, or whatever) to the single binary. Or they can recompile the single binary for every subpackage they want in case they want to absolutely reduce the code size for a single target, even though the sum of binaries will be infinitely bigger than using the single one. In any case, it's not something that will happen soon, except if everyone in the community becomes convinced of the advantage of building QEMU as a single binary, instead of per target binaries. Even if this never converges, there are still benefits left for what is done right now: - Faster multi targets build: less compilation units == less time. - Smaller multi targets build footprint: seems relevant as disk space on GitLab CI is a recurrent complaint. - Clarification of code: I hope C developers are objectively (i.e. not personal preference) convinced that less ifdef soup is better. > It would be nice to hear from packager maintainers in this discussion > so that there is a consensus between developers and package > maintainers. > Sure. Maybe there is a misunderstanding, but at this point, we are not trying to invent anything new. We are just looking for a way to build QAPI generated code only once, so it's possible to link together object files coming from two different targets. My mistake was to not mention introspection in the cover letter, but thanks to Markus and Daniel, I understood the consequences of that, and my position is to keep the current schema and serialization methods *exactly* as they are, so consumers don't see any change. The only place where we need to do changes are scripts/qapi and qapi/. > Stefan Regards, Pierrick
On Mon, Apr 28, 2025 at 3:25 PM Pierrick Bouvier <pierrick.bouvier@linaro.org> wrote: > > Hi Stefan, > > On 4/28/25 11:14 AM, Stefan Hajnoczi wrote: > > On Thu, Apr 24, 2025 at 2:35 PM Pierrick Bouvier > > <pierrick.bouvier@linaro.org> 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) > >> > >> - 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. > > > > Do you anticipate that Linux distributions will change how they > > package QEMU? For example, should they ship a single qemu-all package > > in addition to or as a replacement for the typical model today where > > qemu-system-aarch64, qemu-system-x86_64, etc are shipped as separate > > packages? > > > > Different distributions will have different opinions. > In case we decide one day (which is *not* short term future) to replace > existing binaries with a single one, it's probably a discussion that > will happen. > > My personal "anticipation" is that if we unify all targets in a single > binary (which is not happening tomorrow), distributions can always > create a qemu-system-common package, and depend on it for all targets. > Thus, every qemu-system-X will simply include the expected symlink (or > wrapper script, or whatever) to the single binary. > Or they can recompile the single binary for every subpackage they want > in case they want to absolutely reduce the code size for a single > target, even though the sum of binaries will be infinitely bigger than > using the single one. > In any case, it's not something that will happen soon, except if > everyone in the community becomes convinced of the advantage of building > QEMU as a single binary, instead of per target binaries. > > Even if this never converges, there are still benefits left for what is > done right now: > - Faster multi targets build: less compilation units == less time. > - Smaller multi targets build footprint: seems relevant as disk space on > GitLab CI is a recurrent complaint. > - Clarification of code: I hope C developers are objectively (i.e. not > personal preference) convinced that less ifdef soup is better. > > > It would be nice to hear from packager maintainers in this discussion > > so that there is a consensus between developers and package > > maintainers. > > > > Sure. > > Maybe there is a misunderstanding, but at this point, we are not trying > to invent anything new. We are just looking for a way to build QAPI > generated code only once, so it's possible to link together object files > coming from two different targets. > > My mistake was to not mention introspection in the cover letter, but > thanks to Markus and Daniel, I understood the consequences of that, and > my position is to keep the current schema and serialization methods > *exactly* as they are, so consumers don't see any change. The only place > where we need to do changes are scripts/qapi and qapi/. Okay, if this is just about QAPI for now then it's definitely too early to discuss packaging. I'm still curious which specific use cases you have in mind? Stefan
On 4/28/25 12:54 PM, Stefan Hajnoczi wrote: >> Sure. >> >> Maybe there is a misunderstanding, but at this point, we are not trying >> to invent anything new. We are just looking for a way to build QAPI >> generated code only once, so it's possible to link together object files >> coming from two different targets. >> >> My mistake was to not mention introspection in the cover letter, but >> thanks to Markus and Daniel, I understood the consequences of that, and >> my position is to keep the current schema and serialization methods >> *exactly* as they are, so consumers don't see any change. The only place >> where we need to do changes are scripts/qapi and qapi/. > > Okay, if this is just about QAPI for now then it's definitely too > early to discuss packaging. > > I'm still curious which specific use cases you have in mind? > The (lenghty) cover letter tries to explain where we are going, and why this change is needed for QAPI. If it's not clear, feel free to tell, I'll update this for v2. > Stefan
On 29/04/2025 10.23, Markus Armbruster wrote: ... > I don't wish to derail this thread, but we've been dancing around the > question of how to best fix the target for some time. I think we should > talk about it for real. > > Mind, this is not an objection to your larger "single binary" idea. It > could be only if it was an intractable problem, but I don't think it is. > > You want the single binary you're trying to create to be a drop-in > replacement for per-target binaries. > > "Drop-in replacement" means existing usage continues to work. > Additional interfaces are not a problem. > > To achieve "drop-in replacement", the target needs to be fixed > automatically, and before the management application can further > interact with it. > > If I understand you correctly, you're proposing to use argv[0] for that, > roughly like this: assume it's qemu-system-<target>, extract <target> > first thing in main(), done. > > What if it's not named that way? If I understand you correctly, you're > proposing to fall back to a compiled-in default target. > > I don't think this is going to fly. I tend to disagree. For normal users that consume QEMU via the distros, the check via argv[0] should be good enough. For developers, I think we can assume that they are adaptive enough to use an additional "-target" option in case they mis-named their binary in a bad way. > Developers rename the binary all the time, and expect this not to change > behavior. For instance, I routinely rename qemu-FOO to qemu-FOO.old or > qemu-FOO.COMMIT-HASH to let me compare behavior easily. Developers should already be aware that this can cause trouble, since e.g. the qtests are deriving the target architecture from the binary name already. See the qtest_get_arch() function. > We could relax the assumption to support such renames. Developers then > need to be aware of what renames are supported. Meh. > > The more we relax the pattern, the likelier surprising behavior becomes. > > We could mitigate surprises by eliminating the built-in default target. Just print out a proper error message if the target cannot be derived from argv[0], pointing the users to use "-target", and we should be fine. And if someone renames their "qemu-sytem-aarch64" symlink to "qemu-system-x86_64" and still expect to run aarch64 images that way, that's just plain stupidity. > Users invoke their binaries with their own names, too. If Joe R. User > finds qemu-system-<joe's-fav-target> too much to type, and creates a > symlink named q to it, more power to him! They can also either use shell aliases or short shell scripts to achieve that goal, so that's not really a show stopper. > Distributions have packaged renamed binaries. qemu-kvm has been used > quite widely. Yes, and QEMU already checks for that naming in configure_accelerators() ... so that's rather another indicator that we can go with configuration via argv[0] :-) > In neither of these cases, relaxing the pattern helps. > > The least bad solution I can see so far is a new option -target. > > Instead of turning the target-specific binaries into links to / copies > of the single binary, they become wrappers that pass -target as the > first option. We need to make sure this option is honored in time then, > which should be easy enough. > > If you invoke the single binary directly, you need to pass -target > yourself. If you don't to pass it, or pass it late in the command line, > you open up a window for interaction with indeterminate target. > Target-specific interfaces could exhibit different behavior then, even > fail. That's fine under "additional interfaces are not a problem". > > Thoughts? Shell script wrappers always have the problem that they break the direct usage of debuggers like "valgrind" or "gdb" with the target binary, so that's also not the best solution. I'd go with Pierrick's idea to try to determine the target via argv[0]. And for people who really want to rename their binary in a way that makes it impossible to determine the target automatically, just provide the "-target" option as fallback solution, too. Thomas
On 29/4/25 10:23, Markus Armbruster wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> On 4/28/25 4:07 AM, Markus Armbruster wrote: >>> Peter Krempa <pkrempa@redhat.com> writes: >>> >>>> So what should libvirt do once multiple targets are supported? >>>> >>>> How do we query CPUs for each of the supported targets? >>>> >> >> It's kind of a similar question we have to solve now with QEMU code. >> What happens when a symbol is duplicated, and available only for several >> targets? >> >> In this case, we found various approaches to solve this: >> - unify this symbol for all targets (single implementation) >> - unify all targets to provide this symbol (multiple impl, all targets) >> - rename symbols adding {arch} suffix, so it's disambiguated by name >> - create a proper interface which an available function (multiple impl, >> selective targets) >> >> In the case of query-cpu-definitions, my intuition is that we want to >> have a single implementation, and that we return *all* the cpus, merging >> all architectures. In the end, we (and libvirt also) should think out of >> the "target" box. It's an implementation detail, based on the fact QEMU >> had 'targets' associated to various binaries for a long time and not a >> concept that should leak into all consumers. >> >>>> Will the result be the same if we query them one at a time or all at >>>> once? >>> >>> Pierrick's stated goal is to have no noticable differences between the >>> single binary and the qemu-system-<target> it covers. This is obviously >>> impossible if we can interact with the single binary before the target >>> is fixed. >>> >> >> Right. >> At this point, we can guarantee the target will be fixed before anything >> else, at the start of main(). It's obviously an implementation choice, >> but to be honest, I don't see what we would gain from having a "null" >> default QEMU target, unable to emulate anything. >> >>>>> This requires fixing the target before introspection. Unless this is >>>>> somehow completely transparent (wrapper scripts, or awful hacks based on >>>>> the binary's filename, perhaps), management applications may have to be >>>>> adjusted to actually do that. >>>> >>>> As noted filename will not work. Users can specify any filename and >>>> create override scripts or rename the binary. >>> >>> True. >>> >> >> I would prefer to not open this pandora box on this thread, but don't >> worry, the best will be done to support all those cases, including >> renaming the binary, allowing any prefix, suffix, as long as name stays >> unambiguous. If you rename it to qemu-ok, how can you expect anything? >> >> We can provide the possibility to have a "default" target set at compile >> time, for distributors creating their own specific QEMU binaries. But in >> the context of classical software distribution, it doesn't make any sense. > > I don't wish to derail this thread, but we've been dancing around the > question of how to best fix the target for some time. I think we should > talk about it for real. > > Mind, this is not an objection to your larger "single binary" idea. It > could be only if it was an intractable problem, but I don't think it is. > > You want the single binary you're trying to create to be a drop-in > replacement for per-target binaries. > > "Drop-in replacement" means existing usage continues to work. > Additional interfaces are not a problem. > > To achieve "drop-in replacement", the target needs to be fixed > automatically, and before the management application can further > interact with it. > > If I understand you correctly, you're proposing to use argv[0] for that, > roughly like this: assume it's qemu-system-<target>, extract <target> > first thing in main(), done. > > What if it's not named that way? If I understand you correctly, you're > proposing to fall back to a compiled-in default target. > > I don't think this is going to fly. Rather than using non-constant argv[0] Pierrick suggested to add a single CLI option '-target' which selects the corresponding TargetInfo structure to use at runtime. I.e. for ARM: https://lore.kernel.org/qemu-devel/20250424222112.36194-12-philmd@linaro.org/ For distros qemu-system-arm could be a shell script prepending '-target arm' while passing the arguments calling qemu-system. If a distro wants to name a binary 'qemu-kvm' it can drop the -target option and hard-wire its target_info() to a distro-specific TargetInfo implementation, or &target_info_x86_64_system. > Developers rename the binary all the time, and expect this not to change > behavior. For instance, I routinely rename qemu-FOO to qemu-FOO.old or > qemu-FOO.COMMIT-HASH to let me compare behavior easily. > > We could relax the assumption to support such renames. Developers then > need to be aware of what renames are supported. Meh. > > The more we relax the pattern, the likelier surprising behavior becomes. > > We could mitigate surprises by eliminating the built-in default target. > > Users invoke their binaries with their own names, too. If Joe R. User > finds qemu-system-<joe's-fav-target> too much to type, and creates a > symlink named q to it, more power to him! > > Distributions have packaged renamed binaries. qemu-kvm has been used > quite widely. > > In neither of these cases, relaxing the pattern helps. > > The least bad solution I can see so far is a new option -target. Ah! Same same. > Instead of turning the target-specific binaries into links to / copies > of the single binary, they become wrappers that pass -target as the > first option. We need to make sure this option is honored in time then, > which should be easy enough. > > If you invoke the single binary directly, you need to pass -target > yourself. If you don't to pass it, or pass it late in the command line, > you open up a window for interaction with indeterminate target. > Target-specific interfaces could exhibit different behavior then, even > fail. That's fine under "additional interfaces are not a problem". > > Thoughts? >
On 29/4/25 11:32, Daniel P. Berrangé wrote: > On Tue, Apr 29, 2025 at 11:20:59AM +0200, Thomas Huth via Devel wrote: >> On 29/04/2025 10.23, Markus Armbruster wrote: >> ... >>> I don't wish to derail this thread, but we've been dancing around the >>> question of how to best fix the target for some time. I think we should >>> talk about it for real. >>> >>> Mind, this is not an objection to your larger "single binary" idea. It >>> could be only if it was an intractable problem, but I don't think it is. >>> >>> You want the single binary you're trying to create to be a drop-in >>> replacement for per-target binaries. >>> >>> "Drop-in replacement" means existing usage continues to work. >>> Additional interfaces are not a problem. >>> >>> To achieve "drop-in replacement", the target needs to be fixed >>> automatically, and before the management application can further >>> interact with it. >>> >>> If I understand you correctly, you're proposing to use argv[0] for that, >>> roughly like this: assume it's qemu-system-<target>, extract <target> >>> first thing in main(), done. >>> >>> What if it's not named that way? If I understand you correctly, you're >>> proposing to fall back to a compiled-in default target. >>> >>> I don't think this is going to fly. >> >> I tend to disagree. For normal users that consume QEMU via the distros, the >> check via argv[0] should be good enough. For developers, I think we can >> assume that they are adaptive enough to use an additional "-target" option >> in case they mis-named their binary in a bad way. > >>> Developers rename the binary all the time, and expect this not to change >>> behavior. For instance, I routinely rename qemu-FOO to qemu-FOO.old or >>> qemu-FOO.COMMIT-HASH to let me compare behavior easily. >> >> Developers should already be aware that this can cause trouble, since e.g. >> the qtests are deriving the target architecture from the binary name >> already. See the qtest_get_arch() function. > > Even if we want to allow developers to rename binaries, we don't have to > allow an arbitrary choice of naming. We could define an accepted pattern > for naming that people must follow. > > eg we could allow for clearly distinguished suffixes (ie append '-SUFFIX') > so that > > qemu-system-x86_64-fishfood > > is acceptable while > > qemu-system-fishfood > qemu-system-x86_64fishfood > > would be an unsupported scenarios. It is already a pain to parse mips[n32,64][el], microblaze[el], sparc[32plus,64], ppc[64][le], sh4[eb], xtensa[eb], arm[eb]...
On Tue, 29 Apr 2025, Markus Armbruster wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> On 4/28/25 4:07 AM, Markus Armbruster wrote: >>> Peter Krempa <pkrempa@redhat.com> writes: >>> >>>> So what should libvirt do once multiple targets are supported? >>>> >>>> How do we query CPUs for each of the supported targets? >>>> >> >> It's kind of a similar question we have to solve now with QEMU code. >> What happens when a symbol is duplicated, and available only for several >> targets? >> >> In this case, we found various approaches to solve this: >> - unify this symbol for all targets (single implementation) >> - unify all targets to provide this symbol (multiple impl, all targets) >> - rename symbols adding {arch} suffix, so it's disambiguated by name >> - create a proper interface which an available function (multiple impl, >> selective targets) >> >> In the case of query-cpu-definitions, my intuition is that we want to >> have a single implementation, and that we return *all* the cpus, merging >> all architectures. In the end, we (and libvirt also) should think out of >> the "target" box. It's an implementation detail, based on the fact QEMU >> had 'targets' associated to various binaries for a long time and not a >> concept that should leak into all consumers. >> >>>> Will the result be the same if we query them one at a time or all at >>>> once? >>> >>> Pierrick's stated goal is to have no noticable differences between the >>> single binary and the qemu-system-<target> it covers. This is obviously >>> impossible if we can interact with the single binary before the target >>> is fixed. >>> >> >> Right. >> At this point, we can guarantee the target will be fixed before anything >> else, at the start of main(). It's obviously an implementation choice, >> but to be honest, I don't see what we would gain from having a "null" >> default QEMU target, unable to emulate anything. >> >>>>> This requires fixing the target before introspection. Unless this is >>>>> somehow completely transparent (wrapper scripts, or awful hacks based on >>>>> the binary's filename, perhaps), management applications may have to be >>>>> adjusted to actually do that. >>>> >>>> As noted filename will not work. Users can specify any filename and >>>> create override scripts or rename the binary. >>> >>> True. >>> >> >> I would prefer to not open this pandora box on this thread, but don't >> worry, the best will be done to support all those cases, including >> renaming the binary, allowing any prefix, suffix, as long as name stays >> unambiguous. If you rename it to qemu-ok, how can you expect anything? >> >> We can provide the possibility to have a "default" target set at compile >> time, for distributors creating their own specific QEMU binaries. But in >> the context of classical software distribution, it doesn't make any sense. > > I don't wish to derail this thread, but we've been dancing around the > question of how to best fix the target for some time. I think we should > talk about it for real. > > Mind, this is not an objection to your larger "single binary" idea. It > could be only if it was an intractable problem, but I don't think it is. > > You want the single binary you're trying to create to be a drop-in > replacement for per-target binaries. > > "Drop-in replacement" means existing usage continues to work. > Additional interfaces are not a problem. > > To achieve "drop-in replacement", the target needs to be fixed > automatically, and before the management application can further > interact with it. > > If I understand you correctly, you're proposing to use argv[0] for that, > roughly like this: assume it's qemu-system-<target>, extract <target> > first thing in main(), done. > > What if it's not named that way? If I understand you correctly, you're > proposing to fall back to a compiled-in default target. > > I don't think this is going to fly. > > Developers rename the binary all the time, and expect this not to change > behavior. For instance, I routinely rename qemu-FOO to qemu-FOO.old or > qemu-FOO.COMMIT-HASH to let me compare behavior easily. These would be handled by doing only a prefix match with strncmp instead of strcmp on argv[0]. > We could relax the assumption to support such renames. Developers then > need to be aware of what renames are supported. Meh. > > The more we relax the pattern, the likelier surprising behavior becomes. > > We could mitigate surprises by eliminating the built-in default target. > > Users invoke their binaries with their own names, too. If Joe R. User > finds qemu-system-<joe's-fav-target> too much to type, and creates a > symlink named q to it, more power to him! > > Distributions have packaged renamed binaries. qemu-kvm has been used > quite widely. > > In neither of these cases, relaxing the pattern helps. But completely renaming is not solved even by prefix match. > The least bad solution I can see so far is a new option -target. > > Instead of turning the target-specific binaries into links to / copies > of the single binary, they become wrappers that pass -target as the > first option. We need to make sure this option is honored in time then, > which should be easy enough. I proposed before that since target (or arch because target is used in different senses so may be confusing) is usually tied to the board it could be made part of the board name. Such as ppc:g3beige or x86:pc. Then you can search the board list and find a match for the -machine option and find the arch from that. There are only a few machines that are problematic that behave differently based on which binary they are in and use different default -cpu type based on that, such as mac99 and maybe pc and q35 (I don't know if these x86 machines use same default cpu in qemu-system-i386 and qemu-system-x86_64) but those could be solved by deprecating this behaviour and adding different machines for each variant then by the time we have a single binary they would fit in this scheme. One question is what arch to use for heterogeneous machines using multiple archs. Those still may have one that's considered a main arch so could be grouped there or we may use multi:name format for those that cannot have a main architecture. Or we can list them with multiple names, one for each arch so any of them could be used and on start to select the machine and the binary can check that all of the listed archs are compiled in. Specifying the arch for -machine would also be optional anyway unless we have two machines with same name but different arch which is rare. Regards, BALATON Zoltan > If you invoke the single binary directly, you need to pass -target > yourself. If you don't to pass it, or pass it late in the command line, > you open up a window for interaction with indeterminate target. > Target-specific interfaces could exhibit different behavior then, even > fail. That's fine under "additional interfaces are not a problem". > > Thoughts? > > >
On 4/29/25 12:43 AM, Markus Armbruster wrote: > Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: > >> On 4/25/25 11:21 PM, Markus Armbruster wrote: >>> Trouble is some uses of the second kind are in QAPI conditionals. I can >>> see three options: >>> >>> (1) Drop these conditionals. >>> >>> (2) Replace them by run-time checks. >>> >>> (3) Have target-specific QAPI-generated code for multiple targets >>> coexist in the single binary. >>> >>> As far as I can tell, your RFC series is an incomplete attempt at (2). >>> >>> I gather you considered (3), but you dislike it for its bloat and >>> possibly other reasons. I sympathize; the QAPI-generated code is plenty >>> bloated as it is, in good part to early design decisions (not mine). >>> >>> Your "no noticeable differences" goal precludes (1). >>> >>> Back to (2). In C, replacing compile-time conditionals by run-time >>> checks means replacing #if FOO by if (foo). Such a transformation isn't >>> possible in the QAPI schema. To make it possible, we need to evolve the >>> QAPI schema language. >>> >>> docs/devel/qapi-code-gen.rst describes what we have: >>> >>> COND = STRING >>> | { 'all: [ COND, ... ] } >>> | { 'any: [ COND, ... ] } >>> | { 'not': COND } >>> >>> [....] >>> >>> The C code generated for the definition will then be guarded by an #if >>> preprocessing directive with an operand generated from that condition: >>> >>> * STRING will generate defined(STRING) >>> * { 'all': [COND, ...] } will generate (COND && ...) >>> * { 'any': [COND, ...] } will generate (COND || ...) >>> * { 'not': COND } will generate !COND >>> >>> So, conditions are expression trees where the leaves are preprocessor >>> symbols and the inner nodes are operators. >>> >>> It's not quite obvious to me how to best evolve this to support run-time >>> checks. >>> >> >> After looking at the introspection code, I don't see any major blocker. >> We need to keep some of existing "if", as they are based on config-host, >> and should apply. >> We can introduce a new "available_if" (or any other name), which >> generates a runtime check when building the schema, or when serializing >> a struct. >> >> This way, by modifying the .json with: >> - if: 'TARGET_I386' >> + available_if: 'target_i386()' >> >> This way, we keep the possibility to have ifdef, and we can expose at >> runtime based on available_if. So we can keep the exact same schema we >> have today per target. > > The name is ugly. Naming is hard. No need to worry about it right now. > Sure, when I'll work on a v2, I'll use "whatever_if". Meanwhile, pick a name you like to describe a runtime vs compile time check, and I'll do the sed. > Semantics of having both 'if' and 'available_if'? To work out an > answer, let's consider how to convert conditionals: > > * 'if': STRING > > If STRING is a target-specific macro, replace by 'available_if': PRED, > where PRED is the equivalent run-time predicate. > > Else, no change. > > * 'if': { 'all': [COND, ...] } > > If COND contains only target-specific macros, replace by > 'available_if': { 'all': [PRED, ...] }, where the PRED are the > equivalent run-time predicates. > > If COND contains no target-specific macros, no change. > > What if it contains both? > > - If each COND contains either only target-specific macros, or no > target-specific macros, we could split the target-specific ones off > into an additional 'available_if'. This requires defining the > semantics of having both 'if' and 'available_if' as "both conditions > must be satisfied". > > - What if this isn't the case? > > * 'if' { 'any': [COND, ...] } > > Similar, but to be able to split the COND we need "either condition > must be satisfied". > I don't see any reason to block having both. You may want to condition it by a define derived from config-host.h, and add a runtime check as well. Both "checks" won't apply in the same locations. We can add any restriction on having both at the same time, works for me also. No strong opinion here. > Even if we can make this work somehow, it would likely be a royal mess > to explain in qapi-code-gen.rst. > > We currently don't have "mixed" conditionals. So we could sidestep the > problem: you can have either 'if' or 'available_if', but not both. > Feels like a cop out to me. > > What if we move the "is dynamic" bit from the root of the conditional to > its leaves? So far, the leaves are macro names. What if we > additionally permit a function name? > > Function name, not C expression, to not complicate generating code in > languages other than C too much. > I don't think we should think too much ahead for languages other than C, for one, two, and even three reasons :) - First, it's already broken because we rely on ifdef that won't be there in Rust or Go. - Second, it's code, we can just change it later if needed. - Third, those json are consumed only by QEMU (right?), so we are free to write/modify them as we want. The only thing that must stay the same is what we expose to the consumer in the schema, and which commands we expose in qemu. > Ignore the question of syntax for now, i.e. how to decide whether a leaf > is a macro or a function name. > >>> Whatever we choose should support generating Rust and Go as well. Why? >>> Rust usage in QEMU is growing, and we'll likely need to generate some >>> Rust from the QAPI schema. Victor Toso has been working on Go bindings >>> for use in Go QMP client software. >>> >> >> I don't see any blocker with that. If you mention generating Rust and Go >> from qapi json definitions, it's already dependent on C preprocessor >> because of ifdef constant. So it will have to be adapted anyway. >> Having the same function (target_i386()) name through different >> languages is not something hard to achieve. > > I can't see concrete blockers at this time. I just wanted to make you > aware of the emerging need to support other languages. > > [...] > >>> QAPI was designed to be compile-time static. Revising such fundamental >>> design assumptions is always fraught. I can't give you a confident >>> assessment now. All I can offer you is my willingness to explore >>> solutions. See "really fancy" below. >>> >>> Fun fact: we used to generate the value of query-qmp-schema as a single >>> string. We switched to the current, more bloated representation to >>> support conditionals (commit 7d0f982bfbb). >>> >> >> It's nice to have this, and this is what would allow us to >> conditionnally include or not various definitions/commands/fields. I was >> a bit worried we would have a "static string", but was glad to find a >> static list instead. > > I'm mildly unhappy about the bloat, but not enough to fix it. > > [...] > >>> The only path to true "no observable differences" I can see is to fix >>> the target before the management application interacts with QEMU in any >>> way. This would make QMP commands (query-cpu-definitions, >>> query-qmp-schema, ...) impossible to send before the target is fixed. >>> >> >> The current target will be set at the entry of main() in QEMU, so before >> the monitor is created. Thus, it will be unambiguous. > > I reiterate my dislike for having behavior depend on argv[0]. We'll > figure out something. > I'll answer on the dedicated subthread. > [...] >
On 4/29/25 1:37 AM, Daniel P. Berrangé wrote: > On Tue, Apr 29, 2025 at 09:43:24AM +0200, Markus Armbruster wrote: >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> >>> After looking at the introspection code, I don't see any major blocker. >>> We need to keep some of existing "if", as they are based on config-host, >>> and should apply. >>> We can introduce a new "available_if" (or any other name), which >>> generates a runtime check when building the schema, or when serializing >>> a struct. >>> >>> This way, by modifying the .json with: >>> - if: 'TARGET_I386' >>> + available_if: 'target_i386()' >>> >>> This way, we keep the possibility to have ifdef, and we can expose at >>> runtime based on available_if. So we can keep the exact same schema we >>> have today per target. >> >> The name is ugly. Naming is hard. No need to worry about it right now. >> >> Semantics of having both 'if' and 'available_if'? To work out an >> answer, let's consider how to convert conditionals: >> >> * 'if': STRING >> >> If STRING is a target-specific macro, replace by 'available_if': PRED, >> where PRED is the equivalent run-time predicate. >> >> Else, no change. >> >> * 'if': { 'all': [COND, ...] } >> >> If COND contains only target-specific macros, replace by >> 'available_if': { 'all': [PRED, ...] }, where the PRED are the >> equivalent run-time predicates. >> >> If COND contains no target-specific macros, no change. >> >> What if it contains both? >> >> - If each COND contains either only target-specific macros, or no >> target-specific macros, we could split the target-specific ones off >> into an additional 'available_if'. This requires defining the >> semantics of having both 'if' and 'available_if' as "both conditions >> must be satisfied". >> >> - What if this isn't the case? >> >> * 'if' { 'any': [COND, ...] } >> >> Similar, but to be able to split the COND we need "either condition >> must be satisfied." >> >> Even if we can make this work somehow, it would likely be a royal mess >> to explain in qapi-code-gen.rst. >> >> We currently don't have "mixed" conditionals. So we could sidestep the >> problem: you can have either 'if' or 'available_if', but not both. >> Feels like a cop out to me. >> >> What if we move the "is dynamic" bit from the root of the conditional to >> its leaves? So far, the leaves are macro names. What if we >> additionally permit a function name? >> >> Function name, not C expression, to not complicate generating code in >> languages other than C too much. >> >> Ignore the question of syntax for now, i.e. how to decide whether a leaf >> is a macro or a function name. > > I wonder if any of this is worth the pain in practice..... > > > Looking at the QAPI schema, we apply TARGET_xxxx conditions either to > commands, or to structs/enums that are used in args/return of commands. > We don't conditionalize individual fields, etc. > CpuModelExpansionInfo.data.deprecated_props is a conditional field, and seems to be the only example. Anyway, in terms of code generated, it's not a very big difference between conditioning a complete struct or one of its field. > I tried to query our schema with 'jq' (incidentally rather tedious > because of our JSON-but-not-JSON language[1]). If I select only > commands we get: > > query-cpu-definitions => currently many arches > query-cpu-model-expansion => currently many arches > query-cpu-model-baseline => currently s390x only > query-cpu-model-comparison => currently s390x only > query-s390x-cpu-polarization => inherently s390x only > query-gic-capabilities => inherently arm only > query-sev => inherently x86 only > query-sev-attestation-report => inherently x86 only > query-sev-capabilities => inherently x86 only > query-sev-launch-measure => inherently x86 only > query-sgx => inherently x86 only > query-sgx-capabilities => inherently x86 only > rtc-reset-reinjection => inherently x86 only > set-cpu-topology => inherently s390x only > sev-inject-launch-secret => inherently x86 only > xen-event-inject => currently x86 only > xen-event-list => currently x86 only > > The two Xen commands are currently limited to x86, but if we ever extended > Xen to arm, possibly they would make sense. IOW, conceptually a target > conditional might be useful in future. > > The CPU model commands are the ones where having the target conditions > visible in schema appears to add value, in that they'll allow a mgmt > app to detect when we extend any of them to cover new architectures. > > > Libvirt (and other mgmt apps) want to query the schema to see if commands > exist in the QEMU they're using, before trying to invoke them. To some > degree this is just a "nice to have" that improves error reporting/detection. > > > For the commands that are inherently arch specific, the mgmt app should > conceptually already know what architectures these commands apply to. > These target conditionals provide little (no) value when probing commands > in the schema. > > IOW, if we (for example) have a single binary for x86 and s390, it should > be harmless if we report that 'query-sev' exists regardless of arch, as > the mgmt app should none the less already know to only use it with x86. > > I don't know if libvirt correctly filters based on architecture in the > case of SEV/SGX/GIC/RTC when probing & using these features, but if it > does not, then I'd consider that a pre-existing bug that should be fixed. > > Libvirt doesn't use the Xen commands. > > For query-cpu-model-comparison/baseline, libvirt already filters its > usage of these based on s390 arch, so even if x86 reported them I > believe it won't break libvirt today. If these commands are extended > to other archs in future, libvirt might want a way to detect this. > On the flipside it might not be the end of the world if we just expose > them on all arches and simply have them return an error at runtime > where non-applicable. > > > IOW, while the target conditions could theoretically provide value at > some point in future, it does not look like they do anything /today/ > for libvirt. > > Given that I wonder if we shouldn't just ignore the problem, and > blindly remove all TARGET_nnn conditions from QAPI schema today. Let > our future selves worry about it should this prove insufficient later. > If all the QAPI devs and maintainers are happy with it, I'm fine with it also. That said, CONFIG_KVM will still be an issue as well, because it's a target specific define, so please make sure it can be removed safely also. I can offer to make a prototype adding runtime checks to preserve the same schema, list of commands registered, and identical marshaling functions. If you decide to just drop all those conditional, it's even better, and faster, I'm good with it. My only goal is to compile QAPI generated code only once, no more, no less. > > With regards, > Daniel > > [1] To use QAPI JSON with 'jq' you must convert ' to " and > strip comments. eg > > cat *.json | sed -e "s/'/\"/g" -e 's/#.*//' | jq ...expression...
On 4/29/25 2:20 AM, Thomas Huth wrote: > On 29/04/2025 10.23, Markus Armbruster wrote: > ... >> I don't wish to derail this thread, but we've been dancing around the >> question of how to best fix the target for some time. I think we should >> talk about it for real. >> Sure, and no problem about that, that's welcome! I'm not eluding the conversation, but I just feel that as long as we can't really build this single-binary and share this, it's impossible for people to try it and make a judgment based on rationale facts about it. Opinions and taste matter, but it's even better when we all have the same thing and can try solutions and post facts instead. >> Mind, this is not an objection to your larger "single binary" idea. It >> could be only if it was an intractable problem, but I don't think it is. >> >> You want the single binary you're trying to create to be a drop-in >> replacement for per-target binaries. >> >> "Drop-in replacement" means existing usage continues to work. >> Additional interfaces are not a problem. >> >> To achieve "drop-in replacement", the target needs to be fixed >> automatically, and before the management application can further >> interact with it. >> >> If I understand you correctly, you're proposing to use argv[0] for that, >> roughly like this: assume it's qemu-system-<target>, extract <target> >> first thing in main(), done. >> >> What if it's not named that way? If I understand you correctly, you're >> proposing to fall back to a compiled-in default target. >> >> I don't think this is going to fly. > > I tend to disagree. For normal users that consume QEMU via the distros, the > check via argv[0] should be good enough. For developers, I think we can > assume that they are adaptive enough to use an additional "-target" option > in case they mis-named their binary in a bad way. > >> Developers rename the binary all the time, and expect this not to change >> behavior. For instance, I routinely rename qemu-FOO to qemu-FOO.old or >> qemu-FOO.COMMIT-HASH to let me compare behavior easily. > > Developers should already be aware that this can cause trouble, since e.g. > the qtests are deriving the target architecture from the binary name > already. See the qtest_get_arch() function. > >> We could relax the assumption to support such renames. Developers then >> need to be aware of what renames are supported. Meh. >> >> The more we relax the pattern, the likelier surprising behavior becomes. >> >> We could mitigate surprises by eliminating the built-in default target. > > Just print out a proper error message if the target cannot be derived from > argv[0], pointing the users to use "-target", and we should be fine. > > And if someone renames their "qemu-sytem-aarch64" symlink to > "qemu-system-x86_64" and still expect to run aarch64 images that way, that's > just plain stupidity. > >> Users invoke their binaries with their own names, too. If Joe R. User >> finds qemu-system-<joe's-fav-target> too much to type, and creates a >> symlink named q to it, more power to him! > > They can also either use shell aliases or short shell scripts to achieve > that goal, so that's not really a show stopper. > >> Distributions have packaged renamed binaries. qemu-kvm has been used >> quite widely. > > Yes, and QEMU already checks for that naming in configure_accelerators() ... > so that's rather another indicator that we can go with configuration via > argv[0] :-) > >> In neither of these cases, relaxing the pattern helps. >> >> The least bad solution I can see so far is a new option -target. >> >> Instead of turning the target-specific binaries into links to / copies >> of the single binary, they become wrappers that pass -target as the >> first option. We need to make sure this option is honored in time then, >> which should be easy enough. >> >> If you invoke the single binary directly, you need to pass -target >> yourself. If you don't to pass it, or pass it late in the command line, >> you open up a window for interaction with indeterminate target. >> Target-specific interfaces could exhibit different behavior then, even >> fail. That's fine under "additional interfaces are not a problem". >> >> Thoughts? > > Shell script wrappers always have the problem that they break the direct > usage of debuggers like "valgrind" or "gdb" with the target binary, so > that's also not the best solution. > > I'd go with Pierrick's idea to try to determine the target via argv[0]. And > for people who really want to rename their binary in a way that makes it > impossible to determine the target automatically, just provide the "-target" > option as fallback solution, too. > > Thomas > I agree 100% with what Thomas said. Regarding the "Rename the binary and things should work", I was thinking of something like: guess_target(argv) { binary=argv[0]; if (binary.contains("qemu-system-arm")) { set_target(&target_info_system_arm); } else if (binary.contains("qemu-system-aarch64") { set_target(&target_info_system_aarch64); } unreachable("can't guess target from " + binary); } In short, detect "*qemu-system-{arch}*", with arch being one of the available architecture we added to the single binary. In case someone wants to stress this piece of code by doing: $ mv qemu-system qemu-system-arm-or-qemu-system-aarch64 Well, we can take the first occurence and call it a day. To disambiguate a target, we can provide the -target command line option, and provide a QEMU_TARGET env var. With those three ways, I hope we can cover 99.999999% of the use cases. Anyway, we'll need to have a way to identify the target, and all the possible solutions will have shortcomings. The right question is "Which solution covers the widest percentage of use cases ?", and my opinion is that argv[0] + -target + QEMU_TARGET env is the best compromise for me at this point. I'm not keen to have a default target set, but it's a personal opinion based on fear of "implicit smart choice hurts", so I'll be happy to change my mind with a good argument for it. Regards, Pierrick
On 4/29/25 2:35 AM, Philippe Mathieu-Daudé wrote: > If a distro wants to name a binary 'qemu-kvm' it can drop the > -target option and hard-wire its target_info() to a distro-specific > TargetInfo implementation, or &target_info_x86_64_system. > Having updated my Debian stable to next stable (trixie) last week, I noticed that qemu-kvm was removed [1]. I don't know why, when or how, but it's just an example that things can change, and people can survive to it. [1] https://packages.debian.org/search?keywords=qemu-kvm For the concerned other distros, if at least one packager asks us to provide a "./configure --default-target", it will be an excellent reason and opportunity to do it. But before that, let's first build this single binary, let's see if it's useful, let's see how to use it, and eventually, let's see how to package this and cover corner cases.
On 4/29/25 12:57 PM, Pierrick Bouvier wrote: > On 4/29/25 2:35 AM, Philippe Mathieu-Daudé wrote: >> If a distro wants to name a binary 'qemu-kvm' it can drop the >> -target option and hard-wire its target_info() to a distro-specific >> TargetInfo implementation, or &target_info_x86_64_system. >> > > Having updated my Debian stable to next stable (trixie) last week, I > noticed that qemu-kvm was removed [1]. > > I don't know why, when or how, but it's just an example that things can > change, and people can survive to it. > > [1] https://packages.debian.org/search?keywords=qemu-kvm > > For the concerned other distros, if at least one packager asks us to > provide a "./configure --default-target", it will be an excellent reason > and opportunity to do it. > > But before that, let's first build this single binary, let's see if it's > useful, let's see how to use it, and eventually, let's see how to > package this and cover corner cases. Well, thinking about it twice, it's pretty easy to cover: qemu-kvm means select the target that match host and add -accel kvm to argv.
On 29/04/2025 21.48, Pierrick Bouvier wrote: ... > I'm not keen to have a default target set, but it's a personal opinion based > on fear of "implicit smart choice hurts", so I'll be happy to change my mind > with a good argument for it. No default target, please! We've seen this with the default machines - it looks convenient first, but only gives troubles in the long run. Preferred defaults can change in the course of time, but if you have baked in the logic in hundreds of scripts out there, it's hard to change it afterwards again. Thomas
On 4/29/25 10:40 PM, Thomas Huth wrote: > On 29/04/2025 21.48, Pierrick Bouvier wrote: > ... >> I'm not keen to have a default target set, but it's a personal opinion based >> on fear of "implicit smart choice hurts", so I'll be happy to change my mind >> with a good argument for it. > > No default target, please! We've seen this with the default machines - it > looks convenient first, but only gives troubles in the long run. Preferred > defaults can change in the course of time, but if you have baked in the > logic in hundreds of scripts out there, it's hard to change it afterwards again. > I'll be happy to not change my current mind as well :) > Thomas >
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Apr 29, 2025 at 09:43:24AM +0200, Markus Armbruster wrote: >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> >> > After looking at the introspection code, I don't see any major blocker. >> > We need to keep some of existing "if", as they are based on config-host, >> > and should apply. >> > We can introduce a new "available_if" (or any other name), which >> > generates a runtime check when building the schema, or when serializing >> > a struct. >> > >> > This way, by modifying the .json with: >> > - if: 'TARGET_I386' >> > + available_if: 'target_i386()' >> > >> > This way, we keep the possibility to have ifdef, and we can expose at >> > runtime based on available_if. So we can keep the exact same schema we >> > have today per target. >> >> The name is ugly. Naming is hard. No need to worry about it right now. >> >> Semantics of having both 'if' and 'available_if'? To work out an >> answer, let's consider how to convert conditionals: >> >> * 'if': STRING >> >> If STRING is a target-specific macro, replace by 'available_if': PRED, >> where PRED is the equivalent run-time predicate. >> >> Else, no change. >> >> * 'if': { 'all': [COND, ...] } >> >> If COND contains only target-specific macros, replace by >> 'available_if': { 'all': [PRED, ...] }, where the PRED are the >> equivalent run-time predicates. >> >> If COND contains no target-specific macros, no change. >> >> What if it contains both? >> >> - If each COND contains either only target-specific macros, or no >> target-specific macros, we could split the target-specific ones off >> into an additional 'available_if'. This requires defining the >> semantics of having both 'if' and 'available_if' as "both conditions >> must be satisfied". >> >> - What if this isn't the case? >> >> * 'if' { 'any': [COND, ...] } >> >> Similar, but to be able to split the COND we need "either condition >> must be satisfied." >> >> Even if we can make this work somehow, it would likely be a royal mess >> to explain in qapi-code-gen.rst. >> >> We currently don't have "mixed" conditionals. So we could sidestep the >> problem: you can have either 'if' or 'available_if', but not both. >> Feels like a cop out to me. >> >> What if we move the "is dynamic" bit from the root of the conditional to >> its leaves? So far, the leaves are macro names. What if we >> additionally permit a function name? >> >> Function name, not C expression, to not complicate generating code in >> languages other than C too much. >> >> Ignore the question of syntax for now, i.e. how to decide whether a leaf >> is a macro or a function name. > > I wonder if any of this is worth the pain in practice..... Fair! Pierrick's stated goal is "no noticable differences". We've explored what this means. We can also explore changes that would help us reach the goal more easily, and maybe even consider setting a less ambitious goal. > Looking at the QAPI schema, we apply TARGET_xxxx conditions either to > commands, or to structs/enums that are used in args/return of commands. > We don't conditionalize individual fields, etc. With one exception noted by Pierrick. > I tried to query our schema with 'jq' (incidentally rather tedious > because of our JSON-but-not-JSON language[1]). If I select only > commands we get: > > query-cpu-definitions => currently many arches > query-cpu-model-expansion => currently many arches > query-cpu-model-baseline => currently s390x only > query-cpu-model-comparison => currently s390x only > query-s390x-cpu-polarization => inherently s390x only > query-gic-capabilities => inherently arm only > query-sev => inherently x86 only > query-sev-attestation-report => inherently x86 only > query-sev-capabilities => inherently x86 only > query-sev-launch-measure => inherently x86 only > query-sgx => inherently x86 only > query-sgx-capabilities => inherently x86 only > rtc-reset-reinjection => inherently x86 only > set-cpu-topology => inherently s390x only > sev-inject-launch-secret => inherently x86 only > xen-event-inject => currently x86 only > xen-event-list => currently x86 only > > The two Xen commands are currently limited to x86, but if we ever extended > Xen to arm, possibly they would make sense. IOW, conceptually a target > conditional might be useful in future. > > The CPU model commands are the ones where having the target conditions > visible in schema appears to add value, in that they'll allow a mgmt > app to detect when we extend any of them to cover new architectures. > > > Libvirt (and other mgmt apps) want to query the schema to see if commands > exist in the QEMU they're using, before trying to invoke them. To some > degree this is just a "nice to have" that improves error reporting/detection. Schema introspection is more robust, and often simpler, especially for more complicated questions. > For the commands that are inherently arch specific, the mgmt app should > conceptually already know what architectures these commands apply to. > These target conditionals provide little (no) value when probing commands > in the schema. Fair. Management applications are quite unlikely to ask "does this target support SEV?" If they know anything about SEV, they know it's x86. They may still want to ask "does this QEMU provide SEV stuff?" This remains possible even with the conditionals deleted. Nice to have: "inherently specific to <target>" is blatantly obvious. Target conditionals do that only approximately: they don't tell whether the condition is inherent or just a "currently". If we drop the conditions, we should consider other means, such as clearer doc comments, or reorganizing the section structure. > IOW, if we (for example) have a single binary for x86 and s390, it should > be harmless if we report that 'query-sev' exists regardless of arch, as > the mgmt app should none the less already know to only use it with x86. > > I don't know if libvirt correctly filters based on architecture in the > case of SEV/SGX/GIC/RTC when probing & using these features, but if it > does not, then I'd consider that a pre-existing bug that should be fixed. While we QEMU developers gratefully appreciate an "I don't know whether our management application does <unadvisable-thing>, but if you break it, we'll fix our management application" attitude, we nevertheless try quite hard not to break management applications :) > Libvirt doesn't use the Xen commands. > > For query-cpu-model-comparison/baseline, libvirt already filters its > usage of these based on s390 arch, so even if x86 reported them I > believe it won't break libvirt today. If these commands are extended > to other archs in future, libvirt might want a way to detect this. > On the flipside it might not be the end of the world if we just expose > them on all arches and simply have them return an error at runtime > where non-applicable. query-cpu-definitions can't currently, fail, but the other query-cpu-FOO all have multiple failure modes, which can complicate things if you need to distinguish "failed because non-applicable / not implemented" from other failures. Avoiding such complications is largely why introspection exists. The ideal solution is to implement the query-cpu-FOO for all targets. > IOW, while the target conditions could theoretically provide value at > some point in future, it does not look like they do anything /today/ > for libvirt. > > Given that I wonder if we shouldn't just ignore the problem, and > blindly remove all TARGET_nnn conditions from QAPI schema today. Let > our future selves worry about it should this prove insufficient later. Again, exploring this makes sense. > With regards, > Daniel > > [1] To use QAPI JSON with 'jq' you must convert ' to " and > strip comments. eg > > cat *.json | sed -e "s/'/\"/g" -e 's/#.*//' | jq ...expression...
On 5/7/25 12:55 AM, Markus Armbruster wrote: [...] >> - First, it's already broken because we rely on ifdef that won't be >> there in Rust or Go. > > I don't think it's broken. QAPI 'if' translates straightforwardly to C > #if, but that doesn't mean it cannot be translated to conditional > compilation / metaprogramming in other languages. > > In fact, the value of 'if' used to be C constant expressions suitable > for use with #if, and we changed it to its current form specifically to > enable Rust work, in merge commit c83fcfaf8a5. Marc-André's was trying > to develop Rust bindings back then, and if I remember correctly this > change was enough to let him implement 'if' with Rust. > Sure, I didn't mean "this approach is doomed", simply that it needs changes, the same way code doing a runtime check would need changes as well. >> - Second, it's code, we can just change it later if needed. > > True! > >> - Third, those json are consumed only by QEMU (right?), so we are free >> to write/modify them as we want. > > Also true. > >> The only thing that must stay the same is what we expose to the consumer >> in the schema, and which commands we expose in qemu. > > We may evolve the external interface as long as we honor our > compatibility promise. > > You're aiming for "no change at all" there. I understand why that's > desirable. But if it should turn out that a bit of compatible change > simplifies the job, we can take the simpler route. > I have a local prototype doing what was described: introduce a runtime_if, additional to if (no worries, the name can be changed later), exposing schema parts conditionnally, and same for visiting types and registering commands. It's not too ugly, and easy to combine compilation if and runtime if together. I should be able to post it next week. > [...] > Regards, Pierrick.
On 5/7/25 4:32 AM, Daniel P. Berrangé wrote: > On Wed, May 07, 2025 at 09:55:13AM +0200, Markus Armbruster wrote: >> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes: >> >> [...] >> >>> I don't think we should think too much ahead for languages other than C, >>> for one, two, and even three reasons :) >> >> I agree that thinking ahead too much is a bad habit. So is thinking >> ahead too little :) >> >>> - First, it's already broken because we rely on ifdef that won't be >>> there in Rust or Go. >> >> I don't think it's broken. QAPI 'if' translates straightforwardly to C >> #if, but that doesn't mean it cannot be translated to conditional >> compilation / metaprogramming in other languages. >> >> In fact, the value of 'if' used to be C constant expressions suitable >> for use with #if, and we changed it to its current form specifically to >> enable Rust work, in merge commit c83fcfaf8a5. Marc-André's was trying >> to develop Rust bindings back then, and if I remember correctly this >> change was enough to let him implement 'if' with Rust. > > The usefulness of the conditions to non-C languages may well > vary depended on intended use case. > > If I'm writing a mgmt app that can talk to QEMU, and I want > to be able to talk to both x86 and s390 system emulators, > I am unlikely to want the language bindings to omit features > based on TARGET_xxx conditions. I won't want a separate API > for each QEMU arch target, I'll want one API for all arches. > > Expanding this further, if I'm writing a mgmt app to talk > to QEMU 9.2.0, I am also unlikely to want the language > bindings to omit features based on CONFIG_xxxx conditions, > because I likely want to talk to a QEMU 9.2.0 built by any > OS distro, each of which may have chosen a different set > of --enable flags for configure, and thus having different > CONFIG_xxx conditions. > > TL;DR: in terms of code generation, the prime use case for > conditionally generated code is for QEMU's own internal usage. > For QMP bindings generated for 3rd party app usage, the use > of conditions in generation feels like a niche requirment at > best. > > IMHO the code for any public facing API derived from QMP > schema should be invariant for any given QEMU release. > I respect both opinions: expose the same interface, or relax what we expose, and adapt the consumers. A long term solution could be the latter, but we need to be able to remove target dependencies in QAPI without spending N months on this topic, so I favor the former, keeping things stable. It will not prevent anyone to do further cleanups, nor make it more difficult anyway. > With regards, > Daniel Regards, Pierrick
v1 was posted: https://lore.kernel.org/qemu-devel/20250507231442.879619-1-pierrick.bouvier@linaro.org/ Regards, Pierrick
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