Re: [PATCH v4 2/2] vhost: switch to arrays of feature bits

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Nov 19, 2025 at 10:03:58AM +0800, Jason Wang wrote:
> On Sun, Nov 16, 2025 at 3:45 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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   | 34 +++++++++++++++++++---------------
> >  drivers/vhost/scsi.c  |  9 ++++++---
> >  drivers/vhost/test.c  | 10 ++++++++--
> >  drivers/vhost/vhost.h | 42 ++++++++++++++++++++++++++++++++++--------
> >  drivers/vhost/vsock.c | 10 ++++++----
> >  5 files changed, 73 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index d057ea55f5ad..00d00034a97e 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 {
> > @@ -1734,14 +1734,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,9 +1753,13 @@ 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)))
> > -                       return -EFAULT;
> > +
> > +               {
> > +                       const DEFINE_VHOST_FEATURES_ARRAY(features, vhost_net_features);
> > +
> > +                       if (copy_to_user(argp, features, copied * sizeof(u64)))
> > +                               return -EFAULT;
> > +               }
> 
> Any reason to use a standalone block here?

Just so we can name the variable "features", as well as
having the declaration stand out a bit more.


> >
> >                 /* Zero the trailing space provided by user-space, if any */
> >                 if (clear_user(argp, size_mul(count - copied, sizeof(u64))))
> > @@ -1784,7 +1788,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..d8f1af9a0ff1 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)
> > +{
> > +       unsigned long res = 0;
> 
> Should this be u64?

Ugh. Yes.

> > +
> > +       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)
> >  };
> > --
> > MST
> >
> 
> Thanks