s390/vfio-ap: dynamic configuration support

The queues assigned to a matrix mediated device are currently reset when:

* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.

Immediately after the reset of a queue, a call is made to disable interrupts for the queue. This is entirely unnecessary because the reset of a queue disables interrupts, so this will be removed.

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_drv.c     |  1 -
 drivers/s390/crypto/vfio_ap_ops.c     | 40 +++++++++++++++++----------
 drivers/s390/crypto/vfio_ap_private.h |  1 -
 3 files changed, 26 insertions(+), 16 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -1

index be2520cc010b..ca18c91afec9 100644
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 	apid = AP_QID_CARD(q->apqn);
 	apqi = AP_QID_QUEUE(q->apqn);
 	vfio_ap_mdev_reset_queue(apid, apqi, 1);
-	vfio_ap_irq_disable(q);
 	kfree(q);
 	mutex_unlock(&matrix_dev->lock);
 }

drivers/s390/crypto/vfio_ap_ops.c +6 -10

index 7339043906cf..052f61391ec7 100644
@@ -25,6 +25,7 @@
 #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
 static int match_apqn(struct device *dev, const void *data)
 {

@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
 					int apqn)
 {
 	struct vfio_ap_queue *q;
-	struct device *dev;
 
 	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
 		return NULL;
 	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
 		return NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)
-		return NULL;
-	q = dev_get_drvdata(dev);
-	q->matrix_mdev = matrix_mdev;
-	put_device(dev);
+	q = vfio_ap_find_queue(apqn);
+	if (q)
+		q->matrix_mdev = matrix_mdev;
 
 	return q;
 }

@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	return notify_rc;
 }
 
-static void vfio_ap_irq_disable_apqn(int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
 {
 	struct device *dev;
-	struct vfio_ap_queue *q;
+	struct vfio_ap_queue *q = NULL;
 

drivers/s390/crypto/vfio_ap_ops.c +3 -1

 int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
 			     unsigned int retry)
 {
 	struct ap_queue_status status;
+	struct vfio_ap_queue *q;
+	int ret;
 	int retry2 = 2;
 	int apqn = AP_MKQID(apid, apqi);
 

@@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
 				status = ap_tapq(apqn, NULL);
 			}
 			WARN_ON_ONCE(retry2 <= 0);
-			return 0;
+			ret = 0;

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+		vfio_ap_free_aqic_resources(q);
+
+	return ret;
 }
 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)

@@ -1189,7 +1202,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 			 */
 			if (ret)
 				rc = ret;
-			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
 		}
 	}
 

drivers/s390/crypto/vfio_ap_private.h +0 -1

index f46dde56b464..0db6fb3d56d5 100644
@@ -100,5 +100,4 @@ struct vfio_ap_queue {
 #define VFIO_AP_ISC_INVALID 0xff
 	unsigned char saved_isc;
 };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
 #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 76 lines · On Tue, 22 Dec 2020 20:15:53 -0500 Tony Krowiak <[email protected]> wrote:

The queues assigned to a matrix mediated device are currently reset when:

* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.

Immediately after the reset of a queue, a call is made to disable interrupts for the queue. This is entirely unnecessary because the reset of a queue disables interrupts, so this will be removed.

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_drv.c     |  1 -
  drivers/s390/crypto/vfio_ap_ops.c     | 40 +++++++++++++++++----------
  drivers/s390/crypto/vfio_ap_private.h |  1 -
  3 files changed, 26 insertions(+), 16 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -1

index be2520cc010b..ca18c91afec9 100644
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
  	apid = AP_QID_CARD(q->apqn);
  	apqi = AP_QID_QUEUE(q->apqn);
  	vfio_ap_mdev_reset_queue(apid, apqi, 1);
-	vfio_ap_irq_disable(q);
  	kfree(q);
  	mutex_unlock(&matrix_dev->lock);
  }

drivers/s390/crypto/vfio_ap_ops.c +6 -10

index 7339043906cf..052f61391ec7 100644
@@ -25,6 +25,7 @@
  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
  static int match_apqn(struct device *dev, const void *data)
  {

@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
  					int apqn)
  {
  	struct vfio_ap_queue *q;
-	struct device *dev;
 
  	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
  		return NULL;
  	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
  		return NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)
-		return NULL;
-	q = dev_get_drvdata(dev);
-	q->matrix_mdev = matrix_mdev;
-	put_device(dev);
+	q = vfio_ap_find_queue(apqn);
+	if (q)
+		q->matrix_mdev = matrix_mdev;
 
  	return q;
  }

@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
  	return notify_rc;
  }
 
-static void vfio_ap_irq_disable_apqn(int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
  {
  	struct device *dev;
-	struct vfio_ap_queue *q;
+	struct vfio_ap_queue *q = NULL;
 

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, · line 1138

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -1

  	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
  				 &apqn, match_apqn);
  	if (dev) {
  		q = dev_get_drvdata(dev);
-		vfio_ap_irq_disable(q);
  		put_device(dev);
  	}
+
+	return q;
  }

This hunk and the previous one are a rewrite of vfio_ap_get_queue() and have next to nothing to do with the patch's objective. If we were at an earlier stage, I would ask to split it up.

Quoted 17 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

  int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
  			     unsigned int retry)
  {
  	struct ap_queue_status status;
+	struct vfio_ap_queue *q;
+	int ret;
  	int retry2 = 2;
  	int apqn = AP_MKQID(apid, apqi);
 

@@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
  				status = ap_tapq(apqn, NULL);
  			}
  			WARN_ON_ONCE(retry2 <= 0);
-			return 0;
+			ret = 0;

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1167

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+			goto free_aqic_resources;
  		case AP_RESPONSE_RESET_IN_PROGRESS:
  		case AP_RESPONSE_BUSY:
  			msleep(20);
  			break;
  		default:
  			/* things are really broken, give up */
-			return -EIO;
+			ret = -EIO;
+			goto free_aqic_resources;

Do we really want the unpin here? I mean the reset did not work and we are giving up. So the irqs are potentially still enabled.

Without this patch we try to disable the interrupts using AQIC, and do the cleanup after that.

I'm aware, the comment says we should not take the default branch, but if that's really the case we should IMHO log an error and leak the page.

It's up to you if you want to change this. I don't want to delay the series any further than absolutely necessary.

Acked-by Halil Pasic <[email protected]>

Quoted 3 lines

drivers/s390/crypto/vfio_ap_ops.c +0 -0

  		}
  	} while (retry--);
 

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1180

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +9 -0

  	return -EBUSY;
+
+free_aqic_resources:
+	/*
+	 * In order to free the aqic resources, the queue must be linked to
+	 * the matrix_mdev to which its APQN is assigned and the KVM pointer
+	 * must be available.
+	 */
+	q = vfio_ap_find_queue(apqn);
+	if (q && q->matrix_mdev && q->matrix_mdev->kvm)

Is this of the type "we know there are no aqic resources to be freed" if precondition is false?

vfio_ap_free_aqic_resources() checks the matrix_mdev pointer but not the kvm pointer. Could we just check the kvm pointer in vfio_ap_free_aqic_resources()?

At the end of the series, is seeing q! indicating a bug, or is it something we expect to see under certain circumstances?

Quoted 26 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+		vfio_ap_free_aqic_resources(q);
+
+	return ret;
  }
 
  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)

@@ -1189,7 +1202,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			 */
  			if (ret)
  				rc = ret;
-			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
  		}
  	}
 

drivers/s390/crypto/vfio_ap_private.h +0 -1

index f46dde56b464..0db6fb3d56d5 100644
@@ -100,5 +100,4 @@ struct vfio_ap_queue {
  #define VFIO_AP_ISC_INVALID 0xff
  	unsigned char saved_isc;
  };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
  #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 79 lines · Halil Pasic

Quoted 76 lines · On Tue, 22 Dec 2020 20:15:53 -0500 Tony Krowiak <[email protected]> wrote:

The queues assigned to a matrix mediated device are currently reset when:

* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.

Immediately after the reset of a queue, a call is made to disable interrupts for the queue. This is entirely unnecessary because the reset of a queue disables interrupts, so this will be removed.

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_drv.c     |  1 -
  drivers/s390/crypto/vfio_ap_ops.c     | 40 +++++++++++++++++----------
  drivers/s390/crypto/vfio_ap_private.h |  1 -
  3 files changed, 26 insertions(+), 16 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -1

index be2520cc010b..ca18c91afec9 100644
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
  	apid = AP_QID_CARD(q->apqn);
  	apqi = AP_QID_QUEUE(q->apqn);
  	vfio_ap_mdev_reset_queue(apid, apqi, 1);
-	vfio_ap_irq_disable(q);
  	kfree(q);
  	mutex_unlock(&matrix_dev->lock);
  }

drivers/s390/crypto/vfio_ap_ops.c +6 -10

index 7339043906cf..052f61391ec7 100644
@@ -25,6 +25,7 @@
  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
  static int match_apqn(struct device *dev, const void *data)
  {

@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
  					int apqn)
  {
  	struct vfio_ap_queue *q;
-	struct device *dev;
 
  	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
  		return NULL;
  	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
  		return NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)
