diff mbox series

[09/13] qapi: transform target specific 'if' in runtime checks

Message ID 20250507231442.879619-10-pierrick.bouvier@linaro.org
State New
Headers show
Series single-binary: make QAPI generated files common | expand

Commit Message

Pierrick Bouvier May 7, 2025, 11:14 p.m. UTC 
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 qapi/machine-target.json | 84 ++++++++++++++++++++++++----------------
 qapi/misc-target.json    | 48 ++++++++++++-----------
 scripts/qapi/expr.py     |  9 +++--
 3 files changed, 81 insertions(+), 60 deletions(-)

Comments

Philippe Mathieu-Daudé May 8, 2025, 6:44 a.m. UTC | #1 
On 8/5/25 01:14, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   qapi/machine-target.json | 84 ++++++++++++++++++++++++----------------
>   qapi/misc-target.json    | 48 ++++++++++++-----------
>   scripts/qapi/expr.py     |  9 +++--
>   3 files changed, 81 insertions(+), 60 deletions(-)


> @@ -378,13 +384,18 @@
>               'typename': 'str',
>               '*alias-of' : 'str',
>               'deprecated' : 'bool' },
> -  'if': { 'any': [ 'TARGET_PPC',
> -                   'TARGET_ARM',
> -                   'TARGET_I386',
> -                   'TARGET_S390X',
> -                   'TARGET_MIPS',
> -                   'TARGET_LOONGARCH64',
> -                   'TARGET_RISCV' ] } }
> +  'runtime_if': { 'any': [ 'target_ppc()',
> +                           'target_ppc64()',
> +                           'target_arm()',
> +                           'target_aarch64()',
> +                           'target_i386()',
> +                           'target_x86_64()',
> +                           'target_s390x()',
> +                           'target_mips()',
> +                           'target_mips64()',
> +                           'target_loongarch64()',
> +                           'target_riscv32()',
> +                           'target_riscv64()' ] } }

I'd keep target_riscv() for "any RISC-V".

target_arm() and target_aarch64() could be merged as
target_arm_based()?

> @@ -272,7 +272,7 @@
>   { 'command': 'query-sev-attestation-report',
>     'data': { 'mnonce': 'str' },
>     'returns': 'SevAttestationReport',
> -  'if': 'TARGET_I386' }
> +  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }

Suggested as target_x86().

>   
>   ##
>   # @GICCapability:
> @@ -297,7 +297,7 @@
>     'data': { 'version': 'int',
>               'emulated': 'bool',
>               'kernel': 'bool' },
> -  'if': 'TARGET_ARM' }
> +  'runtime_if': { 'any': [ 'target_arm()', 'target_aarch64()' ] } }

Up to here:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index 5ae26395964..f31f28ecb10 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -638,7 +638,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
>   
>           if meta == 'enum':
>               check_keys(expr, info, meta,
> -                       ['enum', 'data'], ['if', 'features', 'prefix'])
> +                       ['enum', 'data'], ['if', 'runtime_if', 'features',
> +                                          'prefix'])
>               check_enum(expr)
>           elif meta == 'union':
>               check_keys(expr, info, meta,
> @@ -654,7 +655,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
>               check_alternate(expr)
>           elif meta == 'struct':
>               check_keys(expr, info, meta,
> -                       ['struct', 'data'], ['base', 'if', 'features'])
> +                       ['struct', 'data'], ['base', 'if', 'runtime_if',
> +                                            'features'])
>               normalize_members(expr['data'])
>               check_struct(expr)
>           elif meta == 'command':
> @@ -667,7 +669,8 @@ def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
>               check_command(expr)
>           elif meta == 'event':
>               check_keys(expr, info, meta,
> -                       ['event'], ['data', 'boxed', 'if', 'features'])
> +                       ['event'], ['data', 'boxed', 'if', 'runtime_if',
> +                                   'features'])
>               normalize_members(expr.get('data'))
>               check_event(expr)
>           else:

Changes in scripts/qapi/expr.py seem to belong to a previous
patch (existing or not).
Daniel P. Berrangé May 8, 2025, 2:40 p.m. UTC | #2 
On Wed, May 07, 2025 at 04:14:39PM -0700, Pierrick Bouvier wrote:
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@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.

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.

With regards,
Daniel
Pierrick Bouvier May 8, 2025, 8:48 p.m. UTC | #3 
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.bouvier@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
Markus Armbruster May 10, 2025, 6:57 a.m. UTC | #4 
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, May 07, 2025 at 04:14:39PM -0700, Pierrick Bouvier wrote:
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@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' }

