| 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: <fa55e26b-54f7-400f-88f7-530f3a95a0e9@redhat.com>
Date: Thu, 22 May 2025 09:29:27 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: "Michael S. Tsirkin" <mst@...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 5/21/25 6:02 PM, Michael S. Tsirkin wrote:
> 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 ?
Yep, I will add it in the next revision.
>> +
>> +#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.
Sounds good. Will add it in the next revision.
BTW I'm wondering if sharing a pull request from a stable tree would be
better, so that you could pull this also in the virtio/vhost tree and
avoid conflicts in the later pull to Linus.
Thanks,
Paolo
Powered by blists - more mailing lists