-		return NULL;
-	q = dev_get_drvdata(dev);
-	q->matrix_mdev = matrix_mdev;
-	put_device(dev);
+	q = vfio_ap_find_queue(apqn);
+	if (q)
+		q->matrix_mdev = matrix_mdev;
 
  	return q;
  }

@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
  	return notify_rc;
  }
 
-static void (int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
  {
  	struct device *dev;
-	struct vfio_ap_queue *q;
+	struct vfio_ap_queue *q = NULL;
 

Quoted 13 lines

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, · line 1138

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -1

  	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
  				 &apqn, match_apqn);
  	if (dev) {
  		q = dev_get_drvdata(dev);
-		vfio_ap_irq_disable(q);
  		put_device(dev);
  	}
+
+	return q;
  }  

This hunk and the previous one are a rewrite of vfio_ap_get_queue() and have next to nothing to do with the patch's objective. If we were at an earlier stage, I would ask to split it up.

The rewrite of vfio_ap_get_queue() definitely is related to this patch's objective.

Below, in the vfio_ap_mdev_reset_queue()
function, there is the label 'free_aqic_resources' which is where the call to vfio_ap_free_aqic_resources() function is called. That function takes a struct vfio_ap_queue as an argument, so the object needs to be retrieved prior to calling the function. We can't use the vfio_ap_get_queue() function for two reasons:
1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev as a parameter and we do not have a pointer to such at the time.

2. The vfio_ap_get_queue() function is used to link the mdev to the vfio_ap_queue object with the specified APQN.
So, we needed a way to retrieve the vfio_ap_queue object by its APQN only, Rather than creating a function that retrieves the vfio_ap_queue object which duplicates the retrieval code in vfio_ap_get_queue(), I created the vfio_ap_find_queue() function to do just that and modified the vfio_ap_get_queue() function to call it (i.e., code reuse).

Quoted 32 lines

Quoted 19 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

  int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
  			     unsigned int retry)
  {
  	struct ap_queue_status status;
+	struct vfio_ap_queue *q;
+	int ret;
  	int retry2 = 2;
  	int apqn = AP_MKQID(apid, apqi);
 

@@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
  				status = ap_tapq(apqn, NULL);
  			}
  			WARN_ON_ONCE(retry2 <= 0);
-			return 0;
+			ret = 0;

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1167

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+			goto free_aqic_resources;
  		case AP_RESPONSE_RESET_IN_PROGRESS:
  		case AP_RESPONSE_BUSY:
  			msleep(20);
  			break;
  		default:
  			/* things are really broken, give up */
-			return -EIO;
+			ret = -EIO;
+			goto free_aqic_resources;  

Do we really want the unpin here? I mean the reset did not work and we are giving up. So the irqs are potentially still enabled.

Quoted 2 lines

Without this patch we try to disable the interrupts using AQIC, and do the cleanup after that.

If the reset failure lands here, then a subsequent AQIC will also fail, so I see no reason to expend processing time for something that will ultimately fail anyways.

Quoted 1 line

Quoted 3 lines

I'm aware, the comment says we should not take the default branch, but if that's really the case we should IMHO log an error and leak the page.

I do not see a good reason to leak the page, what purpose would it serve?

I don't have a problem with logging an error, do you think it should just be a log message or a WARN_ON type of thing?

Quoted 9 lines

It's up to you if you want to change this. I don't want to delay the series any further than absolutely necessary.

Acked-by Halil Pasic <[email protected]>

Quoted 3 lines

drivers/s390/crypto/vfio_ap_ops.c +0 -0

  		}
  	} while (retry--);
 

Quoted 12 lines

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1180

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +9 -0

  	return -EBUSY;
+
+free_aqic_resources:
+	/*
+	 * In order to free the aqic resources, the queue must be linked to
+	 * the matrix_mdev to which its APQN is assigned and the KVM pointer
+	 * must be available.
+	 */
+	q = vfio_ap_find_queue(apqn);
+	if (q && q->matrix_mdev && q->matrix_mdev->kvm)  

Is this of the type "we know there are no aqic resources to be freed" if precondition is false?

Yes

Quoted 4 lines

vfio_ap_free_aqic_resources() checks the matrix_mdev pointer but not the kvm pointer. Could we just check the kvm pointer in vfio_ap_free_aqic_resources()?

A while back I posted a patch that did just that and someone pushed back because they could not see how the vfio_ap_free_aqic_resources() function would ever be called with a NULL kvm pointer which is why I implemented the above check. The reset is called
when the mdev is removed which can happen only when there
is no kvm pointer, so I agree it would be better to check the kvm pointer in the vfio_ap_free_aqic_resources() function.

Quoted 3 lines

At the end of the series, is seeing q! indicating a bug, or is it something we expect to see under certain circumstances?

I'm not quite sure to what you are referring regarding "the end of the series", but we can expect to see a NULL pointer for q if a queue is manually unbound from the driver.

Quoted 27 lines

Quoted 27 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+		vfio_ap_free_aqic_resources(q);
+
+	return ret;
  }
 
  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)

@@ -1189,7 +1202,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			 */
  			if (ret)
  				rc = ret;
-			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
  		}
  	}
 

drivers/s390/crypto/vfio_ap_private.h +0 -1

index f46dde56b464..0db6fb3d56d5 100644
@@ -100,5 +100,4 @@ struct vfio_ap_queue {
  #define VFIO_AP_ISC_INVALID 0xff
  	unsigned char saved_isc;
  };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
  #endif /* _VFIO_AP_PRIVATE_H_ */  

Quoted 93 lines · On Wed, 13 Jan 2021 12:06:28 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 92 lines · Halil Pasic

Quoted 76 lines · On Tue, 22 Dec 2020 20:15:53 -0500 Tony Krowiak <[email protected]> wrote:

The queues assigned to a matrix mediated device are currently reset when:

* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.

Immediately after the reset of a queue, a call is made to disable interrupts for the queue. This is entirely unnecessary because the reset of a queue disables interrupts, so this will be removed.

Signed-off-by Tony Krowiak <[email protected]>

---
   drivers/s390/crypto/vfio_ap_drv.c     |  1 -
   drivers/s390/crypto/vfio_ap_ops.c     | 40 +++++++++++++++++----------
   drivers/s390/crypto/vfio_ap_private.h |  1 -
   3 files changed, 26 insertions(+), 16 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -1

index be2520cc010b..ca18c91afec9 100644
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
   	apid = AP_QID_CARD(q->apqn);
   	apqi = AP_QID_QUEUE(q->apqn);
   	vfio_ap_mdev_reset_queue(apid, apqi, 1);
-	vfio_ap_irq_disable(q);
   	kfree(q);
   	mutex_unlock(&matrix_dev->lock);
   }

drivers/s390/crypto/vfio_ap_ops.c +6 -10

index 7339043906cf..052f61391ec7 100644
@@ -25,6 +25,7 @@
   #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
   static int match_apqn(struct device *dev, const void *data)
   {

@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
   					int apqn)
   {
   	struct vfio_ap_queue *q;
-	struct device *dev;
 
   	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
   		return NULL;
   	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
   		return NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)
-		return NULL;
-	q = dev_get_drvdata(dev);
-	q->matrix_mdev = matrix_mdev;
-	put_device(dev);
+	q = vfio_ap_find_queue(apqn);
+	if (q)
+		q->matrix_mdev = matrix_mdev;
 
   	return q;
   }

@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
   	return notify_rc;
   }
 
-static void (int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
   {
   	struct device *dev;
-	struct vfio_ap_queue *q;
+	struct vfio_ap_queue *q = NULL;
 

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, · line 1138

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -1

   	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
   				 &apqn, match_apqn);
   	if (dev) {
   		q = dev_get_drvdata(dev);
-		vfio_ap_irq_disable(q);
   		put_device(dev);
   	}
+
+	return q;
   }

This hunk and the previous one are a rewrite of vfio_ap_get_queue() and have next to nothing to do with the patch's objective. If we were at an earlier stage, I would ask to split it up.

Quoted 2 lines

The rewrite of vfio_ap_get_queue() definitely is related to this patch's objective.

Definitively loosely related.

Quoted 17 lines

Below, in the vfio_ap_mdev_reset_queue()
function, there is the label 'free_aqic_resources' which is where the call to vfio_ap_free_aqic_resources() function is called. That function takes a struct vfio_ap_queue as an argument, so the object needs to be retrieved prior to calling the function. We can't use the vfio_ap_get_queue() function for two reasons:
1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev as a parameter and we do not have a pointer to such at the time.
2. The vfio_ap_get_queue() function is used to link the mdev to the vfio_ap_queue object with the specified APQN.
So, we needed a way to retrieve the vfio_ap_queue object by its APQN only, Rather than creating a function that retrieves the vfio_ap_queue object which duplicates the retrieval code in vfio_ap_get_queue(), I created the vfio_ap_find_queue() function to do just that and modified the vfio_ap_get_queue() function to call it (i.e., code reuse).

