vdpa: Sync calls set/get config/status with cf_mutex

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
  drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++
  drivers/vhost/vdpa.c         |  7 +++----
  drivers/virtio/virtio_vdpa.c |  3 +--
  include/linux/vdpa.h         |  3 +++
  4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +19 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
  static DEFINE_MUTEX(vdpa_dev_mutex);
  static DEFINE_IDA(vdpa_index_ida);
 
+u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+	u8 status;
+
+	mutex_lock(&vdev->cf_mutex);
+	status = vdev->config->get_status(vdev);
+	mutex_unlock(&vdev->cf_mutex);
+	return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+	mutex_lock(&vdev->cf_mutex);
+	vdev->config->set_status(vdev, status);
+	mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
  static struct genl_family vdpa_nl_family;
 
  static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644

drivers/vhost/vdpa.c +0 -0

  	if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
  		for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -2

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct virtio_device *vdev)
  static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 status)
  {
  	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
 
-	return ops->set_status(vdpa, status);
+	return vdpa_set_status(vdpa, status);
  }
 
  static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +0 -0

index 9cc4291a79b3..ae047fae2603 100644

Quoted 46 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
  drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++ drivers/vhost/vdpa.c         |  7 +++---- drivers/virtio/virtio_vdpa.c |  3 +-- include/linux/vdpa.h         |  3 +++

  4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +18 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
   static DEFINE_MUTEX(vdpa_dev_mutex);
   static DEFINE_IDA(vdpa_index_ida);
   +u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+    u8 status;
+
+    mutex_lock(&vdev->cf_mutex);
+    status = vdev->config->get_status(vdev);
+    mutex_unlock(&vdev->cf_mutex);
+    return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+    mutex_lock(&vdev->cf_mutex);
+    vdev->config->set_status(vdev, status);
+    mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
   static struct genl_family vdpa_nl_family;
     static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644
@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct 
 vhost_vdpa *v, u8 __user *argp)

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct · line 148

Quoted 7 lines

