| 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: <20250521115217-mutt-send-email-mst@kernel.org>
Date: Wed, 21 May 2025 12:02:04 -0400
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org,
Willem de Bruijn <willemdebruijn.kernel@...il.com>,
Jason Wang <jasowang@...hat.com>,
Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Eugenio Pérez <eperezma@...hat.com>
Subject: Re: [PATCH net-next 1/8] virtio: introduce virtio_features_t
On Wed, May 21, 2025 at 12:32:35PM +0200, Paolo Abeni wrote:
> The virtio specifications allows for up to 128 bits for the
> device features. Soon we are going to use some of the 'extended'
> bits features (above 64) for the virtio_net driver.
>
> Introduce an specific type to represent the virtio features bitmask.
> On platform where 128 bits integer are available use such wide int
> for the features bitmask, otherwise maintain the current u64.
>
> Updates all the relevant virtio API to use the new type.
>
> Note that legacy and transport features don't need any change, as
> they are always in the low 64 bit range.
>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
> drivers/virtio/virtio.c | 12 ++++++------
> drivers/virtio/virtio_mmio.c | 4 ++--
> drivers/virtio/virtio_pci_legacy.c | 2 +-
> drivers/virtio/virtio_pci_modern.c | 7 ++++---
> drivers/virtio/virtio_pci_modern_dev.c | 13 ++++++-------
> drivers/virtio/virtio_vdpa.c | 2 +-
> include/linux/virtio.h | 5 +++--
> include/linux/virtio_config.h | 22 +++++++++++-----------
> include/linux/virtio_features.h | 23 +++++++++++++++++++++++
> include/linux/virtio_pci_modern.h | 11 ++++++++---
> 10 files changed, 65 insertions(+), 36 deletions(-)
> create mode 100644 include/linux/virtio_features.h
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 95d5d7993e5b1..542735d3a12ba 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -272,9 +272,9 @@ static int virtio_dev_probe(struct device *_d)
> int err, i;
> struct virtio_device *dev = dev_to_virtio(_d);
> struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
> - u64 device_features;
> - u64 driver_features;
> - u64 driver_features_legacy;
> + virtio_features_t device_features;
> + virtio_features_t driver_features;
> + virtio_features_t driver_features_legacy;
>
> /* We have a driver! */
> virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> @@ -286,8 +286,8 @@ static int virtio_dev_probe(struct device *_d)
> driver_features = 0;
> for (i = 0; i < drv->feature_table_size; i++) {
> unsigned int f = drv->feature_table[i];
> - BUG_ON(f >= 64);
> - driver_features |= (1ULL << f);
> + BUG_ON(f >= VIRTIO_FEATURES_MAX);
> + driver_features |= VIRTIO_BIT(f);
> }
>
> /* Some drivers have a separate feature table for virtio v1.0 */
> @@ -320,7 +320,7 @@ static int virtio_dev_probe(struct device *_d)
> goto err;
>
> if (drv->validate) {
> - u64 features = dev->features;
> + virtio_features_t features = dev->features;
>
> err = drv->validate(dev);
> if (err)
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 5d78c2d572abf..158c47ac67de7 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -106,10 +106,10 @@ struct virtio_mmio_vq_info {
>
> /* Configuration interface */
>
> -static u64 vm_get_features(struct virtio_device *vdev)
> +static virtio_features_t vm_get_features(struct virtio_device *vdev)
> {
> struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev);
> - u64 features;
> + virtio_features_t features;
>
> writel(1, vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES_SEL);
> features = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_FEATURES);
> diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c
> index d9cbb02b35a11..b2fbc74f74b5c 100644
> --- a/drivers/virtio/virtio_pci_legacy.c
> +++ b/drivers/virtio/virtio_pci_legacy.c
> @@ -18,7 +18,7 @@
> #include "virtio_pci_common.h"
>
> /* virtio config->get_features() implementation */
> -static u64 vp_get_features(struct virtio_device *vdev)
> +static virtio_features_t vp_get_features(struct virtio_device *vdev)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index d50fe030d8253..c3e0ddc7ae9ab 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -22,7 +22,7 @@
>
> #define VIRTIO_AVQ_SGS_MAX 4
>
> -static u64 vp_get_features(struct virtio_device *vdev)
> +static virtio_features_t vp_get_features(struct virtio_device *vdev)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>
> @@ -353,7 +353,8 @@ static void vp_modern_avq_cleanup(struct virtio_device *vdev)
> }
> }
>
> -static void vp_transport_features(struct virtio_device *vdev, u64 features)
> +static void vp_transport_features(struct virtio_device *vdev,
> + virtio_features_t features)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> struct pci_dev *pci_dev = vp_dev->pci_dev;
> @@ -409,7 +410,7 @@ static int vp_check_common_size(struct virtio_device *vdev)
> static int vp_finalize_features(struct virtio_device *vdev)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> - u64 features = vdev->features;
> + virtio_features_t features = vdev->features;
>
> /* Give virtio_ring a chance to accept features. */
> vring_transport_features(vdev);
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index 0d3dbfaf4b236..1d34655f6b658 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -393,11 +393,10 @@ EXPORT_SYMBOL_GPL(vp_modern_remove);
> *
> * Returns the features read from the device
> */
> -u64 vp_modern_get_features(struct virtio_pci_modern_device *mdev)
> +virtio_features_t vp_modern_get_features(struct virtio_pci_modern_device *mdev)
> {
> struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> -
> - u64 features;
> + virtio_features_t features;
>
> vp_iowrite32(0, &cfg->device_feature_select);
> features = vp_ioread32(&cfg->device_feature);
> @@ -414,11 +413,11 @@ EXPORT_SYMBOL_GPL(vp_modern_get_features);
> *
> * Returns the driver features read from the device
> */
> -u64 vp_modern_get_driver_features(struct virtio_pci_modern_device *mdev)
> +virtio_features_t
> +vp_modern_get_driver_features(struct virtio_pci_modern_device *mdev)
> {
> struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> -
> - u64 features;
> + virtio_features_t features;
>
> vp_iowrite32(0, &cfg->guest_feature_select);
> features = vp_ioread32(&cfg->guest_feature);
> @@ -435,7 +434,7 @@ EXPORT_SYMBOL_GPL(vp_modern_get_driver_features);
> * @features: the features set to device
> */
> void vp_modern_set_features(struct virtio_pci_modern_device *mdev,
> - u64 features)
> + virtio_features_t features)
> {
> struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
>
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index 1f60c9d5cb181..b92749174885e 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -409,7 +409,7 @@ static int virtio_vdpa_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> return err;
> }
>
> -static u64 virtio_vdpa_get_features(struct virtio_device *vdev)
> +static virtio_features_t virtio_vdpa_get_features(struct virtio_device *vdev)
> {
> struct vdpa_device *vdpa = vd_get_vdpa(vdev);
> const struct vdpa_config_ops *ops = vdpa->config;
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 64cb4b04be7ad..6e51400d04635 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -11,6 +11,7 @@
> #include <linux/gfp.h>
> #include <linux/dma-mapping.h>
> #include <linux/completion.h>
> +#include <linux/virtio_features.h>
>
> /**
> * struct virtqueue - a queue to register buffers for sending or receiving.
> @@ -159,11 +160,11 @@ struct virtio_device {
> const struct virtio_config_ops *config;
> const struct vringh_config_ops *vringh_config;
> struct list_head vqs;
> - u64 features;
> + virtio_features_t features;
> void *priv;
> #ifdef CONFIG_VIRTIO_DEBUG
> struct dentry *debugfs_dir;
> - u64 debugfs_filter_features;
> + virtio_features_t debugfs_filter_features;
> #endif
> };
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 169c7d367facb..bff57f675fca7 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -77,7 +77,7 @@ struct virtqueue_info {
> * vdev: the virtio_device
> * @get_features: get the array of feature bits for this device.
> * vdev: the virtio_device
> - * Returns the first 64 feature bits (all we currently need).
> + * Returns the first VIRTIO_FEATURES_MAX feature bits (all we currently need).
> * @finalize_features: confirm what device features we'll be using.
> * vdev: the virtio_device
> * This sends the driver feature bits to the device: it can change
> @@ -120,7 +120,7 @@ struct virtio_config_ops {
> struct irq_affinity *desc);
> void (*del_vqs)(struct virtio_device *);
> void (*synchronize_cbs)(struct virtio_device *);
> - u64 (*get_features)(struct virtio_device *vdev);
> + virtio_features_t (*get_features)(struct virtio_device *vdev);
> int (*finalize_features)(struct virtio_device *vdev);
> const char *(*bus_name)(struct virtio_device *vdev);
> int (*set_vq_affinity)(struct virtqueue *vq,
> @@ -149,11 +149,11 @@ static inline bool __virtio_test_bit(const struct virtio_device *vdev,
> {
> /* Did you forget to fix assumptions on max features? */
> if (__builtin_constant_p(fbit))
> - BUILD_BUG_ON(fbit >= 64);
> + BUILD_BUG_ON(fbit >= VIRTIO_FEATURES_MAX);
> else
> - BUG_ON(fbit >= 64);
> + BUG_ON(fbit >= VIRTIO_FEATURES_MAX);
>
> - return vdev->features & BIT_ULL(fbit);
> + return vdev->features & VIRTIO_BIT(fbit);
> }
>
> /**
> @@ -166,11 +166,11 @@ static inline void __virtio_set_bit(struct virtio_device *vdev,
> {
> /* Did you forget to fix assumptions on max features? */
> if (__builtin_constant_p(fbit))
> - BUILD_BUG_ON(fbit >= 64);
> + BUILD_BUG_ON(fbit >= VIRTIO_FEATURES_MAX);
> else
> - BUG_ON(fbit >= 64);
> + BUG_ON(fbit >= VIRTIO_FEATURES_MAX);
>
> - vdev->features |= BIT_ULL(fbit);
> + vdev->features |= VIRTIO_BIT(fbit);
> }
>
> /**
> @@ -183,11 +183,11 @@ static inline void __virtio_clear_bit(struct virtio_device *vdev,
> {
> /* Did you forget to fix assumptions on max features? */
> if (__builtin_constant_p(fbit))
> - BUILD_BUG_ON(fbit >= 64);
> + BUILD_BUG_ON(fbit >= VIRTIO_FEATURES_MAX);
> else
> - BUG_ON(fbit >= 64);
> + BUG_ON(fbit >= VIRTIO_FEATURES_MAX);
>
> - vdev->features &= ~BIT_ULL(fbit);
> + vdev->features &= ~VIRTIO_BIT(fbit);
> }
>
> /**
> diff --git a/include/linux/virtio_features.h b/include/linux/virtio_features.h
> new file mode 100644
> index 0000000000000..2f742eeb45a29
> --- /dev/null
> +++ b/include/linux/virtio_features.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_VIRTIO_FEATURES_H
> +#define _LINUX_VIRTIO_FEATURES_H
> +
> +#include <linux/bits.h>
> +
> +#if IS_ENABLED(CONFIG_ARCH_SUPPORTS_INT128)
> +#define VIRTIO_HAS_EXTENDED_FEATURES
> +#define VIRTIO_FEATURES_MAX 128
> +#define VIRTIO_FEATURES_WORDS 4
> +#define VIRTIO_BIT(b) _BIT128(b)
> +
> +typedef __uint128_t virtio_features_t;
Since we are doing it anyway, what about __bitwise ?
We used to have a lot of bugs where people would stick
a bit number where BIT_ULL would be appropriate. These are all
fixed by now, I presume, but sounds like a good thing to have?
I think the changes would be localized to this patch, since
everyone should going through these macros now.
> +
> +#else
> +#define VIRTIO_FEATURES_MAX 64
> +#define VIRTIO_FEATURES_WORDS 2
> +#define VIRTIO_BIT(b) BIT_ULL(b)
Hmm. We have
#define BIT_ULL(nr) (ULL(1) << (nr))
So this is undefined behaviour if given bit > 63.
How about
(nr > 63 ? 0 : BIT_ULL(b))
I think this will automatically make most code correct
on these platforms.
Some ceremony with a temp variable or an inline function
might be good here to avoid evaluating b twice.
> +
> +typedef u64 virtio_features_t;
> +#endif
> +
> +#endif
> diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> index c0b1b1ca11635..e55fbb272b4d3 100644
> --- a/include/linux/virtio_pci_modern.h
> +++ b/include/linux/virtio_pci_modern.h
> @@ -3,6 +3,7 @@
> #define _LINUX_VIRTIO_PCI_MODERN_H
>
> #include <linux/pci.h>
> +#include <linux/virtio_features.h>
> #include <linux/virtio_pci.h>
>
> /**
> @@ -95,10 +96,14 @@ static inline void vp_iowrite64_twopart(u64 val,
> vp_iowrite32(val >> 32, hi);
> }
>
> -u64 vp_modern_get_features(struct virtio_pci_modern_device *mdev);
> -u64 vp_modern_get_driver_features(struct virtio_pci_modern_device *mdev);
> +virtio_features_t
> +vp_modern_get_features(struct virtio_pci_modern_device *mdev);
> +
> +virtio_features_t
> +vp_modern_get_driver_features(struct virtio_pci_modern_device *mdev);
> +
> void vp_modern_set_features(struct virtio_pci_modern_device *mdev,
> - u64 features);
> + virtio_features_t features);
> u32 vp_modern_generation(struct virtio_pci_modern_device *mdev);
> u8 vp_modern_get_status(struct virtio_pci_modern_device *mdev);
> void vp_modern_set_status(struct virtio_pci_modern_device *mdev,
> --
> 2.49.0
Powered by blists - more mailing lists