[PATCH 0/3] fixes for mlx5_vdpa multiqueue support

[PATCH 0/3] fixes for mlx5_vdpa multiqueue support

[Si-Wei Liu]

This patchset contains the fixes for a few issues uncovered during the
review for the "Allow for configuring max number of virtqueue pairs"
series.

It is based on Eli's fixes:
2e4cda633a22 ("vdpa/mlx5: Fix tracking of current number of VQs")
in the vhost tree.

Si-Wei Liu (3):
  vdpa: factor out vdpa_set_features_unlocked for vdpa internal use
  vdpa/mlx5: set_features should nack MQ if no CTRL_VQ
  vdpa/mlx5: validate the queue pair value from driver

 drivers/vdpa/mlx5/net/mlx5_vnet.c | 26 +++++++++++++++++++++++---
 drivers/vdpa/vdpa.c               |  2 +-
 drivers/vhost/vdpa.c              |  2 +-
 drivers/virtio/virtio_vdpa.c      |  2 +-
 include/linux/vdpa.h              | 18 ++++++++++++------
 5 files changed, 38 insertions(+), 12 deletions(-)

-- 
1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 1/3] vdpa: factor out vdpa_set_features_unlocked for vdpa internal use

[Si-Wei Liu]

No functional change introduced. vdpa bus driver such as virtio_vdpa
or vhost_vdpa is not supposed to take care of the locking for core
by its own. The locked API vdpa_set_features should suffice the
bus driver's need.

Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
---
 drivers/vdpa/vdpa.c          |  2 +-
 drivers/vhost/vdpa.c         |  2 +-
 drivers/virtio/virtio_vdpa.c |  2 +-
 include/linux/vdpa.h         | 18 ++++++++++++------
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9846c9d..1ea5254 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -393,7 +393,7 @@ static void vdpa_get_config_unlocked(struct vdpa_device *vdev,
 	 * If it does happen we assume a legacy guest.
 	 */
 	if (!vdev->features_valid)
-		vdpa_set_features(vdev, 0, true);
+		vdpa_set_features_unlocked(vdev, 0);
 	ops->get_config(vdev, offset, buf, len);
 }
 
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 8515398..ec5249e 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -286,7 +286,7 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v, u64 __user *featurep)
 	if (copy_from_user(&features, featurep, sizeof(features)))
 		return -EFAULT;
 
-	if (vdpa_set_features(vdpa, features, false))
+	if (vdpa_set_features(vdpa, features))
 		return -EINVAL;
 
 	return 0;
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 7767a7f..7650455 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -317,7 +317,7 @@ static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
 	/* Give virtio_ring a chance to accept features. */
 	vring_transport_features(vdev);
 
-	return vdpa_set_features(vdpa, vdev->features, false);
+	return vdpa_set_features(vdpa, vdev->features);
 }
 
 static const char *virtio_vdpa_bus_name(struct virtio_device *vdev)
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 2de442e..721089b 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -401,18 +401,24 @@ static inline int vdpa_reset(struct vdpa_device *vdev)
 	return ret;
 }
 
-static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features, bool locked)
+static inline int vdpa_set_features_unlocked(struct vdpa_device *vdev, u64 features)
 {
 	const struct vdpa_config_ops *ops = vdev->config;
 	int ret;
 
-	if (!locked)
-		mutex_lock(&vdev->cf_mutex);
-
 	vdev->features_valid = true;
 	ret = ops->set_driver_features(vdev, features);
-	if (!locked)
-		mutex_unlock(&vdev->cf_mutex);
+
+	return ret;
+}
+
+static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
+{
+	int ret;
+
+	mutex_lock(&vdev->cf_mutex);
+	ret = vdpa_set_features_unlocked(vdev, features);
+	mutex_unlock(&vdev->cf_mutex);
 
 	return ret;
 }
-- 
1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

[Si-Wei Liu]

Made corresponding change per spec:

The device MUST NOT offer a feature which requires another feature
which was not offered.

Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b53603d..46d4deb 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
 	return ndev->mvdev.mlx_features;
 }
 
