| 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: <CACGkMEuNL+nLb2EvHWqSfKB6iiDScuKD6RJqxZ2-P9v5BDH1HA@mail.gmail.com>
Date: Thu, 12 Jun 2025 08:50:48 +0800
From: Jason Wang <jasowang@...hat.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Willem de Bruijn <willemdebruijn.kernel@...il.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>, Akihiko Odaki <akihiko.odaki@...nix.com>
Subject: Re: [PATCH RFC v3 1/8] virtio: introduce extended features
On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni <pabeni@...hat.com> 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 extended features as a fixed size array of u64. To minimize
> the diffstat allows legacy driver to access the low 64 bits via a
> transparent union.
>
> Introduce an extended get_extended_features128 configuration callback
> that devices supporting the extended features range must implement in
> place of the traditional one.
>
> 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>
> ---
> v2 -> v3:
> - uint128_t -> u64[2];
> v1 -> v2:
> - let u64 VIRTIO_BIT() cope with higher bit values
> - add .get_features128 instead of changing .get_features signature
> ---
> drivers/virtio/virtio.c | 39 +++++++++++-------
> drivers/virtio/virtio_debug.c | 27 +++++++------
> include/linux/virtio.h | 5 ++-
> include/linux/virtio_config.h | 35 ++++++++++++-----
> include/linux/virtio_features.h | 70 +++++++++++++++++++++++++++++++++
> 5 files changed, 137 insertions(+), 39 deletions(-)
> create mode 100644 include/linux/virtio_features.h
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 95d5d7993e5b..ed1ccedc47b3 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -53,7 +53,7 @@ static ssize_t features_show(struct device *_d,
>
> /* We actually represent this as a bitstring, as it could be
> * arbitrary length in future. */
> - for (i = 0; i < sizeof(dev->features)*8; i++)
> + for (i = 0; i < VIRTIO_FEATURES_MAX; i++)
> len += sysfs_emit_at(buf, len, "%c",
> __virtio_test_bit(dev, i) ? '1' : '0');
> len += sysfs_emit_at(buf, len, "\n");
> @@ -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 device_features[VIRTIO_FEATURES_DWORDS];
> + u64 driver_features[VIRTIO_FEATURES_DWORDS];
> u64 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);
> + virtio_get_features(dev, device_features);
>
> /* Figure out what features the driver supports. */
> - driver_features = 0;
> + virtio_features_zero(driver_features);
> 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);
> + virtio_features_set_bit(driver_features, f);
Instead of doing BUG_ON here, could we just stop at 128 bits?
> }
>
> /* Some drivers have a separate feature table for virtio v1.0 */
> @@ -299,20 +299,25 @@ static int virtio_dev_probe(struct device *_d)
> driver_features_legacy |= (1ULL << f);
> }
> } else {
> - driver_features_legacy = driver_features;
> + driver_features_legacy = driver_features[0];
> }
>
> - if (device_features & (1ULL << VIRTIO_F_VERSION_1))
> - dev->features = driver_features & device_features;
> - else
> - dev->features = driver_features_legacy & device_features;
> + if (virtio_features_test_bit(device_features, VIRTIO_F_VERSION_1)) {
> + for (i = 0; i < VIRTIO_FEATURES_DWORDS; ++i)
> + dev->features_array[i] = driver_features[i] &
> + device_features[i];
> + } else {
> + virtio_features_from_u64(dev->features_array,
> + driver_features_legacy &
> + device_features[0]);
> + }
>
> /* When debugging, user may filter some features by hand. */
> virtio_debug_device_filter_features(dev);
>
> /* Transport features always preserved to pass to finalize_features. */
> for (i = VIRTIO_TRANSPORT_F_START; i < VIRTIO_TRANSPORT_F_END; i++)
> - if (device_features & (1ULL << i))
> + if (virtio_features_test_bit(device_features, i))
> __virtio_set_bit(dev, i);
>
> err = dev->config->finalize_features(dev);
> @@ -320,14 +325,15 @@ static int virtio_dev_probe(struct device *_d)
> goto err;
>
> if (drv->validate) {
> - u64 features = dev->features;
> + u64 features[VIRTIO_FEATURES_DWORDS];
>
> + virtio_features_copy(features, dev->features_array);
> err = drv->validate(dev);
> if (err)
> goto err;
>
> /* Did validation change any features? Then write them again. */
> - if (features != dev->features) {
> + if (!virtio_features_equal(features, dev->features_array)) {
> err = dev->config->finalize_features(dev);
> if (err)
> goto err;
> @@ -701,6 +707,9 @@ EXPORT_SYMBOL_GPL(virtio_device_reset_done);
>
> static int virtio_init(void)
> {
> + BUILD_BUG_ON(offsetof(struct virtio_device, features) !=
> + offsetof(struct virtio_device, features_array[0]));
> +
> if (bus_register(&virtio_bus) != 0)
> panic("virtio bus registration failed");
> virtio_debug_init();
> diff --git a/drivers/virtio/virtio_debug.c b/drivers/virtio/virtio_debug.c
> index 95c8fc7705bb..6d066b5e8ec0 100644
> --- a/drivers/virtio/virtio_debug.c
> +++ b/drivers/virtio/virtio_debug.c
> @@ -8,13 +8,13 @@ static struct dentry *virtio_debugfs_dir;
>
> static int virtio_debug_device_features_show(struct seq_file *s, void *data)
> {
> + u64 device_features[VIRTIO_FEATURES_DWORDS];
> struct virtio_device *dev = s->private;
> - u64 device_features;
> unsigned int i;
>
> - device_features = dev->config->get_features(dev);
> - for (i = 0; i < BITS_PER_LONG_LONG; i++) {
> - if (device_features & (1ULL << i))
> + virtio_get_features(dev, device_features);
> + for (i = 0; i < VIRTIO_FEATURES_MAX; i++) {
> + if (virtio_features_test_bit(device_features, i))
> seq_printf(s, "%u\n", i);
> }
> return 0;
> @@ -26,8 +26,8 @@ static int virtio_debug_filter_features_show(struct seq_file *s, void *data)
> struct virtio_device *dev = s->private;
> unsigned int i;
>
> - for (i = 0; i < BITS_PER_LONG_LONG; i++) {
> - if (dev->debugfs_filter_features & (1ULL << i))
> + for (i = 0; i < VIRTIO_FEATURES_MAX; i++) {
> + if (virtio_features_test_bit(dev->debugfs_filter_features, i))
> seq_printf(s, "%u\n", i);
> }
> return 0;
> @@ -39,7 +39,7 @@ static int virtio_debug_filter_features_clear(void *data, u64 val)
> struct virtio_device *dev = data;
>
> if (val == 1)
> - dev->debugfs_filter_features = 0;
> + virtio_features_zero(dev->debugfs_filter_features);
> return 0;
> }
>
> @@ -50,9 +50,10 @@ static int virtio_debug_filter_feature_add(void *data, u64 val)
> {
> struct virtio_device *dev = data;
>
> - if (val >= BITS_PER_LONG_LONG)
> + if (val >= VIRTIO_FEATURES_MAX)
> return -EINVAL;
> - dev->debugfs_filter_features |= BIT_ULL_MASK(val);
> +
> + virtio_features_set_bit(dev->debugfs_filter_features, val);
> return 0;
> }
>
> @@ -63,9 +64,10 @@ static int virtio_debug_filter_feature_del(void *data, u64 val)
> {
> struct virtio_device *dev = data;
>
> - if (val >= BITS_PER_LONG_LONG)
> + if (val >= VIRTIO_FEATURES_MAX)
> return -EINVAL;
> - dev->debugfs_filter_features &= ~BIT_ULL_MASK(val);
> +
> + virtio_features_clear_bit(dev->debugfs_filter_features, val);
> return 0;
> }
>
> @@ -91,7 +93,8 @@ EXPORT_SYMBOL_GPL(virtio_debug_device_init);
>
> void virtio_debug_device_filter_features(struct virtio_device *dev)
> {
> - dev->features &= ~dev->debugfs_filter_features;
> + virtio_features_and_not(dev->features_array, dev->features_array,
> + dev->debugfs_filter_features);
> }
> EXPORT_SYMBOL_GPL(virtio_debug_device_filter_features);
>
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 64cb4b04be7a..dcd3949572bd 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_DECLARE_FEATURES(features);
> void *priv;
> #ifdef CONFIG_VIRTIO_DEBUG
> struct dentry *debugfs_dir;
> - u64 debugfs_filter_features;
> + u64 debugfs_filter_features[VIRTIO_FEATURES_DWORDS];
> #endif
> };
>
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 169c7d367fac..83cf25b3028d 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,8 @@ struct virtio_config_ops {
> void (*del_vqs)(struct virtio_device *);
> void (*synchronize_cbs)(struct virtio_device *);
> u64 (*get_features)(struct virtio_device *vdev);
> + void (*get_extended_features)(struct virtio_device *vdev,
> + u64 *features);
I think it would be better to add a size to simplify the future extension.
> int (*finalize_features)(struct virtio_device *vdev);
> const char *(*bus_name)(struct virtio_device *vdev);
> int (*set_vq_affinity)(struct virtqueue *vq,
> @@ -149,11 +154,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 virtio_features_test_bit(vdev->features_array, fbit);
> }
>
> /**
> @@ -166,11 +171,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);
> + virtio_features_set_bit(vdev->features_array, fbit);
> }
>
> /**
> @@ -183,11 +188,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);
> + virtio_features_clear_bit(vdev->features_array, fbit);
> }
>
> /**
> @@ -204,6 +209,16 @@ static inline bool virtio_has_feature(const struct virtio_device *vdev,
> return __virtio_test_bit(vdev, fbit);
> }
>
> +static inline void virtio_get_features(struct virtio_device *vdev, u64 *features)
> +{
> + if (vdev->config->get_extended_features) {
> + vdev->config->get_extended_features(vdev, features);
> + return;
> + }
> +
> + virtio_features_from_u64(features, 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..42c3c7cee500
> --- /dev/null
> +++ b/include/linux/virtio_features.h
> @@ -0,0 +1,70 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_VIRTIO_FEATURES_H
> +#define _LINUX_VIRTIO_FEATURES_H
> +
> +#include <linux/bits.h>
> +
> +#define VIRTIO_FEATURES_DWORDS 2
> +#define VIRTIO_FEATURES_MAX (VIRTIO_FEATURES_DWORDS * 64)
> +#define VIRTIO_FEATURES_WORDS (VIRTIO_FEATURES_DWORDS * 2)
> +#define VIRTIO_BIT(b) BIT_ULL((b) & 0x3f)
> +#define VIRTIO_DWORD(b) ((b) >> 6)
> +#define VIRTIO_DECLARE_FEATURES(name) \
> + union { \
> + u64 name; \
> + u64 name##_array[VIRTIO_FEATURES_DWORDS];\
> + }
> +
> +static inline bool virtio_features_test_bit(const u64 *features,
> + unsigned int bit)
> +{
> + return !!(features[VIRTIO_DWORD(bit)] & VIRTIO_BIT(bit));
> +}
> +
> +static inline void virtio_features_set_bit(u64 *features,
> + unsigned int bit)
> +{
> + features[VIRTIO_DWORD(bit)] |= VIRTIO_BIT(bit);
> +}
> +
> +static inline void virtio_features_clear_bit(u64 *features,
> + unsigned int bit)
> +{
> + features[VIRTIO_DWORD(bit)] &= ~VIRTIO_BIT(bit);
> +}
> +
> +static inline void virtio_features_zero(u64 *features)
> +{
> + memset(features, 0, sizeof(features[0]) * VIRTIO_FEATURES_DWORDS);
> +}
> +
> +static inline void virtio_features_from_u64(u64 *features, u64 from)
> +{
> + virtio_features_zero(features);
> + features[0] = from;
> +}
> +
> +static inline bool virtio_features_equal(const u64 *f1, const u64 *f2)
> +{
> + u64 diff = 0;
> + int i;
> +
> + for (i = 0; i < VIRTIO_FEATURES_DWORDS; ++i)
> + diff |= f1[i] ^ f2[i];
> + return !!diff;
Nit: we can return false early here.
> +}
> +
> +static inline void virtio_features_copy(u64 *to, const u64 *from)
> +{
> + memcpy(to, from, sizeof(to[0]) * VIRTIO_FEATURES_DWORDS);
> +}
> +
> +static inline void virtio_features_and_not(u64 *to, const u64 *f1, const u64 *f2)
> +{
> + int i;
> +
> + for (i = 0; i < VIRTIO_FEATURES_DWORDS; i++)
> + to[i] = f1[i] & ~f2[i];
> +}
> +
> +#endif
> --
> 2.49.0
Others look good to me.
Thanks
>
Powered by blists - more mailing lists