| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251119012322-mutt-send-email-mst@kernel.org>
Date: Wed, 19 Nov 2025 01:26:17 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: linux-kernel@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
Paolo Abeni <pabeni@...hat.com>,
Eugenio Pérez <eperezma@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Jonathan Corbet <corbet@....net>, kvm@...r.kernel.org,
virtualization@...ts.linux.dev, netdev@...r.kernel.org,
linux-doc@...r.kernel.org,
Mike Christie <michael.christie@...cle.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Stefan Hajnoczi <stefanha@...hat.com>,
Stefano Garzarella <sgarzare@...hat.com>
Subject: Re: [PATCH v4 2/2] vhost: switch to arrays of feature bits
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@...hat.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@...hat.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
Powered by blists - more mailing lists