On Fri, Jul 18, 2025 at 10:37 PM Stefano Garzarella <sgarz...@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 <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);
> >+
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;
}
Thanks
Lei
> >+
> >+ 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;
> >+}
> >+
> > int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> > VhostBackendType backend_type, uint32_t busyloop_timeout,
> > Error **errp)
> > {
> >+ uint64_t features[VIRTIO_FEATURES_DWORDS];
> > unsigned int used, reserved, limit;
> >- uint64_t features;
> > int i, r, n_initialized_vqs = 0;
> >
> > hdev->vdev = NULL;
> >@@ -1531,7 +1560,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> >*opaque,
> > goto fail;
> > }
> >
> >- r = hdev->vhost_ops->vhost_get_features(hdev, &features);
> >+ r = vhost_dev_get_features(hdev, features);
> > if (r < 0) {
> > error_setg_errno(errp, -r, "vhost_get_features failed");
> > goto fail;
> >@@ -1569,7 +1598,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> >*opaque,
> > }
> > }
> >
> >- hdev->features = features;
> >+ virtio_features_copy(hdev->features_ex, features);
> >
> > hdev->memory_listener = (MemoryListener) {
> > .name = "vhost",
> >@@ -1592,7 +1621,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> >*opaque,
> > };
> >
> > if (hdev->migration_blocker == NULL) {
> >- if (!(hdev->features & (0x1ULL << VHOST_F_LOG_ALL))) {
> >+ if (!virtio_has_feature_ex(hdev->features_ex, VHOST_F_LOG_ALL)) {
> > error_setg(&hdev->migration_blocker,
> > "Migration disabled: vhost lacks VHOST_F_LOG_ALL
> > feature.");
> > } else if (vhost_dev_log_is_shared(hdev) &&
> > !qemu_memfd_alloc_check()) {
> >@@ -1871,6 +1900,20 @@ uint64_t vhost_get_features(struct vhost_dev *hdev,
> >const int *feature_bits,
> > return features;
> > }
> >
> >+void vhost_get_features_ex(struct vhost_dev *hdev,
> >+ const int *feature_bits,
> >+ uint64_t *features)
> >+{
> >+ const int *bit = feature_bits;
> >+
> >+ while (*bit != VHOST_INVALID_FEATURE_BIT) {
> >+ if (!virtio_has_feature_ex(hdev->features_ex, *bit)) {
> >+ virtio_clear_feature_ex(features, *bit);
> >+ }
> >+ bit++;
> >+ }
> >+}
> >+
>
> Can we do something similar of what we do in hw/virtio/virtio.c where
> the old virtio_set_features() use the new virtio_set_features_ex()?
>
> > void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> > uint64_t features)
> > {
> >@@ -1884,6 +1927,18 @@ void vhost_ack_features(struct vhost_dev *hdev, const
> >int *feature_bits,
> > }
> > }
> >
> >+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
> >+ const uint64_t *features)
> >+{
> >+ const int *bit = feature_bits;
> >+ while (*bit != VHOST_INVALID_FEATURE_BIT) {
> >+ if (virtio_has_feature_ex(features, *bit)) {
> >+ virtio_add_feature_ex(hdev->acked_features_ex, *bit);
> >+ }
> >+ bit++;
> >+ }
> >+}
> >+
>
> Ditto.
>
> Not a strong opinion, but just to reduce code duplication.
>
> Thanks,
> Stefano
>
> > int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> > uint32_t config_len, Error **errp)
> > {
> >diff --git a/include/hw/virtio/vhost-backend.h
> >b/include/hw/virtio/vhost-backend.h
> >index d6df209a2f..ff94fa1734 100644
> >--- a/include/hw/virtio/vhost-backend.h
> >+++ b/include/hw/virtio/vhost-backend.h
> >@@ -95,6 +95,10 @@ typedef int (*vhost_new_worker_op)(struct vhost_dev *dev,
> > struct vhost_worker_state *worker);
> > typedef int (*vhost_free_worker_op)(struct vhost_dev *dev,
> > struct vhost_worker_state *worker);
> >+typedef int (*vhost_set_features_ex_op)(struct vhost_dev *dev,
> >+ const uint64_t *features);
> >+typedef int (*vhost_get_features_ex_op)(struct vhost_dev *dev,
> >+ uint64_t *features);
> > typedef int (*vhost_set_features_op)(struct vhost_dev *dev,
> > uint64_t features);
> > typedef int (*vhost_get_features_op)(struct vhost_dev *dev,
> >@@ -186,6 +190,8 @@ typedef struct VhostOps {
> > vhost_free_worker_op vhost_free_worker;
> > vhost_get_vring_worker_op vhost_get_vring_worker;
> > vhost_attach_vring_worker_op vhost_attach_vring_worker;
> >+ vhost_set_features_ex_op vhost_set_features_ex;
> >+ vhost_get_features_ex_op vhost_get_features_ex;
> > vhost_set_features_op vhost_set_features;
> > vhost_get_features_op vhost_get_features;
> > vhost_set_backend_cap_op vhost_set_backend_cap;
> >diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >index 66be6afc88..39fbffc6bc 100644
> >--- a/include/hw/virtio/vhost.h
> >+++ b/include/hw/virtio/vhost.h
> >@@ -107,9 +107,9 @@ struct vhost_dev {
> > * future use should be discouraged and the variable retired as
> > * its easy to confuse with the VirtIO backend_features.
> > */
> >- uint64_t features;
> >- uint64_t acked_features;
> >- uint64_t backend_features;
> >+ VIRTIO_DECLARE_FEATURES(features);
> >+ VIRTIO_DECLARE_FEATURES(acked_features);
> >+ VIRTIO_DECLARE_FEATURES(backend_features);
> >
> > /**
> > * @protocol_features: is the vhost-user only feature set by
> >@@ -333,6 +333,21 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> >VirtIODevice *vdev, int n,
> > uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
> > uint64_t features);
> >
> >+/**
> >+ * vhost_get_features_ex() - sanitize the extended features set
> >+ * @hdev: common vhost_dev structure
> >+ * @feature_bits: pointer to terminated table of feature bits
> >+ * @features: original features set, filtered out on return
> >+ *
> >+ * This is the extended variant of vhost_get_features(), supporting the
> >+ * the extended features set. Filter it with the intersection of what is
> >+ * supported by the vhost backend (hdev->features) and the supported
> >+ * feature_bits.
> >+ */
> >+void vhost_get_features_ex(struct vhost_dev *hdev,
> >+ const int *feature_bits,
> >+ uint64_t *features);
> >+
> > /**
> > * vhost_ack_features() - set vhost acked_features
> > * @hdev: common vhost_dev structure
> >@@ -344,6 +359,18 @@ uint64_t vhost_get_features(struct vhost_dev *hdev,
> >const int *feature_bits,
> > */
> > void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> > uint64_t features);
> >+
> >+/**
> >+ * vhost_ack_features_ex() - set vhost full set of acked_features
> >+ * @hdev: common vhost_dev structure
> >+ * @feature_bits: pointer to terminated table of feature bits
> >+ * @features: requested feature set
> >+ *
> >+ * This sets the internal hdev->acked_features to the intersection of
> >+ * the backends advertised features and the supported feature_bits.
> >+ */
> >+void vhost_ack_features_ex(struct vhost_dev *hdev, const int *feature_bits,
> >+ const uint64_t *features);
> > unsigned int vhost_get_max_memslots(void);
> > unsigned int vhost_get_free_memslots(void);
> >
> >--
> >2.50.0
> >
>
>
