[PATCH for-6.1 0/4] Poison more CONFIG switches

[PATCH for-6.1 0/4] Poison more CONFIG switches

[Thomas Huth]

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

[PATCH for-6.1 1/4] include/sysemu: Poison all accelerator CONFIG switches in common code

[Thomas Huth]

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

Re: [PATCH for-6.1 1/4] include/sysemu: Poison all accelerator CONFIG switches in common code

[Philippe Mathieu-Daudé]

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>

[PATCH for-6.1 2/4] migration: Move populate_vfio_info() into a separate file

[Thomas Huth]

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

Re: [PATCH for-6.1 2/4] migration: Move populate_vfio_info() into a separate file

[Dr. David Alan Gilbert]

* 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

Re: [PATCH for-6.1 2/4] migration: Move populate_vfio_info() into a separate file

[Philippe Mathieu-Daudé]

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>

Re: [PATCH for-6.1 2/4] migration: Move populate_vfio_info() into a separate file

[Thomas Huth]

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

[PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code

[Thomas Huth]

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

Re: [PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code

[Markus Armbruster]

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?

Re: [PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code

[Thomas Huth]

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

Re: [PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code

[Markus Armbruster]

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?

Re: [PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code

[Thomas Huth]

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

[PATCH for-6.1 4/4] configure: Poison all current target-specific #defines

[Thomas Huth]

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

Re: [PATCH for-6.1 4/4] configure: Poison all current target-specific #defines

[Philippe Mathieu-Daudé]

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

Re: [PATCH for-6.1 4/4] configure: Poison all current target-specific #defines

[Thomas Huth]

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