Daniel P. Berrangé <berra...@redhat.com> writes:
> 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.....
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...
