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

[Paolo Abeni <pabeni@redhat.com>]

On 7/21/25 4:53 AM, Lei Yang wrote:
> On Fri, Jul 18, 2025 at 10:37 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>> On Fri, Jul 18, 2025 at 10:52:33AM +0200, Paolo Abeni 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);
>>> +
> 
> Hi Paolo
> 
>>> +    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;
>>> +    }
> 
> As we discussed in version 2, this code should be changed to: [1],
> otherwise the problem mentioned last time will occur when compiling.
> 
> [1] if (virtio_features_use_extended(features) &&
>          !dev->vhost_ops->vhost_set_features_ex) {
> -        VHOST_OPS_DEBUG(r, "extended features without device support");
>          r = -EINVAL;
> +        VHOST_OPS_DEBUG(r, "extended features without device support");
>          goto out;
>      }

Yes, I'm sorry, something went wrong here. Likely ENOCOFFEE or similar.
Will hopefully address for good in the next revision.

Thanks,

Paolo