On Wed, Jul 23, 2025 at 12:55 AM Paolo Abeni <pab...@redhat.com> wrote:
>
> On 7/22/25 5:32 AM, Jason Wang wrote:
> > On Fri, Jul 18, 2025 at 4:53 PM Paolo Abeni <pab...@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 <pab...@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?
Yes.
>
> 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.
I'm fine not having that then.
>
> Please LMK if you have strong opinion for a different choice.
>
> Thanks,
>
> Paolo
Thanks
>
