Message-ID: <da18286e-c518-4f6b-99bb-13b52df3555f@daynix.com>
Date: Sat, 31 May 2025 14:37:56 +0900
From: Akihiko Odaki <akihiko.odaki@...nix.com>
To: Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc: 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>, "Michael S. Tsirkin" <mst@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>, Yuri Benditovich <yuri.benditovich@...nix.com>
Subject: Re: [RFC PATCH v2 1/8] virtio: introduce virtio_features_t

On 2025/05/30 23:49, 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.
> 
> Introduce an extended get_features128() configuration callback that
> devices supporting the extended features range must implement in
> place of the traditional one.

The callback is called get_extended_features() in the code, which makes 
more sense as it is actually 64-bit if CONFIG_ARCH_SUPPORTS_INT128 is 
not defined.

That said, it doesn't seem that the code saved by the use of 128-bit 
integer type is significant enough.

I can think of three strategies to support more features:
1) Converting bitmasks to 128-bit, which is this patch does
2) Convert both "features" and "debugfs_filter_features" of the struct
    into bitmaps provided by include/linux/bitmap.h

2) requires more changes, but perhaps there may be a middle ground.

Looking at details, there are two fields that contain feature bitmasks 
in the common structure, struct virtio_device:
- features
- debugfs_filter_features

Fortunately, "debugfs_filter_features" is only referred by
drivers/virtio/virtio_debug.c. The problem is "features", which is 
referred everywhere. Perhaps converting all of them into bitmaps may 
make sense in a long term, but it may be something you want to avoid 
with this patch series.

So there is the following middle-ground option:

3) Adding e.g., "features_hi" for the upper 64-bit of features
    (which is similar to what I suggested for QEMU*),
    and convert only "debugfs_filter_features" into a bitmap.

This substantially reduces the code change required for "features" and 
make them contained in the following three functions:
- virtio_dev_probe() in drivers/virtio/virtio.c
- features_show() in drivers/virtio/virtio.c
- virtio_debug_device_filter_features() in drivers/virtio/virtio_debug.c

These functions can use bitmaps internally, and convert them from/into 
64-bit integers using bitmap_from_arr64() and bitmap_to_arr64().

Since you are adding the support for extended features to 
virtio_pci_modern and virtio_net, you'll later need to change their 
implementations too, but that won't add complexity much; some complexity 
is inevitable even when choosing 1) and "[RFC PATCH v2 2/8] 
virtio_pci_modern: allow configuring extended features" already include it.

And by choosing 3), you can remove the check of 
CONFIG_ARCH_SUPPORTS_INT128 and avoid the need to test platforms without 
the config. I suspect particularly eliminating the need of test 
outweighs the cost of the additional change required for 3).

Regards,
Akihiko Odaki

* 
https://lore.kernel.org/qemu-devel/473516b5-d52b-4671-aeca-d02ad1940364@daynix.com/

> 
> 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>
> ---
> v1 -> v2:
>    - let u64 VIRTIO_BIT() cope with higher bit values
>    - add .get_extended_features instead of changing .get_features signature
> ---
>   drivers/virtio/virtio.c         | 14 +++++++-------
>   drivers/virtio/virtio_debug.c   |  2 +-
>   include/linux/virtio.h          |  5 +++--
>   include/linux/virtio_config.h   | 32 ++++++++++++++++++++++----------
>   include/linux/virtio_features.h | 27 +++++++++++++++++++++++++++
>   5 files changed, 60 insertions(+), 20 deletions(-)
>   create mode 100644 include/linux/virtio_features.h
> 
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 95d5d7993e5b..206ae8fa0654 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -272,22 +272,22 @@ 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);
>   
>   	/* Figure out what features the device supports. */
> -	device_features = dev->config->get_features(dev);
> +	device_features = virtio_get_features(dev);
>   
>   	/* Figure out what features the driver supports. */
>   	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_debug.c b/drivers/virtio/virtio_debug.c
> index 95c8fc7705bb..5ca95422d3ca 100644
> --- a/drivers/virtio/virtio_debug.c
> +++ b/drivers/virtio/virtio_debug.c
> @@ -12,7 +12,7 @@ static int virtio_debug_device_features_show(struct seq_file *s, void *data)
>   	u64 device_features;
>   	unsigned int i;
>   
> -	device_features = dev->config->get_features(dev);
> +	device_features = virtio_get_features(dev);
>   	for (i = 0; i < BITS_PER_LONG_LONG; i++) {
>   		if (device_features & (1ULL << i))
>   			seq_printf(s, "%u\n", i);
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 64cb4b04be7a..6e51400d0463 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 169c7d367fac..1cc43d9cf6e8 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -77,7 +77,10 @@ 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 64 feature bits.
> + * @get_extended_features:
> + *      vdev: the virtio_device
> + *      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
> @@ -121,6 +124,7 @@ struct virtio_config_ops {
>   	void (*del_vqs)(struct virtio_device *);
>   	void (*synchronize_cbs)(struct virtio_device *);
>   	u64 (*get_features)(struct virtio_device *vdev);
> +	virtio_features_t (*get_extended_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 +153,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 +170,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 +187,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);
>   }
>   
>   /**
> @@ -204,6 +208,14 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
>   	return __virtio_test_bit(vdev, fbit);
>   }
>   
> +static inline virtio_features_t virtio_get_features(struct virtio_device *vdev)
> +{
> +	if (vdev->config->get_extended_features)
> +		return vdev->config->get_extended_features(vdev);
> +
> +	return vdev->config->get_features(vdev);
> +}
> +
>   /**
>    * virtio_has_dma_quirk - determine whether this device has the DMA quirk
>    * @vdev: the device
> diff --git a/include/linux/virtio_features.h b/include/linux/virtio_features.h
> new file mode 100644
> index 000000000000..0a7e2265f8b4
> --- /dev/null
> +++ b/include/linux/virtio_features.h
> @@ -0,0 +1,27 @@
> +/* 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;
> +
> +#else
> +#define VIRTIO_FEATURES_MAX	64
> +#define VIRTIO_FEATURES_WORDS	2
> +
> +static inline u64 VIRTIO_BIT(int bit)
> +{
> +	return bit >= 64 ? 0 : BIT_ULL(b);
> +}
> +
> +typedef u64 virtio_features_t;
> +#endif
> +
> +#endif

Powered by blists - more mailing lists