-static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
+static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)
 {
-	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
+	/* minimum features to expect */
+	if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
 		return -EOPNOTSUPP;
 
+	/* Double check features combination sent down by the driver.
+	 * NACK invalid feature due to the absence of depended feature.
+	 * Driver is expected to re-read the negotiated features once
+	 * return from set_driver_features.
+	 */
+	if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
+            BIT_ULL(VIRTIO_NET_F_MQ))
+		*features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
+
 	return 0;
 }
 
@@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
 
 	print_features(mvdev, features, true);
 
-	err = verify_min_features(mvdev, features);
+	err = verify_driver_features(mvdev, &features);
 	if (err)
 		return err;
 
-- 
1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH 3/3] vdpa/mlx5: validate the queue pair value from driver

[Si-Wei Liu]

Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 46d4deb..491127f 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1563,11 +1563,21 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
 
 	switch (cmd) {
 	case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
+		/* This mq feature check aligns with pre-existing userspace implementation,
+		 * although the spec doesn't mandate so.
+		 */
+		if (!MLX5_FEATURE(mvdev, VIRTIO_NET_F_MQ))
+			break;
+
 		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&mq, sizeof(mq));
 		if (read != sizeof(mq))
 			break;
 
 		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
+		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
+		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
+			break;
+
 		if (ndev->cur_num_vqs == 2 * newqps) {
 			status = VIRTIO_NET_OK;
 			break;
-- 
1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/3] vdpa: factor out vdpa_set_features_unlocked for vdpa internal use

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 12:10:49AM -0500, Si-Wei Liu wrote:
> No functional change introduced. vdpa bus driver such as virtio_vdpa
> or vhost_vdpa is not supposed to take care of the locking for core
> by its own. The locked API vdpa_set_features should suffice the
> bus driver's need.
> 
> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>

Not sure I get the explanation here. But I like separate APIs better
than a flag, so there.


> ---
>  drivers/vdpa/vdpa.c          |  2 +-
>  drivers/vhost/vdpa.c         |  2 +-
>  drivers/virtio/virtio_vdpa.c |  2 +-
>  include/linux/vdpa.h         | 18 ++++++++++++------
>  4 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 9846c9d..1ea5254 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -393,7 +393,7 @@ static void vdpa_get_config_unlocked(struct vdpa_device *vdev,
>  	 * If it does happen we assume a legacy guest.
>  	 */
>  	if (!vdev->features_valid)
> -		vdpa_set_features(vdev, 0, true);
> +		vdpa_set_features_unlocked(vdev, 0);
>  	ops->get_config(vdev, offset, buf, len);
>  }
>  
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 8515398..ec5249e 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -286,7 +286,7 @@ static long vhost_vdpa_set_features(struct vhost_vdpa *v, u64 __user *featurep)
>  	if (copy_from_user(&features, featurep, sizeof(features)))
>  		return -EFAULT;
>  
> -	if (vdpa_set_features(vdpa, features, false))
> +	if (vdpa_set_features(vdpa, features))
>  		return -EINVAL;
>  
>  	return 0;
> diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
> index 7767a7f..7650455 100644
> --- a/drivers/virtio/virtio_vdpa.c
> +++ b/drivers/virtio/virtio_vdpa.c
> @@ -317,7 +317,7 @@ static int virtio_vdpa_finalize_features(struct virtio_device *vdev)
>  	/* Give virtio_ring a chance to accept features. */
>  	vring_transport_features(vdev);
>  
> -	return vdpa_set_features(vdpa, vdev->features, false);
> +	return vdpa_set_features(vdpa, vdev->features);
>  }
>  
>  static const char *virtio_vdpa_bus_name(struct virtio_device *vdev)
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index 2de442e..721089b 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -401,18 +401,24 @@ static inline int vdpa_reset(struct vdpa_device *vdev)
>  	return ret;
>  }
>  
> -static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features, bool locked)
> +static inline int vdpa_set_features_unlocked(struct vdpa_device *vdev, u64 features)
>  {
>  	const struct vdpa_config_ops *ops = vdev->config;
>  	int ret;
>  
> -	if (!locked)
> -		mutex_lock(&vdev->cf_mutex);
> -
>  	vdev->features_valid = true;
>  	ret = ops->set_driver_features(vdev, features);
> -	if (!locked)
> -		mutex_unlock(&vdev->cf_mutex);
> +
> +	return ret;
> +}
> +
> +static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
> +{
> +	int ret;
> +
> +	mutex_lock(&vdev->cf_mutex);
> +	ret = vdpa_set_features_unlocked(vdev, features);
> +	mutex_unlock(&vdev->cf_mutex);
>  
>  	return ret;
>  }
> -- 
> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 12:10:50AM -0500, Si-Wei Liu wrote:
> Made corresponding change per spec:


> The device MUST NOT offer a feature which requires another feature
> which was not offered.

Says nothing about the driver though, and you seem to be
doing things to driver features?

pls explain the motivation. which config are you trying to
fix what is current and expected behaviour.

> 
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")


It's all theoretical right? Fixes really means
"if you have commit ABC then you should pick this one up".
not really appropriate for theoretical fixes.

> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b53603d..46d4deb 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
>  	return ndev->mvdev.mlx_features;
>  }
>  
> -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
> +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)


Good rename actually but document in commit log with an
explanation.

>  {
> -	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> +	/* minimum features to expect */
> +	if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>  		return -EOPNOTSUPP;
>  
> +	/* Double check features combination sent down by the driver.
> +	 * NACK invalid feature due to the absence of depended feature.

Pls rewrite this to make it grammatical.  There's no NACK in spec. What
does this do? Fails to set FEATURES_OK?

> +	 * Driver is expected to re-read the negotiated features once
> +	 * return from set_driver_features.

once return is ungrammatical. What to say here depends on what
you mean by this, so I'm not sure.


Here's text from spec:

\item\label{itm:General Initialization And Device Operation /
Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
   understood by the OS and driver to the device.  During this step the
   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.

\item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
   new feature bits after this step.

\item\label{itm:General Initialization And Device Operation / Device Initialization / Re-read FEATURES-OK} Re-read \field{device status} to ensure the FEATURES_OK bit is still
   set: otherwise, the device does not support our subset of features
   and the device is unusable.

\item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
   device, optional per-bus setup, reading and possibly writing the
   device's virtio configuration space, and population of virtqueues.

does not seem to talk about re-reading features.
What did I miss?


> +	 */


This comment confuses more than it clarifies. I would
- quote the spec
- explain why does code do what it does specifically for these features

> +	if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
> +            BIT_ULL(VIRTIO_NET_F_MQ))
> +		*features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
> +
>  	return 0;
>  }
>  
> @@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>  
>  	print_features(mvdev, features, true);
>  
> -	err = verify_min_features(mvdev, features);
> +	err = verify_driver_features(mvdev, &features);
>  	if (err)
>  		return err;
>  
> -- 
> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vdpa/mlx5: validate the queue pair value from driver

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 12:10:51AM -0500, Si-Wei Liu wrote:
> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>

Add motivation for change in the commit log.


> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 46d4deb..491127f 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1563,11 +1563,21 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>  
>  	switch (cmd) {
>  	case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
> +		/* This mq feature check aligns with pre-existing userspace implementation,
> +		 * although the spec doesn't mandate so.

And so ... why do we bother? what breaks if we don't?

> +		 */
> +		if (!MLX5_FEATURE(mvdev, VIRTIO_NET_F_MQ))
> +			break;
> +


this part is not described in the commit log at all.
is it intentional?

>  		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&mq, sizeof(mq));
>  		if (read != sizeof(mq))
>  			break;
>  
>  		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
> +		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
> +		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
> +			break;
> +
>  		if (ndev->cur_num_vqs == 2 * newqps) {
>  			status = VIRTIO_NET_OK;
>  			break;
> -- 
> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/3] fixes for mlx5_vdpa multiqueue support

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 12:10:48AM -0500, Si-Wei Liu wrote:
> This patchset contains the fixes for a few issues uncovered during the
> review for the "Allow for configuring max number of virtqueue pairs"
> series.
> 
> It is based on Eli's fixes:
> 2e4cda633a22 ("vdpa/mlx5: Fix tracking of current number of VQs")
> in the vhost tree.

