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

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 at 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

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 at 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