drivers/vhost/vdpa.c +1 -1

   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
   {
       struct vdpa_device *vdpa = v->vdpa;
-    const struct vdpa_config_ops *ops = vdpa->config;
       u8 status;
   -    status = ops->get_status(vdpa);
+    status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 167

Quoted 9 lines

drivers/vhost/vdpa.c +1 -0

         if (copy_to_user(statusp, &status, sizeof(status)))
           return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
       if (copy_from_user(&status, statusp, sizeof(status)))
           return -EFAULT;
   -    status_old = ops->get_status(vdpa);
+    status_old = vdpa_get_status(vdpa);

Ditto.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 185

Quoted 9 lines

drivers/vhost/vdpa.c +1 -1

         /*
        * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
           if (ret)
               return ret;
       } else
-        ops->set_status(vdpa, status);
+        vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Thanks,
-Siwei

Quoted 23 lines

drivers/vhost/vdpa.c +0 -0

         if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
           for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -1

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct 
 virtio_device *vdev)
   static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 status)
   {
       struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-    const struct vdpa_config_ops *ops = vdpa->config;
   -    return ops->set_status(vdpa, status);
+    return vdpa_set_status(vdpa, status);
   }
     static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +0 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev, 
 unsigned int offset,
                void *buf, unsigned int len);

Quoted 8 lines

include/linux/vdpa.h +3 -0

   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
                const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
   /**
    * struct vdpa_mgmtdev_ops - vdpa device ops

   * @dev_add: Add a vdpa device using alloc and register

Quoted 49 lines · Si-Wei Liu

Quoted 46 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
  drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++ drivers/vhost/vdpa.c         |  7 +++---- drivers/virtio/virtio_vdpa.c |  3 +-- include/linux/vdpa.h         |  3 +++

  4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +18 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
   static DEFINE_MUTEX(vdpa_dev_mutex);
   static DEFINE_IDA(vdpa_index_ida);
   +u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+    u8 status;
+
+    mutex_lock(&vdev->cf_mutex);
+    status = vdev->config->get_status(vdev);
+    mutex_unlock(&vdev->cf_mutex);
+    return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+    mutex_lock(&vdev->cf_mutex);
+    vdev->config->set_status(vdev, status);
+    mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
   static struct genl_family vdpa_nl_family;
     static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644
@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct 
 vhost_vdpa *v, u8 __user *argp)

Quoted 8 lines

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct · line 148

Quoted 7 lines

drivers/vhost/vdpa.c +1 -1

   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
   {
       struct vdpa_device *vdpa = v->vdpa;
-    const struct vdpa_config_ops *ops = vdpa->config;
       u8 status;
   -    status = ops->get_status(vdpa);
+    status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

You may be right but it doesn't harm and it is guaranteed to be correct if we protect it with mutex here.

Thanks

Quoted 57 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 167

Quoted 9 lines

drivers/vhost/vdpa.c +1 -0

         if (copy_to_user(statusp, &status, sizeof(status)))
           return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
       if (copy_from_user(&status, statusp, sizeof(status)))
           return -EFAULT;
   -    status_old = ops->get_status(vdpa);
+    status_old = vdpa_get_status(vdpa);

Ditto.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 185

Quoted 9 lines

drivers/vhost/vdpa.c +1 -1

         /*
        * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
           if (ret)
               return ret;
       } else
-        ops->set_status(vdpa, status);
+        vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Thanks,
-Siwei

Quoted 31 lines

drivers/vhost/vdpa.c +0 -0

         if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
           for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -1

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct 
 virtio_device *vdev)
   static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 status)
   {
       struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-    const struct vdpa_config_ops *ops = vdpa->config;
   -    return ops->set_status(vdpa, status);
+    return vdpa_set_status(vdpa, status);
   }
     static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +3 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev, 
 unsigned int offset,
                void *buf, unsigned int len);
   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
                const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
   /**
    * struct vdpa_mgmtdev_ops - vdpa device ops

   * @dev_add: Add a vdpa device using alloc and register

Quoted 61 lines · Jason Wang

Quoted 57 lines · Si-Wei Liu

Quoted 45 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
  drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++ drivers/vhost/vdpa.c         |  7 +++---- drivers/virtio/virtio_vdpa.c |  3 +-- include/linux/vdpa.h         |  3 +++

  4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +18 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
   static DEFINE_MUTEX(vdpa_dev_mutex);
   static DEFINE_IDA(vdpa_index_ida);
   +u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+    u8 status;
+
+    mutex_lock(&vdev->cf_mutex);
+    status = vdev->config->get_status(vdev);
+    mutex_unlock(&vdev->cf_mutex);
+    return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+    mutex_lock(&vdev->cf_mutex);
+    vdev->config->set_status(vdev, status);
+    mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
   static struct genl_family vdpa_nl_family;
     static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct · line 148

Quoted 8 lines

drivers/vhost/vdpa.c +1 -1

@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct 
 vhost_vdpa *v, u8 __user *argp)
   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
   {
       struct vdpa_device *vdpa = v->vdpa;
-    const struct vdpa_config_ops *ops = vdpa->config;
       u8 status;
   -    status = ops->get_status(vdpa);
+    status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

Quoted 1 line

You may be right but it doesn't harm and it is guaranteed to be correct if we protect it with mutex here.

Is it more for future proof?

Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex.

-Siwei

Quoted 62 lines

Thanks

Quoted 57 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 167

Quoted 9 lines

drivers/vhost/vdpa.c +1 -0

         if (copy_to_user(statusp, &status, sizeof(status)))
           return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
       if (copy_from_user(&status, statusp, sizeof(status)))
           return -EFAULT;
   -    status_old = ops->get_status(vdpa);
+    status_old = vdpa_get_status(vdpa);

Ditto.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 185

Quoted 9 lines

drivers/vhost/vdpa.c +1 -1

         /*
        * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
           if (ret)
               return ret;
       } else
-        ops->set_status(vdpa, status);
+        vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Thanks,
-Siwei

Quoted 31 lines

drivers/vhost/vdpa.c +0 -0

         if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
           for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -1

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct 
 virtio_device *vdev)
   static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 status)
   {
       struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-    const struct vdpa_config_ops *ops = vdpa->config;
   -    return ops->set_status(vdpa, status);
+    return vdpa_set_status(vdpa, status);
   }
     static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +3 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev, 
 unsigned int offset,
                void *buf, unsigned int len);
   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
                const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
   /**
    * struct vdpa_mgmtdev_ops - vdpa device ops

   * @dev_add: Add a vdpa device using alloc and register

Quoted 64 lines · Si-Wei Liu

Quoted 61 lines · Jason Wang

Quoted 57 lines · Si-Wei Liu

Quoted 45 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
  drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++ drivers/vhost/vdpa.c         |  7 +++---- drivers/virtio/virtio_vdpa.c |  3 +-- include/linux/vdpa.h         |  3 +++

  4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +18 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
   static DEFINE_MUTEX(vdpa_dev_mutex);
   static DEFINE_IDA(vdpa_index_ida);
   +u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+    u8 status;
+
+    mutex_lock(&vdev->cf_mutex);
+    status = vdev->config->get_status(vdev);
+    mutex_unlock(&vdev->cf_mutex);
+    return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+    mutex_lock(&vdev->cf_mutex);
+    vdev->config->set_status(vdev, status);
+    mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
   static struct genl_family vdpa_nl_family;
     static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct · line 148

Quoted 8 lines

drivers/vhost/vdpa.c +1 -1

@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct 
 vhost_vdpa *v, u8 __user *argp)
   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)
   {
       struct vdpa_device *vdpa = v->vdpa;
-    const struct vdpa_config_ops *ops = vdpa->config;
       u8 status;
   -    status = ops->get_status(vdpa);
+    status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

Quoted 2 lines

Quoted 1 line

You may be right but it doesn't harm and it is guaranteed to be correct if we protect it with mutex here.

Is it more for future proof?

I think so.

Quoted 3 lines

Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex.

Right, but they are done for different levels, one is for vhost_dev antoher is for vdpa_dev.

Thanks

Quoted 66 lines

-Siwei

Quoted 62 lines

Thanks

Quoted 57 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 167

Quoted 9 lines

drivers/vhost/vdpa.c +1 -0

         if (copy_to_user(statusp, &status, sizeof(status)))
           return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
       if (copy_from_user(&status, statusp, sizeof(status)))
           return -EFAULT;
   -    status_old = ops->get_status(vdpa);
+    status_old = vdpa_get_status(vdpa);

Ditto.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 185

Quoted 9 lines

drivers/vhost/vdpa.c +1 -1

         /*
        * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct 
 vhost_vdpa *v, u8 __user *statusp)
           if (ret)
               return ret;
       } else
-        ops->set_status(vdpa, status);
+        vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Thanks,
-Siwei

Quoted 31 lines

drivers/vhost/vdpa.c +0 -0

         if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
           for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -1

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct 
 virtio_device *vdev)
   static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 status)
   {
       struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-    const struct vdpa_config_ops *ops = vdpa->config;
   -    return ops->set_status(vdpa, status);
+    return vdpa_set_status(vdpa, status);
   }
     static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +3 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev, 
 unsigned int offset,
                void *buf, unsigned int len);
   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
                const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
   /**
    * struct vdpa_mgmtdev_ops - vdpa device ops

   * @dev_add: Add a vdpa device using alloc and register

Quoted 73 lines · Jason Wang

Quoted 69 lines · Si-Wei Liu

Quoted 63 lines · Jason Wang

Quoted 51 lines · Si-Wei Liu

Quoted 48 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
  drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++
  drivers/vhost/vdpa.c         |  7 +++----
  drivers/virtio/virtio_vdpa.c |  3 +--
  include/linux/vdpa.h         |  3 +++
  4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +18 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
  static DEFINE_MUTEX(vdpa_dev_mutex);
  static DEFINE_IDA(vdpa_index_ida);
  +u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+    u8 status;
+
+    mutex_lock(&vdev->cf_mutex);
+    status = vdev->config->get_status(vdev);
+    mutex_unlock(&vdev->cf_mutex);
+    return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+    mutex_lock(&vdev->cf_mutex);
+    vdev->config->set_status(vdev, status);
+    mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
  static struct genl_family vdpa_nl_family;
    static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644
@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct
 vhost_vdpa *v, u8 __user *argp)
  static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8
 __user *statusp)

Quoted 8 lines

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct · line 149

Quoted 6 lines

drivers/vhost/vdpa.c +1 -1

  {
      struct vdpa_device *vdpa = v->vdpa;
-    const struct vdpa_config_ops *ops = vdpa->config;
      u8 status;
  -    status = ops->get_status(vdpa);
+    status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

Quoted 2 lines

You may be right but it doesn't harm and it is guaranteed to be correct if we protect it with mutex here.

Is it more for future proof?

Quoted 1 line

I think so.

I guess in this situation it would be better defer to the future patch to add such locking or wrapper, although right now there are just two additional calls taking the lock needlessly when vhost_vdpa_get_status is called. Maybe it's just me but I'm worried some future patch may calls the locked API wrapper thousands of times unintendedly, then the overhead becomes quite obvious.

Quoted 9 lines

Quoted 7 lines

Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex.

Quoted 2 lines

Right, but they are done for different levels, one is for vhost_dev antoher is for vdpa_dev.

The cf_mutex is introduced to serialize the command line configure thread (via netlink) and the upstream driver calls from either the vhost_dev or virtio_dev. I don't see a case, even in future, where the netlink thread needs to update the virtio status on the fly. Can you enlighten me why that is at all possible?

Thanks,
-Siwei

Quoted 76 lines

Thanks

Quoted 71 lines

-Siwei

Quoted 67 lines

Thanks

Quoted 62 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 167

Quoted 9 lines

drivers/vhost/vdpa.c +1 -0

        if (copy_to_user(statusp, &status, sizeof(status)))
          return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct
 vhost_vdpa *v, u8 __user *statusp)
      if (copy_from_user(&status, statusp, sizeof(status)))
          return -EFAULT;
  -    status_old = ops->get_status(vdpa);
+    status_old = vdpa_get_status(vdpa);

Ditto.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 185

Quoted 9 lines

drivers/vhost/vdpa.c +1 -1

        /*
       * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct
 vhost_vdpa *v, u8 __user *statusp)
          if (ret)
              return ret;
      } else
-        ops->set_status(vdpa, status);
+        vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Thanks,
-Siwei

Quoted 34 lines

drivers/vhost/vdpa.c +0 -0

        if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old &
 VIRTIO_CONFIG_S_DRIVER_OK))

          for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -1

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct
 virtio_device *vdev)
  static void virtio_vdpa_set_status(struct virtio_device *vdev,
 u8 status)
  {
      struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-    const struct vdpa_config_ops *ops = vdpa->config;
  -    return ops->set_status(vdpa, status);
+    return vdpa_set_status(vdpa, status);
  }
    static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +3 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev,
 unsigned int offset,
               void *buf, unsigned int len);
  void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
               const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
  /**
   * struct vdpa_mgmtdev_ops - vdpa device ops

* @dev_add: Add a vdpa device using alloc and register

From: Jason Wang <[email protected]>
Sent: Tuesday, January 11, 2022 10:17 AM

Quoted 1 line

Quoted 6 lines

I guess in this situation it would be better defer to the future patch to add such locking or wrapper, although right now there are just two additional calls taking the lock needlessly when vhost_vdpa_get_status is called. Maybe it's just me but I'm worried some future patch may calls the locked API wrapper thousands of times unintendedly, then the overhead becomes quite obvious.

I'm fine to remove this if others agree on this.

Quoted 16 lines

Quoted 11 lines

Quoted 7 lines

Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex.

Right, but they are done for different levels, one is for vhost_dev antoher is for vdpa_dev.

The cf_mutex is introduced to serialize the command line configure thread (via netlink) and the upstream driver calls from either the vhost_dev or virtio_dev. I don't see a case, even in future, where the netlink thread needs to update the virtio status on the fly. Can you enlighten me why that is at all possible?

After some thought I don't see a case. I can think of NEEDS_RESET but it should come with a config interrupt so we're probably fine without the mutex here.

Quoted 24 lines

From: Jason Wang <[email protected]>
Sent: Tuesday, January 11, 2022 10:17 AM

Quoted 6 lines

I guess in this situation it would be better defer to the future patch to add such locking or wrapper, although right now there are just two additional calls taking the lock needlessly when vhost_vdpa_get_status is called. Maybe it's just me but I'm worried some future patch may calls the locked API wrapper thousands of times unintendedly, then the overhead becomes quite obvious.

I'm fine to remove this if others agree on this.

Quoted 13 lines

Quoted 8 lines

Quoted 5 lines

Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex.

Right, but they are done for different levels, one is for vhost_dev antoher is for vdpa_dev.

The cf_mutex is introduced to serialize the command line configure thread (via netlink) and the upstream driver calls from either the vhost_dev or virtio_dev. I don't see a case, even in future, where the netlink thread needs to update the virtio status on the fly. Can you enlighten me why that is at all possible?

Sorry for my late response.

Netlink reads the whole space while it is not getting modified by vhost/virtio driver side.
A better to convert cf_mutex to rw_sem that clarifies the code more and still ensure that netlink doesn't read bytes while it is getting modified.

Given that config bytes can be updated anytime and not on each field boundary, it is anyway not atomic. It was added where we wanted to modify the fields post creation, but eventually drop that idea.

So yes, cf_mutex removal is fine too.

Quoted 2 lines

After some thought I don't see a case. I can think of NEEDS_RESET but it should come with a config interrupt so we're probably fine without the mutex here.

Quoted 70 lines · Si-Wei Liu

Quoted 67 lines · Jason Wang

Quoted 65 lines · Si-Wei Liu

Quoted 60 lines · Jason Wang

Quoted 50 lines · Si-Wei Liu

Quoted 48 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
   drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++
   drivers/vhost/vdpa.c         |  7 +++----
   drivers/virtio/virtio_vdpa.c |  3 +--
   include/linux/vdpa.h         |  3 +++
   4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +18 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
   static DEFINE_MUTEX(vdpa_dev_mutex);
   static DEFINE_IDA(vdpa_index_ida);
   +u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+    u8 status;
+
+    mutex_lock(&vdev->cf_mutex);
+    status = vdev->config->get_status(vdev);
+    mutex_unlock(&vdev->cf_mutex);
+    return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+    mutex_lock(&vdev->cf_mutex);
+    vdev->config->set_status(vdev, status);
+    mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
   static struct genl_family vdpa_nl_family;
     static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644
@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct
 vhost_vdpa *v, u8 __user *argp)
   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8
 __user *statusp)

Quoted 8 lines

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct · line 149

Quoted 6 lines

drivers/vhost/vdpa.c +1 -1

   {
       struct vdpa_device *vdpa = v->vdpa;
-    const struct vdpa_config_ops *ops = vdpa->config;
       u8 status;
   -    status = ops->get_status(vdpa);
+    status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

Quoted 2 lines

You may be right but it doesn't harm and it is guaranteed to be correct if we protect it with mutex here.

Is it more for future proof?

Quoted 7 lines

Quoted 1 line

I think so.

I guess in this situation it would be better defer to the future patch to add such locking or wrapper, although right now there are just two additional calls taking the lock needlessly when vhost_vdpa_get_status is called. Maybe it's just me but I'm worried some future patch may calls the locked API wrapper thousands of times unintendedly, then the overhead becomes quite obvious.

I'm fine to remove this if others agree on this.

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

Ok, but IMHO it might be better to get some comment here, otherwise it's quite confusing why the lock needs to be held. vhost_vdpa had done its own serialization in vhost_vdpa_unlocked_ioctl() through vhost_dev`mutex.

Right, but they are done for different levels, one is for vhost_dev antoher is for vdpa_dev.

Quoted 5 lines

The cf_mutex is introduced to serialize the command line configure thread (via netlink) and the upstream driver calls from either the vhost_dev or virtio_dev. I don't see a case, even in future, where the netlink thread needs to update the virtio status on the fly. Can you enlighten me why that is at all possible?

After some thought I don't see a case. I can think of NEEDS_RESET but it should come with a config interrupt so we're probably fine without the mutex here.

Thanks

Quoted 62 lines

Thanks,
-Siwei

Quoted 60 lines

Thanks

Quoted 57 lines

-Siwei

Quoted 55 lines

Thanks

Quoted 52 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 167

Quoted 8 lines

drivers/vhost/vdpa.c +1 -0

         if (copy_to_user(statusp, &status, sizeof(status)))
           return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct
 vhost_vdpa *v, u8 __user *statusp)
       if (copy_from_user(&status, statusp, sizeof(status)))
           return -EFAULT;
   -    status_old = ops->get_status(vdpa);
+    status_old = vdpa_get_status(vdpa);

Ditto.

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct · line 185

Quoted 9 lines

drivers/vhost/vdpa.c +1 -1

         /*
        * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct
 vhost_vdpa *v, u8 __user *statusp)
           if (ret)
               return ret;
       } else
-        ops->set_status(vdpa, status);
+        vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Thanks,
-Siwei

Quoted 26 lines

drivers/vhost/vdpa.c +0 -0

         if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old &
 VIRTIO_CONFIG_S_DRIVER_OK))

           for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -1

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct
 virtio_device *vdev)
   static void virtio_vdpa_set_status(struct virtio_device *vdev,
 u8 status)
   {
       struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-    const struct vdpa_config_ops *ops = vdpa->config;
   -    return ops->set_status(vdpa, status);
+    return vdpa_set_status(vdpa, status);
   }
     static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +0 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev,
 unsigned int offset,
                void *buf, unsigned int len);

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

include/linux/vdpa.h +3 -0

   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
                const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
   /**
    * struct vdpa_mgmtdev_ops - vdpa device ops

* @dev_add: Add a vdpa device using alloc and register

Quoted 48 lines · Si-Wei Liu

Quoted 46 lines · Eli Cohen

Add wrappers to get/set status and protect these operations with cf_mutex to serialize these operations with respect to get/set config operations.

Signed-off-by Eli Cohen <[email protected]>

---
   drivers/vdpa/vdpa.c          | 19 +++++++++++++++++++
   drivers/vhost/vdpa.c         |  7 +++----
   drivers/virtio/virtio_vdpa.c |  3 +--
   include/linux/vdpa.h         |  3 +++
   4 files changed, 26 insertions(+), 6 deletions(-)

drivers/vdpa/vdpa.c +19 -0

index 42d71d60d5dc..5134c83c4a22 100644
@@ -21,6 +21,25 @@ static LIST_HEAD(mdev_head);
   static DEFINE_MUTEX(vdpa_dev_mutex);
   static DEFINE_IDA(vdpa_index_ida);
+u8 vdpa_get_status(struct vdpa_device *vdev)
+{
+	u8 status;
+
+	mutex_lock(&vdev->cf_mutex);
+	status = vdev->config->get_status(vdev);
+	mutex_unlock(&vdev->cf_mutex);
+	return status;
+}
+EXPORT_SYMBOL(vdpa_get_status);
+
+void vdpa_set_status(struct vdpa_device *vdev, u8 status)
+{
+	mutex_lock(&vdev->cf_mutex);
+	vdev->config->set_status(vdev, status);
+	mutex_unlock(&vdev->cf_mutex);
+}
+EXPORT_SYMBOL(vdpa_set_status);
+
   static struct genl_family vdpa_nl_family;
   static int vdpa_dev_probe(struct device *d)

drivers/vhost/vdpa.c +0 -0

index ebaa373e9b82..d9d499465e2e 100644
@@ -142,10 +142,9 @@ static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)
   static long vhost_vdpa_get_status(struct vhost_vdpa *v, u8 __user *statusp)

Quoted 9 lines

drivers/vhost/vdpa.c static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp) · line 146

Quoted 6 lines

drivers/vhost/vdpa.c +1 -2

   {
   	struct vdpa_device *vdpa = v->vdpa;
-	const struct vdpa_config_ops *ops = vdpa->config;
   	u8 status;
-	status = ops->get_status(vdpa);
+	status = vdpa_get_status(vdpa);

Not sure why we need to take cf_mutex here. Appears to me only setters (set_status and reset) need to take the lock in this function.

What if the implenentation of get_status() at the upstream driver requires some setup. Taking cf_mutex seems like a good thing to do.

Quoted 10 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) · line 165

Quoted 8 lines

drivers/vhost/vdpa.c +1 -1

   	if (copy_to_user(statusp, &status, sizeof(status)))
   		return -EFAULT;

@@ -164,7 +163,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
   	if (copy_from_user(&status, statusp, sizeof(status)))
   		return -EFAULT;
-	status_old = ops->get_status(vdpa);
+	status_old = vdpa_get_status(vdpa);

Ditto.

Quoted 12 lines

drivers/vhost/vdpa.c static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) · line 184

Quoted 8 lines

drivers/vhost/vdpa.c +1 -1

   	/*
   	 * Userspace shouldn't remove status bits unless reset the

@@ -182,7 +181,7 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp)
   		if (ret)
   			return ret;
   	} else
-		ops->set_status(vdpa, status);
+		vdpa_set_status(vdpa, status);

The reset() call in the if branch above needs to take the cf_mutex, the same way as that for set_status(). The reset() is effectively set_status(vdpa, 0).

Right, I missed that. Will send a fix.

Quoted 24 lines

Thanks,
-Siwei

Quoted 21 lines

drivers/vhost/vdpa.c +0 -0

   	if ((status & VIRTIO_CONFIG_S_DRIVER_OK) && !(status_old & VIRTIO_CONFIG_S_DRIVER_OK))
   		for (i = 0; i < nvqs; i++)

drivers/virtio/virtio_vdpa.c +1 -2

index a84b04ba3195..76504559bc25 100644
@@ -91,9 +91,8 @@ static u8 virtio_vdpa_get_status(struct virtio_device *vdev)
   static void virtio_vdpa_set_status(struct virtio_device *vdev, u8 status)
   {
   	struct vdpa_device *vdpa = vd_get_vdpa(vdev);
-	const struct vdpa_config_ops *ops = vdpa->config;
-	return ops->set_status(vdpa, status);
+	return vdpa_set_status(vdpa, status);
   }
   static void virtio_vdpa_reset(struct virtio_device *vdev)

include/linux/vdpa.h +0 -0

index 9cc4291a79b3..ae047fae2603 100644
@@ -408,6 +408,9 @@ void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
   		     void *buf, unsigned int len);

Quoted 8 lines

include/linux/vdpa.h void vdpa_get_config(struct vdpa_device *vdev, unsigned int offset, · line 416

Quoted 8 lines

include/linux/vdpa.h +3 -0

   void vdpa_set_config(struct vdpa_device *dev, unsigned int offset,
   		     const void *buf, unsigned int length);
+u8 vdpa_get_status(struct vdpa_device *vdev);
+void vdpa_set_status(struct vdpa_device *vdev, u8 status);
+
   /**
    * struct vdpa_mgmtdev_ops - vdpa device ops
    * @dev_add: Add a vdpa device using alloc and register

Add netlink support to configure the max virtqueue pairs for a device. At least one pair is required. The maximum is dictated by the device.

Example:

$ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 4

Signed-off-by Eli Cohen <[email protected]>

---
v6->v7:
1.Serialize set_features and reset using cf_mutex to ensure consistency with netlink set/get

  drivers/vdpa/vdpa.c          | 15 +++++++++++++--
  drivers/vhost/vdpa.c         |  2 +-
  drivers/virtio/virtio_vdpa.c |  2 +-
  include/linux/vdpa.h         | 19 ++++++++++++++++---
  4 files changed, 31 insertions(+), 7 deletions(-)

drivers/vdpa/vdpa.c +0 -0

index 4494325cae91..96d31b80fdce 100644

drivers/vdpa/vdpa.c +12 -1

  	ops->get_config(vdev, offset, buf, len);
  }
 

@@ -581,7 +581,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
  }
 
  #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
-				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
+				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
+				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
 
  static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
  {

@@ -607,6 +608,16 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
  			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
  		config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
  	}
+	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
+		config.net.max_vq_pairs =
+			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
+		if (!config.net.max_vq_pairs) {
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "At least one pair of VQs is required");
+			return -EINVAL;
+		}
+		config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
+	}
 
  	/* Skip checking capability if user didn't prefer to configure any
  	 * device networking attributes. It is likely that user might have used

drivers/vhost/vdpa.c +1 -1

index d9d499465e2e..c37a63ba620a 100644
@@ -285,7 +285,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))
+	if (vdpa_set_features(vdpa, features, false))
  		return -EINVAL;
 
  	return 0;

drivers/virtio/virtio_vdpa.c +1 -1

index 76504559bc25..7767a7f0119b 100644
@@ -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);
+	return vdpa_set_features(vdpa, vdev->features, false);
  }
 
  static const char *virtio_vdpa_bus_name(struct virtio_device *vdev)

include/linux/vdpa.h +1 -0

index ae047fae2603..6d4d7e4fe208 100644
@@ -101,6 +101,7 @@ struct vdpa_dev_set_config {
  	struct {
  		u8 mac[ETH_ALEN];
  		u16 mtu;
+		u16 max_vq_pairs;
  	} net;
  	u64 mask;
  };

@@ -391,17 +392,29 @@ static inline struct device *vdpa_get_dma_dev(struct vdpa_device *vdev)
  static inline int vdpa_reset(struct vdpa_device *vdev)
  {

include/linux/vdpa.h +5 -1

-static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features)
+static inline int vdpa_set_features(struct vdpa_device *vdev, u64 features, bool locked)
  {
  	const struct vdpa_config_ops *ops = vdev->config;
+	int ret;
+
+	if (!locked)
+		mutex_lock(&vdev->cf_mutex);
 

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

  drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
  1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +10 -9

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
  	struct mlx5_vq_restore_info ri;
  };
 
-/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
  static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
  {
  	if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
  	struct mlx5_vdpa_dev mvdev;
  	struct mlx5_vdpa_net_resources res;
  	struct virtio_net_config config;
-	struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-	struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+	struct mlx5_vdpa_virtqueue *vqs;
+	struct vdpa_callback *event_cbs;
 
  	/* Serialize vq resources creation and destruction. This is required
  	 * since memory map might change and we need to destroy and create

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net *ndev)
  {
  	int i;
 
-	for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+	for (i = 0; i < ndev->mvdev.max_vqs; i++)
  		suspend_vq(ndev, &ndev->vqs[i]);
  }
 

@@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
  	void *in;
  	int i, j;
  	int err;
+	int num;
 
-	max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+	if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
+		num = 1;
+	else
+		num = le16_to_cpu(ndev->config.max_virtqueue_pairs);
+
+	max_rqt = min_t(int, roundup_pow_of_two(num),
  			1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
  	if (max_rqt < 1)
  		return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c +24 -3

  	MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
  	err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
  	clear_vqs_ready(ndev);
  	mlx5_vdpa_destroy_mr(&ndev->mvdev);
  	ndev->mvdev.status = 0;
-	memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+	memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
  	ndev->mvdev.actual_features = 0;
  	++mvdev->generation;
  	if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device *vdev)
  	}
  	mlx5_vdpa_free_resources(&ndev->mvdev);
  	mutex_destroy(&ndev->reslock);
+	kfree(ndev->event_cbs);
+	kfree(ndev->vqs);
  }
 
  static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
  		return -EOPNOTSUPP;
  	}
 
-	/* we save one virtqueue for control virtqueue should we require it */
  	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
-	max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+	if (max_vqs < 2) {
+		dev_warn(mdev->device,
+			 "%d virtqueues are supported. At least 2 are required\n",
+			 max_vqs);
+		return -EAGAIN;
+	}
+
+	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+		if (add_config->net.max_vq_pairs > max_vqs / 2)
+			return -EINVAL;
+		max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
+	} else {
+		max_vqs = 2;
+	}
 
  	ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops,
  				 name, false);
  	if (IS_ERR(ndev))
  		return PTR_ERR(ndev);
 