Please tell me what prevented you from doing a doing the splitting out vfio_ap_find_queue() from vfio_ap_get_queue() in a separate patch, that precedes this patch? It would have resulted in simpler diffs, because the split out wouldn't be intermingled with other stuff, i.e. getting rid of vfio_ap_irq_disable_apqn(). Don't you see that the two are intermingled in this diff?

Quoted 38 lines

Quoted 32 lines

Quoted 19 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

   int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
   			     unsigned int retry)
   {
   	struct ap_queue_status status;
+	struct vfio_ap_queue *q;
+	int ret;
   	int retry2 = 2;
   	int apqn = AP_MKQID(apid, apqi);
 

@@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
   				status = ap_tapq(apqn, NULL);
   			}
   			WARN_ON_ONCE(retry2 <= 0);
-			return 0;
+			ret = 0;

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1167

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+			goto free_aqic_resources;
   		case AP_RESPONSE_RESET_IN_PROGRESS:
   		case AP_RESPONSE_BUSY:
   			msleep(20);
   			break;
   		default:
   			/* things are really broken, give up */
-			return -EIO;
+			ret = -EIO;
+			goto free_aqic_resources;

Do we really want the unpin here? I mean the reset did not work and we are giving up. So the irqs are potentially still enabled.

Quoted 2 lines

Without this patch we try to disable the interrupts using AQIC, and do the cleanup after that.

If the reset failure lands here, then a subsequent AQIC will also fail, so I see no reason to expend processing time for something that will ultimately fail anyways.

Quoted 5 lines

Quoted 3 lines

I'm aware, the comment says we should not take the default branch, but if that's really the case we should IMHO log an error and leak the page.

I do not see a good reason to leak the page, what purpose would it serve?

Well, the thing is we don't have a case for AP_RESPONSE_CHECKSTOPPED, which is, AFAIK a valid outcome. I don't remember what is the exact deal with checkstopped regarding interrupts.

If we take the default with something different
than AP_RESPONSE_CHECKSTOPPED, that is AFAICT a bug of the underlying machine.

Quoted 4 lines

I don't have a problem with logging an error, do you think it should just be a log message or a WARN_ON type of thing?

Seeing an outcome we don't expect to see, due to a bug in the underlying machine is in my book worth an error message. Furthermore we may not assume that the interrupts where shut down for the queue. So the only way we can protect the host is by leaking the page.

Quoted 26 lines

Quoted 9 lines

It's up to you if you want to change this. I don't want to delay the series any further than absolutely necessary.

Acked-by Halil Pasic <[email protected]>

Quoted 3 lines

drivers/s390/crypto/vfio_ap_ops.c +0 -0

   		}
   	} while (retry--);
 

Quoted 12 lines

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1180

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +9 -0

   	return -EBUSY;
+
+free_aqic_resources:
+	/*
+	 * In order to free the aqic resources, the queue must be linked to
+	 * the matrix_mdev to which its APQN is assigned and the KVM pointer
+	 * must be available.
+	 */
+	q = vfio_ap_find_queue(apqn);
+	if (q && q->matrix_mdev && q->matrix_mdev->kvm)

Is this of the type "we know there are no aqic resources to be freed" if precondition is false?

Yes

Quoted 3 lines

vfio_ap_free_aqic_resources() checks the matrix_mdev pointer but not the kvm pointer. Could we just check the kvm pointer in vfio_ap_free_aqic_resources()?

Quoted 8 lines

A while back I posted a patch that did just that and someone pushed back because they could not see how the vfio_ap_free_aqic_resources() function would ever be called with a NULL kvm pointer which is why I implemented the above check. The reset is called
when the mdev is removed which can happen only when there
is no kvm pointer, so I agree it would be better to check the kvm pointer in the vfio_ap_free_aqic_resources() function.

I don't remember. Sorry if it was me.

Quoted 5 lines

Quoted 2 lines

At the end of the series, is seeing q! indicating a bug, or is it something we expect to see under certain circumstances?

I'm not quite sure to what you are referring regarding "the end of the series", but we can expect to see a NULL pointer for q if a queue is manually unbound from the driver.

By at the end of the series, I mean with all 15 patches applied.

Regarding the case where the queue is manually unbound form the driver, this is exactly one of the scenarios I was latently concerned about. Let me explain. The manually unbound queue was already reset in vfio_ap_mdev_remove_queue() if necessary, so we don't need to reset it again. And more importantly it is not bound to the vfio_ap driver, so vfio_ap is not allowed to reset it. (It could in theory belong to and be in use by another non-default driver).

I've just checked out vfio_ap_mdev_reset_queues() and it resets all queues in the matrix. The in use mechanism does ensure that zcrypt can't use these queues (together with a[pq]mask), but resetting a queue that does not belong to us is going beyond our authority.

Regards,
Halil

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+		vfio_ap_free_aqic_resources(q);
+
+	return ret;
   }
 
   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)

@@ -1189,7 +1202,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
   			 */
   			if (ret)
   				rc = ret;
-			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
   		}
   	}
 

drivers/s390/crypto/vfio_ap_private.h +0 -1

index f46dde56b464..0db6fb3d56d5 100644
@@ -100,5 +100,4 @@ struct vfio_ap_queue {
   #define VFIO_AP_ISC_INVALID 0xff
   	unsigned char saved_isc;
   };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
   #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 96 lines · Halil Pasic

Quoted 93 lines · On Wed, 13 Jan 2021 12:06:28 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 92 lines · Halil Pasic

Quoted 76 lines · On Tue, 22 Dec 2020 20:15:53 -0500 Tony Krowiak <[email protected]> wrote:

The queues assigned to a matrix mediated device are currently reset when:

* The VFIO_DEVICE_RESET ioctl is invoked
* The mdev fd is closed by userspace (QEMU)
* The mdev is removed from sysfs.

Immediately after the reset of a queue, a call is made to disable interrupts for the queue. This is entirely unnecessary because the reset of a queue disables interrupts, so this will be removed.

Signed-off-by Tony Krowiak <[email protected]>

---
   drivers/s390/crypto/vfio_ap_drv.c     |  1 -
   drivers/s390/crypto/vfio_ap_ops.c     | 40 +++++++++++++++++----------
   drivers/s390/crypto/vfio_ap_private.h |  1 -
   3 files changed, 26 insertions(+), 16 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -1

index be2520cc010b..ca18c91afec9 100644
@@ -79,7 +79,6 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
   	apid = AP_QID_CARD(q->apqn);
   	apqi = AP_QID_QUEUE(q->apqn);
   	vfio_ap_mdev_reset_queue(apid, apqi, 1);
-	vfio_ap_irq_disable(q);
   	kfree(q);
   	mutex_unlock(&matrix_dev->lock);
   }

drivers/s390/crypto/vfio_ap_ops.c +6 -10

index 7339043906cf..052f61391ec7 100644
@@ -25,6 +25,7 @@
   #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
   static int match_apqn(struct device *dev, const void *data)
   {

@@ -49,20 +50,15 @@ static struct vfio_ap_queue *(
   					int apqn)
   {
   	struct vfio_ap_queue *q;
-	struct device *dev;
 
   	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
   		return NULL;
   	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
   		return NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)
-		return NULL;
-	q = dev_get_drvdata(dev);
-	q->matrix_mdev = matrix_mdev;
-	put_device(dev);
+	q = vfio_ap_find_queue(apqn);
+	if (q)
+		q->matrix_mdev = matrix_mdev;
 
   	return q;
   }

@@ -1126,24 +1122,27 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
   	return notify_rc;
   }
 
-static void (int apqn)
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
   {
   	struct device *dev;
-	struct vfio_ap_queue *q;
+	struct vfio_ap_queue *q = NULL;
 

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_group_notifier(struct notifier_block *nb, · line 1138

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -1

   	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
   				 &apqn, match_apqn);
   	if (dev) {
   		q = dev_get_drvdata(dev);
-		vfio_ap_irq_disable(q);
   		put_device(dev);
   	}
+
+	return q;
   }  

This hunk and the previous one are a rewrite of vfio_ap_get_queue() and have next to nothing to do with the patch's objective. If we were at an earlier stage, I would ask to split it up.

Quoted 3 lines

Quoted 2 lines

The rewrite of vfio_ap_get_queue() definitely is related to this patch's objective.

Definitively loosely related.

A matter of opinion I suppose and I respect yours.

Quoted 17 lines

Quoted 17 lines

Below, in the vfio_ap_mdev_reset_queue()
function, there is the label 'free_aqic_resources' which is where the call to vfio_ap_free_aqic_resources() function is called. That function takes a struct vfio_ap_queue as an argument, so the object needs to be retrieved prior to calling the function. We can't use the vfio_ap_get_queue() function for two reasons:
1. The vfio_ap_get_queue() function takes a struct ap_matrix_mdev as a parameter and we do not have a pointer to such at the time.
2. The vfio_ap_get_queue() function is used to link the mdev to the vfio_ap_queue object with the specified APQN.
So, we needed a way to retrieve the vfio_ap_queue object by its APQN only, Rather than creating a function that retrieves the vfio_ap_queue object which duplicates the retrieval code in vfio_ap_get_queue(), I created the vfio_ap_find_queue() function to do just that and modified the vfio_ap_get_queue() function to call it (i.e., code reuse).

