1. Patchew
  2. QEMU
  3. [PATCH for-6.1 0/4] Poison more CONFIG switches
  4. View patch

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

Thomas Huth posted 4 patches 4 years, 10 months ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>, Sunil Muthuswamy <sunilmut@microsoft.com>, Colin Xu <colin.xu@intel.com>, Cameron Esfahani <dirty@apple.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Wenchao Wang <wenchao.wang@intel.com>, Juan Quintela <quintela@redhat.com>, Roman Bolshakov <r.bolshakov@yadro.com>
[PATCH for-6.1 3/4] qapi/qom.json: Do not use CONFIG_VIRTIO_CRYPTO in common code
Posted by Thomas Huth 4 years, 10 months ago 
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
Posted by Markus Armbruster 4 years, 10 months ago 
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
Posted by Thomas Huth 4 years, 9 months ago 
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
Posted by Markus Armbruster 4 years, 9 months ago 
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
Posted by Thomas Huth 4 years, 9 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.