+	ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+	ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
+	if (!ndev->vqs || !ndev->event_cbs) {
+		err = -ENOMEM;
+		goto err_alloc;
+	}
  	ndev->mvdev.max_vqs = max_vqs;
  	mvdev = &ndev->mvdev;
  	mvdev->mdev = mdev;

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
  		mlx5_mpfs_del_mac(pfmdev, config->mac);
  err_mtu:
  	mutex_destroy(&ndev->reslock);
+err_alloc:
  	put_device(&mvdev->vdev.dev);
  	return err;
  }

Quoted 64 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

  drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------

  1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +3 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is required
        * since memory map might change and we need to destroy and create

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net 
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }

  @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net *ndev) void *in;
      int i, j;
      int err;
+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs); +
+    max_rqt = min_t(int, roundup_pow_of_two(num),
              1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); if (max_rqt < 1)
          return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j % 
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Reviewed-by Si-Wei Liu<[email protected]>

Quoted 72 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device 
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * 
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

      ++mvdev->generation;
      if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device 
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct 
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }

  -    /* we save one virtqueue for control virtqueue should we require it */ max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n",
+             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 *
add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops, name, false);
      if (IS_ERR(ndev))
          return PTR_ERR(ndev);
  +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+    ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
      ndev->mvdev.max_vqs = max_vqs;
      mvdev = &ndev->mvdev;
      mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct 
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);

  err_mtu:
      mutex_destroy(&ndev->reslock);
