[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 53/54] qapi: make query-cpu-model-expansion d
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v2 53/54] qapi: make query-cpu-model-expansion depend on s390 or x86 |
Date: |
Wed, 23 Aug 2017 09:58:59 -0400 (EDT) |
Hi
----- Original Message -----
> On Wed, Aug 23, 2017 at 06:21:16AM -0400, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> > > On Tue, Aug 22, 2017 at 03:22:54PM +0200, Marc-André Lureau wrote:
> > > > Signed-off-by: Marc-André Lureau <address@hidden>
> > > > ---
> > > > qapi-schema.json | 4 +++-
> > > > include/sysemu/arch_init.h | 3 ---
> > > > monitor.c | 3 ---
> > > > qmp.c | 7 -------
> > > > stubs/arch-query-cpu-model-expansion.c | 12 ------------
> > > > target/i386/cpu.c | 2 +-
> > > > target/s390x/cpu_models.c | 3 ++-
> > > > stubs/Makefile.objs | 1 -
> > > > 8 files changed, 6 insertions(+), 29 deletions(-)
> > > > delete mode 100644 stubs/arch-query-cpu-model-expansion.c
> > > >
> > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > index 6c1adb35b5..127a2c71c6 100644
> > > > --- a/qapi-schema.json
> > > > +++ b/qapi-schema.json
> > > > @@ -4535,7 +4535,9 @@
> > > > { 'command': 'query-cpu-model-expansion',
> > > > 'data': { 'type': 'CpuModelExpansionType',
> > > > 'model': 'CpuModelInfo' },
> > > > - 'returns': 'CpuModelExpansionInfo' }
> > > > + 'returns': 'CpuModelExpansionInfo',
> > > > + 'if': ['defined(NEED_CPU_H)',
> > > > + 'defined(TARGET_S390X) || defined(TARGET_I386)']}
> > >
> > > Maybe this is already documented somewhere in the series (I'm
> > > still going through the other patches), but: why exactly is
> > > 'defined(NEED_CPU_H)' in the list, too?
> >
> > The point of this series is to make qapi schema configurable.
> >
> > Some types/commands/events are target-specifc. In order to
> > #ifdef on poisoined symbols, we make most of QAPI generated
> > code built per-target in patch 49/54. But the common code still
> > need to compile some units, that's why #ifdef NEED_CPU_U. The
> > clean solution is probably to split the generated schema to
> > common & per-target, that's not covered in the series.
>
> I see. I'm worried about one thing: what if the QMP dispatch
> code is incorrectly moved back to common code later? It will
> silently skip all the arch-dependent commands without any
> warnings.
>
> e.g.: I just applied the following change on top of your series,
> and all arch-dependent commands are silently skipped:
>
And it doesn't fail a full/all-arch make check? It probably should.
> diff --git a/Makefile.objs b/Makefile.objs
> index 2664720..24a4ea0 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -2,7 +2,7 @@
> # Common libraries for tools and emulators
> stub-obj-y = stubs/ crypto/
> util-obj-y = util/ qobject/ qapi/
> -util-obj-y += qapi-types.o qapi-visit.o
> +util-obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
>
> chardev-obj-y = chardev/
>
> @@ -72,6 +72,13 @@ common-obj-y += chardev/
> common-obj-$(CONFIG_SECCOMP) += qemu-seccomp.o
>
> common-obj-$(CONFIG_FDT) += device_tree.o
> +
> +######################################################################
> +# qapi
> +
> +common-obj-y += qmp-marshal.o
> +common-obj-y += qmp-introspect.o
> +common-obj-y += qmp.o hmp.o
> endif
>
> #######################################################################
> diff --git a/Makefile.target b/Makefile.target
> index c5f8ded..7f42c45 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -157,10 +157,6 @@ endif
>
> GENERATED_FILES += hmp-commands.h hmp-commands-info.h
>
> -obj-y += qmp-introspect.o qapi-types.o qapi-visit.o qapi-event.o
> -obj-y += qmp-marshal.o qmp-introspect.o
> -obj-y += qmp.o hmp.o
> -
> endif # CONFIG_SOFTMMU
>
> # Workaround for http://gcc.gnu.org/PR55489, see configure.
>
>
> We wouldn't need to check NEED_CPU_H at all if the condition on
> QMP commands affect only the actual QMP dispatch code, and not
> the generated types or declarations. e.g.:
>
Possible, but is it desirable? I would prefer if all the generated code would
be configured, so we can more easily compile out dead code when disabling a
feature.
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 194859f..67b46ad 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3578,7 +3578,7 @@
> ##
> { 'command': 'dump-skeys',
> 'data': { 'filename': 'str' },
> - 'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
> + 'if': ['defined(TARGET_S390X)']}
>
> ##
> # @netdev_add:
> @@ -4434,8 +4434,7 @@
> # Since: 1.2.0
> ##
> { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'],
> - 'if': ['defined(NEED_CPU_H)',
> - 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386)
> || defined(TARGET_S390X)'] }
> + 'if': ['defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386)
> || defined(TARGET_S390X)'] }
>
> ##
> # @CpuModelInfo:
> @@ -4538,8 +4537,7 @@
> 'data': { 'type': 'CpuModelExpansionType',
> 'model': 'CpuModelInfo' },
> 'returns': 'CpuModelExpansionInfo',
> - 'if': ['defined(NEED_CPU_H)',
> - 'defined(TARGET_S390X) || defined(TARGET_I386)']}
> + 'if': ['defined(TARGET_S390X) || defined(TARGET_I386)']}
>
> ##
> # @CpuModelCompareResult:
> @@ -4627,7 +4625,7 @@
> { 'command': 'query-cpu-model-comparison',
> 'data': { 'modela': 'CpuModelInfo', 'modelb': 'CpuModelInfo' },
> 'returns': 'CpuModelCompareInfo',
> - 'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
> + 'if': ['defined(TARGET_S390X)']}
>
>
> ##
> @@ -4681,7 +4679,7 @@
> 'data': { 'modela': 'CpuModelInfo',
> 'modelb': 'CpuModelInfo' },
> 'returns': 'CpuModelBaselineInfo',
> - 'if': ['defined(NEED_CPU_H)', 'defined(TARGET_S390X)']}
> + 'if': ['defined(TARGET_S390X)']}
>
> ##
> # @AddfdInfo:
> @@ -6303,7 +6301,7 @@
> #
> ##
> { 'command': 'rtc-reset-reinjection',
> - 'if': ['defined(NEED_CPU_H)', 'defined(TARGET_I386)'] }
> + 'if': ['defined(TARGET_I386)'] }
>
> # Rocker ethernet network switch
> { 'include': 'qapi/rocker.json' }
> @@ -6466,7 +6464,7 @@
> #
> ##
> { 'command': 'query-gic-capabilities', 'returns': ['GICCapability'],
> - 'if': ['defined(NEED_CPU_H)', 'defined(TARGET_ARM)']}
> + 'if': ['defined(TARGET_ARM)']}
>
> ##
> # @CpuInstanceProperties:
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index bcd8d37..769a730 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1720,8 +1720,7 @@ class QAPISchema(object):
> ifcond = expr.get('if')
> if isinstance(data, OrderedDict):
> data = self._make_implicit_object_type(
> - name, info, doc, 'arg', self._make_members(data, info),
> - ifcond)
> + name, info, doc, 'arg', self._make_members(data, info))
> if isinstance(rets, list):
> assert len(rets) == 1
> rets = self._make_array_type(rets[0], info)
> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
> index db45415..2baa73f 100644
> --- a/scripts/qapi-commands.py
> +++ b/scripts/qapi-commands.py
> @@ -228,7 +228,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
> self.defn = None
> self._regy = None
> self._visited_ret_types = None
> - self.if_members = ['decl', 'defn', '_regy']
> + self.if_members = ['defn', '_regy']
>
> def visit_begin(self, schema):
> self.decl = ''
>
>
> --
> Eduardo
>
- [Qemu-devel] [PATCH v2 06/54] qapi: introduce qapi_enum_lookup(), (continued)
[Qemu-devel] [PATCH v2 27/54] qapi-types: add #if conditions to types, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 49/54] build-sys: make qemu qapi objects per-target, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 23/54] qapi-commands: add #if conditions to commands, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 40/54] docs: document schema configuration, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 46/54] qapi: add conditions to SPICE type/commands/events on the schema, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 41/54] qapi2texi: add 'If:' section to generated documentation, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 44/54] qapi2texi: add condition to variants, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 47/54] qapi: add conditions to REPLICATION type/commands on the schema, Marc-André Lureau, 2017/08/22
[Qemu-devel] [PATCH v2 52/54] qapi: make query-gic-capabilities depend on TARGET_ARM, Marc-André Lureau, 2017/08/22