Quoted 6 lines

Please tell me what prevented you from doing a doing the splitting out vfio_ap_find_queue() from vfio_ap_get_queue() in a separate patch, that precedes this patch? It would have resulted in simpler diffs, because the split out wouldn't be intermingled with other stuff, i.e. getting rid of vfio_ap_irq_disable_apqn(). Don't you see that the two are intermingled in this diff?

I included this here for the reasons I stated above.
If I was reviewing these patches and saw this in a separate patch I would wonder why it was being done since it would be an isolated change requiring examination of subsequent patches to figure out why it was done.  Since you have taken the time to bring this up again I'll go ahead and do it since I have no major objections and it is a fairly simple change.

Quoted 38 lines

Quoted 38 lines

Quoted 32 lines

Quoted 19 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

   int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
   			     unsigned int retry)
   {
   	struct ap_queue_status status;
+	struct vfio_ap_queue *q;
+	int ret;
   	int retry2 = 2;
   	int apqn = AP_MKQID(apid, apqi);
 

@@ -1156,18 +1155,32 @@ int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
   				status = ap_tapq(apqn, NULL);
   			}
   			WARN_ON_ONCE(retry2 <= 0);
-			return 0;
+			ret = 0;

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1167

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

+			goto free_aqic_resources;
   		case AP_RESPONSE_RESET_IN_PROGRESS:
   		case AP_RESPONSE_BUSY:
   			msleep(20);
   			break;
   		default:
   			/* things are really broken, give up */
-			return -EIO;
+			ret = -EIO;
+			goto free_aqic_resources;  

Do we really want the unpin here? I mean the reset did not work and we are giving up. So the irqs are potentially still enabled.

Quoted 2 lines

Without this patch we try to disable the interrupts using AQIC, and do the cleanup after that.

If the reset failure lands here, then a subsequent AQIC will also fail, so I see no reason to expend processing time for something that will ultimately fail anyways.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

I'm aware, the comment says we should not take the default branch, but if that's really the case we should IMHO log an error and leak the page.

I do not see a good reason to leak the page, what purpose would it serve?

Well, the thing is we don't have a case for AP_RESPONSE_CHECKSTOPPED, which is, AFAIK a valid outcome. I don't remember what is the exact deal with checkstopped regarding interrupts.

The AP_RESPONSE_CHECKSTOPPED response code is set when the AP function can not be performed due to a machine failure resulting in loss of connectivity to the queue. I find it hard to believe that interrupts would continue to be signaled in that case. I will check with the architecture folks for verification.

Quoted 1 line

Quoted 3 lines

If we take the default with something different
than AP_RESPONSE_CHECKSTOPPED, that is AFAICT a bug of the underlying machine.

I think AP_RESPONSE_CHECKSTOPPED indicates a problem with the machine also.

Quoted 8 lines

Quoted 4 lines

I don't have a problem with logging an error, do you think it should just be a log message or a WARN_ON type of thing?

Seeing an outcome we don't expect to see, due to a bug in the underlying machine is in my book worth an error message. Furthermore we may not assume that the interrupts where shut down for the queue. So the only way we can protect the host is by leaking the page.

I won't assume anything - although I seriously doubt interrupts will continue with a broken device - so I will get input from the architecture folks regarding interrupts after a non-zero response code.

Quoted 51 lines

Quoted 26 lines

Quoted 9 lines

It's up to you if you want to change this. I don't want to delay the series any further than absolutely necessary.

Acked-by Halil Pasic <[email protected]>

Quoted 3 lines

drivers/s390/crypto/vfio_ap_ops.c +0 -0

   		}
   	} while (retry--);
 

Quoted 12 lines

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi, · line 1180

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +9 -0

   	return -EBUSY;
+
+free_aqic_resources:
+	/*
+	 * In order to free the aqic resources, the queue must be linked to
+	 * the matrix_mdev to which its APQN is assigned and the KVM pointer
+	 * must be available.
+	 */
+	q = vfio_ap_find_queue(apqn);
+	if (q && q->matrix_mdev && q->matrix_mdev->kvm)  

Is this of the type "we know there are no aqic resources to be freed" if precondition is false?

Yes

Quoted 3 lines

vfio_ap_free_aqic_resources() checks the matrix_mdev pointer but not the kvm pointer. Could we just check the kvm pointer in vfio_ap_free_aqic_resources()?

Quoted 8 lines

A while back I posted a patch that did just that and someone pushed back because they could not see how the vfio_ap_free_aqic_resources() function would ever be called with a NULL kvm pointer which is why I implemented the above check. The reset is called
when the mdev is removed which can happen only when there
is no kvm pointer, so I agree it would be better to check the kvm pointer in the vfio_ap_free_aqic_resources() function.

I don't remember. Sorry if it was me.

Quoted 5 lines

Quoted 2 lines

At the end of the series, is seeing q! indicating a bug, or is it something we expect to see under certain circumstances?

I'm not quite sure to what you are referring regarding "the end of the series", but we can expect to see a NULL pointer for q if a queue is manually unbound from the driver.

By at the end of the series, I mean with all 15 patches applied.

Regarding the case where the queue is manually unbound form the driver, this is exactly one of the scenarios I was latently concerned about. Let me explain. The manually unbound queue was already reset in vfio_ap_mdev_remove_queue() if necessary, so we don't need to reset it again. And more importantly it is not bound to the vfio_ap driver, so vfio_ap is not allowed to reset it. (It could in theory belong to and be in use by another non-default driver).

Quoted 4 lines

I've just checked out vfio_ap_mdev_reset_queues() and it resets all queues in the matrix. The in use mechanism does ensure that zcrypt can't use these queues (together with a[pq]mask), but resetting a queue that does not belong to us is going beyond our authority.

I agree which is why in the next version I am only resetting a queue if it is bound at the time of the reset.

Let's create links between each queue device bound to the vfio_ap device driver and the matrix mdev to which the queue's APQN is assigned. The idea is to facilitate efficient retrieval of the objects representing the queue devices and matrix mdevs as well as to verify that a queue assigned to a matrix mdev is bound to the driver.

The links will be created as follows:

* When the queue device is probed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be linked.

* When an adapter or domain is assigned to a matrix mdev, for each new APQN assigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be linked.

The links will be removed as follows:

* When the queue device is removed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be unlinked.

* When an adapter or domain is unassigned from a matrix mdev, for each APQN unassigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be unlinked.

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_ops.c     | 140 +++++++++++++++++++++-----
 drivers/s390/crypto/vfio_ap_private.h |   3 +
 2 files changed, 117 insertions(+), 26 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c +106 -26

index 835c963ae16d..cdcc6378b4a5 100644
@@ -27,33 +27,17 @@
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
-/**
- * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
- * @matrix_mdev: the associated mediated matrix
- * @apqn: The queue APQN
- *
- * Retrieve a queue with a specific APQN from the list of the
- * devices of the vfio_ap_drv.
- * Verify that the APID and the APQI are set in the matrix.
- *
- * Returns the pointer to the associated vfio_ap_queue
- */
-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,
-					int apqn)
+static struct vfio_ap_queue *
+vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
 {
-	struct vfio_ap_queue *q = NULL;
-
-	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
-		return NULL;
-	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
-		return NULL;
+	struct vfio_ap_queue *q;
 
-	q = vfio_ap_find_queue(apqn);
-	if (q)
-		q->matrix_mdev = matrix_mdev;
+	hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
+		if (q && (q->apqn == apqn))
+			return q;
+	}
 