+err_alloc:
      put_device(&mvdev->vdev.dev);
      return err;
  }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device 
 *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;

Quoted 8 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask = 
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask = 

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP); mgtdev->madev = madev;
        err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 76 lines · Si-Wei Liu

Quoted 65 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

  drivers/vdpa/mlx5/net/mlx5_vnet.c | 51
++++++++++++++++++++++---------

  1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +3 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is required
        * since memory map might change and we need to destroy and create

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net 
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }

  @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net *ndev) void *in;
      int i, j;
      int err;
+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs); +
+    max_rqt = min_t(int, roundup_pow_of_two(num),
              1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); if (max_rqt < 1)
          return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j % 
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly. Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 81 lines

Quoted 80 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device 
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * 
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

      ++mvdev->generation;
      if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device 
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct 
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }

  -    /* we save one virtqueue for control virtqueue should we require it */ max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n",
+             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 *
add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops, name, false);
      if (IS_ERR(ndev))
          return PTR_ERR(ndev);
  +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+    ndev->event_cbs = kcalloc(max_vqs + 1,
sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
      ndev->mvdev.max_vqs = max_vqs;
      mvdev = &ndev->mvdev;
      mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct 
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);

  err_mtu:
      mutex_destroy(&ndev->reslock);
+err_alloc:
      put_device(&mvdev->vdev.dev);
      return err;
  }

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device 
 *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;
       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask = 
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask = 

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP); mgtdev->madev = madev;
        err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 80 lines · Si-Wei Liu

Quoted 77 lines · Si-Wei Liu

Quoted 65 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

  drivers/vdpa/mlx5/net/mlx5_vnet.c | 51
++++++++++++++++++++++---------

  1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +3 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is required
        * since memory map might change and we need to destroy and create

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net 
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }

  @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net *ndev) void *in;
      int i, j;
      int err;
+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs); +
+    max_rqt = min_t(int, roundup_pow_of_two(num),
              1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size)); if (max_rqt < 1)
          return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net · line 1268

Quoted 7 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net 
 *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j % 
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 3 lines

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly. Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

