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