On Thu, May 29, 2025 at 7:10 PM Paolo Abeni [off-list ref] wrote:

On 5/27/25 5:56 AM, Jason Wang wrote:

quoted

On Mon, May 26, 2025 at 6:57 PM Paolo Abeni [off-list ref] wrote:

quoted

On 5/26/25 2:47 AM, Jason Wang wrote:

quoted

On Wed, May 21, 2025 at 6:33 PM Paolo Abeni [off-list ref] wrote:

quoted

Use the extended feature type for 'acked_features' and implement
two new ioctls operation to get and set the extended features.

Note that the legacy ioctls implicitly truncate the negotiated
features to the lower 64 bits range.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 drivers/vhost/net.c        | 26 +++++++++++++++++++++++++-
 drivers/vhost/vhost.h      |  2 +-
 include/uapi/linux/vhost.h |  8 ++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 7cbfc7d718b3f..b894685dded3e 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c

@@ -77,6 +77,10 @@ enum {
                         (1ULL << VIRTIO_F_RING_RESET)
 };

+#ifdef VIRTIO_HAS_EXTENDED_FEATURES
+#define VHOST_NET_FEATURES_EX VHOST_NET_FEATURES
+#endif
+
 enum {
        VHOST_NET_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
 };

@@ -1614,7 +1618,7 @@ static long vhost_net_reset_owner(struct vhost_net *n)
        return err;
 }

-static int vhost_net_set_features(struct vhost_net *n, u64 features)
+static int vhost_net_set_features(struct vhost_net *n, virtio_features_t features)
 {
        size_t vhost_hlen, sock_hlen, hdr_len;
        int i;

@@ -1704,6 +1708,26 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
                if (features & ~VHOST_NET_FEATURES)
                        return -EOPNOTSUPP;
                return vhost_net_set_features(n, features);
+#ifdef VIRTIO_HAS_EXTENDED_FEATURES

Vhost doesn't depend on virtio. But this invents a dependency, and I
don't understand why we need to do that.

What do you mean with "dependency" here? vhost has already a build
dependency vs virtio, including several virtio headers. It has also a
logical dependency, using several virtio features.

Do you mean a build dependency? this change does not introduce such a thing.

I mean vhost can be built without virtio drivers. So old vhost can run
new virtio drivers on top. So I don't see why vhost needs to check if
virtio of the same source tree supports 128 bit or not.

We can just accept an array of features now as

1) the changes are limited to vhost so it wouldn't be too much
2) we don't have to have VHOST_GET_FEATURES_EX2 in the future.

AFAICS the ioctl() interface code wise only impacts on the device
implementing extended features support, I guess it could be changed to
to something alike:

struct vhost_virtio_features {
        __u64 count;
        __u64 features[];
};

#define VHOST_GET_FEATURES_VECTOR _IOR(VHOST_VIRTIO, 0x83, struct
vhost_virtio_features)
#define VHOST_SET_FEATURES_VECTOR _IOW(VHOST_VIRTIO, 0x83, struct
vhost_virtio_features)

I could drop the above #ifdef, and the implementation would copy in/out
only the known/supported number of features.

WDYT?

This looks good.

Thanks

/P