Such validation is not mandated in the spec. And if we want to do that, it needs to be done in a separate patch.

Thanks

Quoted 90 lines

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 82 lines

Quoted 81 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);

      err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen, &ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device 
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * 
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

      ++mvdev->generation;
      if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device 
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct 
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }

  -    /* we save one virtqueue for control virtqueue should we require it */ max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are
required\n",
+             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 *
add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops, name, false);
      if (IS_ERR(ndev))
          return PTR_ERR(ndev);
  +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+    ndev->event_cbs = kcalloc(max_vqs + 1,
sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
      ndev->mvdev.max_vqs = max_vqs;
      mvdev = &ndev->mvdev;
      mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct 
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);

  err_mtu:
      mutex_destroy(&ndev->reslock);
+err_alloc:
      put_device(&mvdev->vdev.dev);
      return err;
  }

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct 
 auxiliary_device *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;
       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask = 
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask = 

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
+ BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP); mgtdev->madev = madev;
        err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 81 lines · Si-Wei Liu

Quoted 79 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

   drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
   1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is

required
       * since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }
   @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)
      void *in;
      int i, j;
      int err;
+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
               1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
       if (max_rqt < 1)
           return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Reviewed-by Si-Wei Liu<[email protected]>

Quoted 3 lines

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

create RQT with 1 entry only.

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

I think it is ok to create the RQT always with the value configured when the device was created.

Quoted 86 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 76 lines

Quoted 76 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

      ++mvdev->generation;
      if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }
   -    /* we save one virtqueue for control virtqueue should we

require it */
      max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS); +    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n", +             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) { +        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,
mdev->device, &mlx5_vdpa_ops,
                   name, false);
      if (IS_ERR(ndev))
          return PTR_ERR(ndev);
  +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+    ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
      ndev->mvdev.max_vqs = max_vqs;
      mvdev = &ndev->mvdev;
      mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);
   err_mtu:
       mutex_destroy(&ndev->reslock);
+err_alloc:
       put_device(&mvdev->vdev.dev);
       return err;

  }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask =
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask =

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
      mgtdev->madev = madev;
        err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 93 lines · Si-Wei Liu

Quoted 90 lines · Eli Cohen

Quoted 79 lines · Si-Wei Liu

Quoted 77 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

   drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
   1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is

required
* since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }
   @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)

       void *in;
       int i, j;
       int err;

drivers/vdpa/mlx5/net/mlx5_vnet.c +7 -0

+    int num;
   -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
+        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);
+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
               1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
       if (max_rqt < 1)
           return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 5 lines

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

Quoted 1 line

Once VM is rebooted, the cur_num_vqs has to reset to 0 in the reset() op.

Why reset to 0? The correct value to set here is
config->max_virtqueue_pairs.

This is what happens when you add the
device. Setting cur_num_vqs to config->max_virtqueue_pairs will address your concerns with respect to modify_rqt. Once you reset cur_num_vqs to config->max_virtqueue_pairs your concerns with respect to changing mumber of QPs should be addressed. We could even leave create_rqt untouched or modify the code to use cur_num_vqs. It should work the same.

Quoted 140 lines

Quoted 1 line

create RQT with 1 entry only.

Here cur_num_vqs will be loaded with the newly negotiated value (max_rqt) again.

Quoted 2 lines

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

It won't fail as the cur_num_vqs will be consistent with the number of queues newly negotiated (i.e the max_rqt in create_rqt) for modify.

Quoted 3 lines

I think it is ok to create the RQT always with the value configured when the device was created.

The problem of not setting cur_num_vqs in create_rqt (and resetting it in mlx5_vdpa_reset) is that, once the VM is rebooted or the device is reset, the value doesn't go with the actual number of rqt entries hence would break various assumptions in change_num_qps() or modify_rqt(). For instances,

create vdpa device with max data VQs = 16
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6.
cur_num_vqs was 2 while the actual RQT size is 8 (= 16 / 2)
change_num_qps would fail as the wrong increment branch (rather than the decrement) was taken

and this case too, which calls out the need to validate the presence of VIRTIO_NET_F_MQ in handle_ctrl_mq()

create vdpa device with max data VQs = 16 (RQT size created with 8 entries)
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with no MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6. suppose guest runs a custom kernel without checking the #channels to configure change_num_qps can succeed and no host side check prohibiting a single queue guest to set multi-queue

Thanks,
-Siwei

Quoted 94 lines

Quoted 94 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 84 lines

Quoted 84 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

       ++mvdev->generation;
       if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }
   -    /* we save one virtqueue for control virtqueue should we

require it */
max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);

drivers/vdpa/mlx5/net/mlx5_vnet.c +11 -1

-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n",
+             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 *

add_config->net.max_vq_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +3 -0

+    } else {
+        max_vqs = 2;
+    }
         ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,

mdev->device, &mlx5_vdpa_ops,

                    name, false);
       if (IS_ERR(ndev))
           return PTR_ERR(ndev);

+ ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+ ndev->event_cbs = kcalloc(max_vqs + 1,
sizeof(*ndev->event_cbs), GFP_KERNEL);

drivers/vdpa/mlx5/net/mlx5_vnet.c +5 -0

+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
       ndev->mvdev.max_vqs = max_vqs;
       mvdev = &ndev->mvdev;
       mvdev->mdev = mdev;

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);
   err_mtu:
       mutex_destroy(&ndev->reslock);
+err_alloc:
       put_device(&mvdev->vdev.dev);
       return err;

   }

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;
       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask =
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask =

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |

+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
       mgtdev->madev = madev;
         err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 95 lines · Eli Cohen

Quoted 93 lines · Si-Wei Liu

Quoted 90 lines · Eli Cohen

Quoted 79 lines · Si-Wei Liu

Quoted 77 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

   drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
   1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is

required
* since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }
   @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)

       void *in;
       int i, j;
       int err;

drivers/vdpa/mlx5/net/mlx5_vnet.c +7 -0

+    int num;
   -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
+        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);
+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
               1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
       if (max_rqt < 1)
           return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 5 lines

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

Quoted 4 lines

Quoted 1 line

Once VM is rebooted, the cur_num_vqs has to reset to 0 in the reset() op.

Why reset to 0? The correct value to set here is
config->max_virtqueue_pairs.

Multiqueue is disabled by default so I think it should be 1 here. This is the behavior of Qemu.

It looks to me we need to clarify this in the spec.

Thanks

Quoted 141 lines

This is what happens when you add the
device. Setting cur_num_vqs to config->max_virtqueue_pairs will address your concerns with respect to modify_rqt. Once you reset cur_num_vqs to config->max_virtqueue_pairs your concerns with respect to changing mumber of QPs should be addressed. We could even leave create_rqt untouched or modify the code to use cur_num_vqs. It should work the same.

Quoted 133 lines

Quoted 1 line

create RQT with 1 entry only.

Here cur_num_vqs will be loaded with the newly negotiated value (max_rqt) again.

Quoted 2 lines

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

It won't fail as the cur_num_vqs will be consistent with the number of queues newly negotiated (i.e the max_rqt in create_rqt) for modify.

Quoted 3 lines

I think it is ok to create the RQT always with the value configured when the device was created.

The problem of not setting cur_num_vqs in create_rqt (and resetting it in mlx5_vdpa_reset) is that, once the VM is rebooted or the device is reset, the value doesn't go with the actual number of rqt entries hence would break various assumptions in change_num_qps() or modify_rqt(). For instances,

create vdpa device with max data VQs = 16
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6.
cur_num_vqs was 2 while the actual RQT size is 8 (= 16 / 2)
change_num_qps would fail as the wrong increment branch (rather than the decrement) was taken

and this case too, which calls out the need to validate the presence of VIRTIO_NET_F_MQ in handle_ctrl_mq()

create vdpa device with max data VQs = 16 (RQT size created with 8 entries)
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with no MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6. suppose guest runs a custom kernel without checking the #channels to configure change_num_qps can succeed and no host side check prohibiting a single queue guest to set multi-queue

Thanks,
-Siwei

Quoted 88 lines

Quoted 88 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 78 lines

Quoted 78 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

       ++mvdev->generation;
       if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }
   -    /* we save one virtqueue for control virtqueue should we

require it */
max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);

drivers/vdpa/mlx5/net/mlx5_vnet.c +11 -1

-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n",
+             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 *

add_config->net.max_vq_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +3 -0

+    } else {
+        max_vqs = 2;
+    }
         ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,

mdev->device, &mlx5_vdpa_ops,

                    name, false);
       if (IS_ERR(ndev))
           return PTR_ERR(ndev);

+ ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+ ndev->event_cbs = kcalloc(max_vqs + 1,
sizeof(*ndev->event_cbs), GFP_KERNEL);

drivers/vdpa/mlx5/net/mlx5_vnet.c +5 -0

+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
       ndev->mvdev.max_vqs = max_vqs;
       mvdev = &ndev->mvdev;
       mvdev->mdev = mdev;

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);
   err_mtu:
       mutex_destroy(&ndev->reslock);
