Thread (37 messages) 37 messages, 3 authors, 2025-06-17

Re: [PATCH RFC v3 3/8] vhost-net: allow configuring extended features

From: Akihiko Odaki <hidden>
Date: 2025-06-08 06:16:59

On 2025/06/06 20:45, Paolo Abeni wrote:
quoted hunk ↗ jump to hunk
Use the extended feature type for 'acked_features' and implement
two new ioctls operation allowing the user-space to set/query an
unbounded amount of features.

The actual number of processed features is limited by VIRTIO_FEATURES_MAX
and attempts to set features above such limit fail with
EOPNOTSUPP.

Note that: the legacy ioctls implicitly truncate the negotiated
features to the lower 64 bits range and the 'acked_backend_features'
field don't need conversion, as the only negotiated feature there
is in the low 64 bit range.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v2 -> v3:
   - virtio_features_t -> u64[2]
   - add __counted_by annotation to vhost_features_array

v1 -> v2:
   - change the ioctl to use an extensible API
---
  drivers/vhost/net.c              | 85 +++++++++++++++++++++++++++-----
  drivers/vhost/vhost.c            |  2 +-
  drivers/vhost/vhost.h            |  4 +-
  include/uapi/linux/vhost.h       |  7 +++
  include/uapi/linux/vhost_types.h |  5 ++
  5 files changed, 88 insertions(+), 15 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 7cbfc7d718b3..0291fce24bbf 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -77,6 +77,8 @@ enum {
  			 (1ULL << VIRTIO_F_RING_RESET)
  };
  
+const u64 VHOST_NET_ALL_FEATURES[VIRTIO_FEATURES_DWORDS] = { VHOST_NET_FEATURES };
This should have static.

Probably it should be lower-case too. 
Documentation/process/coding-style.rst says: "Names of macros defining 
constants and labels in enums are capitalized". Note that variables are 
not named here.

I think it's also better to remove the definition of VHOST_NET_FEATURES 
since having two definitions with similar names and meaning is 
confusing. (Just in case you wonder: GCC is able to optimize accesses 
like "VHOST_NET_ALL_FEATURES[0]" to eliminate array accesses, by the way.)
quoted hunk ↗ jump to hunk
+
  enum {
  	VHOST_NET_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
  };
