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

On Fri, Apr 25, 2025 at 17:38:44 +0200, Markus Armbruster via Devel wrote: > Pierrick Bouvier <pierrick.bouvier(a)linaro.org&gt; writes: [...] > 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. > > 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

I've had a look at what bits of the QMP schema are depending on the above defines which libvirt uses. In many cases libvirt could restrict the use of given command/property to only supported architectures. We decided to simply probe the presence of the command because it's convenient to not have to filter them any more - query-gic-capabilities - libvirt already calls this only for ARM guests based on the definition - query-sev and friends - libvirt uses presence of 'query-sev' to decide if the binary supports it; patching in a platofrm check is possible although inconvenient - query-sgx and friends - similar to sev -query-cpu-definitions and friends - see below

> > What we try to do here is to build them only once > instead. > > You're trying to eliminate target-specific QAPI conditionals. Correct? > > > 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. Indeed and libvirt already uses this in few cases as noded above. > > 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. > > 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." > > 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. I wonder how this will work if libvirt is probing a binary. Libvirt does not look at the filename. It can't because it can be a user-specified/compiled binary, override script, or a distro that chose to rename the binary. The second thing that libvirt does after 'query-version' is 'query-target'. So what should libvirt do once multiple targets are supported? How do we query CPUs for each of the supported targets? Will the result be the same if we query them one at a time or all at once?

> 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.

> 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. Ah I see you had a similar question :D > > Of course, "fixing the target" stops making sense once we move to > heterogeneous machines with multiple 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