-	return q;
+	return NULL;
 }
 
 /**

@@ -166,7 +150,6 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 		  status.response_code);
 end_free:
 	vfio_ap_free_aqic_resources(q);
-	q->matrix_mdev = NULL;
 	return status;
 }
 

@@ -282,7 +265,7 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
 	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
 				   struct ap_matrix_mdev, pqap_hook);
 
-	q = vfio_ap_get_queue(matrix_mdev, apqn);
+	q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
 	if (!q)
 		goto out_unlock;
 

@@ -325,6 +308,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 
 	matrix_mdev->mdev = mdev;
 	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
+	hash_init(matrix_mdev->qtable);
 	mdev_set_drvdata(mdev, matrix_mdev);
 	matrix_mdev->pqap_hook.hook = handle_pqap;
 	matrix_mdev->pqap_hook.owner = THIS_MODULE;

@@ -553,6 +537,50 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 	return 0;
 }
 
+static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
+				    struct vfio_ap_queue *q)
+{
+	if (q) {
+		q->matrix_mdev = matrix_mdev;
+		hash_add(matrix_mdev->qtable,
+			 &q->mdev_qnode, q->apqn);
+	}
+}
+
+static void vfio_ap_mdev_link_apqn(struct ap_matrix_mdev *matrix_mdev, int apqn)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_find_queue(apqn);
+	vfio_ap_mdev_link_queue(matrix_mdev, q);
+}
+
+static void vfio_ap_mdev_unlink_queue(struct vfio_ap_queue *q)
+{
+	if (q) {
+		q->matrix_mdev = NULL;
+		hash_del(&q->mdev_qnode);
+	}
+}
+
+static void vfio_ap_mdev_unlink_apqn(int apqn)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_find_queue(apqn);
+	vfio_ap_mdev_unlink_queue(q);
+}
+
+static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
+				      unsigned long apid)
+{
+	unsigned long apqi;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
+		vfio_ap_mdev_link_apqn(matrix_mdev,
+				       AP_MKQID(apid, apqi));
+}
+
 /**
  * assign_adapter_store
  *

@@ -622,6 +650,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 	if (ret)
 		goto share_err;
 
+	vfio_ap_mdev_link_adapter(matrix_mdev, apid);
 	ret = count;
 	goto done;
 

@@ -634,6 +663,15 @@ static ssize_t assign_adapter_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(assign_adapter);
 
+static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
+					unsigned long apid)
+{
+	unsigned long apqi;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
+		vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
+}
+
 /**
  * unassign_adapter_store
  *

@@ -673,6 +711,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
+	vfio_ap_mdev_unlink_adapter(matrix_mdev, apid);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;

@@ -699,6 +738,15 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
 	return 0;
 }
 
+static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
+				     unsigned long apqi)
+{
+	unsigned long apid;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
+		vfio_ap_mdev_link_apqn(matrix_mdev, AP_MKQID(apid, apqi));
+}
+
 /**
  * assign_domain_store
  *

@@ -763,6 +811,7 @@ static ssize_t assign_domain_store(struct device *dev,
 	if (ret)
 		goto share_err;
 
+	vfio_ap_mdev_link_domain(matrix_mdev, apqi);
 	ret = count;
 	goto done;
 

@@ -775,6 +824,14 @@ static ssize_t assign_domain_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(assign_domain);
 
+static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
+				       unsigned long apqi)
+{
+	unsigned long apid;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
+		vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
+}
 
 /**
  * unassign_domain_store

@@ -815,6 +872,7 @@ static ssize_t unassign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
+	vfio_ap_mdev_unlink_domain(matrix_mdev, apqi);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;

@@ -1317,6 +1375,28 @@ void vfio_ap_mdev_unregister(void)
 	mdev_unregister_device(&matrix_dev->device);
 }
 
+/*
+ * vfio_ap_queue_link_mdev
+ *
+ * @q: The queue to link with the matrix mdev.
+ *
+ * Links @q with the matrix mdev to which the queue's APQN is assigned.
+ */
+static void vfio_ap_queue_link_mdev(struct vfio_ap_queue *q)
+{
+	unsigned long apid = AP_QID_CARD(q->apqn);
+	unsigned long apqi = AP_QID_QUEUE(q->apqn);
+	struct ap_matrix_mdev *matrix_mdev;
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
+		    test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
+			vfio_ap_mdev_link_queue(matrix_mdev, q);
+			break;
+		}
+	}
+}
+
 int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 {
 	struct vfio_ap_queue *q;

drivers/s390/crypto/vfio_ap_ops.c +4 -0

 	return 0;
 }
 

@@ -1341,6 +1425,10 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 	apid = AP_QID_CARD(q->apqn);
 	apqi = AP_QID_QUEUE(q->apqn);
 	vfio_ap_mdev_reset_queue(apid, apqi, 1);
+
+	if (q->matrix_mdev)
+		vfio_ap_mdev_unlink_queue(q);
+
 	kfree(q);
 	mutex_unlock(&matrix_dev->lock);
 }

drivers/s390/crypto/vfio_ap_private.h +3 -0

index d9003de4fbad..4e5cc72fc0db 100644
@@ -18,6 +18,7 @@
 #include <linux/delay.h>
 #include <linux/mutex.h>
 #include <linux/kvm_host.h>
+#include <linux/hashtable.h>
 
 #include "ap_bus.h"
 

@@ -86,6 +87,7 @@ struct ap_matrix_mdev {
 	struct kvm *kvm;
 	struct kvm_s390_module_hook pqap_hook;
 	struct mdev_device *mdev;
+	DECLARE_HASHTABLE(qtable, 8);
 };
 
 extern int vfio_ap_mdev_register(void);

@@ -97,6 +99,7 @@ struct vfio_ap_queue {
 	int	apqn;
 #define VFIO_AP_ISC_INVALID 0xff
 	unsigned char saved_isc;
+	struct hlist_node mdev_qnode;
 };
 
 int vfio_ap_mdev_probe_queue(struct ap_device *queue);

Quoted 23 lines · On Tue, 22 Dec 2020 20:15:56 -0500 Tony Krowiak <[email protected]> wrote:

Let's create links between each queue device bound to the vfio_ap device driver and the matrix mdev to which the queue's APQN is assigned. The idea is to facilitate efficient retrieval of the objects representing the queue devices and matrix mdevs as well as to verify that a queue assigned to a matrix mdev is bound to the driver.

The links will be created as follows:

* When the queue device is probed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be linked.

* When an adapter or domain is assigned to a matrix mdev, for each new APQN assigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be linked.

The links will be removed as follows:

* When the queue device is removed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be unlinked.

Quoted 6 lines

* When an adapter or domain is unassigned from a matrix mdev, for each APQN unassigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be unlinked.

Signed-off-by Tony Krowiak <[email protected]>

Reviewed-by Halil Pasic <[email protected]>

Quoted 241 lines

---
  drivers/s390/crypto/vfio_ap_ops.c     | 140 +++++++++++++++++++++-----
  drivers/s390/crypto/vfio_ap_private.h |   3 +
  2 files changed, 117 insertions(+), 26 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c +107 -26

index 835c963ae16d..cdcc6378b4a5 100644
@@ -27,33 +27,17 @@
  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
  static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
-/**
- * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
- * @matrix_mdev: the associated mediated matrix
- * @apqn: The queue APQN
- *
- * Retrieve a queue with a specific APQN from the list of the
- * devices of the vfio_ap_drv.
- * Verify that the APID and the APQI are set in the matrix.
- *
- * Returns the pointer to the associated vfio_ap_queue
- */
-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,
-					int apqn)
+static struct vfio_ap_queue *
+vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
  {
-	struct vfio_ap_queue *q = NULL;
-
-	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
-		return NULL;
-	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
-		return NULL;
+	struct vfio_ap_queue *q;
 
-	q = vfio_ap_find_queue(apqn);
-	if (q)
-		q->matrix_mdev = matrix_mdev;
+	hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
+		if (q && (q->apqn == apqn))
+			return q;
+	}
 