@@ -1614,16 +1616,17 @@ 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, const u64 *features)
  {
  	size_t vhost_hlen, sock_hlen, hdr_len;
  	int i;
  
-	hdr_len = (features & ((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
-			       (1ULL << VIRTIO_F_VERSION_1))) ?
-			sizeof(struct virtio_net_hdr_mrg_rxbuf) :
-			sizeof(struct virtio_net_hdr);
-	if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
+	hdr_len = virtio_features_test_bit(features, VIRTIO_NET_F_MRG_RXBUF) ||
+		  virtio_features_test_bit(features, VIRTIO_F_VERSION_1) ?
+		  sizeof(struct virtio_net_hdr_mrg_rxbuf) :
+		  sizeof(struct virtio_net_hdr);
+
+	if (virtio_features_test_bit(features, VHOST_NET_F_VIRTIO_NET_HDR)) {
  		/* vhost provides vnet_hdr */
  		vhost_hlen = hdr_len;
  		sock_hlen = 0;
@@ -1633,18 +1636,19 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
  		sock_hlen = hdr_len;
  	}
  	mutex_lock(&n->dev.mutex);
-	if ((features & (1 << VHOST_F_LOG_ALL)) &&
+	if (virtio_features_test_bit(features, VHOST_F_LOG_ALL) &&
  	    !vhost_log_access_ok(&n->dev))
  		goto out_unlock;
  
-	if ((features & (1ULL << VIRTIO_F_ACCESS_PLATFORM))) {
+	if (virtio_features_test_bit(features, VIRTIO_F_ACCESS_PLATFORM)) {
  		if (vhost_init_device_iotlb(&n->dev))
  			goto out_unlock;
  	}
  
  	for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
  		mutex_lock(&n->vqs[i].vq.mutex);
-		n->vqs[i].vq.acked_features = features;
+		virtio_features_copy(n->vqs[i].vq.acked_features_array,
+				     features);
  		n->vqs[i].vhost_hlen = vhost_hlen;
  		n->vqs[i].sock_hlen = sock_hlen;
  		mutex_unlock(&n->vqs[i].vq.mutex);
@@ -1681,12 +1685,13 @@ static long vhost_net_set_owner(struct vhost_net *n)
  static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
  			    unsigned long arg)
  {
+	u64 all_features[VIRTIO_FEATURES_DWORDS];
  	struct vhost_net *n = f->private_data;
  	void __user *argp = (void __user *)arg;
  	u64 __user *featurep = argp;
  	struct vhost_vring_file backend;
-	u64 features;
-	int r;
+	u64 features, count;
+	int r, i;
  
  	switch (ioctl) {
  	case VHOST_NET_SET_BACKEND:
@@ -1703,7 +1708,63 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
  			return -EFAULT;
  		if (features & ~VHOST_NET_FEATURES)
  			return -EOPNOTSUPP;
-		return vhost_net_set_features(n, features);
+
+		virtio_features_from_u64(all_features, features);
+		return vhost_net_set_features(n, all_features);
+	case VHOST_GET_FEATURES_ARRAY:
+	{
+		if (copy_from_user(&count, argp, sizeof(u64)))
+			return -EFAULT;
+
+		/* Copy the net features, up to the user-provided buffer size */
+		virtio_features_copy(all_features, VHOST_NET_ALL_FEATURES);
+		argp += sizeof(u64);
+		for (i = 0; i < min(count, VIRTIO_FEATURES_DWORDS); ++i) {
+			i = array_index_nospec(i, VIRTIO_FEATURES_DWORDS);
+			if (copy_to_user(argp, &all_features[i], sizeof(u64)))
+				return -EFAULT;
+
+			argp += sizeof(u64);
+		}
Simpler:

copy_to_user(argp, all_features, min(count, VIRTIO_FEATURES_DWORDS) * 
sizeof(u64));
+
+		/* Zero the trailing space provided by user-space, if any */
+		if (i < count && clear_user(argp, (count - i) * sizeof(u64)))
I think checking i < count is a premature optimization; it doesn't 
matter even if we spend a bit longer because of the lack of the check.
+			return -EFAULT;
+		return 0;
+	}
+	case VHOST_SET_FEATURES_ARRAY:
+	{
+		u64 tmp[VIRTIO_FEATURES_DWORDS];
+
+		if (copy_from_user(&count, argp, sizeof(u64)))
+			return -EFAULT;
+
+		virtio_features_zero(all_features);
+		for (i = 0; i < min(count, VIRTIO_FEATURES_DWORDS); ++i) {
+			argp += sizeof(u64);
+			if (copy_from_user(&features, argp, sizeof(u64)))
+				return -EFAULT;
+
+			all_features[i] = features;
+		}
+
+		/* Any feature specified by user-space above VIRTIO_FEATURES_MAX is
+		 * not supported by definition.
+		 */
+		for (; i < count; ++i) {
+			if (copy_from_user(&features, argp, sizeof(u64)))
+				return -EFAULT;
+			if (features)
+				return -EOPNOTSUPP;
+		}
+
+		virtio_features_and_not(tmp, all_features, VHOST_NET_ALL_FEATURES);
+		for (i = 0; i < VIRTIO_FEATURES_DWORDS; i++)
+			if (tmp[i])
I think using virtio_features_and_not() helps much. Instead, we can 
check all_features[i] & ~VHOST_NET_ALL_FEATURES[i] here, allowing to 
remove the tmp array.

Regards,
Akihiko Odaki
quoted hunk ↗ jump to hunk
+				return -EOPNOTSUPP;> +
+		return vhost_net_set_features(n, all_features);
+	}
  	case VHOST_GET_BACKEND_FEATURES:
  		features = VHOST_NET_BACKEND_FEATURES;
  		if (copy_to_user(featurep, &features, sizeof(features)))
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 63612faeab72..6d3b9f0a9163 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -372,7 +372,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
  	vq->log_used = false;
  	vq->log_addr = -1ull;
  	vq->private_data = NULL;
-	vq->acked_features = 0;
+	virtio_features_zero(vq->acked_features_array);
  	vq->acked_backend_features = 0;
  	vq->log_base = NULL;
  	vq->error_ctx = NULL;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..d1aed35c4b07 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -133,7 +133,7 @@ struct vhost_virtqueue {
  	struct vhost_iotlb *umem;
  	struct vhost_iotlb *iotlb;
  	void *private_data;
-	u64 acked_features;
+	VIRTIO_DECLARE_FEATURES(acked_features);
  	u64 acked_backend_features;
  	/* Log write descriptors */
  	void __user *log_base;
@@ -291,7 +291,7 @@ static inline void *vhost_vq_get_backend(struct vhost_virtqueue *vq)
  
  static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
  {
-	return vq->acked_features & (1ULL << bit);
+	return virtio_features_test_bit(vq->acked_features_array, bit);
  }
  
  static inline bool vhost_backend_has_feature(struct vhost_virtqueue *vq, int bit)
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index d4b3e2ae1314..d6ad01fbb8d2 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,11 @@
   */
  #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
  					      struct vhost_vring_state)
+
+/* Extended features manipulation */
+#define VHOST_GET_FEATURES_ARRAY _IOR(VHOST_VIRTIO, 0x83, \
+				       struct vhost_features_array)
+#define VHOST_SET_FEATURES_ARRAY _IOW(VHOST_VIRTIO, 0x83, \
+				       struct vhost_features_array)
+
  #endif
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h
index d7656908f730..1c39cc5f5a31 100644
--- a/include/uapi/linux/vhost_types.h
+++ b/include/uapi/linux/vhost_types.h
@@ -110,6 +110,11 @@ struct vhost_msg_v2 {
  	};
  };
  
+struct vhost_features_array {
+	__u64 count; /* number of entries present in features array */
+	__u64 features[] __counted_by(count);
+};
+
  struct vhost_memory_region {
  	__u64 guest_phys_addr;
  	__u64 memory_size; /* bytes */
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help