It's really cleanups more than fixes. I'm not sure about the
code changes (the vdpa change looks ok, mlx5 ones need review
by nvidia folks) but from documentation POV this patchset needs
more work.


> 
> Si-Wei Liu (3):
>   vdpa: factor out vdpa_set_features_unlocked for vdpa internal use
>   vdpa/mlx5: set_features should nack MQ if no CTRL_VQ
>   vdpa/mlx5: validate the queue pair value from driver
> 
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 26 +++++++++++++++++++++++---
>  drivers/vdpa/vdpa.c               |  2 +-
>  drivers/vhost/vdpa.c              |  2 +-
>  drivers/virtio/virtio_vdpa.c      |  2 +-
>  include/linux/vdpa.h              | 18 ++++++++++++------
>  5 files changed, 38 insertions(+), 12 deletions(-)
> 
> -- 
> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

[Jason Wang]

On Thu, Jan 13, 2022 at 4:09 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Jan 13, 2022 at 12:10:50AM -0500, Si-Wei Liu wrote:
> > Made corresponding change per spec:
> >
> > The device MUST NOT offer a feature which requires another feature
> > which was not offered.
> >
> > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
> > ---
> >  drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index b53603d..46d4deb 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
> >       return ndev->mvdev.mlx_features;
> >  }
> >
> > -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
> > +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)
> >  {
> > -     if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> > +     /* minimum features to expect */
> > +     if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> >               return -EOPNOTSUPP;
> >
> > +     /* Double check features combination sent down by the driver.
> > +      * NACK invalid feature due to the absence of depended feature.
> > +      * Driver is expected to re-read the negotiated features once
> > +      * return from set_driver_features.
> > +      */
> > +     if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
> > +            BIT_ULL(VIRTIO_NET_F_MQ))
> > +             *features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
>
> I would not expect this kind check to be enforced in vhost_vdpa and
> apply to all drivers.

We want to make vhost_vdpa type agnostic to make it simple and clean.
So there's no type specific code there. So my understanding is that,
if this is the mandate behavior of the device, it needs to be done at
device level (mlx5_vdpa) right now.

Thanks

>
> > +
> >       return 0;
> >  }
> >
> > @@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> >
> >       print_features(mvdev, features, true);
> >
> > -     err = verify_min_features(mvdev, features);
> > +     err = verify_driver_features(mvdev, &features);
> >       if (err)
> >               return err;
> >
> > --
> > 1.8.3.1
> >
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

[Si-Wei Liu]

On 1/12/2022 10:57 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 13, 2022 at 12:10:50AM -0500, Si-Wei Liu wrote:
>> Made corresponding change per spec:
>
>> The device MUST NOT offer a feature which requires another feature
>> which was not offered.
> Says nothing about the driver though, and you seem to be
> doing things to driver features?
Yes, it's about validation for driver features, though the spec doesn't 
have clear way how to deal with this situation. I guess this in reality 
leaves quite some space for the implementation. To step back, in recent 
days with latent spec revision for feature negotiation due to endianness 
and MTU validation, what do we expect device to work if the driver is 
not compliant and comes up with invalid features set? To clear a subset 
of driver features unsupported by the device, such that driver may 
figure out by reading it from device config space later on? Or fail the 
entire features and have driver to re-try a different setting? Do you 
feel its possible for device to clear a subset of invalid or unsupported 
features sent down by the driver, which may allow driver continue to 
work without having to fail the entire feature negotiation?

The current userspace implementation in qemu may filter out invalid 
features from driver by clearing a subset and tailor it to fit what 
host/device can offer. I thought it should be safe to follow the 
existing practice. That way guest driver can get know of the effective 
features during feature negotiation, or after features_ok is set (that's 
what I call by "re-read" of features, sorry if I used the wrong term). 
Did I miss something? I can definitely add more explanation for the 
motivation, remove the reference to spec and delete the Fixes tag to 
avoid confusions. Do you think this would work?

