| 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: <CACGkMEvH8cq+AdvJMO0eV0jps_-t1tUMc-cbdfReJdWFThOVuw@mail.gmail.com>
Date: Thu, 12 Jun 2025 08:57:42 +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 2/8] virtio_pci_modern: allow configuring 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.
>
> Extend the virtio pci modern driver to support configuring the full
> virtio features range, replacing the unrolled loops reading and
> writing the features space with explicit one bounded to the actual
> features space size in word and implementing the get_extended_features
> callback.
>
> Note that in vp_finalize_features() we only need to cache the lower
> 64 features bits, to process the transport features.
>
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
> v2 -> v3:
> - virtio_features_t -> u64 *
>
> v1 -> v2:
> - use get_extended_features
> ---
> drivers/virtio/virtio_pci_modern.c | 10 ++--
> drivers/virtio/virtio_pci_modern_dev.c | 69 +++++++++++++++-----------
> include/linux/virtio_pci_modern.h | 46 +++++++++++++++--
> 3 files changed, 87 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index d50fe030d825..255203441201 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -22,11 +22,11 @@
>
> #define VIRTIO_AVQ_SGS_MAX 4
>
> -static u64 vp_get_features(struct virtio_device *vdev)
> +static void vp_get_features(struct virtio_device *vdev, u64 *features)
> {
> struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>
> - return vp_modern_get_features(&vp_dev->mdev);
> + vp_modern_get_extended_features(&vp_dev->mdev, features);
> }
>
> static int vp_avq_index(struct virtio_device *vdev, u16 *index, u16 *num)
> @@ -426,7 +426,7 @@ static int vp_finalize_features(struct virtio_device *vdev)
> if (vp_check_common_size(vdev))
> return -EINVAL;
>
> - vp_modern_set_features(&vp_dev->mdev, vdev->features);
> + vp_modern_set_extended_features(&vp_dev->mdev, vdev->features_array);
>
> return 0;
> }
> @@ -1223,7 +1223,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> .find_vqs = vp_modern_find_vqs,
> .del_vqs = vp_del_vqs,
> .synchronize_cbs = vp_synchronize_vectors,
> - .get_features = vp_get_features,
> + .get_extended_features = vp_get_features,
> .finalize_features = vp_finalize_features,
> .bus_name = vp_bus_name,
> .set_vq_affinity = vp_set_vq_affinity,
> @@ -1243,7 +1243,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> .find_vqs = vp_modern_find_vqs,
> .del_vqs = vp_del_vqs,
> .synchronize_cbs = vp_synchronize_vectors,
> - .get_features = vp_get_features,
> + .get_extended_features = vp_get_features,
> .finalize_features = vp_finalize_features,
> .bus_name = vp_bus_name,
> .set_vq_affinity = vp_set_vq_affinity,
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index 0d3dbfaf4b23..d665f8f73ea8 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -388,63 +388,74 @@ void vp_modern_remove(struct virtio_pci_modern_device *mdev)
> EXPORT_SYMBOL_GPL(vp_modern_remove);
>
> /*
> - * vp_modern_get_features - get features from device
> + * vp_modern_get_extended_features - get features from device
> * @mdev: the modern virtio-pci device
> + * @features: the features array to be filled
> *
> - * Returns the features read from the device
> + * Fill the specified features array with the features read from the device
> */
> -u64 vp_modern_get_features(struct virtio_pci_modern_device *mdev)
> +void vp_modern_get_extended_features(struct virtio_pci_modern_device *mdev,
> + u64 *features)
> {
> struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> + int i;
>
> - u64 features;
> + virtio_features_zero(features);
> + for (i = 0; i < VIRTIO_FEATURES_WORDS; i++) {
> + u64 cur;
>
> - vp_iowrite32(0, &cfg->device_feature_select);
> - features = vp_ioread32(&cfg->device_feature);
> - vp_iowrite32(1, &cfg->device_feature_select);
> - features |= ((u64)vp_ioread32(&cfg->device_feature) << 32);
> -
> - return features;
> + vp_iowrite32(i, &cfg->device_feature_select);
> + cur = vp_ioread32(&cfg->device_feature);
> + features[i >> 1] |= cur << (32 * (i & 1));
> + }
> }
> -EXPORT_SYMBOL_GPL(vp_modern_get_features);
> +EXPORT_SYMBOL_GPL(vp_modern_get_extended_features);
>
> /*
> * vp_modern_get_driver_features - get driver features from device
> * @mdev: the modern virtio-pci device
> + * @features: the features array to be filled
> *
> - * Returns the driver features read from the device
> + * Fill the specified features array with the driver features read from the
> + * device
> */
> -u64 vp_modern_get_driver_features(struct virtio_pci_modern_device *mdev)
> +void
> +vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev,
> + u64 *features)
> {
> struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> + int i;
>
> - u64 features;
> -
> - vp_iowrite32(0, &cfg->guest_feature_select);
> - features = vp_ioread32(&cfg->guest_feature);
> - vp_iowrite32(1, &cfg->guest_feature_select);
> - features |= ((u64)vp_ioread32(&cfg->guest_feature) << 32);
> + virtio_features_zero(features);
> + for (i = 0; i < VIRTIO_FEATURES_WORDS; i++) {
> + u64 cur;
>
> - return features;
> + vp_iowrite32(i, &cfg->guest_feature_select);
> + cur = vp_ioread32(&cfg->guest_feature);
> + features[i >> 1] |= cur << (32 * (i & 1));
> + }
> }
> -EXPORT_SYMBOL_GPL(vp_modern_get_driver_features);
> +EXPORT_SYMBOL_GPL(vp_modern_get_driver_extended_features);
>
> /*
> - * vp_modern_set_features - set features to device
> + * vp_modern_set_extended_features - set features to device
> * @mdev: the modern virtio-pci device
> * @features: the features set to device
> */
> -void vp_modern_set_features(struct virtio_pci_modern_device *mdev,
> - u64 features)
> +void vp_modern_set_extended_features(struct virtio_pci_modern_device *mdev,
> + const u64 *features)
> {
> struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> + int i;
> +
> + for (i = 0; i < VIRTIO_FEATURES_WORDS; i++) {
> + u32 cur = features[i >> 1] >> (32 * (i & 1));
>
> - vp_iowrite32(0, &cfg->guest_feature_select);
> - vp_iowrite32((u32)features, &cfg->guest_feature);
> - vp_iowrite32(1, &cfg->guest_feature_select);
> - vp_iowrite32(features >> 32, &cfg->guest_feature);
> + vp_iowrite32(i, &cfg->guest_feature_select);
> + vp_iowrite32(cur, &cfg->guest_feature);
> + }
> }
> -EXPORT_SYMBOL_GPL(vp_modern_set_features);
> +EXPORT_SYMBOL_GPL(vp_modern_set_extended_features);
>
> /*
> * vp_modern_generation - get the device genreation
> diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> index c0b1b1ca1163..0764802a50ea 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_config.h>
> #include <linux/virtio_pci.h>
>
> /**
> @@ -95,10 +96,47 @@ 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);
> -void vp_modern_set_features(struct virtio_pci_modern_device *mdev,
> - u64 features);
> +void
> +vp_modern_get_driver_extended_features(struct virtio_pci_modern_device *mdev,
> + u64 *features);
> +void vp_modern_get_extended_features(struct virtio_pci_modern_device *mdev,
> + u64 *features);
> +void vp_modern_set_extended_features(struct virtio_pci_modern_device *mdev,
> + const u64 *features);
> +
> +static inline u64
> +vp_modern_get_features(struct virtio_pci_modern_device *mdev)
> +{
> + u64 features_array[VIRTIO_FEATURES_DWORDS];
> + int i;
> +
> + vp_modern_get_extended_features(mdev, features_array);
> + for (i = 1; i < VIRTIO_FEATURES_DWORDS; ++i)
> + WARN_ON_ONCE(features_array[i]);
It looks to me it's sufficient and safe to just return
featuers_array[0] here. Or maybe we need some comment to explain why
we need WARN here.
Thanks
Powered by blists - more mailing lists