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