Another option would be just return failure for the 
set_driver_features() call when seeing (MQ && !CTRL_VQ). Simple enough 
and easy to implement. Efficient to indicate which individual feature is 
failing? Probably not, driver has to retry a few times using binary 
search to know.

> pls explain the motivation. which config are you trying to
> fix what is current and expected behaviour.
The current mq code for mlx5_vdpa driver is written in the assumption 
that MQ must come together with CTRL_VQ. I would like to point out that 
right now there's nowhere in the host side even QEMU to guarantee this 
assumption would hold. Were there a malicious driver sending down MQ 
without CTRL_VQ, it would compromise various spots such as 
is_index_valid() and is_ctrl_vq_idx(). This doesn't end up with 
immediate panic or security loophole in the host currently, but still 
the chance for this being taken advantage of is not zero, especially 
when future code change is involved. You can say it's code cleanup, but 
the added check helps harden the crispy assumption and assures peace of 
mind.

>
>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>
> It's all theoretical right? Fixes really means
> "if you have commit ABC then you should pick this one up".
> not really appropriate for theoretical fixes.
Yeah. This was discovered in code review. Didn't see a real issue. I can 
remove the tag.

-Siwei
>
>> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index b53603d..46d4deb 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
>>   	return ndev->mvdev.mlx_features;
>>   }
>>   
>> -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
>> +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)
>
> Good rename actually but document in commit log with an
> explanation.
>
>>   {
>> -	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>> +	/* minimum features to expect */
>> +	if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>   		return -EOPNOTSUPP;
>>   
>> +	/* Double check features combination sent down by the driver.
>> +	 * NACK invalid feature due to the absence of depended feature.
> Pls rewrite this to make it grammatical.  There's no NACK in spec. What
> does this do? Fails to set FEATURES_OK?
>
>> +	 * Driver is expected to re-read the negotiated features once
>> +	 * return from set_driver_features.
> once return is ungrammatical. What to say here depends on what
> you mean by this, so I'm not sure.
>
>
> Here's text from spec:
>
> \item\label{itm:General Initialization And Device Operation /
> Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
>     understood by the OS and driver to the device.  During this step the
>     driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
>
> \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
>     new feature bits after this step.
>
> \item\label{itm:General Initialization And Device Operation / Device Initialization / Re-read FEATURES-OK} Re-read \field{device status} to ensure the FEATURES_OK bit is still
>     set: otherwise, the device does not support our subset of features
>     and the device is unusable.
>
> \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
>     device, optional per-bus setup, reading and possibly writing the
>     device's virtio configuration space, and population of virtqueues.
>
> does not seem to talk about re-reading features.
> What did I miss?
>
>
>> +	 */
>
> This comment confuses more than it clarifies. I would
> - quote the spec
> - explain why does code do what it does specifically for these features
>
>> +	if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
>> +            BIT_ULL(VIRTIO_NET_F_MQ))
>> +		*features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>   
>>   	print_features(mvdev, features, true);
>>   
>> -	err = verify_min_features(mvdev, features);
>> +	err = verify_driver_features(mvdev, &features);
>>   	if (err)
>>   		return err;
>>   
>> -- 
>> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 3/3] vdpa/mlx5: validate the queue pair value from driver

[Si-Wei Liu]