+err_alloc:
       put_device(&mvdev->vdev.dev);
       return err;

   }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -1

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;
       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask =

Quoted 8 lines

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask =

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |

+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
       mgtdev->madev = madev;
         err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 101 lines · Jason Wang

Quoted 99 lines · Eli Cohen

Quoted 88 lines · Si-Wei Liu

Quoted 85 lines · Eli Cohen

Quoted 84 lines · Si-Wei Liu

Quoted 77 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

   drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
   1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
       struct mlx5_vq_restore_info ri;
   };
   -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
   static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
   {
       if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
       struct mlx5_vdpa_dev mvdev;
       struct mlx5_vdpa_net_resources res;
       struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
         /* Serialize vq resources creation and destruction. This is

required
* since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
   {
       int i;
   -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
           suspend_vq(ndev, &ndev->vqs[i]);
   }
   @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)

       void *in;
       int i, j;
       int err;

drivers/vdpa/mlx5/net/mlx5_vnet.c +7 -0

+    int num;
   -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
+        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);
+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
               1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
       if (max_rqt < 1)
           return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
       MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
       list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
       for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

Quoted 6 lines

Quoted 5 lines

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

Once VM is rebooted, the cur_num_vqs has to reset to 0 in the reset() op.

Why reset to 0? The correct value to set here is
config->max_virtqueue_pairs.

Quoted 6 lines

Multiqueue is disabled by default so I think it should be 1 here. This is the behavior of Qemu.

It looks to me we need to clarify this in the spec.

Thanks

Not sure what do you want to clarify.

Quoted 143 lines

Quoted 143 lines

This is what happens when you add the
device. Setting cur_num_vqs to config->max_virtqueue_pairs will address your concerns with respect to modify_rqt. Once you reset cur_num_vqs to config->max_virtqueue_pairs your concerns with respect to changing mumber of QPs should be addressed. We could even leave create_rqt untouched or modify the code to use cur_num_vqs. It should work the same.

Quoted 133 lines

Quoted 1 line

create RQT with 1 entry only.

Here cur_num_vqs will be loaded with the newly negotiated value (max_rqt) again.

Quoted 2 lines

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

It won't fail as the cur_num_vqs will be consistent with the number of queues newly negotiated (i.e the max_rqt in create_rqt) for modify.

Quoted 3 lines

I think it is ok to create the RQT always with the value configured when the device was created.

The problem of not setting cur_num_vqs in create_rqt (and resetting it in mlx5_vdpa_reset) is that, once the VM is rebooted or the device is reset, the value doesn't go with the actual number of rqt entries hence would break various assumptions in change_num_qps() or modify_rqt(). For instances,

create vdpa device with max data VQs = 16
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6.
cur_num_vqs was 2 while the actual RQT size is 8 (= 16 / 2)
change_num_qps would fail as the wrong increment branch (rather than the decrement) was taken

and this case too, which calls out the need to validate the presence of VIRTIO_NET_F_MQ in handle_ctrl_mq()

create vdpa device with max data VQs = 16 (RQT size created with 8 entries)
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with no MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6. suppose guest runs a custom kernel without checking the #channels to configure change_num_qps can succeed and no host side check prohibiting a single queue guest to set multi-queue

Thanks,
-Siwei

Quoted 88 lines

Quoted 88 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 78 lines

Quoted 78 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

         MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
       err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
       clear_vqs_ready(ndev);
       mlx5_vdpa_destroy_mr(&ndev->mvdev);
       ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
       ndev->mvdev.actual_features = 0;

       ++mvdev->generation;
       if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
       }
       mlx5_vdpa_free_resources(&ndev->mvdev);
       mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
   }
     static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           return -EOPNOTSUPP;
       }
   -    /* we save one virtqueue for control virtqueue should we

require it */
max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);

drivers/vdpa/mlx5/net/mlx5_vnet.c +11 -1

-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
+    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n",
+             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) {
+        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 *

add_config->net.max_vq_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +3 -0

+    } else {
+        max_vqs = 2;
+    }
         ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,

mdev->device, &mlx5_vdpa_ops,

                    name, false);
       if (IS_ERR(ndev))
           return PTR_ERR(ndev);

+ ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+ ndev->event_cbs = kcalloc(max_vqs + 1,
sizeof(*ndev->event_cbs), GFP_KERNEL);

drivers/vdpa/mlx5/net/mlx5_vnet.c +5 -0

+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
       ndev->mvdev.max_vqs = max_vqs;
       mvdev = &ndev->mvdev;
       mvdev->mdev = mdev;

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
           mlx5_mpfs_del_mac(pfmdev, config->mac);
   err_mtu:
       mutex_destroy(&ndev->reslock);
+err_alloc:
       put_device(&mvdev->vdev.dev);
       return err;

   }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -1

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
       mgtdev->mgtdev.ops = &mdev_ops;
       mgtdev->mgtdev.device = mdev->device;
       mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask =

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask =

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |

+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
       mgtdev->madev = madev;
         err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 85 lines · Si-Wei Liu

Quoted 83 lines · Eli Cohen

Quoted 82 lines · Si-Wei Liu

Quoted 76 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

    drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
    1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
        struct mlx5_vq_restore_info ri;
    };
    -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
    static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
    {
        if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
        struct mlx5_vdpa_dev mvdev;
        struct mlx5_vdpa_net_resources res;
        struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
          /* Serialize vq resources creation and destruction. This is

required
       * since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
    {
        int i;
    -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
            suspend_vq(ndev, &ndev->vqs[i]);
    }
    @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)
      void *in;
      int i, j;
      int err;
+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
                1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
        if (max_rqt < 1)
            return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
        MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
        list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
        for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

Quoted 6 lines

Quoted 5 lines

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

Once VM is rebooted, the cur_num_vqs has to reset to 0 in the reset() op.

Why reset to 0? The correct value to set here is
config->max_virtqueue_pairs.

This is what happens when you add the
device.

Setting cur_num_vqs to config->max_virtqueue_pairs will address
your concerns with respect to modify_rqt. Once you reset cur_num_vqs to config->max_virtqueue_pairs your concerns with respect to changing mumber of QPs should be addressed.

We could even leave create_rqt
untouched or modify the code to use cur_num_vqs. It should work the same.

Quoted 129 lines

Quoted 1 line

create RQT with 1 entry only.

Here cur_num_vqs will be loaded with the newly negotiated value (max_rqt) again.

Quoted 2 lines

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

It won't fail as the cur_num_vqs will be consistent with the number of queues newly negotiated (i.e the max_rqt in create_rqt) for modify.

Quoted 2 lines

I think it is ok to create the RQT always with the value configured when the device was created.

The problem of not setting cur_num_vqs in create_rqt (and resetting it in mlx5_vdpa_reset) is that, once the VM is rebooted or the device is reset, the value doesn't go with the actual number of rqt entries hence would break various assumptions in change_num_qps() or modify_rqt(). For instances,

create vdpa device with max data VQs = 16
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6.
cur_num_vqs was 2 while the actual RQT size is 8 (= 16 / 2)
change_num_qps would fail as the wrong increment branch (rather than the decrement) was taken

and this case too, which calls out the need to validate the presence of VIRTIO_NET_F_MQ in handle_ctrl_mq()

create vdpa device with max data VQs = 16 (RQT size created with 8 entries)
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with no MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6. suppose guest runs a custom kernel without checking the #channels to configure change_num_qps can succeed and no host side check prohibiting a single queue guest to set multi-queue

Thanks,
-Siwei

Quoted 85 lines

Quoted 85 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 76 lines

Quoted 76 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

          MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
        err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
        clear_vqs_ready(ndev);
        mlx5_vdpa_destroy_mr(&ndev->mvdev);
        ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
        ndev->mvdev.actual_features = 0;

        ++mvdev->generation;
        if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
        }
        mlx5_vdpa_free_resources(&ndev->mvdev);
        mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
    }
      static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
            return -EOPNOTSUPP;
        }
    -    /* we save one virtqueue for control virtqueue should we

require it */
      max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS); +    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n", +             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) { +        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,
mdev->device, &mlx5_vdpa_ops,
                   name, false);
      if (IS_ERR(ndev))
          return PTR_ERR(ndev);
  +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+    ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
      ndev->mvdev.max_vqs = max_vqs;
      mvdev = &ndev->mvdev;
      mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
            mlx5_mpfs_del_mac(pfmdev, config->mac);
    err_mtu:
        mutex_destroy(&ndev->reslock);
+err_alloc:
        put_device(&mvdev->vdev.dev);
        return err;

    }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
        mgtdev->mgtdev.ops = &mdev_ops;
        mgtdev->mgtdev.device = mdev->device;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

        mgtdev->mgtdev.id_table = id_table;
-    mgtdev->mgtdev.config_attr_mask =
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask =

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
      mgtdev->madev = madev;
        err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 134 lines · Si-Wei Liu

