Message-ID: <CACGkMEuuLn20jWA_SzJxj1j+2AUnONoXc0MqPk6aWbS=kO=+kg@mail.gmail.com>
Date: Wed, 18 Jun 2025 10:27:21 +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 v4 net-next 2/8] virtio_pci_modern: allow configuring
 extended features

On Wed, Jun 18, 2025 at 12:12 AM 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>
> ---
> v3 -> v4:
>   - dropped unneeded check in vp_modern_get_features()
>
> 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      | 43 ++++++++++++++--
>  3 files changed, 84 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index 7182f43ed055..dd0e65f71d41 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)
> @@ -437,7 +437,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;
>  }
> @@ -1234,7 +1234,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,
> @@ -1254,7 +1254,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));

Nit: why not simply cast features to u32 * then everything is simplified.

Others look good.

Acked-by: Jason Wang <jasowang@...hat.com>

Thanks

Powered by blists - more mailing lists