On 1/12/2022 11:00 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 13, 2022 at 12:10:51AM -0500, Si-Wei Liu wrote:
>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
> Add motivation for change in the commit log.
>
>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 46d4deb..491127f 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1563,11 +1563,21 @@ static virtio_net_ctrl_ack handle_ctrl_mq(struct mlx5_vdpa_dev *mvdev, u8 cmd)
>>   
>>   	switch (cmd) {
>>   	case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
>> +		/* This mq feature check aligns with pre-existing userspace implementation,
>> +		 * although the spec doesn't mandate so.
> And so ... why do we bother? what breaks if we don't?
Without it, a malicious driver could fake a config multiqueue request 
down to a non-mq backend that may cause kernel to panic due to 
uninitialized resources for the queue. Even with a well behaving guest 
driver, it is not expected to allow such kind of change.
>
>> +		 */
>> +		if (!MLX5_FEATURE(mvdev, VIRTIO_NET_F_MQ))
>> +			break;
>> +
>
> this part is not described in the commit log at all.
> is it intentional?

No, I can add it.
>
>>   		read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void *)&mq, sizeof(mq));
>>   		if (read != sizeof(mq))
>>   			break;
>>   
>>   		newqps = mlx5vdpa16_to_cpu(mvdev, mq.virtqueue_pairs);
>> +		if (newqps < VIRTIO_NET_CTRL_MQ_VQ_PAIRS_MIN ||
>> +		    newqps > mlx5_vdpa_max_qps(mvdev->max_vqs))
>> +			break;
>> +
>>   		if (ndev->cur_num_vqs == 2 * newqps) {
>>   			status = VIRTIO_NET_OK;
>>   			break;
>> -- 
>> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/3] fixes for mlx5_vdpa multiqueue support

[Si-Wei Liu]

On 1/12/2022 11:03 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 13, 2022 at 12:10:48AM -0500, Si-Wei Liu wrote:
>> This patchset contains the fixes for a few issues uncovered during the
>> review for the "Allow for configuring max number of virtqueue pairs"
>> series.
>>
>> It is based on Eli's fixes:
>> 2e4cda633a22 ("vdpa/mlx5: Fix tracking of current number of VQs")
>> in the vhost tree.
> It's really cleanups more than fixes. I'm not sure about the
> code changes (the vdpa change looks ok, mlx5 ones need review
> by nvidia folks) but from documentation POV this patchset needs
> more work.
Yeah, this changeset is aiming to tighten the lose end found during code 
review.  Will try to improve commit message/comment and get back.

There will be another set that I am working on for real issues. Stay tuned.

-Siwei
>
>> Si-Wei Liu (3):
>>    vdpa: factor out vdpa_set_features_unlocked for vdpa internal use
>>    vdpa/mlx5: set_features should nack MQ if no CTRL_VQ
>>    vdpa/mlx5: validate the queue pair value from driver
>>
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 26 +++++++++++++++++++++++---
>>   drivers/vdpa/vdpa.c               |  2 +-
>>   drivers/vhost/vdpa.c              |  2 +-
>>   drivers/virtio/virtio_vdpa.c      |  2 +-
>>   include/linux/vdpa.h              | 18 ++++++++++++------
>>   5 files changed, 38 insertions(+), 12 deletions(-)
>>
>> -- 
>> 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

[Michael S. Tsirkin]

On Fri, Jan 14, 2022 at 12:51:55AM -0800, Si-Wei Liu wrote:
> 
> 
> On 1/12/2022 10:57 PM, Michael S. Tsirkin wrote:
> > On Thu, Jan 13, 2022 at 12:10:50AM -0500, Si-Wei Liu wrote:
> > > Made corresponding change per spec:
> > 
> > > The device MUST NOT offer a feature which requires another feature
> > > which was not offered.
> > Says nothing about the driver though, and you seem to be
> > doing things to driver features?
> Yes, it's about validation for driver features, though the spec doesn't have
> clear way how to deal with this situation. I guess this in reality leaves
> quite some space for the implementation. To step back, in recent days with
> latent spec revision for feature negotiation due to endianness and MTU
> validation, what do we expect device to work if the driver is not compliant
> and comes up with invalid features set? To clear a subset of driver features
> unsupported by the device, such that driver may figure out by reading it
> from device config space later on? Or fail the entire features and have
> driver to re-try a different setting? Do you feel its possible for device to
> clear a subset of invalid or unsupported features sent down by the driver,
> which may allow driver continue to work without having to fail the entire
> feature negotiation?
> 
> The current userspace implementation in qemu may filter out invalid features
> from driver by clearing a subset and tailor it to fit what host/device can
> offer. I thought it should be safe to follow the existing practice. That way
> guest driver can get know of the effective features during feature
> negotiation, or after features_ok is set (that's what I call by "re-read" of
> features, sorry if I used the wrong term). Did I miss something? I can
> definitely add more explanation for the motivation, remove the reference to
> spec and delete the Fixes tag to avoid confusions. Do you think this would
> work?
> 
> Another option would be just return failure for the set_driver_features()
> call when seeing (MQ && !CTRL_VQ). Simple enough and easy to implement.
> Efficient to indicate which individual feature is failing? Probably not,
> driver has to retry a few times using binary search to know.
> 
> > pls explain the motivation. which config are you trying to
> > fix what is current and expected behaviour.
> The current mq code for mlx5_vdpa driver is written in the assumption that
> MQ must come together with CTRL_VQ. I would like to point out that right now
> there's nowhere in the host side even QEMU to guarantee this assumption
> would hold. Were there a malicious driver sending down MQ without CTRL_VQ,
> it would compromise various spots such as is_index_valid() and
> is_ctrl_vq_idx(). This doesn't end up with immediate panic or security
> loophole in the host currently, but still the chance for this being taken
> advantage of is not zero, especially when future code change is involved.
> You can say it's code cleanup, but the added check helps harden the crispy
> assumption and assures peace of mind.