Quoted 86 lines · Eli Cohen

Quoted 85 lines · Si-Wei Liu

Quoted 83 lines · Eli Cohen

Quoted 82 lines · Si-Wei Liu

Quoted 76 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

    drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
    1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
        struct mlx5_vq_restore_info ri;
    };
    -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
    static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
    {
        if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
        struct mlx5_vdpa_dev mvdev;
        struct mlx5_vdpa_net_resources res;
        struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
          /* Serialize vq resources creation and destruction. This is

required
       * since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
    {
        int i;
    -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
            suspend_vq(ndev, &ndev->vqs[i]);
    }
    @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)
      void *in;
      int i, j;
      int err;
+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
                1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
        if (max_rqt < 1)
            return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
        MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
        list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
        for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

Once VM is rebooted, the cur_num_vqs has to reset to 0 in the reset() op.

Why reset to 0? The correct value to set here is
config->max_virtqueue_pairs.

I am not sure I get you, perhaps post a patch to show what you want to do? If you intend to set to 2 (non-MQ) or 2 * max_virtqueue_pairs (MQ) later in create_rqt(), maybe it's not quite relevant whether or not to reset. But in the habit of keeping things consistent I'd prefer reset to a value to reflect the fact that this field won't have a valid value until features are negotiated. Whenever there's a need in future, this field can be easily exposed to userspace.

Quoted 2 lines

This is what happens when you add the
device.

This is not relevant, and doesn't matter. Essentially that line of code to set cur_num_vqs in vdpa_add can be removed. The value of cur_num_vqs won't be effective before the MQ feature is negotiated, i.e. you don't get a sensible cur_num_vqs value before the size of RQT is derived from the MQ feature and the table gets created.

Quoted 4 lines

Setting cur_num_vqs to config->max_virtqueue_pairs will address
your concerns with respect to modify_rqt. Once you reset cur_num_vqs to config->max_virtqueue_pairs your concerns with respect to changing mumber of QPs should be addressed.

No, it won't. If you look at the caller of change_num_qps(), in handle_ctrl_mq() there's following code:

                if (ndev->cur_num_vqs == 2 * newqps) { status = VIRTIO_NET_OK; break;
                }

So if you set cur_num_vqs to max_virtqueue_pairs while MQ is not negotiated, it will result in a very weird success for a non-MQ supporting driver to be able to change the number of queues without changing anything effectively. You may argue that we can fail the non-MQ case early before coming to this code. But this is another patch, and would make code more obscure, not less. Intuitively people won't realize your cur_num_vqs doesn't apply to non-MQ just by follow the name.

Quoted 3 lines

Regards,
-Siwei

Si-Wei:
So your point is that you want to see cur_num_vqs 0 feature negotiated and other 2 for non MQ, and whatever configured through ethtool otherwise.

I can send a patch to address this concern of yours.

Quesiton is how we want to proceed here. I do want this series to get into 5.16. Should I send a new series or fixes on top of this? Also, it would be helpful if you could list all the changes you think should be made.

Michael: what do you think?

Quoted 135 lines

Quoted 135 lines

We could even leave create_rqt
untouched or modify the code to use cur_num_vqs. It should work the same.

Quoted 130 lines

Quoted 1 line

create RQT with 1 entry only.

Here cur_num_vqs will be loaded with the newly negotiated value (max_rqt) again.

Quoted 2 lines

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

It won't fail as the cur_num_vqs will be consistent with the number of queues newly negotiated (i.e the max_rqt in create_rqt) for modify.

Quoted 2 lines

I think it is ok to create the RQT always with the value configured when the device was created.

The problem of not setting cur_num_vqs in create_rqt (and resetting it in mlx5_vdpa_reset) is that, once the VM is rebooted or the device is reset, the value doesn't go with the actual number of rqt entries hence would break various assumptions in change_num_qps() or modify_rqt(). For instances,

create vdpa device with max data VQs = 16
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6.
cur_num_vqs was 2 while the actual RQT size is 8 (= 16 / 2)
change_num_qps would fail as the wrong increment branch (rather than the decrement) was taken

and this case too, which calls out the need to validate the presence of VIRTIO_NET_F_MQ in handle_ctrl_mq()

create vdpa device with max data VQs = 16 (RQT size created with 8 entries)
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with no MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6. suppose guest runs a custom kernel without checking the #channels to configure change_num_qps can succeed and no host side check prohibiting a single queue guest to set multi-queue

Thanks,
-Siwei

Quoted 86 lines

Quoted 86 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 77 lines

Quoted 77 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

          MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
        err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
        clear_vqs_ready(ndev);
        mlx5_vdpa_destroy_mr(&ndev->mvdev);
        ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
        ndev->mvdev.actual_features = 0;

        ++mvdev->generation;
        if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
        }
        mlx5_vdpa_free_resources(&ndev->mvdev);
        mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
    }
      static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
            return -EOPNOTSUPP;
        }
    -    /* we save one virtqueue for control virtqueue should we

require it */
      max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS); +    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n", +             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) { +        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,
mdev->device, &mlx5_vdpa_ops,
                   name, false);
      if (IS_ERR(ndev))
          return PTR_ERR(ndev);
  +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);
+    ndev->event_cbs = kcalloc(max_vqs + 1, sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }
      ndev->mvdev.max_vqs = max_vqs;
      mvdev = &ndev->mvdev;
      mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
            mlx5_mpfs_del_mac(pfmdev, config->mac);
    err_mtu:
        mutex_destroy(&ndev->reslock);
+err_alloc:
        put_device(&mvdev->vdev.dev);
        return err;

    }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
        mgtdev->mgtdev.ops = &mdev_ops;
        mgtdev->mgtdev.device = mdev->device;
        mgtdev->mgtdev.id_table = id_table;

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

-    mgtdev->mgtdev.config_attr_mask =
 BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+    mgtdev->mgtdev.config_attr_mask =

BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
+                      BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
      mgtdev->madev = madev;
        err = vdpa_mgmtdev_register(&mgtdev->mgtdev);

Quoted 132 lines · Si-Wei Liu

Quoted 85 lines · Eli Cohen

Quoted 84 lines · Si-Wei Liu

Quoted 83 lines · Eli Cohen

Quoted 82 lines · Si-Wei Liu

Quoted 76 lines · Si-Wei Liu

Quoted 67 lines · Eli Cohen

Check whether the max number of data virtqueue pairs was provided when a adding a new device and verify the new value does not exceed device capabilities.

In addition, change the arrays holding virtqueue and callback contexts to be dynamically allocated.

Signed-off-by Eli Cohen <[email protected]>

---
v6 -> v7:
1. Evaluate RQT table size based on config.max_virtqueue_pairs.

     drivers/vdpa/mlx5/net/mlx5_vnet.c | 51 ++++++++++++++++++++++---------
     1 file changed, 37 insertions(+), 14 deletions(-)

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -6

