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.
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.
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...
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