-	return q;
+	return NULL;
  }
 
  /**

@@ -166,7 +150,6 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
  		  status.response_code);
  end_free:
  	vfio_ap_free_aqic_resources(q);
-	q->matrix_mdev = NULL;
  	return status;
  }
 

@@ -282,7 +265,7 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
  	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
  				   struct ap_matrix_mdev, pqap_hook);
 
-	q = vfio_ap_get_queue(matrix_mdev, apqn);
+	q = vfio_ap_mdev_get_queue(matrix_mdev, apqn);
  	if (!q)
  		goto out_unlock;
 

@@ -325,6 +308,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 
  	matrix_mdev->mdev = mdev;
  	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
+	hash_init(matrix_mdev->qtable);
  	mdev_set_drvdata(mdev, matrix_mdev);
  	matrix_mdev->pqap_hook.hook = handle_pqap;
  	matrix_mdev->pqap_hook.owner = THIS_MODULE;

@@ -553,6 +537,50 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
  	return 0;
  }
 
+static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
+				    struct vfio_ap_queue *q)
+{
+	if (q) {
+		q->matrix_mdev = matrix_mdev;
+		hash_add(matrix_mdev->qtable,
+			 &q->mdev_qnode, q->apqn);
+	}
+}
+
+static void vfio_ap_mdev_link_apqn(struct ap_matrix_mdev *matrix_mdev, int apqn)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_find_queue(apqn);
+	vfio_ap_mdev_link_queue(matrix_mdev, q);
+}
+
+static void vfio_ap_mdev_unlink_queue(struct vfio_ap_queue *q)
+{
+	if (q) {
+		q->matrix_mdev = NULL;
+		hash_del(&q->mdev_qnode);
+	}
+}
+
+static void vfio_ap_mdev_unlink_apqn(int apqn)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_find_queue(apqn);
+	vfio_ap_mdev_unlink_queue(q);
+}
+
+static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
+				      unsigned long apid)
+{
+	unsigned long apqi;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
+		vfio_ap_mdev_link_apqn(matrix_mdev,
+				       AP_MKQID(apid, apqi));
+}
+
  /**
   * assign_adapter_store
   *

@@ -622,6 +650,7 @@ static ssize_t assign_adapter_store(struct device *dev,
  	if (ret)
  		goto share_err;
 
+	vfio_ap_mdev_link_adapter(matrix_mdev, apid);
  	ret = count;
  	goto done;
 

@@ -634,6 +663,15 @@ static ssize_t assign_adapter_store(struct device *dev,
  }
  static DEVICE_ATTR_WO(assign_adapter);
 
+static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
+					unsigned long apid)
+{
+	unsigned long apqi;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, AP_DOMAINS)
+		vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
+}
+
  /**
   * unassign_adapter_store
   *

@@ -673,6 +711,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
  	mutex_lock(&matrix_dev->lock);
  	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
+	vfio_ap_mdev_unlink_adapter(matrix_mdev, apid);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;

@@ -699,6 +738,15 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
  	return 0;
  }
 
+static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
+				     unsigned long apqi)
+{
+	unsigned long apid;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
+		vfio_ap_mdev_link_apqn(matrix_mdev, AP_MKQID(apid, apqi));
+}
+
  /**
   * assign_domain_store
   *

@@ -763,6 +811,7 @@ static ssize_t assign_domain_store(struct device *dev,
  	if (ret)
  		goto share_err;
 
+	vfio_ap_mdev_link_domain(matrix_mdev, apqi);
  	ret = count;
  	goto done;
 

@@ -775,6 +824,14 @@ static ssize_t assign_domain_store(struct device *dev,
  }
  static DEVICE_ATTR_WO(assign_domain);
 
+static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
+				       unsigned long apqi)
+{
+	unsigned long apid;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, AP_DEVICES)
+		vfio_ap_mdev_unlink_apqn(AP_MKQID(apid, apqi));
+}
 
  /**
   * unassign_domain_store

@@ -815,6 +872,7 @@ static ssize_t unassign_domain_store(struct device *dev,
 
  	mutex_lock(&matrix_dev->lock);
  	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
+	vfio_ap_mdev_unlink_domain(matrix_mdev, apqi);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;

@@ -1317,6 +1375,28 @@ void vfio_ap_mdev_unregister(void)
  	mdev_unregister_device(&matrix_dev->device);
  }
 
+/*
+ * vfio_ap_queue_link_mdev
+ *
+ * @q: The queue to link with the matrix mdev.
+ *
+ * Links @q with the matrix mdev to which the queue's APQN is assigned.
+ */
+static void vfio_ap_queue_link_mdev(struct vfio_ap_queue *q)
+{
+	unsigned long apid = AP_QID_CARD(q->apqn);
+	unsigned long apqi = AP_QID_QUEUE(q->apqn);
+	struct ap_matrix_mdev *matrix_mdev;
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
+		    test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
+			vfio_ap_mdev_link_queue(matrix_mdev, q);
+			break;
+		}
+	}
+}
+
  int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
  {
  	struct vfio_ap_queue *q;

@@ -1324,9 +1404,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
  	q = kzalloc(sizeof(*q), GFP_KERNEL);
  	if (!q)
  		return -ENOMEM;
+	mutex_lock(&matrix_dev->lock);
  	dev_set_drvdata(&apdev->device, q);

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_probe_queue(struct ap_device *apdev) · line 1413

Quoted 5 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -0

  	q->apqn = to_ap_queue(&apdev->device)->qid;
  	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_queue_link_mdev(q);
+	mutex_unlock(&matrix_dev->lock);
+

Does the critical section have to include more than just vfio_ap_queue_link_mdev()? Did we need the critical section before this patch?

Quoted 43 lines

drivers/s390/crypto/vfio_ap_ops.c +4 -0

  	return 0;
  }
 

@@ -1341,6 +1425,10 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
  	apid = AP_QID_CARD(q->apqn);
  	apqi = AP_QID_QUEUE(q->apqn);
  	vfio_ap_mdev_reset_queue(apid, apqi, 1);
+
+	if (q->matrix_mdev)
+		vfio_ap_mdev_unlink_queue(q);
+
  	kfree(q);
  	mutex_unlock(&matrix_dev->lock);
  }

drivers/s390/crypto/vfio_ap_private.h +3 -0

index d9003de4fbad..4e5cc72fc0db 100644
@@ -18,6 +18,7 @@
  #include <linux/delay.h>
  #include <linux/mutex.h>
  #include <linux/kvm_host.h>
+#include <linux/hashtable.h>
 
  #include "ap_bus.h"
 

@@ -86,6 +87,7 @@ struct ap_matrix_mdev {
  	struct kvm *kvm;
  	struct kvm_s390_module_hook pqap_hook;
  	struct mdev_device *mdev;
+	DECLARE_HASHTABLE(qtable, 8);
  };
 
  extern int vfio_ap_mdev_register(void);

@@ -97,6 +99,7 @@ struct vfio_ap_queue {
  	int	apqn;
  #define VFIO_AP_ISC_INVALID 0xff
  	unsigned char saved_isc;
+	struct hlist_node mdev_qnode;
  };
 
  int vfio_ap_mdev_probe_queue(struct ap_device *queue);

Quoted 34 lines · Halil Pasic

Quoted 23 lines · On Tue, 22 Dec 2020 20:15:56 -0500 Tony Krowiak <[email protected]> wrote:

Let's create links between each queue device bound to the vfio_ap device driver and the matrix mdev to which the queue's APQN is assigned. The idea is to facilitate efficient retrieval of the objects representing the queue devices and matrix mdevs as well as to verify that a queue assigned to a matrix mdev is bound to the driver.

The links will be created as follows:

* When the queue device is probed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be linked.

* When an adapter or domain is assigned to a matrix mdev, for each new APQN assigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be linked.

The links will be removed as follows:

* When the queue device is removed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be unlinked.

Quoted 6 lines

* When an adapter or domain is unassigned from a matrix mdev, for each APQN unassigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be unlinked.

Signed-off-by Tony Krowiak <[email protected]>

Reviewed-by Halil Pasic <[email protected]>

Quoted 10 lines

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -0

+
  int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
  {
  	struct vfio_ap_queue *q;

@@ -1324,9 +1404,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
  	q = kzalloc(sizeof(*q), GFP_KERNEL);
  	if (!q)
  		return -ENOMEM;
+	mutex_lock(&matrix_dev->lock);
  	dev_set_drvdata(&apdev->device, q);

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_probe_queue(struct ap_device *apdev) · line 1413

Quoted 5 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -0

  	q->apqn = to_ap_queue(&apdev->device)->qid;
  	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_queue_link_mdev(q);
+	mutex_unlock(&matrix_dev->lock);
+  

Does the critical section have to include more than just vfio_ap_queue_link_mdev()? Did we need the critical section before this patch?

We did not need the critical section before this patch because the only function that retrieved the vfio_ap_queue via the queue device's drvdata was the remove callback. I included the initialization of the vfio_ap_queue object under lock because the vfio_ap_find_queue() function retrieves the vfio_ap_queue object from the queue device's drvdata so it might be advantageous to initialize it under the mdev lock. On the other hand, I can't come up with a good argument to change this.

Quoted 35 lines · On Wed, 13 Jan 2021 16:41:27 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 34 lines · Halil Pasic

Quoted 23 lines · On Tue, 22 Dec 2020 20:15:56 -0500 Tony Krowiak <[email protected]> wrote:

Let's create links between each queue device bound to the vfio_ap device driver and the matrix mdev to which the queue's APQN is assigned. The idea is to facilitate efficient retrieval of the objects representing the queue devices and matrix mdevs as well as to verify that a queue assigned to a matrix mdev is bound to the driver.

The links will be created as follows:

* When the queue device is probed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be linked.

* When an adapter or domain is assigned to a matrix mdev, for each new APQN assigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be linked.

The links will be removed as follows:

* When the queue device is removed, if its APQN is assigned to a matrix mdev, the structures representing the queue device and the matrix mdev will be unlinked.

Quoted 6 lines

* When an adapter or domain is unassigned from a matrix mdev, for each APQN unassigned that references a queue device bound to the vfio_ap device driver, the structures representing the queue device and the matrix mdev will be unlinked.

Signed-off-by Tony Krowiak <[email protected]>

Reviewed-by Halil Pasic <[email protected]>

[..]

Quoted 18 lines

Quoted 10 lines

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -0

+
   int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
   {
   	struct vfio_ap_queue *q;

@@ -1324,9 +1404,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
   	q = kzalloc(sizeof(*q), GFP_KERNEL);
   	if (!q)
   		return -ENOMEM;
+	mutex_lock(&matrix_dev->lock);
   	dev_set_drvdata(&apdev->device, q);

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_probe_queue(struct ap_device *apdev) · line 1413

Quoted 5 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -0

   	q->apqn = to_ap_queue(&apdev->device)->qid;
   	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_queue_link_mdev(q);
+	mutex_unlock(&matrix_dev->lock);
+

Does the critical section have to include more than just vfio_ap_queue_link_mdev()? Did we need the critical section before this patch?

Quoted 10 lines

We did not need the critical section before this patch because the only function that retrieved the vfio_ap_queue via the queue device's drvdata was the remove callback. I included the initialization of the vfio_ap_queue object under lock because the vfio_ap_find_queue() function retrieves the vfio_ap_queue object from the queue device's drvdata so it might be advantageous to initialize it under the mdev lock. On the other hand, I can't come up with a good argument to change this.

I was asking out of curiosity, not because I want it changed. I was also wondering if somebody could see a partially initialized device:
we even first call dev_set_drvdata() and only then finish the initialization. Before 's390/vfio-ap: use new AP bus interface to search for queue devices', which is the previous patch, we had the klist code in between, which uses spinlocks, which I think ensure, that all effects of probe are seen when we get the queue from vfio_ap_find_queue(). But with patch 4 in place that is not the case any more. Or am I wrong?

Regards,
Halil

The current implementation does not allow assignment of an AP adapter or domain to an mdev device if each APQN resulting from the assignment does not reference an AP queue device that is bound to the vfio_ap device driver. This patch allows assignment of AP resources to the matrix mdev as long as the APQNs resulting from the assignment:
1. Are not reserved by the AP BUS for use by the zcrypt device drivers.
2. Are not assigned to another matrix mdev.

The rationale behind this is twofold:
1. The AP architecture does not preclude assignment of APQNs to an AP configuration that are not available to the system.
2. APQNs that do not reference a queue device bound to the vfio_ap device driver will not be assigned to the guest's CRYCB, so the guest will not get access to queues not bound to the vfio_ap driver.

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_ops.c | 241 ++++++++----------------------
 1 file changed, 62 insertions(+), 179 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c +5 -27

index cdcc6378b4a5..2d58b39977be 100644
@@ -379,134 +379,37 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
 	NULL,
 };
 
-struct vfio_ap_queue_reserved {
-	unsigned long *apid;
-	unsigned long *apqi;
-	bool reserved;
-};
+#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \
+			 "already assigned to %s"
 
-/**
- * vfio_ap_has_queue
- *
- * @dev: an AP queue device
- * @data: a struct vfio_ap_queue_reserved reference
- *
- * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
- * apid or apqi specified in @data:
- *
- * - If @data contains both an apid and apqi value, then @data will be flagged
- *   as reserved if the APID and APQI fields for the AP queue device matches
- *
- * - If @data contains only an apid value, @data will be flagged as
- *   reserved if the APID field in the AP queue device matches
- *
- * - If @data contains only an apqi value, @data will be flagged as
- *   reserved if the APQI field in the AP queue device matches
- *
- * Returns 0 to indicate the input to function succeeded. Returns -EINVAL if
- * @data does not contain either an apid or apqi.
- */
-static int vfio_ap_has_queue(struct device *dev, void *data)
+static void vfio_ap_mdev_log_sharing_err(const char *mdev_name,
+					 unsigned long *apm,
+					 unsigned long *aqm)

}