index 4a2149f70f1e..d4720444bf78 100644
@@ -131,11 +131,6 @@ struct mlx5_vdpa_virtqueue {
         struct mlx5_vq_restore_info ri;
     };
     -/* We will remove this limitation once mlx5_vdpa_alloc_resources()
- * provides for driver space allocation
- */
-#define MLX5_MAX_SUPPORTED_VQS 16
-
     static bool is_index_valid(struct mlx5_vdpa_dev *mvdev, u16 idx)
     {
         if (unlikely(idx > mvdev->max_idx))

@@ -148,8 +143,8 @@ struct mlx5_vdpa_net {
         struct mlx5_vdpa_dev mvdev;
         struct mlx5_vdpa_net_resources res;
         struct virtio_net_config config;
-    struct mlx5_vdpa_virtqueue vqs[MLX5_MAX_SUPPORTED_VQS];
-    struct vdpa_callback event_cbs[MLX5_MAX_SUPPORTED_VQS + 1];
+    struct mlx5_vdpa_virtqueue *vqs;
+    struct vdpa_callback *event_cbs;
           /* Serialize vq resources creation and destruction. This is

required
       * since memory map might change and we need to destroy and create

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -1218,7 +1213,7 @@ static void suspend_vqs(struct mlx5_vdpa_net
 *ndev)
     {
         int i;
     -    for (i = 0; i < MLX5_MAX_SUPPORTED_VQS; i++)
+    for (i = 0; i < ndev->mvdev.max_vqs; i++)
             suspend_vq(ndev, &ndev->vqs[i]);
     }
     @@ -1244,8 +1239,14 @@ static int create_rqt(struct mlx5_vdpa_net

*ndev)

         void *in;
         int i, j;
         int err;

+    int num;
  -    max_rqt = min_t(int, MLX5_MAX_SUPPORTED_VQS / 2,
+    if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))) +        num = 1;
+    else
+        num = le16_to_cpu(ndev->config.max_virtqueue_pairs);

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

+
+    max_rqt = min_t(int, roundup_pow_of_two(num),
                 1 << MLX5_CAP_GEN(ndev->mvdev.mdev, log_max_rqt_size));
         if (max_rqt < 1)
             return -EOPNOTSUPP;

drivers/vdpa/mlx5/net/mlx5_vnet.c static int create_rqt(struct mlx5_vdpa_net *ndev) · line 1267

Quoted 6 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -1262,7 +1263,7 @@ static int create_rqt(struct mlx5_vdpa_net *ndev)
         MLX5_SET(rqtc, rqtc, rqt_max_size, max_rqt);
         list = MLX5_ADDR_OF(rqtc, rqtc, rq_num[0]);
         for (i = 0, j = 0; i < max_rqt; i++, j += 2)
-        list[i] = cpu_to_be32(ndev->vqs[j %
 ndev->mvdev.max_vqs].virtq_id);
+        list[i] = cpu_to_be32(ndev->vqs[j % (2 * num)].virtq_id);

Good catch. LGTM.

Quoted 2 lines

Reviewed-by Si-Wei Liu<[email protected]>

Apologies to reply to myself. It looks to me we need to set cur_num_vqs to the negotiated num of queues. Otherwise any site referencing cur_num_vqs won't work properly.

Quoted 8 lines

Quoted 6 lines

Quoted 5 lines

You can't really use cur_num_vqs. Consider this scenario:
create vdpa device with max VQs = 16 (RQT size created with 8 entries) boot VM
ethtool modify num QPs to 2. cur_num_vqs now equals 2. reboot VM.

Once VM is rebooted, the cur_num_vqs has to reset to 0 in the reset() op.

Why reset to 0? The correct value to set here is
config->max_virtqueue_pairs.

I am not sure I get you, perhaps post a patch to show what you want to do? If you intend to set to 2 (non-MQ) or 2 * max_virtqueue_pairs (MQ) later in create_rqt(), maybe it's not quite relevant whether or not to reset. But in the habit of keeping things consistent I'd prefer reset to a value to reflect the fact that this field won't have a valid value until features are negotiated. Whenever there's a need in future, this field can be easily exposed to userspace.

Quoted 2 lines

This is what happens when you add the
device.

This is not relevant, and doesn't matter. Essentially that line of code to set cur_num_vqs in vdpa_add can be removed. The value of cur_num_vqs won't be effective before the MQ feature is negotiated, i.e. you don't get a sensible cur_num_vqs value before the size of RQT is derived from the MQ feature and the table gets created.

Quoted 4 lines

   Setting cur_num_vqs to config->max_virtqueue_pairs will address

your concerns with respect to modify_rqt. Once you reset cur_num_vqs to config->max_virtqueue_pairs your concerns with respect to changing mumber of QPs should be addressed.

No, it won't. If you look at the caller of change_num_qps(), in handle_ctrl_mq() there's following code:

                if (ndev->cur_num_vqs == 2 * newqps) { status = VIRTIO_NET_OK; break;
                }

So if you set cur_num_vqs to max_virtqueue_pairs while MQ is not negotiated, it will result in a very weird success for a non-MQ supporting driver to be able to change the number of queues without changing anything effectively. You may argue that we can fail the non-MQ case early before coming to this code. But this is another patch, and would make code more obscure, not less. Intuitively people won't realize your cur_num_vqs doesn't apply to non-MQ just by follow the name.

Quoted 3 lines

Regards,
-Siwei

Si-Wei:
So your point is that you want to see cur_num_vqs 0 feature negotiated and other 2 for non MQ, and whatever configured through ethtool otherwise.

I can send a patch to address this concern of yours.

Quesiton is how we want to proceed here. I do want this series to get into 5.16. Should I send a new series or fixes on top of this?

Also, it
would be helpful if you could list all the changes you think should be made.

Michael: what do you think?

Quoted 133 lines

Quoted 133 lines

We could even leave create_rqt
untouched or modify the code to use cur_num_vqs. It should work the same.

Quoted 129 lines

Quoted 1 line

create RQT with 1 entry only.

Here cur_num_vqs will be loaded with the newly negotiated value (max_rqt) again.

Quoted 2 lines

ethtool modify num QPs to 4. modify RQT will fail since it was created with max QPs equals 1.

It won't fail as the cur_num_vqs will be consistent with the number of queues newly negotiated (i.e the max_rqt in create_rqt) for modify.

Quoted 2 lines

I think it is ok to create the RQT always with the value configured when the device was created.

The problem of not setting cur_num_vqs in create_rqt (and resetting it in mlx5_vdpa_reset) is that, once the VM is rebooted or the device is reset, the value doesn't go with the actual number of rqt entries hence would break various assumptions in change_num_qps() or modify_rqt(). For instances,

create vdpa device with max data VQs = 16
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6.
cur_num_vqs was 2 while the actual RQT size is 8 (= 16 / 2)
change_num_qps would fail as the wrong increment branch (rather than the decrement) was taken

and this case too, which calls out the need to validate the presence of VIRTIO_NET_F_MQ in handle_ctrl_mq()

create vdpa device with max data VQs = 16 (RQT size created with 8 entries)
boot VM
features_ok set with MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 2. cur_num_vqs now equals 2.
reboot VM
features_ok set with no MQ negotiated
RQT created with 8 entries in create_rqt
ethtool modify num QPs to 6. suppose guest runs a custom kernel without checking the #channels to configure change_num_qps can succeed and no host side check prohibiting a single queue guest to set multi-queue

Thanks,
-Siwei

Quoted 85 lines

Quoted 85 lines

Further, we need to validate VIRTIO_NET_F_MQ is present in handle_ctrl_mq() before changing the number of queue pairs.

So just disregard my previous R-b for this patch.

Thanks,
-Siwei

Quoted 76 lines

Quoted 76 lines

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

           MLX5_SET(rqtc, rqtc, rqt_actual_size, max_rqt);
         err = mlx5_vdpa_create_rqt(&ndev->mvdev, in, inlen,

&ndev->res.rqtn);

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -1

@@ -2220,7 +2221,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
 *vdev)
         clear_vqs_ready(ndev);
         mlx5_vdpa_destroy_mr(&ndev->mvdev);
         ndev->mvdev.status = 0;
-    memset(ndev->event_cbs, 0, sizeof(ndev->event_cbs));
+    memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
 (mvdev->max_vqs + 1));
         ndev->mvdev.actual_features = 0;

         ++mvdev->generation;
         if (MLX5_CAP_GEN(mvdev->mdev, umem_uid_0)) {

drivers/vdpa/mlx5/net/mlx5_vnet.c +2 -0

@@ -2293,6 +2294,8 @@ static void mlx5_vdpa_free(struct vdpa_device
 *vdev)
         }
         mlx5_vdpa_free_resources(&ndev->mvdev);
         mutex_destroy(&ndev->reslock);
+    kfree(ndev->event_cbs);
+    kfree(ndev->vqs);
     }
       static struct vdpa_notification_area

mlx5_get_vq_notification(struct vdpa_device *vdev, u16 idx)

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2538,15 +2541,33 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
             return -EOPNOTSUPP;
         }
     -    /* we save one virtqueue for control virtqueue should we

require it */
      max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev,
max_num_virtio_queues);
-    max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS); +    if (max_vqs < 2) {
+        dev_warn(mdev->device,
+             "%d virtqueues are supported. At least 2 are required\n", +             max_vqs);
+        return -EAGAIN;
+    }
+
+    if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP)) { +        if (add_config->net.max_vq_pairs > max_vqs / 2)
+            return -EINVAL;
+        max_vqs = min_t(u32, max_vqs, 2 * add_config->net.max_vq_pairs);
+    } else {
+        max_vqs = 2;
+    }
        ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev,
mdev->device, &mlx5_vdpa_ops,

                      name, false);
         if (IS_ERR(ndev))
             return PTR_ERR(ndev);
     +    ndev->vqs = kcalloc(max_vqs, sizeof(*ndev->vqs), GFP_KERNEL);

+    ndev->event_cbs = kcalloc(max_vqs + 1,
sizeof(*ndev->event_cbs), GFP_KERNEL);
+    if (!ndev->vqs || !ndev->event_cbs) {
+        err = -ENOMEM;
+        goto err_alloc;
+    }

         ndev->mvdev.max_vqs = max_vqs;
         mvdev = &ndev->mvdev;
         mvdev->mdev = mdev;

drivers/vdpa/mlx5/net/mlx5_vnet.c +1 -0

@@ -2627,6 +2648,7 @@ static int mlx5_vdpa_dev_add(struct
 vdpa_mgmt_dev *v_mdev, const char *name,
             mlx5_mpfs_del_mac(pfmdev, config->mac);
     err_mtu:
         mutex_destroy(&ndev->reslock);
+err_alloc:
         put_device(&mvdev->vdev.dev);
         return err;

     }

drivers/vdpa/mlx5/net/mlx5_vnet.c +0 -0

@@ -2669,7 +2691,8 @@ static int mlx5v_probe(struct auxiliary_device
 *adev,
         mgtdev->mgtdev.ops = &mdev_ops;
         mgtdev->mgtdev.device = mdev->device;