Re: [PATCH RFC v3 08/13] qmp: update virtio features map to support extended features

[Paolo Abeni <pabeni@redhat.com>]

On 7/19/25 8:57 AM, Markus Armbruster wrote:
> Paolo Abeni <pabeni@redhat.com> writes:
> 
>> Extend the VirtioDeviceFeatures struct with an additional u64
>> to track unknown features in the 64-127 bit range and decode
>> the full virtio features spaces for vhost and virtio devices.
>>
>> Also add entries for the soon-to-be-supported virtio net GSO over
>> UDP features.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> [...]
> 
>> diff --git a/hw/virtio/virtio-qmp.c b/hw/virtio/virtio-qmp.c
>> index 3b6377cf0d..03c6163cf4 100644
>> --- a/hw/virtio/virtio-qmp.c
>> +++ b/hw/virtio/virtio-qmp.c
> 
> [...]
> 
>> @@ -680,9 +715,10 @@ VirtioDeviceFeatures *qmp_decode_features(uint16_t device_id, uint64_t bitmap)
>>          g_assert_not_reached();
>>      }
>>  
>> -    features->has_unknown_dev_features = bitmap != 0;
>> +    features->has_unknown_dev_features = !virtio_features_empty(bitmap);
>>      if (features->has_unknown_dev_features) {
>> -        features->unknown_dev_features = bitmap;
>> +        features->unknown_dev_features = bitmap[0];
>> +        features->unknown_dev_features2 = bitmap[1];
>>      }
> 
> Why not assign unconditionally?

I stuck with the old/previous code style. I can move to unconditional
assignment in the next revision.

>>      return features;
> 
> [...]
> 
>> diff --git a/qapi/virtio.json b/qapi/virtio.json
>> index 9d652fe4a8..f2e2dd6e97 100644
>> --- a/qapi/virtio.json
>> +++ b/qapi/virtio.json
>> @@ -490,14 +490,18 @@
>>  #     unique features)
>>  #
>>  # @unknown-dev-features: Virtio device features bitmap that have not
>> -#     been decoded
>> +#     been decoded (bits 0-63)
>> +#
>> +# @unknown-dev-features2: Virtio device features bitmap that have not
>> +#     been decoded (bits 64-127)
>>  #
>>  # Since: 7.2
>>  ##
>>  { 'struct': 'VirtioDeviceFeatures',
>>    'data': { 'transports': [ 'str' ],
>>              '*dev-features': [ 'str' ],
>> -            '*unknown-dev-features': 'uint64' } }
>> +            '*unknown-dev-features': 'uint64',
>> +            '*unknown-dev-features2': 'uint64' } }
>>  
>>  ##
>>  # @VirtQueueStatus:
> 
> I wish we could simply widen @unknown-dev-features, but we don't have
> uint128, and adding it would risk breaking QMP clients.  64 bit integers
> are already troublesome in JSON.
> 
> Does the example in x-query-virtio-status's doc comment need an update?

Yes, you are right, the output for the 'unknown features' field will
include leading zeros for the high 64 bits.

I will update it in the next revision, thanks!

Paolo