Re: [PATCH RFC v3 07/13] vhost: add support for negotiating extended features

[Paolo Abeni <pabeni@redhat.com>]

On 7/22/25 5:32 AM, Jason Wang wrote:
> On Fri, Jul 18, 2025 at 4:53 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> Similar to virtio infra, vhost core maintains the features status
>> in the full extended format and allows the devices to implement
>> extended version of the getter/setter.
>>
>> Note that 'protocol_features' are not extended: they are only
>> used by vhost-user, and the latter device is not going to implement
>> extended features soon.
>>
>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> ---
>> v2 -> v3:
>>   - fix compile warning
>>   - _array -> _ex
>>
>> v1 -> v2:
>>   - uint128_t -> uint64_t[]
>>   - add _ex() variant of features manipulation helpers
>> ---
>>  hw/virtio/vhost.c                 | 73 +++++++++++++++++++++++++++----
>>  include/hw/virtio/vhost-backend.h |  6 +++
>>  include/hw/virtio/vhost.h         | 33 ++++++++++++--
>>  3 files changed, 100 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index c30ea1156e..85ae1e4d4c 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -972,20 +972,34 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
>>  static int vhost_dev_set_features(struct vhost_dev *dev,
>>                                    bool enable_log)
>>  {
>> -    uint64_t features = dev->acked_features;
>> +    uint64_t features[VIRTIO_FEATURES_DWORDS];
>>      int r;
>> +
>> +    virtio_features_copy(features, dev->acked_features_ex);
>>      if (enable_log) {
>> -        features |= 0x1ULL << VHOST_F_LOG_ALL;
>> +        virtio_add_feature_ex(features, VHOST_F_LOG_ALL);
>>      }
>>      if (!vhost_dev_has_iommu(dev)) {
>> -        features &= ~(0x1ULL << VIRTIO_F_IOMMU_PLATFORM);
>> +        virtio_clear_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
>>      }
>>      if (dev->vhost_ops->vhost_force_iommu) {
>>          if (dev->vhost_ops->vhost_force_iommu(dev) == true) {
>> -            features |= 0x1ULL << VIRTIO_F_IOMMU_PLATFORM;
>> +            virtio_add_feature_ex(features, VIRTIO_F_IOMMU_PLATFORM);
>>         }
>>      }
>> -    r = dev->vhost_ops->vhost_set_features(dev, features);
>> +
>> +    if (virtio_features_use_extended(features) &&
>> +        !dev->vhost_ops->vhost_set_features_ex) {
>> +        VHOST_OPS_DEBUG(r, "extended features without device support");
>> +        r = -EINVAL;
>> +        goto out;
>> +    }
>> +
>> +    if (dev->vhost_ops->vhost_set_features_ex) {
>> +        r = dev->vhost_ops->vhost_set_features_ex(dev, features);
>> +    } else {
>> +        r = dev->vhost_ops->vhost_set_features(dev, features[0]);
>> +    }
>>      if (r < 0) {
>>          VHOST_OPS_DEBUG(r, "vhost_set_features failed");
>>          goto out;
>> @@ -1506,12 +1520,27 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
>>      }
>>  }
>>
>> +static int vhost_dev_get_features(struct vhost_dev *hdev,
>> +                                  uint64_t *features)
>> +{
>> +    uint64_t features64;
>> +    int r;
>> +
>> +    if (hdev->vhost_ops->vhost_get_features_ex) {
>> +        return hdev->vhost_ops->vhost_get_features_ex(hdev, features);
>> +    }
>> +
>> +    r = hdev->vhost_ops->vhost_get_features(hdev, &features64);
>> +    virtio_features_from_u64(features, features64);
>> +    return r;
>> +}
> 
> Nit: let's have a vhost_dev_set_features() as well?

I guess you mean to factor out the
vhost_set_features_ex()/vhost_set_features() in a specific helper am I
correct?

Note that there is already a vhost_dev_set_features() function. It's
touched by the previous chunk. I opted for not creating the mentioned
helper to avoid some weird naming issues, as such helper would not lead
to any code deduplication.

Please LMK if you have strong opinion for a different choice.

Thanks,

Paolo