Using target-specific CONFIG_xxx switches in common code via "#ifdef"s is wrong, since these macros are only defined for target-specific code. We already poison many switches in common code to avoid the bugs with dead code here, but these problems still keep creeping in ... This series now improves the situation by poisoning more symbols, especially by generating these from the target-specific config headers automatically. Thomas Huth (4): include/sysemu: Poison all accelerator CONFIG switches in common code migration: Move populate_vfio_info() into a separate file qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code configure: Poison all current target-specific #defines Makefile | 2 +- configure | 5 +++++ include/exec/poison.h | 6 ++++++ include/sysemu/hax.h | 4 ++++ include/sysemu/hvf.h | 4 ++++ include/sysemu/whpx.h | 4 ++++ migration/meson.build | 3 ++- migration/migration.c | 15 --------------- migration/migration.h | 2 ++ migration/target.c | 25 +++++++++++++++++++++++++ qapi/qom.json | 4 ++-- 11 files changed, 55 insertions(+), 19 deletions(-) create mode 100644 migration/target.c -- 2.27.0
We are already poisoning CONFIG_KVM since this switch is not working in common code. Do the same with the other accelerator switches, too (except for CONFIG_TCG, which is special, since it is also defined in config-host.h). Signed-off-by: Thomas Huth <thuth@redhat.com> --- include/exec/poison.h | 4 ++++ include/sysemu/hax.h | 4 ++++ include/sysemu/hvf.h | 4 ++++ include/sysemu/whpx.h | 4 ++++ 4 files changed, 16 insertions(+) diff --git a/include/exec/poison.h b/include/exec/poison.h index 4cd3f8abb4..3250fc1d52 100644 --- a/include/exec/poison.h +++ b/include/exec/poison.h @@ -88,8 +88,12 @@ #pragma GCC poison CONFIG_SPARC_DIS #pragma GCC poison CONFIG_XTENSA_DIS +#pragma GCC poison CONFIG_HAX +#pragma GCC poison CONFIG_HVF #pragma GCC poison CONFIG_LINUX_USER #pragma GCC poison CONFIG_KVM #pragma GCC poison CONFIG_SOFTMMU +#pragma GCC poison CONFIG_WHPX +#pragma GCC poison CONFIG_XEN #endif diff --git a/include/sysemu/hax.h b/include/sysemu/hax.h index 12fb54f990..247f0661d1 100644 --- a/include/sysemu/hax.h +++ b/include/sysemu/hax.h @@ -24,6 +24,8 @@ int hax_sync_vcpus(void); +#ifdef NEED_CPU_H + #ifdef CONFIG_HAX int hax_enabled(void); @@ -34,4 +36,6 @@ int hax_enabled(void); #endif /* CONFIG_HAX */ +#endif /* NEED_CPU_H */ + #endif /* QEMU_HAX_H */ diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h index c98636bc81..bb70082e45 100644 --- a/include/sysemu/hvf.h +++ b/include/sysemu/hvf.h @@ -16,6 +16,8 @@ #include "qemu/accel.h" #include "qom/object.h" +#ifdef NEED_CPU_H + #ifdef CONFIG_HVF uint32_t hvf_get_supported_cpuid(uint32_t func, uint32_t idx, int reg); @@ -26,6 +28,8 @@ extern bool hvf_allowed; #define hvf_get_supported_cpuid(func, idx, reg) 0 #endif /* !CONFIG_HVF */ +#endif /* NEED_CPU_H */ + #define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf") typedef struct HVFState HVFState; diff --git a/include/sysemu/whpx.h b/include/sysemu/whpx.h index 8ca1c1c4ac..2889fa2278 100644 --- a/include/sysemu/whpx.h +++ b/include/sysemu/whpx.h @@ -13,6 +13,8 @@ #ifndef QEMU_WHPX_H #define QEMU_WHPX_H +#ifdef NEED_CPU_H + #ifdef CONFIG_WHPX int whpx_enabled(void); @@ -25,4 +27,6 @@ bool whpx_apic_in_platform(void); #endif /* CONFIG_WHPX */ +#endif /* NEED_CPU_H */ + #endif /* QEMU_WHPX_H */ -- 2.27.0
The CONFIG_VFIO switch only works in target specific code. Since migration/migration.c is common code, the #ifdef does not have the intended behavior here. Move the related code to a separate file now which gets compiled via specific_ss instead. Fixes: 3710586caa ("qapi: Add VFIO devices migration stats in Migration stats") Signed-off-by: Thomas Huth <thuth@redhat.com> --- migration/meson.build | 3 ++- migration/migration.c | 15 --------------- migration/migration.h | 2 ++ migration/target.c | 25 +++++++++++++++++++++++++ 4 files changed, 29 insertions(+), 16 deletions(-) create mode 100644 migration/target.c diff --git a/migration/meson.build b/migration/meson.build index 3ecedce94d..f8714dcb15 100644 --- a/migration/meson.build +++ b/migration/meson.build @@ -31,4 +31,5 @@ softmmu_ss.add(when: ['CONFIG_RDMA', rdma], if_true: files('rdma.c')) softmmu_ss.add(when: 'CONFIG_LIVE_BLOCK_MIGRATION', if_true: files('block.c')) softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.c')) -specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c', 'ram.c')) +specific_ss.add(when: 'CONFIG_SOFTMMU', + if_true: files('dirtyrate.c', 'ram.c', 'target.c')) diff --git a/migration/migration.c b/migration/migration.c index 8ca034136b..db8c378079 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -60,10 +60,6 @@ #include "qemu/yank.h" #include "sysemu/cpus.h" -#ifdef CONFIG_VFIO -#include "hw/vfio/vfio-common.h" -#endif - #define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */ /* Amount of time to allocate to each "chunk" of bandwidth-throttled @@ -1059,17 +1055,6 @@ static void populate_disk_info(MigrationInfo *info) } } -static void populate_vfio_info(MigrationInfo *info) -{ -#ifdef CONFIG_VFIO - if (vfio_mig_active()) { - info->has_vfio = true; - info->vfio = g_malloc0(sizeof(*info->vfio)); - info->vfio->transferred = vfio_mig_bytes_transferred(); - } -#endif -} - static void fill_source_migration_info(MigrationInfo *info) { MigrationState *s = migrate_get_current(); diff --git a/migration/migration.h b/migration/migration.h index db6708326b..2730fa05c0 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -376,4 +376,6 @@ void migration_make_urgent_request(void); void migration_consume_urgent_request(void); bool migration_rate_limit(void); +void populate_vfio_info(MigrationInfo *info); + #endif diff --git a/migration/target.c b/migration/target.c new file mode 100644 index 0000000000..907ebf0a0a --- /dev/null +++ b/migration/target.c @@ -0,0 +1,25 @@ +/* + * QEMU live migration - functions that need to be compiled target-specific + * + * This work is licensed under the terms of the GNU GPL, version 2 + * or (at your option) any later version. + */ + +#include "qemu/osdep.h" +#include "qapi/qapi-types-migration.h" +#include "migration.h" + +#ifdef CONFIG_VFIO +#include "hw/vfio/vfio-common.h" +#endif + +void populate_vfio_info(MigrationInfo *info) +{ +#ifdef CONFIG_VFIO + if (vfio_mig_active()) { + info->has_vfio = true; + info->vfio = g_malloc0(sizeof(*info->vfio)); + info->vfio->transferred = vfio_mig_bytes_transferred(); + } +#endif +} -- 2.27.0
The ObjectType enum and ObjectOptions are included from qapi-types-qom.h into common code. We should not use target-specific config switches like CONFIG_VIRTIO_CRYPTO here, since this is not defined in common code and thus the enum will look differently between common and target specific code. For this case, it's hopefully enough to check for CONFIG_VHOST_CRYPTO only (which is a host specific config switch, i.e. it's the same on all targets). Signed-off-by: Thomas Huth <thuth@redhat.com> --- qapi/qom.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qapi/qom.json b/qapi/qom.json index db5ac419b1..cd0e76d564 100644 --- a/qapi/qom.json +++ b/qapi/qom.json @@ -752,7 +752,7 @@ 'cryptodev-backend', 'cryptodev-backend-builtin', { 'name': 'cryptodev-vhost-user', - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, 'dbus-vmstate', 'filter-buffer', 'filter-dump', @@ -809,7 +809,7 @@ 'cryptodev-backend': 'CryptodevBackendProperties', 'cryptodev-backend-builtin': 'CryptodevBackendProperties', 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties', - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, 'dbus-vmstate': 'DBusVMStateProperties', 'filter-buffer': 'FilterBufferProperties', 'filter-dump': 'FilterDumpProperties', -- 2.27.0
We are generating a lot of target-specific defines in the *-config-devices.h and *-config-target.h files. Using them in common code is wrong and leads to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there as expected. To avoid these issues, we are already poisoning many of the macros in include/exec/poison.h - but it's cumbersome to maintain this list manually. Thus let's generate an additional list of poisoned macros automatically from the current config switches - this should give us a much better test coverage via the different CI configurations. Note that CONFIG_TCG (which is also defined in config-host.h) and CONFIG_USER_ONLY are special, so we have to filter these out. Signed-off-by: Thomas Huth <thuth@redhat.com> --- Makefile | 2 +- configure | 5 +++++ include/exec/poison.h | 2 ++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index bcbbec71a1..4cab10a2a4 100644 --- a/Makefile +++ b/Makefile @@ -213,7 +213,7 @@ qemu-%.tar.bz2: distclean: clean -$(quiet-@)test -f build.ninja && $(NINJA) $(NINJAFLAGS) -t clean -g || : - rm -f config-host.mak config-host.h* + rm -f config-host.mak config-host.h* config-poison.h rm -f tests/tcg/config-*.mak rm -f config-all-disas.mak config.status rm -f roms/seabios/config.mak roms/vgabios/config.mak diff --git a/configure b/configure index 4f374b4889..a0f0601e7e 100755 --- a/configure +++ b/configure @@ -6440,6 +6440,11 @@ if test -n "${deprecated_features}"; then echo " features: ${deprecated_features}" fi +sed -n -e '/CONFIG_TCG/d' -e '/CONFIG_USER_ONLY/d' \ + -e '/^#define / { s///; s/ .*//; s/^/#pragma GCC poison /p; }' \ + *-config-devices.h *-config-target.h | \ + sort -u > config-poison.h + # Save the configure command line for later reuse. cat <<EOD >config.status #!/bin/sh diff --git a/include/exec/poison.h b/include/exec/poison.h index 3250fc1d52..ae2f9d1e70 100644 --- a/include/exec/poison.h +++ b/include/exec/poison.h @@ -4,6 +4,8 @@ #ifndef HW_POISON_H #define HW_POISON_H +#include "config-poison.h" + #pragma GCC poison TARGET_I386 #pragma GCC poison TARGET_X86_64 #pragma GCC poison TARGET_AARCH64 -- 2.27.0
* Thomas Huth (thuth@redhat.com) wrote: > The CONFIG_VFIO switch only works in target specific code. Since > migration/migration.c is common code, the #ifdef does not have > the intended behavior here. Move the related code to a separate > file now which gets compiled via specific_ss instead. > > Fixes: 3710586caa ("qapi: Add VFIO devices migration stats in Migration stats") > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> althoguh I wonder if it would be easier to put the populate_vfio_info into hw/vfio and add a stub function? Dave > --- > migration/meson.build | 3 ++- > migration/migration.c | 15 --------------- > migration/migration.h | 2 ++ > migration/target.c | 25 +++++++++++++++++++++++++ > 4 files changed, 29 insertions(+), 16 deletions(-) > create mode 100644 migration/target.c > > diff --git a/migration/meson.build b/migration/meson.build > index 3ecedce94d..f8714dcb15 100644 > --- a/migration/meson.build > +++ b/migration/meson.build > @@ -31,4 +31,5 @@ softmmu_ss.add(when: ['CONFIG_RDMA', rdma], if_true: files('rdma.c')) > softmmu_ss.add(when: 'CONFIG_LIVE_BLOCK_MIGRATION', if_true: files('block.c')) > softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.c')) > > -specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c', 'ram.c')) > +specific_ss.add(when: 'CONFIG_SOFTMMU', > + if_true: files('dirtyrate.c', 'ram.c', 'target.c')) > diff --git a/migration/migration.c b/migration/migration.c > index 8ca034136b..db8c378079 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -60,10 +60,6 @@ > #include "qemu/yank.h" > #include "sysemu/cpus.h" > > -#ifdef CONFIG_VFIO > -#include "hw/vfio/vfio-common.h" > -#endif > - > #define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */ > > /* Amount of time to allocate to each "chunk" of bandwidth-throttled > @@ -1059,17 +1055,6 @@ static void populate_disk_info(MigrationInfo *info) > } > } > > -static void populate_vfio_info(MigrationInfo *info) > -{ > -#ifdef CONFIG_VFIO > - if (vfio_mig_active()) { > - info->has_vfio = true; > - info->vfio = g_malloc0(sizeof(*info->vfio)); > - info->vfio->transferred = vfio_mig_bytes_transferred(); > - } > -#endif > -} > - > static void fill_source_migration_info(MigrationInfo *info) > { > MigrationState *s = migrate_get_current(); > diff --git a/migration/migration.h b/migration/migration.h > index db6708326b..2730fa05c0 100644 > --- a/migration/migration.h > +++ b/migration/migration.h > @@ -376,4 +376,6 @@ void migration_make_urgent_request(void); > void migration_consume_urgent_request(void); > bool migration_rate_limit(void); > > +void populate_vfio_info(MigrationInfo *info); > + > #endif > diff --git a/migration/target.c b/migration/target.c > new file mode 100644 > index 0000000000..907ebf0a0a > --- /dev/null > +++ b/migration/target.c > @@ -0,0 +1,25 @@ > +/* > + * QEMU live migration - functions that need to be compiled target-specific > + * > + * This work is licensed under the terms of the GNU GPL, version 2 > + * or (at your option) any later version. > + */ > + > +#include "qemu/osdep.h" > +#include "qapi/qapi-types-migration.h" > +#include "migration.h" > + > +#ifdef CONFIG_VFIO > +#include "hw/vfio/vfio-common.h" > +#endif > + > +void populate_vfio_info(MigrationInfo *info) > +{ > +#ifdef CONFIG_VFIO > + if (vfio_mig_active()) { > + info->has_vfio = true; > + info->vfio = g_malloc0(sizeof(*info->vfio)); > + info->vfio->transferred = vfio_mig_bytes_transferred(); > + } > +#endif > +} > -- > 2.27.0 > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 4/14/21 1:20 PM, Thomas Huth wrote:
> The CONFIG_VFIO switch only works in target specific code. Since
> migration/migration.c is common code, the #ifdef does not have
> the intended behavior here. Move the related code to a separate
> file now which gets compiled via specific_ss instead.
>
> Fixes: 3710586caa ("qapi: Add VFIO devices migration stats in Migration stats")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> migration/meson.build | 3 ++-
> migration/migration.c | 15 ---------------
> migration/migration.h | 2 ++
> migration/target.c | 25 +++++++++++++++++++++++++
> 4 files changed, 29 insertions(+), 16 deletions(-)
> create mode 100644 migration/target.c
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Thomas Huth <thuth@redhat.com> writes: > The ObjectType enum and ObjectOptions are included from qapi-types-qom.h > into common code. We should not use target-specific config switches like > CONFIG_VIRTIO_CRYPTO here, since this is not defined in common code and > thus the enum will look differently between common and target specific > code. For this case, it's hopefully enough to check for CONFIG_VHOST_CRYPTO > only (which is a host specific config switch, i.e. it's the same on all > targets). Drawback: introspection now claims cryptodev-vhost-user is among the values of qom-type, which is a lie when !defined(CONFIG_VIRTIO_CRYPTO). Is this the first lie about QOM object types? Do we care? > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > qapi/qom.json | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/qapi/qom.json b/qapi/qom.json > index db5ac419b1..cd0e76d564 100644 > --- a/qapi/qom.json > +++ b/qapi/qom.json > @@ -752,7 +752,7 @@ > 'cryptodev-backend', > 'cryptodev-backend-builtin', > { 'name': 'cryptodev-vhost-user', > - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, > + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, > 'dbus-vmstate', > 'filter-buffer', > 'filter-dump', > @@ -809,7 +809,7 @@ > 'cryptodev-backend': 'CryptodevBackendProperties', > 'cryptodev-backend-builtin': 'CryptodevBackendProperties', > 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties', > - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, > + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, > 'dbus-vmstate': 'DBusVMStateProperties', > 'filter-buffer': 'FilterBufferProperties', > 'filter-dump': 'FilterDumpProperties', Could CryptodevVhostUserProperties be conditional, too?
On 4/14/21 1:20 PM, Thomas Huth wrote:
> We are already poisoning CONFIG_KVM since this switch is not working
> in common code. Do the same with the other accelerator switches, too
> (except for CONFIG_TCG, which is special, since it is also defined in
> config-host.h).
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> include/exec/poison.h | 4 ++++
> include/sysemu/hax.h | 4 ++++
> include/sysemu/hvf.h | 4 ++++
> include/sysemu/whpx.h | 4 ++++
> 4 files changed, 16 insertions(+)
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 14/04/2021 15.55, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> The ObjectType enum and ObjectOptions are included from qapi-types-qom.h >> into common code. We should not use target-specific config switches like >> CONFIG_VIRTIO_CRYPTO here, since this is not defined in common code and >> thus the enum will look differently between common and target specific >> code. For this case, it's hopefully enough to check for CONFIG_VHOST_CRYPTO >> only (which is a host specific config switch, i.e. it's the same on all >> targets). > > Drawback: introspection now claims cryptodev-vhost-user is among the > values of qom-type, which is a lie when !defined(CONFIG_VIRTIO_CRYPTO). > > Is this the first lie about QOM object types? > > Do we care? I don't think we really care, since there are other entries in the list which are obviously only available on certain targets or configurations, but not fenced with "if"s, e.g. s390-pv-guest, input-linux or rng-random. Or do you see a special problem with cryptodev-vhost-user here? >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> qapi/qom.json | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/qapi/qom.json b/qapi/qom.json >> index db5ac419b1..cd0e76d564 100644 >> --- a/qapi/qom.json >> +++ b/qapi/qom.json >> @@ -752,7 +752,7 @@ >> 'cryptodev-backend', >> 'cryptodev-backend-builtin', >> { 'name': 'cryptodev-vhost-user', >> - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, >> + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, >> 'dbus-vmstate', >> 'filter-buffer', >> 'filter-dump', >> @@ -809,7 +809,7 @@ >> 'cryptodev-backend': 'CryptodevBackendProperties', >> 'cryptodev-backend-builtin': 'CryptodevBackendProperties', >> 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties', >> - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, >> + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, >> 'dbus-vmstate': 'DBusVMStateProperties', >> 'filter-buffer': 'FilterBufferProperties', >> 'filter-dump': 'FilterDumpProperties', > > Could CryptodevVhostUserProperties be conditional, too? That's certainly a question for the QOM experts here... Thomas
Thomas Huth <thuth@redhat.com> writes: > On 14/04/2021 15.55, Markus Armbruster wrote: >> Thomas Huth <thuth@redhat.com> writes: >> >>> The ObjectType enum and ObjectOptions are included from qapi-types-qom.h >>> into common code. We should not use target-specific config switches like >>> CONFIG_VIRTIO_CRYPTO here, since this is not defined in common code and >>> thus the enum will look differently between common and target specific >>> code. For this case, it's hopefully enough to check for CONFIG_VHOST_CRYPTO >>> only (which is a host specific config switch, i.e. it's the same on all >>> targets). >> >> Drawback: introspection now claims cryptodev-vhost-user is among the >> values of qom-type, which is a lie when !defined(CONFIG_VIRTIO_CRYPTO). >> >> Is this the first lie about QOM object types? >> >> Do we care? > > I don't think we really care, since there are other entries in the list > which are obviously only available on certain targets or configurations, but > not fenced with "if"s, e.g. s390-pv-guest, input-linux or rng-random. So introspection already flawed, and adding another instance doesn't really make it worse. > Or do you see a special problem with cryptodev-vhost-user here? No, only the general problem that query-qmp-schema can't reliably tell us what QOM types are available. I see no need to revert the patch. >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> qapi/qom.json | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/qapi/qom.json b/qapi/qom.json >>> index db5ac419b1..cd0e76d564 100644 >>> --- a/qapi/qom.json >>> +++ b/qapi/qom.json >>> @@ -752,7 +752,7 @@ >>> 'cryptodev-backend', >>> 'cryptodev-backend-builtin', >>> { 'name': 'cryptodev-vhost-user', >>> - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, >>> + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, >>> 'dbus-vmstate', >>> 'filter-buffer', >>> 'filter-dump', >>> @@ -809,7 +809,7 @@ >>> 'cryptodev-backend': 'CryptodevBackendProperties', >>> 'cryptodev-backend-builtin': 'CryptodevBackendProperties', >>> 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties', >>> - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' }, >>> + 'if': 'defined(CONFIG_VHOST_CRYPTO)' }, >>> 'dbus-vmstate': 'DBusVMStateProperties', >>> 'filter-buffer': 'FilterBufferProperties', >>> 'filter-dump': 'FilterDumpProperties', >> >> Could CryptodevVhostUserProperties be conditional, too? > > That's certainly a question for the QOM experts here... Here's the expert's method to find out: slap on the conditional, compile with all targets enabled, see whether any of them explode. Mind to try?
On 15/04/2021 09.44, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> On 14/04/2021 15.55, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> The ObjectType enum and ObjectOptions are included from qapi-types-qom.h
>>>> into common code. We should not use target-specific config switches like
>>>> CONFIG_VIRTIO_CRYPTO here, since this is not defined in common code and
>>>> thus the enum will look differently between common and target specific
>>>> code. For this case, it's hopefully enough to check for CONFIG_VHOST_CRYPTO
>>>> only (which is a host specific config switch, i.e. it's the same on all
>>>> targets).
>>>
>>> Drawback: introspection now claims cryptodev-vhost-user is among the
>>> values of qom-type, which is a lie when !defined(CONFIG_VIRTIO_CRYPTO).
>>>
>>> Is this the first lie about QOM object types?
>>>
>>> Do we care?
>>
>> I don't think we really care, since there are other entries in the list
>> which are obviously only available on certain targets or configurations, but
>> not fenced with "if"s, e.g. s390-pv-guest, input-linux or rng-random.
>
> So introspection already flawed, and adding another instance doesn't
> really make it worse.
>
>> Or do you see a special problem with cryptodev-vhost-user here?
>
> No, only the general problem that query-qmp-schema can't reliably tell
> us what QOM types are available.
>
> I see no need to revert the patch.
>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> qapi/qom.json | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>> index db5ac419b1..cd0e76d564 100644
>>>> --- a/qapi/qom.json
>>>> +++ b/qapi/qom.json
>>>> @@ -752,7 +752,7 @@
>>>> 'cryptodev-backend',
>>>> 'cryptodev-backend-builtin',
>>>> { 'name': 'cryptodev-vhost-user',
>>>> - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
>>>> + 'if': 'defined(CONFIG_VHOST_CRYPTO)' },
>>>> 'dbus-vmstate',
>>>> 'filter-buffer',
>>>> 'filter-dump',
>>>> @@ -809,7 +809,7 @@
>>>> 'cryptodev-backend': 'CryptodevBackendProperties',
>>>> 'cryptodev-backend-builtin': 'CryptodevBackendProperties',
>>>> 'cryptodev-vhost-user': { 'type': 'CryptodevVhostUserProperties',
>>>> - 'if': 'defined(CONFIG_VIRTIO_CRYPTO) && defined(CONFIG_VHOST_CRYPTO)' },
>>>> + 'if': 'defined(CONFIG_VHOST_CRYPTO)' },
>>>> 'dbus-vmstate': 'DBusVMStateProperties',
>>>> 'filter-buffer': 'FilterBufferProperties',
>>>> 'filter-dump': 'FilterDumpProperties',
>>>
>>> Could CryptodevVhostUserProperties be conditional, too?
>>
>> That's certainly a question for the QOM experts here...
>
> Here's the expert's method to find out: slap on the conditional,
> compile with all targets enabled, see whether any of them explode.
>
> Mind to try?
Yes. I've currently got plenty of other stuff to do. So I'd prefer if you
could give it a try instead.
Thomas
+Richard/Claudio On 4/14/21 1:20 PM, Thomas Huth wrote: > We are generating a lot of target-specific defines in the *-config-devices.h > and *-config-target.h files. Using them in common code is wrong and leads > to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there > as expected. To avoid these issues, we are already poisoning many of the > macros in include/exec/poison.h - but it's cumbersome to maintain this > list manually. Thus let's generate an additional list of poisoned macros > automatically from the current config switches - this should give us a > much better test coverage via the different CI configurations. > > Note that CONFIG_TCG (which is also defined in config-host.h) and > CONFIG_USER_ONLY are special, so we have to filter these out. I know if we poison CONFIG_TCG, almost nothing build, but I fail to see how it is different from the other accelerators. I suppose in the future (maybe with Claudio's effort) we could have it not special. > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > Makefile | 2 +- > configure | 5 +++++ > include/exec/poison.h | 2 ++ > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/Makefile b/Makefile > index bcbbec71a1..4cab10a2a4 100644 > --- a/Makefile > +++ b/Makefile > @@ -213,7 +213,7 @@ qemu-%.tar.bz2: > > distclean: clean > -$(quiet-@)test -f build.ninja && $(NINJA) $(NINJAFLAGS) -t clean -g || : > - rm -f config-host.mak config-host.h* > + rm -f config-host.mak config-host.h* config-poison.h > rm -f tests/tcg/config-*.mak > rm -f config-all-disas.mak config.status > rm -f roms/seabios/config.mak roms/vgabios/config.mak > diff --git a/configure b/configure > index 4f374b4889..a0f0601e7e 100755 > --- a/configure > +++ b/configure > @@ -6440,6 +6440,11 @@ if test -n "${deprecated_features}"; then > echo " features: ${deprecated_features}" > fi > Maybe a one line comment (but since it is obvious, I don't mind): # Filter out CONFIG_TCG and CONFIG_USER_ONLY which are special > +sed -n -e '/CONFIG_TCG/d' -e '/CONFIG_USER_ONLY/d' \ > + -e '/^#define / { s///; s/ .*//; s/^/#pragma GCC poison /p; }' \ > + *-config-devices.h *-config-target.h | \ > + sort -u > config-poison.h > + Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
On 15/04/2021 10.34, Philippe Mathieu-Daudé wrote: > +Richard/Claudio > > On 4/14/21 1:20 PM, Thomas Huth wrote: >> We are generating a lot of target-specific defines in the *-config-devices.h >> and *-config-target.h files. Using them in common code is wrong and leads >> to very subtle bugs since a "#ifdef CONFIG_SOMETHING" is not working there >> as expected. To avoid these issues, we are already poisoning many of the >> macros in include/exec/poison.h - but it's cumbersome to maintain this >> list manually. Thus let's generate an additional list of poisoned macros >> automatically from the current config switches - this should give us a >> much better test coverage via the different CI configurations. >> >> Note that CONFIG_TCG (which is also defined in config-host.h) and >> CONFIG_USER_ONLY are special, so we have to filter these out. > > I know if we poison CONFIG_TCG, almost nothing build, but I fail to > see how it is different from the other accelerators. You could argue that TCG is not specific to a target, i.e. it is either available for all targets, or it is disabled for all targets, so this is rather like a host config option than a target specific option. >> diff --git a/configure b/configure >> index 4f374b4889..a0f0601e7e 100755 >> --- a/configure >> +++ b/configure >> @@ -6440,6 +6440,11 @@ if test -n "${deprecated_features}"; then >> echo " features: ${deprecated_features}" >> fi >> > > Maybe a one line comment (but since it is obvious, I don't mind): > > # Filter out CONFIG_TCG and CONFIG_USER_ONLY which are special Ok, makes sense, I can add that. >> +sed -n -e '/CONFIG_TCG/d' -e '/CONFIG_USER_ONLY/d' \ >> + -e '/^#define / { s///; s/ .*//; s/^/#pragma GCC poison /p; }' \ >> + *-config-devices.h *-config-target.h | \ >> + sort -u > config-poison.h >> + > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> Thanks! Thomas
On 14/04/2021 13.20, Thomas Huth wrote:
> The CONFIG_VFIO switch only works in target specific code. Since
> migration/migration.c is common code, the #ifdef does not have
> the intended behavior here. Move the related code to a separate
> file now which gets compiled via specific_ss instead.
>
> Fixes: 3710586caa ("qapi: Add VFIO devices migration stats in Migration stats")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> migration/meson.build | 3 ++-
> migration/migration.c | 15 ---------------
> migration/migration.h | 2 ++
> migration/target.c | 25 +++++++++++++++++++++++++
> 4 files changed, 29 insertions(+), 16 deletions(-)
> create mode 100644 migration/target.c
>
> diff --git a/migration/meson.build b/migration/meson.build
> index 3ecedce94d..f8714dcb15 100644
> --- a/migration/meson.build
> +++ b/migration/meson.build
> @@ -31,4 +31,5 @@ softmmu_ss.add(when: ['CONFIG_RDMA', rdma], if_true: files('rdma.c'))
> softmmu_ss.add(when: 'CONFIG_LIVE_BLOCK_MIGRATION', if_true: files('block.c'))
> softmmu_ss.add(when: zstd, if_true: files('multifd-zstd.c'))
>
> -specific_ss.add(when: 'CONFIG_SOFTMMU', if_true: files('dirtyrate.c', 'ram.c'))
> +specific_ss.add(when: 'CONFIG_SOFTMMU',
> + if_true: files('dirtyrate.c', 'ram.c', 'target.c'))
> diff --git a/migration/migration.c b/migration/migration.c
> index 8ca034136b..db8c378079 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -60,10 +60,6 @@
> #include "qemu/yank.h"
> #include "sysemu/cpus.h"
>
> -#ifdef CONFIG_VFIO
> -#include "hw/vfio/vfio-common.h"
> -#endif
> -
> #define MAX_THROTTLE (128 << 20) /* Migration transfer speed throttling */
>
> /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> @@ -1059,17 +1055,6 @@ static void populate_disk_info(MigrationInfo *info)
> }
> }
>
> -static void populate_vfio_info(MigrationInfo *info)
> -{
> -#ifdef CONFIG_VFIO
> - if (vfio_mig_active()) {
> - info->has_vfio = true;
> - info->vfio = g_malloc0(sizeof(*info->vfio));
> - info->vfio->transferred = vfio_mig_bytes_transferred();
> - }
> -#endif
> -}
> -
> static void fill_source_migration_info(MigrationInfo *info)
> {
> MigrationState *s = migrate_get_current();
> diff --git a/migration/migration.h b/migration/migration.h
> index db6708326b..2730fa05c0 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -376,4 +376,6 @@ void migration_make_urgent_request(void);
> void migration_consume_urgent_request(void);
> bool migration_rate_limit(void);
>
> +void populate_vfio_info(MigrationInfo *info);
> +
> #endif
> diff --git a/migration/target.c b/migration/target.c
> new file mode 100644
> index 0000000000..907ebf0a0a
> --- /dev/null
> +++ b/migration/target.c
> @@ -0,0 +1,25 @@
> +/*
> + * QEMU live migration - functions that need to be compiled target-specific
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2
> + * or (at your option) any later version.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/qapi-types-migration.h"
> +#include "migration.h"
> +
> +#ifdef CONFIG_VFIO
> +#include "hw/vfio/vfio-common.h"
> +#endif
> +
> +void populate_vfio_info(MigrationInfo *info)
> +{
> +#ifdef CONFIG_VFIO
> + if (vfio_mig_active()) {
> + info->has_vfio = true;
> + info->vfio = g_malloc0(sizeof(*info->vfio));
> + info->vfio->transferred = vfio_mig_bytes_transferred();
> + }
> +#endif
> +}
>
Ping!
David, Juan, any comments on this one?
Thomas