/**
* vfio_ap_mdev_verify_no_sharing
*
- * Verifies that the APQNs derived from the cross product of the AP adapter IDs
- * and AP queue indexes comprising the AP matrix are not configured for another
- * mediated device. AP queue sharing is not allowed.
+ * Verifies that each APQN derived from the Cartesian product of the AP adapter
+ * IDs and AP queue indexes comprising the AP matrix are not configured for
+ * another mediated device. AP queue sharing is not allowed.
*
- * @matrix_mdev: the mediated matrix device
+ * @matrix_mdev: the mediated matrix device to which the APQNs being verified
+ * are assigned.
+ * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
+ * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified *
- * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
+ * Returns 0 if the APQNs are not shared, otherwise; returns -EBUSY. */

drivers/s390/crypto/vfio_ap_ops.c +4 -3

-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
 {
 	struct ap_matrix_mdev *lstdev;
 	DECLARE_BITMAP(apm, AP_DEVICES);

@@ -523,20 +426,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 		 * We work on full longs, as we can only exclude the leftover
 		 * bits in non-inverse order. The leftover is all zeros.
 		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
 			continue;
 

drivers/s390/crypto/vfio_ap_ops.c +10 -0

 	}
 
 	return 0;
 }
 
+static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
+				       unsigned long *mdev_apm,
+				       unsigned long *mdev_aqm)
+{
+	if (ap_apqn_in_matrix_owned_by_def_drv(mdev_apm, mdev_aqm))
+		return -EADDRNOTAVAIL;
+
+	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, mdev_apm, mdev_aqm);
+}
+
 static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
 				    struct vfio_ap_queue *q)
 {

Quoted 8 lines · On Tue, 22 Dec 2020 20:15:57 -0500 Tony Krowiak <[email protected]> wrote:

The current implementation does not allow assignment of an AP adapter or domain to an mdev device if each APQN resulting from the assignment does not reference an AP queue device that is bound to the vfio_ap device driver. This patch allows assignment of AP resources to the matrix mdev as long as the APQNs resulting from the assignment:
1. Are not reserved by the AP BUS for use by the zcrypt device drivers.
2. Are not assigned to another matrix mdev.

Quoted 6 lines

The rationale behind this is twofold:
1. The AP architecture does not preclude assignment of APQNs to an AP configuration that are not available to the system.
2. APQNs that do not reference a queue device bound to the vfio_ap device driver will not be assigned to the guest's CRYCB, so the guest will not get access to queues not bound to the vfio_ap driver.

You didn't tell us about the changed error code.

Also notice that this point we don't have neither filtering nor in-use. This used to be patch 11, and most of that stuff used to be in place. But I'm going to trust you, if you say its fine to enable it this early.

Quoted 47 lines

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c | 241 ++++++++----------------------
  1 file changed, 62 insertions(+), 179 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c +5 -27

index cdcc6378b4a5..2d58b39977be 100644
@@ -379,134 +379,37 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
  	NULL,
  };
 
-struct vfio_ap_queue_reserved {
-	unsigned long *apid;
-	unsigned long *apqi;
-	bool reserved;
-};
+#define MDEV_SHARING_ERR "Userspace may not re-assign queue %02lx.%04lx " \
+			 "already assigned to %s"
 
-/**
- * vfio_ap_has_queue
- *
- * @dev: an AP queue device
- * @data: a struct vfio_ap_queue_reserved reference
- *
- * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
- * apid or apqi specified in @data:
- *
- * - If @data contains both an apid and apqi value, then @data will be flagged
- *   as reserved if the APID and APQI fields for the AP queue device matches
- *
- * - If @data contains only an apid value, @data will be flagged as
- *   reserved if the APID field in the AP queue device matches
- *
- * - If @data contains only an apqi value, @data will be flagged as
- *   reserved if the APQI field in the AP queue device matches
- *
- * Returns 0 to indicate the input to function succeeded. Returns -EINVAL if
- * @data does not contain either an apid or apqi.
- */
-static int vfio_ap_has_queue(struct device *dev, void *data)
+static void vfio_ap_mdev_log_sharing_err(const char *mdev_name,
+					 unsigned long *apm,
+					 unsigned long *aqm)

[..]

drivers/s390/crypto/vfio_ap_ops.c static struct attribute_group *vfio_ap_mdev_type_groups[] = { · line 393

Quoted 4 lines

drivers/s390/crypto/vfio_ap_ops.c +3 -1

-	return 0;
+	for_each_set_bit_inv(apid, apm, AP_DEVICES)
+		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS)
+			pr_warn(MDEV_SHARING_ERR, apid, apqi, mdev_name);

I would prefer dev_warn() here. We know which device is about to get more queues, and this device can provide a clue regarding the initiator.

Also I believe a warning is too heavy handed here. Warnings should not be ignored. This is a condition that can emerge during normal operation, AFAIU. Or am I worng?

Quoted 43 lines

  }

/**
* vfio_ap_mdev_verify_no_sharing
*
- * Verifies that the APQNs derived from the cross product of the AP adapter IDs
- * and AP queue indexes comprising the AP matrix are not configured for another
- * mediated device. AP queue sharing is not allowed.
+ * Verifies that each APQN derived from the Cartesian product of the AP adapter
+ * IDs and AP queue indexes comprising the AP matrix are not configured for
+ * another mediated device. AP queue sharing is not allowed.
*
- * @matrix_mdev: the mediated matrix device
+ * @matrix_mdev: the mediated matrix device to which the APQNs being verified
+ * are assigned.
+ * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
+ * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified *
- * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
+ * Returns 0 if the APQNs are not shared, otherwise; returns -EBUSY. */

drivers/s390/crypto/vfio_ap_ops.c +5 -5

-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
  {
  	struct ap_matrix_mdev *lstdev;
  	DECLARE_BITMAP(apm, AP_DEVICES);

@@ -523,20 +426,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
  		 * We work on full longs, as we can only exclude the leftover
  		 * bits in non-inverse order. The leftover is all zeros.
  		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
  			continue;
 
-		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-				lstdev->matrix.aqm, AP_DOMAINS))
+		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
  			continue;
 

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev) · line 438

