The current interface where caller has to know in which 64 bit chunk
each bit is, is inelegant and fragile.
Let's simply use arrays of bits.
By using unroll macros text size grows only slightly.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 29 +++++++++++++++--------------
drivers/vhost/scsi.c | 9 ++++++---
drivers/vhost/test.c | 10 ++++++++--
drivers/vhost/vhost.h | 42 ++++++++++++++++++++++++++++++++++--------
drivers/vhost/vsock.c | 10 ++++++----
5 files changed, 69 insertions(+), 31 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index d057ea55f5ad..cb778f2bf8f8 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -69,15 +69,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
-static const u64 vhost_net_features[VIRTIO_FEATURES_U64S] = {
- VHOST_FEATURES |
- (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
- (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
- (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
- (1ULL << VIRTIO_F_RING_RESET) |
- (1ULL << VIRTIO_F_IN_ORDER),
- VIRTIO_BIT(VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO) |
- VIRTIO_BIT(VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO),
+static const int vhost_net_features[] = {
+ VHOST_FEATURES,
+ VHOST_NET_F_VIRTIO_NET_HDR,
+ VIRTIO_NET_F_MRG_RXBUF,
+ VIRTIO_F_ACCESS_PLATFORM,
+ VIRTIO_F_RING_RESET,
+ VIRTIO_F_IN_ORDER,
+ VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO,
+ VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO
};
enum {
@@ -1720,6 +1720,7 @@ static long vhost_net_set_owner(struct vhost_net *n)
static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
unsigned long arg)
{
+ const DEFINE_VHOST_FEATURES_ARRAY(features_array, vhost_net_features);
u64 all_features[VIRTIO_FEATURES_U64S];
struct vhost_net *n = f->private_data;
void __user *argp = (void __user *)arg;
@@ -1734,14 +1735,14 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
return -EFAULT;
return vhost_net_set_backend(n, backend.index, backend.fd);
case VHOST_GET_FEATURES:
- features = vhost_net_features[0];
+ features = VHOST_FEATURES_U64(vhost_net_features, 0);
if (copy_to_user(featurep, &features, sizeof features))
return -EFAULT;
return 0;
case VHOST_SET_FEATURES:
if (copy_from_user(&features, featurep, sizeof features))
return -EFAULT;
- if (features & ~vhost_net_features[0])
+ if (features & ~VHOST_FEATURES_U64(vhost_net_features, 0))
return -EOPNOTSUPP;
virtio_features_from_u64(all_features, features);
@@ -1753,8 +1754,8 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
/* Copy the net features, up to the user-provided buffer size */
argp += sizeof(u64);
copied = min(count, (u64)VIRTIO_FEATURES_U64S);
- if (copy_to_user(argp, vhost_net_features,
- copied * sizeof(u64)))
+
+ if (copy_to_user(argp, features_array, copied * sizeof(u64)))
return -EFAULT;
/* Zero the trailing space provided by user-space, if any */
@@ -1784,7 +1785,7 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
}
for (i = 0; i < VIRTIO_FEATURES_U64S; i++)
- if (all_features[i] & ~vhost_net_features[i])
+ if (all_features[i] & ~VHOST_FEATURES_U64(vhost_net_features, i))
return -EOPNOTSUPP;
return vhost_net_set_features(n, all_features);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 98e4f68f4e3c..04fcbe7efd77 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -197,11 +197,14 @@ enum {
};
/* Note: can't set VIRTIO_F_VERSION_1 yet, since that implies ANY_LAYOUT. */
-enum {
- VHOST_SCSI_FEATURES = VHOST_FEATURES | (1ULL << VIRTIO_SCSI_F_HOTPLUG) |
- (1ULL << VIRTIO_SCSI_F_T10_PI)
+static const int vhost_scsi_features[] = {
+ VHOST_FEATURES,
+ VIRTIO_SCSI_F_HOTPLUG,
+ VIRTIO_SCSI_F_T10_PI
};
+#define VHOST_SCSI_FEATURES VHOST_FEATURES_U64(vhost_scsi_features, 0)
+
#define VHOST_SCSI_MAX_TARGET 256
#define VHOST_SCSI_MAX_IO_VQ 1024
#define VHOST_SCSI_MAX_EVENT 128
diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
index 42c955a5b211..af727fccfe40 100644
--- a/drivers/vhost/test.c
+++ b/drivers/vhost/test.c
@@ -308,6 +308,12 @@ static long vhost_test_set_backend(struct vhost_test *n, unsigned index, int fd)
return r;
}
+static const int vhost_test_features[] = {
+ VHOST_FEATURES
+};
+
+#define VHOST_TEST_FEATURES VHOST_FEATURES_U64(vhost_test_features, 0)
+
static long vhost_test_ioctl(struct file *f, unsigned int ioctl,
unsigned long arg)
{
@@ -328,14 +334,14 @@ static long vhost_test_ioctl(struct file *f, unsigned int ioctl,
return -EFAULT;
return vhost_test_set_backend(n, backend.index, backend.fd);
case VHOST_GET_FEATURES:
- features = VHOST_FEATURES;
+ features = VHOST_TEST_FEATURES;
if (copy_to_user(featurep, &features, sizeof features))
return -EFAULT;
return 0;
case VHOST_SET_FEATURES:
if (copy_from_user(&features, featurep, sizeof features))
return -EFAULT;
- if (features & ~VHOST_FEATURES)
+ if (features & ~VHOST_TEST_FEATURES)
return -EOPNOTSUPP;
return vhost_test_set_features(n, features);
case VHOST_RESET_OWNER:
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 621a6d9a8791..c7b92730668e 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -14,6 +14,7 @@
#include <linux/atomic.h>
#include <linux/vhost_iotlb.h>
#include <linux/irqbypass.h>
+#include <linux/unroll.h>
struct vhost_work;
struct vhost_task;
@@ -279,14 +280,39 @@ void vhost_iotlb_map_free(struct vhost_iotlb *iotlb,
eventfd_signal((vq)->error_ctx);\
} while (0)
-enum {
- VHOST_FEATURES = (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
- (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
- (1ULL << VIRTIO_RING_F_EVENT_IDX) |
- (1ULL << VHOST_F_LOG_ALL) |
- (1ULL << VIRTIO_F_ANY_LAYOUT) |
- (1ULL << VIRTIO_F_VERSION_1)
-};
+#define VHOST_FEATURES \
+ VIRTIO_F_NOTIFY_ON_EMPTY, \
+ VIRTIO_RING_F_INDIRECT_DESC, \
+ VIRTIO_RING_F_EVENT_IDX, \
+ VHOST_F_LOG_ALL, \
+ VIRTIO_F_ANY_LAYOUT, \
+ VIRTIO_F_VERSION_1
+
+static inline u64 vhost_features_u64(const int *features, int size, int idx)
+{
+ u64 res = 0;
+
+ unrolled_count(VIRTIO_FEATURES_BITS)
+ for (int i = 0; i < size; ++i) {
+ int bit = features[i];
+
+ if (virtio_features_chk_bit(bit) && VIRTIO_U64(bit) == idx)
+ res |= VIRTIO_BIT(bit);
+ }
+ return res;
+}
+
+#define VHOST_FEATURES_U64(features, idx) \
+ vhost_features_u64(features, ARRAY_SIZE(features), idx)
+
+#define DEFINE_VHOST_FEATURES_ARRAY_ENTRY(idx, features) \
+ [idx] = VHOST_FEATURES_U64(features, idx),
+
+#define DEFINE_VHOST_FEATURES_ARRAY(array, features) \
+ u64 array[VIRTIO_FEATURES_U64S] = { \
+ UNROLL(VIRTIO_FEATURES_U64S, \
+ DEFINE_VHOST_FEATURES_ARRAY_ENTRY, features) \
+ }
/**
* vhost_vq_set_backend - Set backend.
diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index ae01457ea2cd..16662f2b87c1 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -29,12 +29,14 @@
*/
#define VHOST_VSOCK_PKT_WEIGHT 256
-enum {
- VHOST_VSOCK_FEATURES = VHOST_FEATURES |
- (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
- (1ULL << VIRTIO_VSOCK_F_SEQPACKET)
+static const int vhost_vsock_features[] = {
+ VHOST_FEATURES,
+ VIRTIO_F_ACCESS_PLATFORM,
+ VIRTIO_VSOCK_F_SEQPACKET
};
+#define VHOST_VSOCK_FEATURES VHOST_FEATURES_U64(vhost_vsock_features, 0)
+
enum {
VHOST_VSOCK_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
};
--
MSTOn 11/19/25 7:55 AM, Michael S. Tsirkin wrote:
> @@ -1720,6 +1720,7 @@ static long vhost_net_set_owner(struct vhost_net *n)
> static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> unsigned long arg)
> {
> + const DEFINE_VHOST_FEATURES_ARRAY(features_array, vhost_net_features);
I'm sorry for the late feedback, I was drowning in other stuff.
I have just a couple of non blocking suggestions, feel free to ignore.
I think that if you rename `vhost_net_features` as
`vhost_net_features_bits` and `features_array` as `vhost_net_features`
the diffstat could be smaller and possibly clearer.
> u64 all_features[VIRTIO_FEATURES_U64S];
> struct vhost_net *n = f->private_data;
> void __user *argp = (void __user *)arg;
> @@ -1734,14 +1735,14 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> return -EFAULT;
> return vhost_net_set_backend(n, backend.index, backend.fd);
> case VHOST_GET_FEATURES:
> - features = vhost_net_features[0];
> + features = VHOST_FEATURES_U64(vhost_net_features, 0);
Here and below you could use directly:
features = features_array[0];
if you apply the rename mentioned above, this chunk and the following 3
should not be needed.
[...]> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> index 42c955a5b211..af727fccfe40 100644
> --- a/drivers/vhost/test.c
> +++ b/drivers/vhost/test.c
> @@ -308,6 +308,12 @@ static long vhost_test_set_backend(struct vhost_test *n, unsigned index, int fd)
> return r;
> }
>
> +static const int vhost_test_features[] = {
> + VHOST_FEATURES
> +};
> +
> +#define VHOST_TEST_FEATURES VHOST_FEATURES_U64(vhost_test_features, 0)
If you rename `VHOST_FEATURES` to `VHOST_FEATURES_BITS` and
`VHOST_TEST_FEATURES` to `VHOST_FEATURES`, the following two chunks
should not be needed.
Thanks,
Paolo
On Wed, Nov 19, 2025 at 12:04:12PM +0100, Paolo Abeni wrote:
> On 11/19/25 7:55 AM, Michael S. Tsirkin wrote:
> > @@ -1720,6 +1720,7 @@ static long vhost_net_set_owner(struct vhost_net *n)
> > static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> > unsigned long arg)
> > {
> > + const DEFINE_VHOST_FEATURES_ARRAY(features_array, vhost_net_features);
>
> I'm sorry for the late feedback, I was drowning in other stuff.
>
> I have just a couple of non blocking suggestions, feel free to ignore.
Oh this is really nice.
I did exactly this and the diff is smaller while the compiler
was smart enough to figure it out and the generated code is the same.
Thanks!
> I think that if you rename `vhost_net_features` as
> `vhost_net_features_bits` and `features_array` as `vhost_net_features`
> the diffstat could be smaller and possibly clearer.
>
> > u64 all_features[VIRTIO_FEATURES_U64S];
> > struct vhost_net *n = f->private_data;
> > void __user *argp = (void __user *)arg;
> > @@ -1734,14 +1735,14 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> > return -EFAULT;
> > return vhost_net_set_backend(n, backend.index, backend.fd);
> > case VHOST_GET_FEATURES:
> > - features = vhost_net_features[0];
> > + features = VHOST_FEATURES_U64(vhost_net_features, 0);
>
> Here and below you could use directly:
>
> features = features_array[0];
>
> if you apply the rename mentioned above, this chunk and the following 3
> should not be needed.
>
> [...]> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > index 42c955a5b211..af727fccfe40 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -308,6 +308,12 @@ static long vhost_test_set_backend(struct vhost_test *n, unsigned index, int fd)
> > return r;
> > }
> >
> > +static const int vhost_test_features[] = {
> > + VHOST_FEATURES
> > +};
> > +
> > +#define VHOST_TEST_FEATURES VHOST_FEATURES_U64(vhost_test_features, 0)
>
> If you rename `VHOST_FEATURES` to `VHOST_FEATURES_BITS` and
> `VHOST_TEST_FEATURES` to `VHOST_FEATURES`, the following two chunks
> should not be needed.
>
> Thanks,
>
> Paolo
On Wed, Nov 19, 2025 at 12:04:12PM +0100, Paolo Abeni wrote:
> On 11/19/25 7:55 AM, Michael S. Tsirkin wrote:
> > @@ -1720,6 +1720,7 @@ static long vhost_net_set_owner(struct vhost_net *n)
> > static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> > unsigned long arg)
> > {
> > + const DEFINE_VHOST_FEATURES_ARRAY(features_array, vhost_net_features);
>
> I'm sorry for the late feedback, I was drowning in other stuff.
>
> I have just a couple of non blocking suggestions, feel free to ignore.
>
> I think that if you rename `vhost_net_features` as
> `vhost_net_features_bits` and `features_array` as `vhost_net_features`
> the diffstat could be smaller and possibly clearer.
>
> > u64 all_features[VIRTIO_FEATURES_U64S];
> > struct vhost_net *n = f->private_data;
> > void __user *argp = (void __user *)arg;
> > @@ -1734,14 +1735,14 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
> > return -EFAULT;
> > return vhost_net_set_backend(n, backend.index, backend.fd);
> > case VHOST_GET_FEATURES:
> > - features = vhost_net_features[0];
> > + features = VHOST_FEATURES_U64(vhost_net_features, 0);
>
> Here and below you could use directly:
>
> features = features_array[0];
Ah. Good point.
> if you apply the rename mentioned above, this chunk and the following 3
> should not be needed.
>
> [...]> diff --git a/drivers/vhost/test.c b/drivers/vhost/test.c
> > index 42c955a5b211..af727fccfe40 100644
> > --- a/drivers/vhost/test.c
> > +++ b/drivers/vhost/test.c
> > @@ -308,6 +308,12 @@ static long vhost_test_set_backend(struct vhost_test *n, unsigned index, int fd)
> > return r;
> > }
> >
> > +static const int vhost_test_features[] = {
> > + VHOST_FEATURES
> > +};
> > +
> > +#define VHOST_TEST_FEATURES VHOST_FEATURES_U64(vhost_test_features, 0)
>
> If you rename `VHOST_FEATURES` to `VHOST_FEATURES_BITS` and
> `VHOST_TEST_FEATURES` to `VHOST_FEATURES`,
This one I don't want to do, people tend to copy/paste code
and this is not what devices should be doing.
> the following two chunks
> should not be needed.
>
> Thanks,
>
> Paolo
© 2016 - 2025 Red Hat, Inc.