I think that right now the right thing to do is to validate untrusted
input and fail invalid operations.
The spec does say "VIRTIO_NET_F_MQ Requires VIRTIO_NET_F_CTRL_VQ".
If there are existing legacy drivers
violating some rules, then we should consider working around that (and
maybe documenting that in the spec in the legacy section).


> > 
> > > Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
> > 
> > It's all theoretical right? Fixes really means
> > "if you have commit ABC then you should pick this one up".
> > not really appropriate for theoretical fixes.
> Yeah. This was discovered in code review. Didn't see a real issue. I can
> remove the tag.
> 
> -Siwei
> > 
> > > Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
> > > ---
> > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
> > >   1 file changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index b53603d..46d4deb 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
> > >   	return ndev->mvdev.mlx_features;
> > >   }
> > > -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
> > > +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)
> > 
> > Good rename actually but document in commit log with an
> > explanation.
> > 
> > >   {
> > > -	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> > > +	/* minimum features to expect */
> > > +	if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
> > >   		return -EOPNOTSUPP;
> > > +	/* Double check features combination sent down by the driver.
> > > +	 * NACK invalid feature due to the absence of depended feature.
> > Pls rewrite this to make it grammatical.  There's no NACK in spec. What
> > does this do? Fails to set FEATURES_OK?
> > 
> > > +	 * Driver is expected to re-read the negotiated features once
> > > +	 * return from set_driver_features.
> > once return is ungrammatical. What to say here depends on what
> > you mean by this, so I'm not sure.
> > 
> > 
> > Here's text from spec:
> > 
> > \item\label{itm:General Initialization And Device Operation /
> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> >     understood by the OS and driver to the device.  During this step the
> >     driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > 
> > \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
> >     new feature bits after this step.
> > 
> > \item\label{itm:General Initialization And Device Operation / Device Initialization / Re-read FEATURES-OK} Re-read \field{device status} to ensure the FEATURES_OK bit is still
> >     set: otherwise, the device does not support our subset of features
> >     and the device is unusable.
> > 
> > \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
> >     device, optional per-bus setup, reading and possibly writing the
> >     device's virtio configuration space, and population of virtqueues.
> > 
> > does not seem to talk about re-reading features.
> > What did I miss?
> > 
> > 
> > > +	 */
> > 
> > This comment confuses more than it clarifies. I would
> > - quote the spec
> > - explain why does code do what it does specifically for these features
> > 
> > > +	if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
> > > +            BIT_ULL(VIRTIO_NET_F_MQ))
> > > +		*features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
> > > +
> > >   	return 0;
> > >   }
> > > @@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > >   	print_features(mvdev, features, true);
> > > -	err = verify_min_features(mvdev, features);
> > > +	err = verify_driver_features(mvdev, &features);
> > >   	if (err)
> > >   		return err;
> > > -- 
> > > 1.8.3.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization