On 5/8/25 7:40 AM, Daniel P. Berrangé wrote:
On Wed, May 07, 2025 at 04:14:39PM -0700, Pierrick Bouvier wrote:
Signed-off-by: Pierrick Bouvier <pierrick.bouv...@linaro.org>
---
qapi/machine-target.json | 84 ++++++++++++++++++++++++----------------
qapi/misc-target.json | 48 ++++++++++++-----------
scripts/qapi/expr.py | 9 +++--
3 files changed, 81 insertions(+), 60 deletions(-)
diff --git a/qapi/machine-target.json b/qapi/machine-target.json
index 541f93eeb78..6174b7291ca 100644
--- a/qapi/machine-target.json
+++ b/qapi/machine-target.json
@@ -96,7 +96,7 @@
##
{ 'struct': 'CpuModelBaselineInfo',
'data': { 'model': 'CpuModelInfo' },
- 'if': 'TARGET_S390X' }
+ 'runtime_if': 'target_s390x()' }
The existing 'if' conditions are already slightly uncomfortable
for QAPI when considering alternate code generators, because the
definition of what "CONFIG_xxx" or "TARGET_xxx" condition means,
is essentially known only to our build system. While we expose
the conditions in the docs, the meaning of those conditions is
totally opaque to anyone reading the docs. Essentially our QAPI
schema ceased to be self-documenting/self-describing when we
introduced the 'if' conditions :-(
In retrospect, IMHO, for 'if' conditions we probably should have
created some kind of built-in QAPI concept of feature flag constants
with well defined & documented meaning.
eg hypothetically
##
# @target-s390x
#
# Whether this is an s390x emulator target
{ 'constant': 'target-s390x' }
##
# @accel-kvm
#
# Whether the KVM accelerator is built
{ 'constant': 'accel-kvm' }
Then our 'if' conditions would have only been permitted to
reference defined 'constant'.
{ 'struct': 'CpuModelCompareInfo',
'data': { 'result': 'CpuModelCompareResult',
'responsible-properties': ['str'] },
'if': 'target-s390x' }
The build system would need generate an input document for the
QAPI visitor that defines whether each constant is set to true
or false, based on suitable CONFIG/TARGET conditions from meson.
With this QAPI schemas would have remained fully self-contained.
Anyway, this is a long way of saying that having 'runtime_if'
conditions directly referencing the names of internal C
functions makes me even more uncomfortable than I already am
with the 'if' conditions.
I understand the concern.
However, one argument may be that QAPI json, as perfect as we could
expect they are, are simply known and used inside QEMU right now, which
is written in C at the moment.
Even if that assumption should change, I'm pretty sure we can create
functions named in the same way in C, python, Go, Rust or any other
languages we would like to generate code for.
It's not like if we started using complex expressions that only works in
nightly version of Rust.
The meaning of the QAPI schema now varies based on both the build
system, and an arbitrary amount of C, and is thus (conceptually)
even more opaque, even if you could infer some meaning from the
'target_s390x()' function name you've used. I think this is a very
undesirable characteristic for what is our public API definition.
Correct if I'm wrong, but it was said in previous threads that those
json are simply used and consumed by QEMU itself, and not by any
external projects.
I would be prudent to call this a public API definition, when it's just
a DSL for an ad-hoc code generator internal to QEMU.
With regards,
Daniel
Overall, and I would like to state it again, I'm really open to any
solution to get rid of TARGET_* compile time defines in QAPI json.
And I appreciate the solution you posted as well.
For now, it's blocking the single binary work, because any file pulling
QAPI definitions is tainted with TARGET_* defines, so we are blocked
waiting for them to be removed upstream, before being able to post a
series touching those files.
I hope we can get a clear answer from QAPI maintainers, so you or I can
take ownership of this and finish the work. From the previous thread, I
thought (and I may be wrong) that Markus was more enclined to the
current solution, but it was not entirely clear for me to be honest.
Let's hope that having both approaches implemented will give a good
insight of where things should go.
Thanks,
Pierrick