Quoted 5 lines

drivers/s390/crypto/vfio_ap_ops.c +4 -1

-		return -EADDRINUSE;
+		vfio_ap_mdev_log_sharing_err(dev_name(mdev_dev(lstdev->mdev)),
+					     apm, aqm);
+
+		return -EBUSY;

Why do we change -EADDRINUSE to -EBUSY? This gets bubbled up to userspace, or? So a tool that checks for the other mdev has it condition by checking for -EADDRINUSE, would be confused...

Quoted 22 lines

drivers/s390/crypto/vfio_ap_ops.c +10 -0

  	}
 
  	return 0;
  }
 
+static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
+				       unsigned long *mdev_apm,
+				       unsigned long *mdev_aqm)
+{
+	if (ap_apqn_in_matrix_owned_by_def_drv(mdev_apm, mdev_aqm))
+		return -EADDRNOTAVAIL;
+
+	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, mdev_apm, mdev_aqm);
+}
+
  static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
  				    struct vfio_ap_queue *q)
  {

@@ -608,10 +522,10 @@ static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev,
   *	   driver; or, if no APQIs have yet been assigned, the APID is not
   *	   contained in an APQN bound to the vfio_ap device driver.

drivers/s390/crypto/vfio_ap_ops.c static void vfio_ap_mdev_link_adapter(struct ap_matrix_mdev *matrix_mdev, · line 528

Quoted 7 lines

drivers/s390/crypto/vfio_ap_ops.c +2 -2

   *
- *	4. -EADDRINUSE
+ *	4. -EBUSY
   *	   An APQN derived from the cross product of the APID being assigned
   *	   and the APQIs previously assigned is being used by another mediated
- *	   matrix device
+ *	   matrix device or the mdev lock could not be acquired.

This is premature. We don't use try_lock yet.

[..]

drivers/s390/crypto/vfio_ap_ops.c static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev, · line 666

Quoted 13 lines

static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
unsigned long apqi)
{

drivers/s390/crypto/vfio_ap_ops.c +2 -2

@@ -774,10 +660,10 @@ static void vfio_ap_mdev_link_domain(struct ap_matrix_mdev *matrix_mdev,
   *	   driver; or, if no APIDs have yet been assigned, the APQI is not
   *	   contained in an APQN bound to the vfio_ap device driver.
   *
- *	4. -EADDRINUSE
+ *	4. -BUSY
   *	   An APQN derived from the cross product of the APQI being assigned
   *	   and the APIDs previously assigned is being used by another mediated
- *	   matrix device
+ *	   matrix device or the mdev lock could not be acquired.

Same here as above.

Otherwise looks good.

[..]

Quoted 11 lines · Halil Pasic

Quoted 8 lines · On Tue, 22 Dec 2020 20:15:57 -0500 Tony Krowiak <[email protected]> wrote:

The current implementation does not allow assignment of an AP adapter or domain to an mdev device if each APQN resulting from the assignment does not reference an AP queue device that is bound to the vfio_ap device driver. This patch allows assignment of AP resources to the matrix mdev as long as the APQNs resulting from the assignment:
1. Are not reserved by the AP BUS for use by the zcrypt device drivers.
2. Are not assigned to another matrix mdev.

Quoted 7 lines

Quoted 6 lines

The rationale behind this is twofold:
1. The AP architecture does not preclude assignment of APQNs to an AP configuration that are not available to the system.
2. APQNs that do not reference a queue device bound to the vfio_ap device driver will not be assigned to the guest's CRYCB, so the guest will not get access to queues not bound to the vfio_ap driver.

You didn't tell us about the changed error code.

I am assuming you are talking about returning -EBUSY from the vfio_ap_mdev_verify_no_sharing() function instead of -EADDRINUSE. I'm going to change this back per your comments below.

Quoted 4 lines

Also notice that this point we don't have neither filtering nor in-use. This used to be patch 11, and most of that stuff used to be in place. But I'm going to trust you, if you say its fine to enable it this early.

The patch order was changed due to your review comments in in Message ID <[email protected]>, patch 07/17 in the v12 series. In order to ensure that only queues bound to the vfio_ap driver are given to the guest, I'm going to create a patch that will preceded this one which introduces the filtering code currently introduced in the patch 12/17, the hot plug patch.

Quoted 40 lines

Quoted 40 lines

/**
* vfio_ap_mdev_verify_no_sharing
*
- * Verifies that the APQNs derived from the cross product of the AP adapter IDs
- * and AP queue indexes comprising the AP matrix are not configured for another
- * mediated device. AP queue sharing is not allowed.
+ * Verifies that each APQN derived from the Cartesian product of the AP adapter
+ * IDs and AP queue indexes comprising the AP matrix are not configured for
+ * another mediated device. AP queue sharing is not allowed.
*
- * @matrix_mdev: the mediated matrix device
+ * @matrix_mdev: the mediated matrix device to which the APQNs being verified
+ * are assigned.
+ * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
+ * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified *
- * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
+ * Returns 0 if the APQNs are not shared, otherwise; returns -EBUSY. */

drivers/s390/crypto/vfio_ap_ops.c +5 -5

-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
  {
  	struct ap_matrix_mdev *lstdev;
  	DECLARE_BITMAP(apm, AP_DEVICES);

@@ -523,20 +426,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
  		 * We work on full longs, as we can only exclude the leftover
  		 * bits in non-inverse order. The leftover is all zeros.
  		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
  			continue;
 
-		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-				lstdev->matrix.aqm, AP_DOMAINS))
+		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
  			continue;
 

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev) · line 438

Quoted 5 lines

drivers/s390/crypto/vfio_ap_ops.c +4 -1

-		return -EADDRINUSE;
+		vfio_ap_mdev_log_sharing_err(dev_name(mdev_dev(lstdev->mdev)),
+					     apm, aqm);
+
+		return -EBUSY;  

Why do we change -EADDRINUSE to -EBUSY? This gets bubbled up to userspace, or? So a tool that checks for the other mdev has it condition by checking for -EADDRINUSE, would be confused...

Back in v8 of the series, Christian suggested the occurrences of -EADDRINUSE should be replaced by the more appropriate -EBUSY (Message ID <[email protected]>), so I changed it here. It does get bubbled up to userspace, so you make a valid point. I will
change it back. I will, however, set the value returned from the __verify_card_reservations() function in ap_bus.c to -EBUSY as suggested by Christian.

Quoted 48 lines · On Thu, 14 Jan 2021 12:54:39 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 40 lines

Quoted 40 lines

   /**
    * vfio_ap_mdev_verify_no_sharing
    *

- * Verifies that the APQNs derived from the cross product of the AP adapter IDs
- * and AP queue indexes comprising the AP matrix are not configured for another
- * mediated device. AP queue sharing is not allowed.
+ * Verifies that each APQN derived from the Cartesian product of the AP adapter
+ * IDs and AP queue indexes comprising the AP matrix are not configured for
+ * another mediated device. AP queue sharing is not allowed.

    *

- * @matrix_mdev: the mediated matrix device
+ * @matrix_mdev: the mediated matrix device to which the APQNs being verified
+ * are assigned.
+ * @mdev_apm: mask indicating the APIDs of the APQNs to be verified
+ * @mdev_aqm: mask indicating the APQIs of the APQNs to be verified

    *
- * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
+ * Returns 0 if the APQNs are not shared, otherwise; returns -EBUSY.
    */

drivers/s390/crypto/vfio_ap_ops.c +5 -5

-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
   {
   	struct ap_matrix_mdev *lstdev;
   	DECLARE_BITMAP(apm, AP_DEVICES);

@@ -523,20 +426,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
   		 * We work on full longs, as we can only exclude the leftover
   		 * bits in non-inverse order. The leftover is all zeros.
   		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
   			continue;
 
-		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-				lstdev->matrix.aqm, AP_DOMAINS))
+		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
   			continue;
 

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev) · line 438

Quoted 5 lines

drivers/s390/crypto/vfio_ap_ops.c +4 -1

-		return -EADDRINUSE;
+		vfio_ap_mdev_log_sharing_err(dev_name(mdev_dev(lstdev->mdev)),
+					     apm, aqm);
+
+		return -EBUSY;

Why do we change -EADDRINUSE to -EBUSY? This gets bubbled up to userspace, or? So a tool that checks for the other mdev has it condition by checking for -EADDRINUSE, would be confused...

Quoted 8 lines

Back in v8 of the series, Christian suggested the occurrences of -EADDRINUSE should be replaced by the more appropriate -EBUSY (Message ID <[email protected]>), so I changed it here. It does get bubbled up to userspace, so you make a valid point. I will
change it back. I will, however, set the value returned from the __verify_card_reservations() function in ap_bus.c to -EBUSY as suggested by Christian.

As long as the error code for an ephemeral failure due to can't take a lock right now, and the error code for a failure due to a sharing conflict are (which most likely requires admin action to be resolved) I'm fine.