This adds mandatory declaration of identifiers used in conditions to the
QAPI schema language.

The declarations can then serve as hooks for doc comments.  These should
ineed result in a more complete and useful generated manual.  John,
thoughts?

The condition documentation would supplement / partially duplicate the
configure flag documentation in meson_options.txt.

We'd need to tie the two together to make the code work (see your next
paragraph).  We should also tie their documentation together somehow.
Feels like a solvable problem.

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

I think the conditions that are evaluated at build time in handwritten C
code (with #if) should also be evaluated at build time in generated C
code.

Certain conditions are evaluated at build time in target-specific code,
and at runtime in target-independent code.  Again, I think handwritten
and generated code should work the same way.

Thus, to eliminate target-specific QAPI-generated code, we either
evaluate them at runtime, or simply eliminate them.  Elsewhere, we've
come to the conclusion (I think) that the latter should do at least for
now, likely forever, so we should try that first.

> With this QAPI schemas would have remained fully self-contained.

Fortunately, this is merely a matter of filling a gap we left, not a
matter of replacing a fundamentally flawed design.

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

I don't see much of a difference, to be honest.

Both kinds of conditionals have the exact same argument structure:
expression tree where the leaves are identifiers.

The meaning of these identifiers is not documented in the QAPI schema
now, and barely documented in the code.

This defect could be remedied the exact same way whether the identifiers
are preprocessor macros or function names.

I actually find another argument of yours (not repeated above) more
compelling: that certain aspects of the external interface should not
vary at runtime.
Pierrick Bouvier May 13, 2025, 12:36 a.m. UTC | #5 
On 5/9/25 11:57 PM, Markus Armbruster wrote:
>> 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.
> 
> I think the conditions that are evaluated at build time in handwritten C
> code (with #if) should also be evaluated at build time in generated C
> code.
> 
> Certain conditions are evaluated at build time in target-specific code,
> and at runtime in target-independent code.  Again, I think handwritten
> and generated code should work the same way.
> 
> Thus, to eliminate target-specific QAPI-generated code, we either
> evaluate them at runtime, or simply eliminate them.  Elsewhere, we've
> come to the conclusion (I think) that the latter should do at least for
> now, likely forever, so we should try that first.
>

I'm not sure if you mean you'd prefer to eradicate #if completely.

We have to keep in mind that some config host #if have to stay there, or 
they expose things that the rest of QEMU code is not supposed to see 
(hidden under those same CONFIG_ ifdef also).
So we would need both if and runtime_if.

>> With this QAPI schemas would have remained fully self-contained.
> 
> Fortunately, this is merely a matter of filling a gap we left, not a
> matter of replacing a fundamentally flawed design.
> 
>> 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.
>>
>> 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.
> 
> I don't see much of a difference, to be honest.
> Both kinds of conditionals have the exact same argument structure:> expression tree where the leaves are identifiers.
> 
> The meaning of these identifiers is not documented in the QAPI schema
> now, and barely documented in the code.
>

I agree on that, and I'm not sure that introducing another level of 
symbolic names (target-s390x vs target_s390x()) to match the same C 
function name really adds any value to the QAPI definition.
As mentioned before, it's easy to guarantee that the function is named 
the same in any language, and ban any use of C complex expression 
(through code review), so the argument about generating bindings in 
other languages does not really matter IMHO.

> This defect could be remedied the exact same way whether the identifiers
> are preprocessor macros or function names.
> 
> I actually find another argument of yours (not repeated above) more
> compelling: that certain aspects of the external interface should not
> vary at runtime.
>
Markus Armbruster May 13, 2025, 7:08 a.m. UTC | #6 
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 5/9/25 11:57 PM, Markus Armbruster wrote:
>>> 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.
>> 
>> I think the conditions that are evaluated at build time in handwritten C
>> code (with #if) should also be evaluated at build time in generated C
>> code.
>> 
>> Certain conditions are evaluated at build time in target-specific code,
>> and at runtime in target-independent code.  Again, I think handwritten
>> and generated code should work the same way.
>> 
>> Thus, to eliminate target-specific QAPI-generated code, we either
>> evaluate them at runtime, or simply eliminate them.  Elsewhere, we've
>> come to the conclusion (I think) that the latter should do at least for
>> now, likely forever, so we should try that first.
>>
>
> I'm not sure if you mean you'd prefer to eradicate #if completely.

I do not!

> We have to keep in mind that some config host #if have to stay there, or 
> they expose things that the rest of QEMU code is not supposed to see 
> (hidden under those same CONFIG_ ifdef also).

Letting people configure their QEMU build is useful and must stay.

We provide this via conditional compilation, of complete source files
(done in meson), as well as within source files (#if in C and 'if' in
QAPI).

> So we would need both if and runtime_if.

I don't understand the need for runtime_if.  Can you give an example?

[...]
Pierrick Bouvier May 13, 2025, 10:52 p.m. UTC | #7 
On 5/13/25 12:08 AM, Markus Armbruster wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 5/9/25 11:57 PM, Markus Armbruster wrote:
>>>> 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.
>>>
>>> I think the conditions that are evaluated at build time in handwritten C
>>> code (with #if) should also be evaluated at build time in generated C
>>> code.
>>>
>>> Certain conditions are evaluated at build time in target-specific code,
>>> and at runtime in target-independent code.  Again, I think handwritten
>>> and generated code should work the same way.
>>>
>>> Thus, to eliminate target-specific QAPI-generated code, we either
>>> evaluate them at runtime, or simply eliminate them.  Elsewhere, we've
>>> come to the conclusion (I think) that the latter should do at least for
>>> now, likely forever, so we should try that first.
>>>
>>
>> I'm not sure if you mean you'd prefer to eradicate #if completely.
> 
> I do not!
> 
>> We have to keep in mind that some config host #if have to stay there, or
>> they expose things that the rest of QEMU code is not supposed to see
>> (hidden under those same CONFIG_ ifdef also).
> 
> Letting people configure their QEMU build is useful and must stay.
> 
> We provide this via conditional compilation, of complete source files
> (done in meson), as well as within source files (#if in C and 'if' in
> QAPI).
> 
>> So we would need both if and runtime_if.
> 
> I don't understand the need for runtime_if.  Can you give an example?
>

That is the point of this whole series, which explores introducing a 
'runtime' if in the schema, to keep it as it is today, while removing 
target specific compile time defines.
It is another approach that one Daniel followed on his series.

I invite you to give a quick read to this series, especially the related 
commit introducing 'runtime_if' 
(20250507231442.879619-2-pierrick.bouvier@linaro.org).

As well, I would appreciate if you could state clearly where we are 
going with all this (or at least, where you *don't* want this to go), so 
we can avoid spending time in the wrong direction.

I am ok to pursue Daniel's approach, or continue the approach in the 
current series, no strong opinion in my side, I am just trying to move 
QAPI generated code out of the way for the single binary goal.

Thanks,
Pierrick
Markus Armbruster May 14, 2025, 7:13 a.m. UTC | #8 
Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 5/13/25 12:08 AM, Markus Armbruster wrote:
>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>> 
>>> On 5/9/25 11:57 PM, Markus Armbruster wrote:
>>>>> 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.
>>>>
>>>> I think the conditions that are evaluated at build time in handwritten C
>>>> code (with #if) should also be evaluated at build time in generated C
>>>> code.
>>>>
>>>> Certain conditions are evaluated at build time in target-specific code,
>>>> and at runtime in target-independent code.  Again, I think handwritten
>>>> and generated code should work the same way.
>>>>
>>>> Thus, to eliminate target-specific QAPI-generated code, we either
>>>> evaluate them at runtime, or simply eliminate them.  Elsewhere, we've
>>>> come to the conclusion (I think) that the latter should do at least for
>>>> now, likely forever, so we should try that first.
>>>>
>>>
>>> I'm not sure if you mean you'd prefer to eradicate #if completely.
>> 
>> I do not!
>> 
>>> We have to keep in mind that some config host #if have to stay there, or
>>> they expose things that the rest of QEMU code is not supposed to see
>>> (hidden under those same CONFIG_ ifdef also).
>> 
>> Letting people configure their QEMU build is useful and must stay.
>> 
>> We provide this via conditional compilation, of complete source files
>> (done in meson), as well as within source files (#if in C and 'if' in
>> QAPI).
>> 
>>> So we would need both if and runtime_if.
>> 
>> I don't understand the need for runtime_if.  Can you give an example?
>>
>
> That is the point of this whole series, which explores introducing a 
> 'runtime' if in the schema, to keep it as it is today, while removing 
> target specific compile time defines.

Ah, I lost the wider context, sorry!

We identified three ways to deal with target-specific conditionals in a
single binary, and 'runtime_if' is one of them:

(1) Drop target-specific conditionals.

(2) Replace them by run-time checks.

(3) Have target-specific QAPI-generated code for multiple targets
    coexist in the single binary.

Both (2) and (3) keep the QAPI schema work as it does now.  None of us
likes (3) due to bloat and complexity.

The other two look both workable to me.  (2) keeps the QAPI schema work
exactly as it does now.  (1) is simpler, but the external interface
changes somewhat.  Its users seem to be okay with it.

So let's go with (1).

> It is another approach that one Daniel followed on his series.
>
> I invite you to give a quick read to this series, especially the related 
> commit introducing 'runtime_if' 
> (20250507231442.879619-2-pierrick.bouvier@linaro.org).

I can't afford a thorough review now, but I'll have a look, and I will
hold onto your series just in case.

> As well, I would appreciate if you could state clearly where we are 
> going with all this (or at least, where you *don't* want this to go), so 
> we can avoid spending time in the wrong direction.

The discussion was spread over multiple threads, which makes it hard to
follow.  I hope the conclusion is clear now.  If not, please ask for
further clarification.

> I am ok to pursue Daniel's approach, or continue the approach in the 
> current series, no strong opinion in my side, I am just trying to move 
> QAPI generated code out of the way for the single binary goal.

Understood!
Markus Armbruster May 14, 2025, 2:09 p.m. UTC | #9 
Fails to build:

        /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_comparison':
        /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:66:(.text+0x10c9): undefined reference to `qmp_query_cpu_model_comparison'
        /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_baseline':
        /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:131:(.text+0x143c): undefined reference to `qmp_query_cpu_model_baseline'
        /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_set_cpu_topology':
        /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:306:(.text+0x1de1): undefined reference to `qmp_set_cpu_topology'
        /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_s390x_cpu_polarization':
        /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:362:(.text+0x20c6): undefined reference to `qmp_query_s390x_cpu_polarization'
        /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-misc-target.c.o: in function `qmp_marshal_query_gic_capabilities':
        /work/armbru/qemu/bld-x86/qapi/qapi-commands-misc-target.c:393:(.text+0x2d2d): undefined reference to `qmp_query_gic_capabilities'
        collect2: error: ld returned 1 exit status

The next commit fixes it.  It then fails tests:

    stdout:
    --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/unknown-expr-key.err
    +++ 
    @@ -1,3 +1,3 @@
     unknown-expr-key.json: In struct 'Bar':
     unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
    -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
    +Valid keys are 'base', 'data', 'features', 'if', 'runtime_if', 'struct'.
    stderr:
    unknown-expr-key FAIL

Fixup:

diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
index f2538e3ce7..8184f3c768 100644
--- a/tests/qapi-schema/unknown-expr-key.err
+++ b/tests/qapi-schema/unknown-expr-key.err
@@ -1,3 +1,3 @@
 unknown-expr-key.json: In struct 'Bar':
 unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
-Valid keys are 'base', 'data', 'features', 'if', 'struct'.
+Valid keys are 'base', 'data', 'features', 'if', 'runtime_if', 'struct'.
Pierrick Bouvier May 14, 2025, 4:50 p.m. UTC | #10 
On 5/14/25 7:09 AM, Markus Armbruster wrote:
> Fails to build:
> 
>          /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_comparison':
>          /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:66:(.text+0x10c9): undefined reference to `qmp_query_cpu_model_comparison'
>          /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_baseline':
>          /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:131:(.text+0x143c): undefined reference to `qmp_query_cpu_model_baseline'
>          /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_set_cpu_topology':
>          /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:306:(.text+0x1de1): undefined reference to `qmp_set_cpu_topology'
>          /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_s390x_cpu_polarization':
>          /work/armbru/qemu/bld-x86/qapi/qapi-commands-machine-target.c:362:(.text+0x20c6): undefined reference to `qmp_query_s390x_cpu_polarization'
>          /usr/bin/ld: libqemu-x86_64-softmmu.a.p/meson-generated_.._qapi_qapi-commands-misc-target.c.o: in function `qmp_marshal_query_gic_capabilities':
>          /work/armbru/qemu/bld-x86/qapi/qapi-commands-misc-target.c:393:(.text+0x2d2d): undefined reference to `qmp_query_gic_capabilities'
>          collect2: error: ld returned 1 exit status
> 
> The next commit fixes it.  It then fails tests:
> 

Oops indeed, I wrongly ordered this when rebasing (stubs should come first).

>      stdout:
>      --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/unknown-expr-key.err
>      +++
>      @@ -1,3 +1,3 @@
>       unknown-expr-key.json: In struct 'Bar':
>       unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
>      -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
>      +Valid keys are 'base', 'data', 'features', 'if', 'runtime_if', 'struct'.
>      stderr:
>      unknown-expr-key FAIL
> 
> Fixup:
> 
> diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
> index f2538e3ce7..8184f3c768 100644
> --- a/tests/qapi-schema/unknown-expr-key.err
> +++ b/tests/qapi-schema/unknown-expr-key.err
> @@ -1,3 +1,3 @@
>   unknown-expr-key.json: In struct 'Bar':
>   unknown-expr-key.json:2: struct has unknown keys 'bogus', 'phony'
> -Valid keys are 'base', 'data', 'features', 'if', 'struct'.
> +Valid keys are 'base', 'data', 'features', 'if', 'runtime_if', 'struct'.
> 

Yes, I noticed this test failure, but as mentioned in cover letter, I 
let tests and doc out of the scope of this series.
Pierrick Bouvier May 14, 2025, 4:54 p.m. UTC | #11 
On 5/14/25 12:13 AM, Markus Armbruster wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 5/13/25 12:08 AM, Markus Armbruster wrote:
>>> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
>>>
>>>> On 5/9/25 11:57 PM, Markus Armbruster wrote:
>>>>>> 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.
>>>>>
>>>>> I think the conditions that are evaluated at build time in handwritten C
>>>>> code (with #if) should also be evaluated at build time in generated C
>>>>> code.
>>>>>
>>>>> Certain conditions are evaluated at build time in target-specific code,
>>>>> and at runtime in target-independent code.  Again, I think handwritten
>>>>> and generated code should work the same way.
>>>>>
>>>>> Thus, to eliminate target-specific QAPI-generated code, we either
>>>>> evaluate them at runtime, or simply eliminate them.  Elsewhere, we've
>>>>> come to the conclusion (I think) that the latter should do at least for
>>>>> now, likely forever, so we should try that first.
>>>>>
>>>>
>>>> I'm not sure if you mean you'd prefer to eradicate #if completely.
>>>
>>> I do not!
>>>
>>>> We have to keep in mind that some config host #if have to stay there, or
>>>> they expose things that the rest of QEMU code is not supposed to see
>>>> (hidden under those same CONFIG_ ifdef also).
>>>
>>> Letting people configure their QEMU build is useful and must stay.
>>>
>>> We provide this via conditional compilation, of complete source files
>>> (done in meson), as well as within source files (#if in C and 'if' in
>>> QAPI).
>>>
>>>> So we would need both if and runtime_if.
>>>
>>> I don't understand the need for runtime_if.  Can you give an example?
>>>
>>
>> That is the point of this whole series, which explores introducing a
>> 'runtime' if in the schema, to keep it as it is today, while removing
>> target specific compile time defines.
> 
> Ah, I lost the wider context, sorry!
>

No worries, I was not sure if you missed information or simply waited 
for more feedback, so I did well to ask you directly.

> We identified three ways to deal with target-specific conditionals in a
> single binary, and 'runtime_if' is one of them:
> 
> (1) Drop target-specific conditionals.
> 
> (2) Replace them by run-time checks.
> 
> (3) Have target-specific QAPI-generated code for multiple targets
>      coexist in the single binary.
> 
> Both (2) and (3) keep the QAPI schema work as it does now.  None of us
> likes (3) due to bloat and complexity.
> 
> The other two look both workable to me.  (2) keeps the QAPI schema work
> exactly as it does now.  (1) is simpler, but the external interface
> changes somewhat.  Its users seem to be okay with it.
> 
> So let's go with (1).
>

Sounds good to me. I agree that Daniel approach is simpler and better as 
well, as long as we accept the resulting changes in the schema, which 
seems ok for users.

Thus, I'll drop current series, and focus on respinning Daniel series 
and adding the changes requested.

>> It is another approach that one Daniel followed on his series.
>>
>> I invite you to give a quick read to this series, especially the related
>> commit introducing 'runtime_if'
>> (20250507231442.879619-2-pierrick.bouvier@linaro.org).
> 
> I can't afford a thorough review now, but I'll have a look, and I will
> hold onto your series just in case.
> 
>> As well, I would appreciate if you could state clearly where we are
>> going with all this (or at least, where you *don't* want this to go), so
>> we can avoid spending time in the wrong direction.
> 
> The discussion was spread over multiple threads, which makes it hard to
> follow.  I hope the conclusion is clear now.  If not, please ask for
> further clarification.
>

Yes, I agree. At least we are all on the same page right now, I'll 
pursue Daniel's approach.

>> I am ok to pursue Daniel's approach, or continue the approach in the
>> current series, no strong opinion in my side, I am just trying to move
>> QAPI generated code out of the way for the single binary goal.
> 
> Understood!
> 

Thanks Markus!

Pierrick
diff mbox series

Patch

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()' }
 
 ##
 # @CpuModelCompareInfo:
@@ -120,7 +120,7 @@ 
 { 'struct': 'CpuModelCompareInfo',
   'data': { 'result': 'CpuModelCompareResult',
             'responsible-properties': ['str'] },
-  'if': 'TARGET_S390X' }
+  'runtime_if': 'target_s390x()' }
 
 ##
 # @query-cpu-model-comparison:
@@ -179,7 +179,7 @@ 
 { 'command': 'query-cpu-model-comparison',
   'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
   'returns': 'CpuModelCompareInfo',
-  'if': 'TARGET_S390X' }
+  'runtime_if': 'target_s390x()' }
 
 ##
 # @query-cpu-model-baseline:
@@ -235,7 +235,7 @@ 
   'data': { 'modela': 'CpuModelInfo',
             'modelb': 'CpuModelInfo' },
   'returns': 'CpuModelBaselineInfo',
-  'if': 'TARGET_S390X' }
+  'runtime_if': 'target_s390x()' }
 
 ##
 # @CpuModelExpansionInfo:
@@ -256,12 +256,15 @@ 
 { 'struct': 'CpuModelExpansionInfo',
   'data': { 'model': 'CpuModelInfo',
             'deprecated-props' : { 'type': ['str'],
-                                   'if': 'TARGET_S390X' } },
-  'if': { 'any': [ 'TARGET_S390X',
-                   'TARGET_I386',
-                   'TARGET_ARM',
-                   'TARGET_LOONGARCH64',
-                   'TARGET_RISCV' ] } }
+                                   'runtime_if': 'target_s390x()'} },
+  'runtime_if': { 'any': [ 'target_s390x()',
+                           'target_i386()',
+                           'target_x86_64()',
+                           'target_arm()',
+                           'target_aarch64()',
+                           'target_loongarch64()',
+                           'target_riscv32()',
+                           'target_riscv64()' ] } }
 
 ##
 # @query-cpu-model-expansion:
@@ -311,11 +314,14 @@ 
   'data': { 'type': 'CpuModelExpansionType',
             'model': 'CpuModelInfo' },
   'returns': 'CpuModelExpansionInfo',
-  'if': { 'any': [ 'TARGET_S390X',
-                   'TARGET_I386',
-                   'TARGET_ARM',
-                   'TARGET_LOONGARCH64',
-                   'TARGET_RISCV' ] } }
+  'runtime_if': { 'any': [ 'target_s390x()',
+                           'target_i386()',
+                           'target_x86_64()',
+                           'target_arm()',
+                           'target_aarch64()',
+                           'target_loongarch64()',
+                           'target_riscv32()',
+                           'target_riscv64()' ] } }
 
 ##
 # @CpuDefinitionInfo:
@@ -378,13 +384,18 @@ 
             'typename': 'str',
             '*alias-of' : 'str',
             'deprecated' : 'bool' },
-  'if': { 'any': [ 'TARGET_PPC',
-                   'TARGET_ARM',
-                   'TARGET_I386',
-                   'TARGET_S390X',
-                   'TARGET_MIPS',
-                   'TARGET_LOONGARCH64',
-                   'TARGET_RISCV' ] } }
+  'runtime_if': { 'any': [ 'target_ppc()',
+                           'target_ppc64()',
+                           'target_arm()',
+                           'target_aarch64()',
+                           'target_i386()',
+                           'target_x86_64()',
+                           'target_s390x()',
+                           'target_mips()',
+                           'target_mips64()',
+                           'target_loongarch64()',
+                           'target_riscv32()',
+                           'target_riscv64()' ] } }
 
 ##
 # @query-cpu-definitions:
@@ -396,13 +407,18 @@ 
 # Since: 1.2
 ##
 { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
-  'if': { 'any': [ 'TARGET_PPC',
-                   'TARGET_ARM',
-                   'TARGET_I386',
-                   'TARGET_S390X',
-                   'TARGET_MIPS',
-                   'TARGET_LOONGARCH64',
-                   'TARGET_RISCV' ] } }
+  'runtime_if': { 'any': [ 'target_ppc()',
+                           'target_ppc64()',
+                           'target_arm()',
+                           'target_aarch64()',
+                           'target_i386()',
+                           'target_x86_64()',
+                           'target_s390x()',
+                           'target_mips()',
+                           'target_mips64()',
+                           'target_loongarch64()',
+                           'target_riscv32()',
+                           'target_riscv64()' ] } }
 
 ##
 # @S390CpuPolarization:
@@ -414,7 +430,7 @@ 
 ##
 { 'enum': 'S390CpuPolarization',
   'data': [ 'horizontal', 'vertical' ],
-  'if': 'TARGET_S390X'
+  'runtime_if': 'target_s390x()'
 }
 
 ##
@@ -453,7 +469,7 @@ 
       '*dedicated': 'bool'
   },
   'features': [ 'unstable' ],
-  'if': { 'all': [ 'TARGET_S390X' , 'CONFIG_KVM' ] }
+  'runtime_if': { 'all': [ 'target_s390x()' , 'target_has_kvm()' ] }
 }
 
 ##
@@ -489,7 +505,7 @@ 
 { 'event': 'CPU_POLARIZATION_CHANGE',
   'data': { 'polarization': 'S390CpuPolarization' },
   'features': [ 'unstable' ],
-  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
+  'runtime_if': { 'all': [ 'target_s390x()' , 'target_has_kvm()' ] }
 }
 
 ##
@@ -503,7 +519,7 @@ 
 ##
 { 'struct': 'CpuPolarizationInfo',
   'data': { 'polarization': 'S390CpuPolarization' },
-  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
+  'runtime_if': { 'all': [ 'target_s390x()' , 'target_has_kvm()' ] }
 }
 
 ##
@@ -519,5 +535,5 @@ 
 ##
 { 'command': 'query-s390x-cpu-polarization', 'returns': 'CpuPolarizationInfo',
   'features': [ 'unstable' ],
-  'if': { 'all': [ 'TARGET_S390X', 'CONFIG_KVM' ] }
+  'runtime_if': { 'all': [ 'target_s390x()' , 'target_has_kvm()' ] }
 }
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index 42e4a7417dc..54500533e5b 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -17,7 +17,7 @@ 
 #     <- { "return": {} }
 ##
 { 'command': 'rtc-reset-reinjection',
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevState:
@@ -45,7 +45,7 @@ 
 { 'enum': 'SevState',
   'data': ['uninit', 'launch-update', 'launch-secret', 'running',
            'send-update', 'receive-update' ],
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevGuestType:
@@ -60,7 +60,7 @@ 
 ##
 { 'enum': 'SevGuestType',
   'data': [ 'sev', 'sev-snp' ],
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevGuestInfo:
@@ -76,7 +76,7 @@ 
 { 'struct': 'SevGuestInfo',
   'data': { 'policy': 'uint32',
             'handle': 'uint32' },
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevSnpGuestInfo:
@@ -89,7 +89,7 @@ 
 ##
 { 'struct': 'SevSnpGuestInfo',
   'data': { 'snp-policy': 'uint64' },
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevInfo:
@@ -121,7 +121,7 @@ 
   'data': {
       'sev': 'SevGuestInfo',
       'sev-snp': 'SevSnpGuestInfo' },
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 
 ##
@@ -141,7 +141,7 @@ 
 #                      "handle" : 1 } }
 ##
 { 'command': 'query-sev', 'returns': 'SevInfo',
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevLaunchMeasureInfo:
@@ -153,7 +153,7 @@ 
 # Since: 2.12
 ##
 { 'struct': 'SevLaunchMeasureInfo', 'data': {'data': 'str'},
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @query-sev-launch-measure:
@@ -170,7 +170,7 @@ 
 #     <- { "return": { "data": "4l8LXeNlSPUDlXPJG5966/8%YZ" } }
 ##
 { 'command': 'query-sev-launch-measure', 'returns': 'SevLaunchMeasureInfo',
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevCapability:
@@ -197,7 +197,7 @@ 
             'cpu0-id': 'str',
             'cbitpos': 'int',
             'reduced-phys-bits': 'int'},
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @query-sev-capabilities:
@@ -217,7 +217,7 @@ 
 #                      "cbitpos": 47, "reduced-phys-bits": 1}}
 ##
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @sev-inject-launch-secret:
@@ -234,7 +234,7 @@ 
 ##
 { 'command': 'sev-inject-launch-secret',
   'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' },
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @SevAttestationReport:
@@ -248,7 +248,7 @@ 
 ##
 { 'struct': 'SevAttestationReport',
   'data': { 'data': 'str'},
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @query-sev-attestation-report:
@@ -272,7 +272,7 @@ 
 { 'command': 'query-sev-attestation-report',
   'data': { 'mnonce': 'str' },
   'returns': 'SevAttestationReport',
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @GICCapability:
@@ -297,7 +297,7 @@ 
   'data': { 'version': 'int',
             'emulated': 'bool',
             'kernel': 'bool' },
-  'if': 'TARGET_ARM' }
+  'runtime_if': { 'any': [ 'target_arm()', 'target_aarch64()' ] } }
 
 ##
 # @query-gic-capabilities:
@@ -316,7 +316,7 @@ 
 #                     { "version": 3, "emulated": false, "kernel": true } ] }
 ##
 { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
-  'if': 'TARGET_ARM' }
+  'runtime_if': { 'any': [ 'target_arm()', 'target_aarch64()' ] } }
 
 ##
 # @SGXEPCSection:
@@ -356,7 +356,7 @@ 
             'sgx2': 'bool',
             'flc': 'bool',
             'sections': ['SGXEPCSection']},
-   'if': 'TARGET_I386' }
+   'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @query-sgx:
@@ -375,7 +375,8 @@ 
 #                      "sections": [{"node": 0, "size": 67108864},
 #                      {"node": 1, "size": 29360128}]} }
 ##
-{ 'command': 'query-sgx', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
+{ 'command': 'query-sgx', 'returns': 'SGXInfo',
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @query-sgx-capabilities:
@@ -394,7 +395,8 @@ 
 #                      "section" : [{"node": 0, "size": 67108864},
 #                      {"node": 1, "size": 29360128}]} }
 ##
-{ 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo', 'if': 'TARGET_I386' }
+{ 'command': 'query-sgx-capabilities', 'returns': 'SGXInfo',
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 
 ##
@@ -418,7 +420,7 @@ 
 ##
 { 'enum': 'EvtchnPortType',
   'data': ['closed', 'unbound', 'interdomain', 'pirq', 'virq', 'ipi'],
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @EvtchnInfo:
@@ -449,7 +451,7 @@ 
            'target': 'uint16',
            'pending': 'bool',
            'masked': 'bool'},
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 
 ##
@@ -488,7 +490,7 @@ 
 ##
 { 'command': 'xen-event-list',
   'returns': ['EvtchnInfo'],
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
 
 ##
 # @xen-event-inject:
@@ -506,4 +508,4 @@ 
 ##
 { 'command': 'xen-event-inject',
   'data': { 'port': 'uint32' },
-  'if': 'TARGET_I386' }
+  'runtime_if': { 'any': [ 'target_i386()', 'target_x86_64()' ] } }
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 5ae26395964..f31f28ecb10 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -638,7 +638,8 @@  def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
 
         if meta == 'enum':
             check_keys(expr, info, meta,
-                       ['enum', 'data'], ['if', 'features', 'prefix'])
+                       ['enum', 'data'], ['if', 'runtime_if', 'features',
+                                          'prefix'])
             check_enum(expr)
         elif meta == 'union':
             check_keys(expr, info, meta,
@@ -654,7 +655,8 @@  def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
             check_alternate(expr)
         elif meta == 'struct':
             check_keys(expr, info, meta,
-                       ['struct', 'data'], ['base', 'if', 'features'])
+                       ['struct', 'data'], ['base', 'if', 'runtime_if',
+                                            'features'])
             normalize_members(expr['data'])
             check_struct(expr)
         elif meta == 'command':
@@ -667,7 +669,8 @@  def check_exprs(exprs: List[QAPIExpression]) -> List[QAPIExpression]:
             check_command(expr)
         elif meta == 'event':
             check_keys(expr, info, meta,
-                       ['event'], ['data', 'boxed', 'if', 'features'])
+                       ['event'], ['data', 'boxed', 'if', 'runtime_if',
+                                   'features'])
             normalize_members(expr.get('data'))
             check_event(expr)
         else: