Re: [RFC PATCH 0/3] single-binary: make QAPI generated files common

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

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