Re: [PATCH net-next 1/8] virtio: introduce virtio_features_t

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

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@redhat.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