s390/vfio-ap: dynamic configuration support

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

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

---
 drivers/s390/crypto/vfio_ap_drv.c     | 28 +++------
 drivers/s390/crypto/vfio_ap_ops.c     | 90 ++++++++++++++-------------
 drivers/s390/crypto/vfio_ap_private.h | 10 ++-
 3 files changed, 60 insertions(+), 68 deletions(-)

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

index 5c0f53c6dde7..134860934fe7 100644
@@ -26,45 +26,16 @@
 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 
-static int match_apqn(struct device *dev, const void *data)
-{
-	struct vfio_ap_queue *q = dev_get_drvdata(dev);
-
-	return (q->apqn == *(int *)(data)) ? 1 : 0;
-}
-

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

-
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)

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

+
+	return NULL;
 }
 
 /**

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

This also looks like a nice code simplification.

Quoted 7 lines

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

---
  drivers/s390/crypto/vfio_ap_drv.c     | 28 +++------
  drivers/s390/crypto/vfio_ap_ops.c     | 90 ++++++++++++++-------------
  drivers/s390/crypto/vfio_ap_private.h | 10 ++-
  3 files changed, 60 insertions(+), 68 deletions(-)

(...)

Quoted 19 lines

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

index 5c0f53c6dde7..134860934fe7 100644
@@ -26,45 +26,16 @@
 
  static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 
-static int match_apqn(struct device *dev, const void *data)
-{
-	struct vfio_ap_queue *q = dev_get_drvdata(dev);
-
-	return (q->apqn == *(int *)(data)) ? 1 : 0;
-}
-
-/**
- * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
- * @matrix_mdev: the associated mediated matrix
- * @apqn: The queue APQN

drivers/s390/crypto/vfio_ap_ops.c line 45

Quoted 6 lines

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

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

Any reason you're killing this comment, instead of adapting it? The function is even no longer static...

Quoted 4 lines

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

- */
-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,

drivers/s390/crypto/vfio_ap_ops.c line 57

Quoted 10 lines

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

-					int apqn)
+struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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;

These were just optimizations and therefore can be dropped now?

Quoted 10 lines

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

-
-	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);
 

drivers/s390/crypto/vfio_ap_ops.c line 36

Quoted 5 lines

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

-	return q;
+	hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) {
+		if (q && (apqn == q->apqn))
+			return q;
+	}

Do we need any serialization here? Previously, the driver core made sure we could get a reference only if the device was still registered; not sure if we need any further guarantees now.

Quoted 6 lines

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

+
+	return NULL;
  }
 
  /**

(...)

Quoted 17 lines · Cornelia Huck

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

This also looks like a nice code simplification.

Quoted 7 lines

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

---
  drivers/s390/crypto/vfio_ap_drv.c     | 28 +++------
  drivers/s390/crypto/vfio_ap_ops.c     | 90 ++++++++++++++-------------
  drivers/s390/crypto/vfio_ap_private.h | 10 ++-
  3 files changed, 60 insertions(+), 68 deletions(-)

(...)

Quoted 14 lines

drivers/s390/crypto/vfio_ap_ops.c line 57

Quoted 13 lines

- */

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

-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,
-					int apqn)
+struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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;  

These were just optimizations and therefore can be dropped now?

The purpose of this function has changed from its previous incarnation. This function was originally called from the handle_pqap() function and served two purposes: It retrieved the struct vfio_ap_queue as driver data and linked the matrix_mdev to the  vfio_ap_queue. The linking of the matrix_mdev and the vfio_ap_queue are now done when queue devices are probed and when adapters and domains are assigned; so now, the handle_pqap() function calls this function to retrieve both the vfio_ap_queue as well as the matrix_mdev to which it is linked. Consequently,
the above code is no longer needed.

Quoted 11 lines

Quoted 11 lines

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

-
-	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);

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c line 36

Quoted 5 lines

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

-	return q;
+	hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) {
+		if (q && (apqn == q->apqn))
+			return q;
+	}  

Do we need any serialization here? Previously, the driver core made sure we could get a reference only if the device was still registered; not sure if we need any further guarantees now.

The vfio_ap_queue structs are created when the queue device is probed and removed when the queue device is removed.

Quoted 9 lines

Quoted 7 lines

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

+
+	return NULL;
  }
 
  /**  

(...)

Quoted 31 lines · On Wed, 8 Apr 2020 11:38:07 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 16 lines · Cornelia Huck

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

This also looks like a nice code simplification.

Quoted 7 lines

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

---
   drivers/s390/crypto/vfio_ap_drv.c     | 28 +++------
   drivers/s390/crypto/vfio_ap_ops.c     | 90 ++++++++++++++-------------
   drivers/s390/crypto/vfio_ap_private.h | 10 ++-
   3 files changed, 60 insertions(+), 68 deletions(-)

(...)

Quoted 14 lines

drivers/s390/crypto/vfio_ap_ops.c line 57

Quoted 13 lines

- */

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

-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,
-					int apqn)
+struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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;

These were just optimizations and therefore can be dropped now?

Quoted 10 lines

The purpose of this function has changed from its previous incarnation. This function was originally called from the handle_pqap() function and served two purposes: It retrieved the struct vfio_ap_queue as driver data and linked the matrix_mdev to the  vfio_ap_queue. The linking of the matrix_mdev and the vfio_ap_queue are now done when queue devices are probed and when adapters and domains are assigned; so now, the handle_pqap() function calls this function to retrieve both the vfio_ap_queue as well as the matrix_mdev to which it is linked. Consequently,
the above code is no longer needed.

Thanks for the explanation, that makes sense.

Quoted 18 lines

Quoted 10 lines

Quoted 10 lines

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

-
-	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);

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c line 36

Quoted 5 lines

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

-	return q;
+	hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) {
+		if (q && (apqn == q->apqn))
+			return q;
+	}

Do we need any serialization here? Previously, the driver core made sure we could get a reference only if the device was still registered; not sure if we need any further guarantees now.

Quoted 2 lines

The vfio_ap_queue structs are created when the queue device is probed and removed when the queue device is removed.

Ok, so anything further is not needed.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

+
+	return NULL;
   }

Quoted 3 lines

Quoted 3 lines

Quoted 1 line

   /**

(...)

Looks good to me, then. With vfio_ap_get_queue made static and the kerneldoc restored/updated:

Reviewed-by Cornelia Huck <[email protected]>

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

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

---
 drivers/s390/crypto/vfio_ap_drv.c     | 28 +++------
 drivers/s390/crypto/vfio_ap_ops.c     | 90 ++++++++++++++-------------
 drivers/s390/crypto/vfio_ap_private.h | 10 ++-
 3 files changed, 60 insertions(+), 68 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +7 -21

index be2520cc010b..e9c226c0730e 100644
@@ -51,15 +51,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
  */
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
-	struct vfio_ap_queue *q;
-
-	q = kzalloc(sizeof(*q), GFP_KERNEL);
-	if (!q)
-		return -ENOMEM;
-	dev_set_drvdata(&apdev->device, q);
-	q->apqn = to_ap_queue(&apdev->device)->qid;
-	q->saved_isc = VFIO_AP_ISC_INVALID;
-	return 0;
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	return vfio_ap_mdev_probe_queue(queue);
 }
 
 /**

@@ -70,18 +64,9 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
  */
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-	struct vfio_ap_queue *q;
-	int apid, apqi;
-
-	mutex_lock(&matrix_dev->lock);
-	q = dev_get_drvdata(&apdev->device);
-	dev_set_drvdata(&apdev->device, NULL);
-	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);
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	vfio_ap_mdev_remove_queue(queue);
 }
 
 static void vfio_ap_matrix_dev_release(struct device *dev)

@@ -135,6 +120,7 @@ static int vfio_ap_matrix_dev_create(void)
 
 	mutex_init(&matrix_dev->lock);
 	INIT_LIST_HEAD(&matrix_dev->mdev_list);
+	hash_init(matrix_dev->qtable);
 
 	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
 	matrix_dev->device.parent = root_device;

drivers/s390/crypto/vfio_ap_ops.c +46 -44

index 5c0f53c6dde7..134860934fe7 100644
@@ -26,45 +26,16 @@
 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 
-static int match_apqn(struct device *dev, const void *data)
-{
-	struct vfio_ap_queue *q = dev_get_drvdata(dev);
-
-	return (q->apqn == *(int *)(data)) ? 1 : 0;
-}
-
-/**
- * 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)
+struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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);
 
-	return q;
+	hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) {
+		if (q && (apqn == q->apqn))
+			return q;
+	}
+
+	return NULL;
 }
 
 /**

@@ -293,10 +264,11 @@ 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_get_queue(apqn);
 	if (!q)
 		goto out_unlock;
 
+	q->matrix_mdev = matrix_mdev;
 	status = vcpu->run->s.regs.gprs[1];
 
 	/* If IR bit(16) is set we enable the interrupt */

@@ -1116,16 +1088,11 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 
 static void vfio_ap_irq_disable_apqn(int apqn)
 {
-	struct device *dev;
 	struct vfio_ap_queue *q;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (dev) {
-		q = dev_get_drvdata(dev);
+	q = vfio_ap_get_queue(apqn);
+	if (q)
 		vfio_ap_irq_disable(q);
-		put_device(dev);
-	}
 }
 
 int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,

@@ -1302,3 +1269,38 @@ void vfio_ap_mdev_unregister(void)
 {
 	mdev_unregister_device(&matrix_dev->device);
 }
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
+{
+	struct vfio_ap_queue *q;
+
+	q = kzalloc(sizeof(*q), GFP_KERNEL);
+	if (!q)
+		return -ENOMEM;
+
+	mutex_lock(&matrix_dev->lock);
+	dev_set_drvdata(&queue->ap_dev.device, q);
+	q->apqn = queue->qid;
+	q->saved_isc = VFIO_AP_ISC_INVALID;
+	hash_add(matrix_dev->qtable, &q->qnode, q->apqn);
+	mutex_unlock(&matrix_dev->lock);
+
+	return 0;
+}
+
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
+{
+	struct vfio_ap_queue *q;
+	int apid, apqi;
+
+	mutex_lock(&matrix_dev->lock);
+	q = dev_get_drvdata(&queue->ap_dev.device);
+	dev_set_drvdata(&queue->ap_dev.device, NULL);
+	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);
+	hash_del(&q->qnode);
+	kfree(q);
+	mutex_unlock(&matrix_dev->lock);
+}

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

index f46dde56b464..e1f8c82cc55d 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"
 

@@ -43,6 +44,7 @@ struct ap_matrix_dev {
 	struct list_head mdev_list;
 	struct mutex lock;
 	struct ap_driver  *vfio_ap_drv;
+	DECLARE_HASHTABLE(qtable, 8);
 };
 
 extern struct ap_matrix_dev *matrix_dev;

@@ -90,8 +92,6 @@ struct ap_matrix_mdev {
 
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
-int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
-			     unsigned int retry);
 
 struct vfio_ap_queue {
 	struct ap_matrix_mdev *matrix_mdev;

@@ -99,6 +99,10 @@ struct vfio_ap_queue {
 	int	apqn;
 #define VFIO_AP_ISC_INVALID 0xff
 	unsigned char saved_isc;
+	struct hlist_node qnode;
 };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue);
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Quoted 230 lines

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

---
 drivers/s390/crypto/vfio_ap_drv.c     | 28 +++------
 drivers/s390/crypto/vfio_ap_ops.c     | 90 ++++++++++++++-------------
 drivers/s390/crypto/vfio_ap_private.h | 10 ++-
 3 files changed, 60 insertions(+), 68 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +7 -21

index be2520cc010b..e9c226c0730e 100644
@@ -51,15 +51,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
  */
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
-	struct vfio_ap_queue *q;
-
-	q = kzalloc(sizeof(*q), GFP_KERNEL);
-	if (!q)
-		return -ENOMEM;
-	dev_set_drvdata(&apdev->device, q);
-	q->apqn = to_ap_queue(&apdev->device)->qid;
-	q->saved_isc = VFIO_AP_ISC_INVALID;
-	return 0;
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	return vfio_ap_mdev_probe_queue(queue);
 }
 
 /**

@@ -70,18 +64,9 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
  */
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-	struct vfio_ap_queue *q;
-	int apid, apqi;
-
-	mutex_lock(&matrix_dev->lock);
-	q = dev_get_drvdata(&apdev->device);
-	dev_set_drvdata(&apdev->device, NULL);
-	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);
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	vfio_ap_mdev_remove_queue(queue);
 }
 
 static void vfio_ap_matrix_dev_release(struct device *dev)

@@ -135,6 +120,7 @@ static int vfio_ap_matrix_dev_create(void)
 
 	mutex_init(&matrix_dev->lock);
 	INIT_LIST_HEAD(&matrix_dev->mdev_list);
+	hash_init(matrix_dev->qtable);
 
 	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
 	matrix_dev->device.parent = root_device;

drivers/s390/crypto/vfio_ap_ops.c +46 -44

index 5c0f53c6dde7..134860934fe7 100644
@@ -26,45 +26,16 @@
 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 
-static int match_apqn(struct device *dev, const void *data)
-{
-	struct vfio_ap_queue *q = dev_get_drvdata(dev);
-
-	return (q->apqn == *(int *)(data)) ? 1 : 0;
-}
-
-/**
- * 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)
+struct vfio_ap_queue *vfio_ap_get_queue(unsigned long 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);
 
-	return q;
+	hash_for_each_possible(matrix_dev->qtable, q, qnode, apqn) {
+		if (q && (apqn == q->apqn))
+			return q;
+	}
+
+	return NULL;
 }
 
 /**

@@ -293,10 +264,11 @@ 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_get_queue(apqn);
 	if (!q)
 		goto out_unlock;
 
+	q->matrix_mdev = matrix_mdev;
 	status = vcpu->run->s.regs.gprs[1];
 
 	/* If IR bit(16) is set we enable the interrupt */

@@ -1116,16 +1088,11 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 
 static void vfio_ap_irq_disable_apqn(int apqn)
 {
-	struct device *dev;
 	struct vfio_ap_queue *q;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (dev) {
-		q = dev_get_drvdata(dev);
+	q = vfio_ap_get_queue(apqn);
+	if (q)
 		vfio_ap_irq_disable(q);
-		put_device(dev);
-	}
 }
 
 int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,

@@ -1302,3 +1269,38 @@ void vfio_ap_mdev_unregister(void)
 {
 	mdev_unregister_device(&matrix_dev->device);
 }
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
+{
+	struct vfio_ap_queue *q;
+
+	q = kzalloc(sizeof(*q), GFP_KERNEL);
+	if (!q)
+		return -ENOMEM;
+
+	mutex_lock(&matrix_dev->lock);
+	dev_set_drvdata(&queue->ap_dev.device, q);
+	q->apqn = queue->qid;
+	q->saved_isc = VFIO_AP_ISC_INVALID;
+	hash_add(matrix_dev->qtable, &q->qnode, q->apqn);
+	mutex_unlock(&matrix_dev->lock);
+
+	return 0;
+}
+
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
+{
+	struct vfio_ap_queue *q;
+	int apid, apqi;
+
+	mutex_lock(&matrix_dev->lock);
+	q = dev_get_drvdata(&queue->ap_dev.device);
+	dev_set_drvdata(&queue->ap_dev.device, NULL);
+	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);
+	hash_del(&q->qnode);
+	kfree(q);
+	mutex_unlock(&matrix_dev->lock);
+}

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

index f46dde56b464..e1f8c82cc55d 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"
 

@@ -43,6 +44,7 @@ struct ap_matrix_dev {
 	struct list_head mdev_list;
 	struct mutex lock;
 	struct ap_driver  *vfio_ap_drv;
+	DECLARE_HASHTABLE(qtable, 8);
 };
 
 extern struct ap_matrix_dev *matrix_dev;

@@ -90,8 +92,6 @@ struct ap_matrix_mdev {
 
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
-int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
-			     unsigned int retry);
 
 struct vfio_ap_queue {
 	struct ap_matrix_mdev *matrix_mdev;

@@ -99,6 +99,10 @@ struct vfio_ap_queue {
 	int	apqn;
 #define VFIO_AP_ISC_INVALID 0xff
 	unsigned char saved_isc;
+	struct hlist_node qnode;
 };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue);
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 17 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Quoted 2 lines

Regards,
Halil

Hi Halil

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific.

Regards,
Halil

Quoted 2 lines

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

Quoted 25 lines · Halil Pasic

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 7 lines

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

The code you reference, _zcrypt_send_ep11_cprb(), does loop through each queue associated with each card, but it doesn't appear to be looking for
a queue with a particular APQN. It appears to be looking for a queue meeting a specific set of conditions. At least that's my take after taking a very
brief look at the code, so I'm not sure that applies here.

Quoted 14 lines

If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific.

Regards,
Halil

Quoted 2 lines

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

Quoted 34 lines · On Mon, 27 Apr 2020 17:48:58 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 25 lines · Halil Pasic

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 7 lines

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

Quoted 8 lines

The code you reference, _zcrypt_send_ep11_cprb(), does loop through each queue associated with each card, but it doesn't appear to be looking for
a queue with a particular APQN. It appears to be looking for a queue meeting a specific set of conditions. At least that's my take after taking a very
brief look at the code, so I'm not sure that applies here.

One of the possible conditions is that the APQN is in the targets array. Please have another look at the code below, is_desired_ep11_queue() and is_desired_ep11_card() do APQI and APID part of the check respectively:

        for_each_zcrypt_card(zc) {
                /* Check for online EP11 cards */
                if (!zc->online || !(zc->card->functions & 0x04000000))
                        continue;
                /* Check for user selected EP11 card */
                if (targets &&
                    !is_desired_ep11_card(zc->card->id, target_num, targets))
                        continue;
                /* check if device node has admission for this card */
                if (!zcrypt_check_card(perms, zc->card->id))
                        continue;
                /* get weight index of the card device  */
                weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY];
                if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight))
                        continue;
                for_each_zcrypt_queue(zq, zc) {
                        /* check if device is online and eligible */
                        if (!zq->online ||
                            !zq->ops->send_ep11_cprb ||
                            (targets &&
                             !is_desired_ep11_queue(zq->queue->qid,
                                                    target_num, targets)))

Yes the size of targets may or may not be 1 (example for size == 1 is the invocation form ep11_cryptsingle()) and the respective costs depend on the usual size of the array. Since the goal of the whole exercise seems to be to pick a single queue, and we settle with the first suitable (first not in the input array, but in our lists) that is suitable, I assumed we wouldn't need many hashtable lookups.

Regards,
Halil

Quoted 12 lines

Quoted 12 lines

If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific.

Regards,
Halil

Quoted 2 lines

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

Quoted 80 lines · Halil Pasic

Quoted 33 lines · On Mon, 27 Apr 2020 17:48:58 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 25 lines · Halil Pasic

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 7 lines

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

Quoted 8 lines

The code you reference, _zcrypt_send_ep11_cprb(), does loop through each queue associated with each card, but it doesn't appear to be looking for
a queue with a particular APQN. It appears to be looking for a queue meeting a specific set of conditions. At least that's my take after taking a very
brief look at the code, so I'm not sure that applies here.

One of the possible conditions is that the APQN is in the targets array. Please have another look at the code below, is_desired_ep11_queue() and is_desired_ep11_card() do APQI and APID part of the check respectively:

         for_each_zcrypt_card(zc) {
                 /* Check for online EP11 cards */
                 if (!zc->online || !(zc->card->functions & 0x04000000))
                         continue;
                 /* Check for user selected EP11 card */
                 if (targets &&
                     !is_desired_ep11_card(zc->card->id, target_num, targets))
                         continue;
                 /* check if device node has admission for this card */
                 if (!zcrypt_check_card(perms, zc->card->id))
                         continue;
                 /* get weight index of the card device  */
                 weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY];
                 if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight))
                         continue;
                 for_each_zcrypt_queue(zq, zc) {
                         /* check if device is online and eligible */
                         if (!zq->online ||
                             !zq->ops->send_ep11_cprb ||
                             (targets &&
                              !is_desired_ep11_queue(zq->queue->qid,
                                                     target_num, targets)))

Yes the size of targets may or may not be 1 (example for size == 1 is the invocation form ep11_cryptsingle()) and the respective costs depend on the usual size of the array. Since the goal of the whole exercise seems to be to pick a single queue, and we settle with the first suitable (first not in the input array, but in our lists) that is suitable, I assumed we wouldn't need many hashtable lookups.

Quoted 2 lines

Regards,
Halil

again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards. If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that for you.
One note for the improvement via hash list with the argument about the max 65535 objects.
Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve the linear search through an array or list but can you measure the performance gain and then compare this to the complexity.  ... just some thoughts about beautifying code ...

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific.

Regards,
Halil

Quoted 2 lines

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

Quoted 83 lines · Harald Freudenberger

Quoted 80 lines · Halil Pasic

Quoted 33 lines · On Mon, 27 Apr 2020 17:48:58 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 25 lines · Halil Pasic

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

On Tue,  7 Apr 2020 15:20:01 -0400
Tony Krowiak <[email protected]> wrote:

Quoted 3 lines

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 7 lines

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

Quoted 8 lines

The code you reference, _zcrypt_send_ep11_cprb(), does loop through each queue associated with each card, but it doesn't appear to be looking for
a queue with a particular APQN. It appears to be looking for a queue meeting a specific set of conditions. At least that's my take after taking a very
brief look at the code, so I'm not sure that applies here.

One of the possible conditions is that the APQN is in the targets array. Please have another look at the code below, is_desired_ep11_queue() and is_desired_ep11_card() do APQI and APID part of the check respectively:

         for_each_zcrypt_card(zc) {
                 /* Check for online EP11 cards */
                 if (!zc->online || !(zc->card->functions & 0x04000000)) continue;
                 /* Check for user selected EP11 card */
                 if (targets && !is_desired_ep11_card(zc->card->id, target_num, targets)) continue;
                 /* check if device node has admission for this card */ if (!zcrypt_check_card(perms, zc->card->id)) continue;
                 /* get weight index of the card device  */
                 weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY]; if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight)) continue;
                 for_each_zcrypt_queue(zq, zc) {
                         /* check if device is online and eligible */ if (!zq->online || !zq->ops->send_ep11_cprb ||
                             (targets && !is_desired_ep11_queue(zq->queue->qid, target_num, targets)))

Yes the size of targets may or may not be 1 (example for size == 1 is the invocation form ep11_cryptsingle()) and the respective costs depend on the usual size of the array. Since the goal of the whole exercise seems to be to pick a single queue, and we settle with the first suitable (first not in the input array, but in our lists) that is suitable, I assumed we wouldn't need many hashtable lookups.

Quoted 2 lines

Regards,
Halil

Quoted 11 lines

again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards. If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that for you.
One note for the improvement via hash list with the argument about the max 65535 objects.
Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve the linear search through an array or list but can you measure the performance gain and then compare this to the complexity.  ... just some thoughts about beautifying code ...

I set up a test case to compare searching using a hashtable verses using a list. I created both a hashtable and a list of 5100 objects. Each structure had a single APQN field. I then randomly searched both the hashtable and the list for each APQN. The following table contains the result of 5 test runs. The elapsed times are in nanoseconds.

Test:                              List Search    Hashtable Search ------                              ----------- ---------------- Avg. Per APQN:             11651           81
Total per 5500 APQNs:  60164268     1085368

Avg. Per APQN:              10925           78
Total per 5500 APQNs:   56482780    1084590

Avg. Per APQN:              10190           80
Total per 5500 APQNs:   52714920    1123205

Avg. Per APQN:              8431             76
Total per 5500 APQNs:   43748838    1061414

Avg. Per APQN:              9678             75
Total per 5500 APQNs:   50103437    1044427
-----------------------------------------------
Per APQN Search Avg:   10175          78            Hashtable is 130 times faster Total Search 5500 Avg:  52642848    1079800  Hashtable is 49 times faster

Note that the list search was just a straight search of an object in a list, not a device attached to a bus. I don't know if that would add time, but it seems that the savings using a hashtable are significant.

So I have two questions:

1. Would it make more sense to provide AP bus interfaces to search for queue devices by APQN?

2. If so, shall we store the queue devices in a hashtable to make the searches more efficient?

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific.

Regards,
Halil

Quoted 2 lines

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

Quoted 81 lines · Halil Pasic

Quoted 34 lines · On Mon, 27 Apr 2020 17:48:58 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 25 lines · Halil Pasic

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 7 lines

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

Quoted 8 lines

The code you reference, _zcrypt_send_ep11_cprb(), does loop through each queue associated with each card, but it doesn't appear to be looking for
a queue with a particular APQN. It appears to be looking for a queue meeting a specific set of conditions. At least that's my take after taking a very
brief look at the code, so I'm not sure that applies here.

One of the possible conditions is that the APQN is in the targets array. Please have another look at the code below, is_desired_ep11_queue() and is_desired_ep11_card() do APQI and APID part of the check respectively:

        for_each_zcrypt_card(zc) {
                /* Check for online EP11 cards */
                if (!zc->online || !(zc->card->functions & 0x04000000))
                        continue;
                /* Check for user selected EP11 card */
                if (targets &&
                    !is_desired_ep11_card(zc->card->id, target_num, targets))
                        continue;
                /* check if device node has admission for this card */
                if (!zcrypt_check_card(perms, zc->card->id))
                        continue;
                /* get weight index of the card device  */
                weight = speed_idx_ep11(func_code) * zc->speed_rating[SECKEY];
                if (zcrypt_card_compare(zc, pref_zc, weight, pref_weight))
                        continue;
                for_each_zcrypt_queue(zq, zc) {
                        /* check if device is online and eligible */
                        if (!zq->online ||
                            !zq->ops->send_ep11_cprb ||
                            (targets &&
                             !is_desired_ep11_queue(zq->queue->qid,
                                                    target_num, targets)))

Yes the size of targets may or may not be 1 (example for size == 1 is the invocation form ep11_cryptsingle()) and the respective costs depend on the usual size of the array. Since the goal of the whole exercise seems to be to pick a single queue, and we settle with the first suitable (first not in the input array, but in our lists) that is suitable, I assumed we wouldn't need many hashtable lookups.

Quoted 2 lines

Regards,
Halil

again, this is all code related to zcrypt card and queues and has nothing directly to do with ap queue and ap cards.

If you want to have a look how this works for ap devices, have a look into the scan routines for the ap bus in ap_bus.c There you can find a bus_for_each_device() which would fit together with the right matching function for your needs. And this is exactly what Tony implemented in the first shot. However, as written I can provide something like that for you.
One note for the improvement via hash list with the argument about the max 65535 objects.
Think about a real big machine which has currently up to 30 crypto cards (z15 GA1.5) which when CEX7S are plugged appear as 60 crypto adapters and have up to 85 domains each. When all these crypto resources are assigned to one LPAR we end up in 60x85 = 5100 APQNs. Well, of course with a hash you can improve the linear search through an array or list but can you measure the performance gain and then compare this to the complexity.  ... just some thoughts about beautifying code ...

Quoted 22 lines · On Mon, 27 Apr 2020 15:05:23 +0200 Harald Freudenberger <[email protected]>…

Quoted 11 lines · Halil Pasic

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:01 -0400 Tony Krowiak <[email protected]> wrote:

Rather than looping over potentially 65535 objects, let's store the structures for caching information about queue devices bound to the vfio_ap device driver in a hash table keyed by APQN.

@Harald:
Would it make sense to make the efficient lookup of an apqueue base on its APQN core AP functionality instead of each driver figuring it out on it's own?

Quoted 8 lines

If I'm not wrong the zcrypt device/driver(s) must the problem of looking up a queue based on its APQN as well.

For instance struct ep11_cprb has a target_id filed (arch/s390/include/uapi/asm/zcrypt.h).

Regards,
Halil

Hi Halil

Quoted 2 lines

no, the zcrypt drivers don't have this problem. They build up their own device object which includes a pointer to the base ap device.

I'm a bit confused. Doesn't your code loop first trough the ap_card objects to find the APID portion of the APQN, and then loop the queue list of the matching card to find the right ap_queue object? Or did I miss something? Isn't that what _zcrypt_send_ep11_cprb() does? Can you point me to the code that avoids the lookup (by apqn) for zcrypt?

If you look at the new function of vfio_ap_get_queue(unsigned long apqn) it basically about finding the queue based on the apqn, with the difference that it is vfio specific.

Regards,
Halil

Quoted 2 lines

However, this is not a big issue, as the ap_bus holds a list of ap_card objects and within each ap_card object there exists a list of ap_queues.

A vfio_ap_queue structure is created for each queue device probed. To ensure that the matrix mdev to which a queue's APQN is assigned is linked to the queue structure as long as the queue device is bound to the vfio_ap device driver, let's go ahead and manage these links when the queue device is probed and removed as well as whenever an adapter or domain is assigned to or unassigned from the matrix mdev.

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

---
 drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 5 deletions(-)

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

+ *
+ * @matrix_mdev: a matrix mediated device
+ * @apqi:	 the APID of one or more APQNs assigned to @matrix_mdev
+ *
+ * Set the link to @matrix_mdev for each queue device bound to the vfio_ap
+ * device driver with an APQN assigned to @matrix_mdev with the specified @apid.
+ *
+ * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed.
+ */
+static void vfio_ap_mdev_qlinks_for_apid(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apid)
+{
+	unsigned long apqi;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+			     matrix_mdev->matrix.aqm_max + 1) {
+		q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+		if (q)
+			q->matrix_mdev = matrix_mdev;
+	}
+}
+
 /**
  * assign_adapter_store
  *

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

+ *
+ * @matrix_mdev: a matrix mediated device
+ * @apqi:	 the APQI of one or more APQNs assigned to @matrix_mdev
+ *
+ * Set the link to @matrix_mdev for each queue device bound to the vfio_ap
+ * device driver with an APQN assigned to @matrix_mdev with the specified @apqi.
+ *
+ * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed.
+ */
+static void vfio_ap_mdev_qlinks_for_apqi(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apqi)
+{
+	unsigned long apid;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+		if (q)
+			q->matrix_mdev = matrix_mdev;
+	}
+}
+
 /**
  * assign_domain_store
  *

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

+{
+	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)) {
+			q->matrix_mdev = matrix_mdev;
+			break;
+		}
+	}
+}
+
 int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
 {
 	struct vfio_ap_queue *q;

Quoted 11 lines · On Tue, 7 Apr 2020 15:20:02 -0400 Tony Krowiak <[email protected]> wrote:

A vfio_ap_queue structure is created for each queue device probed. To ensure that the matrix mdev to which a queue's APQN is assigned is linked to the queue structure as long as the queue device is bound to the vfio_ap device driver, let's go ahead and manage these links when the queue device is probed and removed as well as whenever an adapter or domain is assigned to or unassigned from the matrix mdev.

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

---
  drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++++++++++++---
  1 file changed, 70 insertions(+), 5 deletions(-)

(...)

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

Quoted 6 lines

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

@@ -536,6 +531,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
  	return 0;
  }
 
+/**
+ * vfio_ap_mdev_qlinks_for_apid

Hm... maybe the function name should express that there's some actual (un)linking going on?

vfio_ap_mdev_link_by_apid?

Or make this vfio_ap_mdev_link_queues() and pass in an indicator whether the passed value is an apid or an aqid? Both function names look so very similar to be easily confused (at least to me).

Quoted 27 lines

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

+ *
+ * @matrix_mdev: a matrix mediated device
+ * @apqi:	 the APID of one or more APQNs assigned to @matrix_mdev
+ *
+ * Set the link to @matrix_mdev for each queue device bound to the vfio_ap
+ * device driver with an APQN assigned to @matrix_mdev with the specified @apid.
+ *
+ * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed.
+ */
+static void vfio_ap_mdev_qlinks_for_apid(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apid)
+{
+	unsigned long apqi;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+			     matrix_mdev->matrix.aqm_max + 1) {
+		q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+		if (q)
+			q->matrix_mdev = matrix_mdev;
+	}
+}
+
  /**
   * assign_adapter_store
   *

(...)

drivers/s390/crypto/vfio_ap_ops.c vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev, · line 708

Quoted 6 lines

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

@@ -682,6 +704,31 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
  	return 0;
  }
 
+/**
+ * vfio_ap_mdev_qlinks_for_apqi

See my comment above.

Quoted 26 lines

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

+ *
+ * @matrix_mdev: a matrix mediated device
+ * @apqi:	 the APQI of one or more APQNs assigned to @matrix_mdev
+ *
+ * Set the link to @matrix_mdev for each queue device bound to the vfio_ap
+ * device driver with an APQN assigned to @matrix_mdev with the specified @apqi.
+ *
+ * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed.
+ */
+static void vfio_ap_mdev_qlinks_for_apqi(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apqi)
+{
+	unsigned long apid;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+		if (q)
+			q->matrix_mdev = matrix_mdev;
+	}
+}
+
  /**
   * assign_domain_store
   *

(...)

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_unregister(void) · line 1322

Quoted 5 lines

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

@@ -1270,6 +1319,21 @@ void vfio_ap_mdev_unregister(void)
  	mdev_unregister_device(&matrix_dev->device);
  }
 
+static void vfio_ap_mdev_for_queue(struct vfio_ap_queue *q)

vfio_ap_queue_link_mdev()? It is the other direction from the linking above.

Quoted 19 lines

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

+{
+	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)) {
+			q->matrix_mdev = matrix_mdev;
+			break;
+		}
+	}
+}
+
  int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
  {
  	struct vfio_ap_queue *q;

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_probe_queue(struct ap_queue *queue) · line 1352

Quoted 7 lines

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

@@ -1282,6 +1346,7 @@ int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
  	dev_set_drvdata(&queue->ap_dev.device, q);
  	q->apqn = queue->qid;
  	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_mdev_for_queue(q);
  	hash_add(matrix_dev->qtable, &q->qnode, q->apqn);
  	mutex_unlock(&matrix_dev->lock);
 

In general, looks sane.

Quoted 11 lines · On Tue, 7 Apr 2020 15:20:02 -0400 Tony Krowiak <[email protected]> wrote:

A vfio_ap_queue structure is created for each queue device probed. To ensure that the matrix mdev to which a queue's APQN is assigned is linked to the queue structure as long as the queue device is bound to the vfio_ap device driver, let's go ahead and manage these links when the queue device is probed and removed as well as whenever an adapter or domain is assigned to or unassigned from the matrix mdev.

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

---
  drivers/s390/crypto/vfio_ap_ops.c | 75 ++++++++++++++++++++++++++++---
  1 file changed, 70 insertions(+), 5 deletions(-)

(...)

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

Quoted 6 lines

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

@@ -536,6 +531,31 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
  	return 0;
  }
 
+/**
+ * vfio_ap_mdev_qlinks_for_apid

Hm... maybe the function name should express that there's some actual (un)linking going on?

vfio_ap_mdev_link_by_apid?

Or make this vfio_ap_mdev_link_queues() and pass in an indicator whether the passed value is an apid or an aqid? Both function names look so very similar to be easily confused (at least to me).

Quoted 27 lines

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

+ *
+ * @matrix_mdev: a matrix mediated device
+ * @apqi:	 the APID of one or more APQNs assigned to @matrix_mdev
+ *
+ * Set the link to @matrix_mdev for each queue device bound to the vfio_ap
+ * device driver with an APQN assigned to @matrix_mdev with the specified @apid.
+ *
+ * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed.
+ */
+static void vfio_ap_mdev_qlinks_for_apid(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apid)
+{
+	unsigned long apqi;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+			     matrix_mdev->matrix.aqm_max + 1) {
+		q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+		if (q)
+			q->matrix_mdev = matrix_mdev;
+	}
+}
+
  /**
   * assign_adapter_store
   *

(...)

drivers/s390/crypto/vfio_ap_ops.c vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev, · line 708

Quoted 6 lines

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

@@ -682,6 +704,31 @@ vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
  	return 0;
  }
 
+/**
+ * vfio_ap_mdev_qlinks_for_apqi

See my comment above.

Quoted 26 lines

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

+ *
+ * @matrix_mdev: a matrix mediated device
+ * @apqi:	 the APQI of one or more APQNs assigned to @matrix_mdev
+ *
+ * Set the link to @matrix_mdev for each queue device bound to the vfio_ap
+ * device driver with an APQN assigned to @matrix_mdev with the specified @apqi.
+ *
+ * Note: If @matrix_mdev is NULL, the link to @matrix_mdev will be severed.
+ */
+static void vfio_ap_mdev_qlinks_for_apqi(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apqi)
+{
+	unsigned long apid;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+		if (q)
+			q->matrix_mdev = matrix_mdev;
+	}
+}
+
  /**
   * assign_domain_store
   *

(...)

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_unregister(void) · line 1322

Quoted 5 lines

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

@@ -1270,6 +1319,21 @@ void vfio_ap_mdev_unregister(void)
  	mdev_unregister_device(&matrix_dev->device);
  }
 
+static void vfio_ap_mdev_for_queue(struct vfio_ap_queue *q)

vfio_ap_queue_link_mdev()? It is the other direction from the linking above.

Quoted 19 lines

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

+{
+	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)) {
+			q->matrix_mdev = matrix_mdev;
+			break;
+		}
+	}
+}
+
  int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
  {
  	struct vfio_ap_queue *q;

drivers/s390/crypto/vfio_ap_ops.c int vfio_ap_mdev_probe_queue(struct ap_queue *queue) · line 1352

Quoted 7 lines

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

@@ -1282,6 +1346,7 @@ int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
  	dev_set_drvdata(&queue->ap_dev.device, q);
  	q->apqn = queue->qid;
  	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_mdev_for_queue(q);
  	hash_add(matrix_dev->qtable, &q->qnode, q->apqn);
  	mutex_unlock(&matrix_dev->lock);
 

In general, looks sane.

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it. The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

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

---
 drivers/s390/crypto/ap_bus.c | 144 +++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   4 +
 2 files changed, 142 insertions(+), 6 deletions(-)

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

index 5256e3ce84e5..af15c095e76a 100644
@@ -35,6 +35,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/debugfs.h>
 #include <linux/ctype.h>
+#include <linux/module.h>
 
 #include "ap_bus.h"
 #include "ap_debug.h"

@@ -995,9 +996,11 @@ int ap_parse_mask_str(const char *str,
 	newmap = kmalloc(size, GFP_KERNEL);

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

 	}
 
 	if (*str == '+' || *str == '-') {

Quoted 37 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it. The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

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

---
  drivers/s390/crypto/ap_bus.c | 144 +++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   4 +
  2 files changed, 142 insertions(+), 6 deletions(-)

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

index 5256e3ce84e5..af15c095e76a 100644
@@ -35,6 +35,7 @@
  #include <linux/mod_devicetable.h>
  #include <linux/debugfs.h>
  #include <linux/ctype.h>
+#include <linux/module.h>
 
  #include "ap_bus.h"
  #include "ap_debug.h"

@@ -995,9 +996,11 @@ int ap_parse_mask_str(const char *str,
  	newmap = kmalloc(size, GFP_KERNEL);

drivers/s390/crypto/ap_bus.c int ap_parse_mask_str(const char *str, · line 1003

Quoted 10 lines

drivers/s390/crypto/ap_bus.c +5 -3

  	if (!newmap)
  		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}

This whole function is a bit odd. It seems all masks we want to manipulate are always guarded by the ap_perms_mutex, and the need for allowing lock == NULL comes from wanting to call this function with the ap_perms_mutex already held.

That would argue for a locked/unlocked version of this function... but looking at it, why do we lock the way we do? The one thing this function (prior to this patch) does outside of the holding of the mutex is the allocation and freeing of newmap. But with this patch, we do the allocation and freeing of newmap while holding the mutex. Something seems a bit weird here.

Quoted 4 lines

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

  	}
 
  	if (*str == '+' || *str == '-') {

Quoted 20 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c int ap_parse_mask_str(const char *str, · line 1003

Quoted 12 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

drivers/s390/crypto/ap_bus.c +5 -3

@@ -995,9 +996,11 @@ int ap_parse_mask_str(const char *str,
  	newmap = kmalloc(size, GFP_KERNEL);
  	if (!newmap)
  		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}  

This whole function is a bit odd. It seems all masks we want to manipulate are always guarded by the ap_perms_mutex, and the need for allowing lock == NULL comes from wanting to call this function with the ap_perms_mutex already held.

Quoted 6 lines

That would argue for a locked/unlocked version of this function... but looking at it, why do we lock the way we do? The one thing this function (prior to this patch) does outside of the holding of the mutex is the allocation and freeing of newmap. But with this patch, we do the allocation and freeing of newmap while holding the mutex. Something seems a bit weird here.

Note that the ap_parse_mask function copies the newmap
to the bitmap passed in as a parameter to the function. Prior to the introduction of this patch, the calling functions - i.e., apmask_store(), aqmask_store() and ap_perms_init() - passed in the actual bitmap (i.e., ap_perms.apm or ap_perms aqm), so the ap_perms were changed directly by this function.

With this patch, the apmask_store() and aqmask_store() functions now pass in a copy of those bitmaps. This is so we can verify that any APQNs being removed are not in use by the vfio_ap device driver before committing the change to ap_perms. Consequently, it is now necessary to take the lock for the until the changes are committed.

Having explained that, you make a valid argument that this calls for a locked/unlocked version of this function, so I will modify this patch to that effect.

Quoted 33 lines · On Wed, 15 Apr 2020 13:10:18 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 19 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c int ap_parse_mask_str(const char *str, · line 1003

Quoted 12 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

drivers/s390/crypto/ap_bus.c +5 -3

@@ -995,9 +996,11 @@ int ap_parse_mask_str(const char *str,
   	newmap = kmalloc(size, GFP_KERNEL);
   	if (!newmap)
   		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}

This whole function is a bit odd. It seems all masks we want to manipulate are always guarded by the ap_perms_mutex, and the need for allowing lock == NULL comes from wanting to call this function with the ap_perms_mutex already held.

Quoted 6 lines

That would argue for a locked/unlocked version of this function... but looking at it, why do we lock the way we do? The one thing this function (prior to this patch) does outside of the holding of the mutex is the allocation and freeing of newmap. But with this patch, we do the allocation and freeing of newmap while holding the mutex. Something seems a bit weird here.

Note that the ap_parse_mask function copies the newmap
to the bitmap passed in as a parameter to the function. Prior to the introduction of this patch, the calling functions - i.e., apmask_store(), aqmask_store() and ap_perms_init() - passed in the actual bitmap (i.e., ap_perms.apm or ap_perms aqm), so the ap_perms were changed directly by this function.

Quoted 6 lines

With this patch, the apmask_store() and aqmask_store() functions now pass in a copy of those bitmaps. This is so we can verify that any APQNs being removed are not in use by the vfio_ap device driver before committing the change to ap_perms. Consequently, it is now necessary to take the lock for the until the changes are committed.

Yes, but every caller actually takes the mutex before calling this function already :)

Quoted 4 lines

Having explained that, you make a valid argument that this calls for a locked/unlocked version of this function, so I will modify this patch to that effect.

Ok.

The other thing I found weird is that the function does
alloc newmap -> grab mutex -> do manipulation -> release mutex -> free newmap while the new callers do
(mutex already held) -> alloc newmap

so why grab/release the mutex the way the function does now? IOW, why not have an unlocked __ap_parse_mask_string() and do

int ap_parse_mask_string(...)

{
	int rc;

	if (mutex_lock_interruptible(&ap_perms_mutex))
		return -ERESTARTSYS;
	rc = __ap_parse_mask_string(...);
         mutex_unlock(&ap_perms_mutex);
	return rc;
}

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it. The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

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

---
 drivers/s390/crypto/ap_bus.c | 144 +++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   4 +
 2 files changed, 142 insertions(+), 6 deletions(-)

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

@@ -1196,12 +1202,75 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 

Quoted 22 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it. The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

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

---
 drivers/s390/crypto/ap_bus.c | 144 +++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   4 +
 2 files changed, 142 insertions(+), 6 deletions(-)

(...)

Quoted 4 lines

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

@@ -1196,12 +1202,75 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1214

Quoted 10 lines

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

+int __verify_card_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newapm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify cards reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.

I had to read that one several times... what about

"No need to verify whether the driver is using the queues if it is the default driver."

?

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1221

Quoted 7 lines

drivers/s390/crypto/ap_bus.c +7 -0

+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;

Is that really needed? I would have thought that the driver core's klist usage would make sure that the callback would not be invoked for drivers that are not registered anymore. Or am I missing a window?

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1224

Quoted 4 lines

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

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1230

Quoted 6 lines

drivers/s390/crypto/ap_bus.c +6 -0

+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}

(Same comments for the other changes further along in this patch.)

Quoted 9 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1221

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

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

+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;  

Is that really needed? I would have thought that the driver core's klist usage would make sure that the callback would not be invoked for drivers that are not registered anymore. Or am I missing a window?

The try_module_get() and module_put() is a result of review feedback from my side. The ap bus core is static in the kernel whereas the vfio dd is a kernel module. So there may be a race condition between calling the callback function and removal of the vfio dd module. There is similar code in zcrypt_api which does the same for the zcrypt device drivers before using some variables or functions from the modules. Help me, it this is outdated code and there is no need to adjust the module reference counter any more, then I would be happy to remove this code :-)

Quoted 9 lines · On Wed, 15 Apr 2020 08:08:24 +0200 Harald Freudenberger <[email protected]>…

Quoted 8 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1221

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

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

+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;  

Is that really needed? I would have thought that the driver core's klist usage would make sure that the callback would not be invoked for drivers that are not registered anymore. Or am I missing a window?

Quoted 9 lines

The try_module_get() and module_put() is a result of review feedback from my side. The ap bus core is static in the kernel whereas the vfio dd is a kernel module. So there may be a race condition between calling the callback function and removal of the vfio dd module. There is similar code in zcrypt_api which does the same for the zcrypt device drivers before using some variables or functions from the modules. Help me, it this is outdated code and there is no need to adjust the module reference counter any more, then I would be happy to remove this code :-)

I think the driver core already should keep us safe. A built-in bus calling a driver in a module is a very common pattern, and I think ->owner was introduced exactly for that case.

Unless I'm really missing something obvious?

Quoted 22 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it. The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

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

---
  drivers/s390/crypto/ap_bus.c | 144 +++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   4 +
  2 files changed, 142 insertions(+), 6 deletions(-)

(...)

Quoted 4 lines

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

@@ -1196,12 +1202,75 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
  	return rc;
  }
 

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1214

Quoted 10 lines

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

+int __verify_card_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newapm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify cards reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.

I had to read that one several times... what about
"No need to verify whether the driver is using the queues if it is the default driver."

?

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1221

Quoted 8 lines

drivers/s390/crypto/ap_bus.c +7 -0

+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;

Is that really needed? I would have thought that the driver core's klist usage would make sure that the callback would not be invoked for drivers that are not registered anymore. Or am I missing a window?

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1224

Quoted 3 lines

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

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1230

Quoted 7 lines

drivers/s390/crypto/ap_bus.c +6 -0

+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}

(Same comments for the other changes further along in this patch.)

Quoted 8 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1224

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

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

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))  

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

One of the things on my TODO list is to add logging to the vfio_ap module which will track all significant activity within the device driver. I plan to do that with a patch or set of patches specifically put together for that purpose. Having said that, the best place to log this would be in the in_use callback in the vfio_ap device driver (see next patch) where the APQNs that are in use can be identified. For now, I will log a message to the dmesg log indicating which APQNs are in use by the matrix mdev.

Quoted 8 lines

Quoted 8 lines

drivers/s390/crypto/ap_bus.c +6 -0

+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}  

Quoted 10 lines · On Wed, 15 Apr 2020 13:10:10 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1224

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

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

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))  

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

Quoted 8 lines

One of the things on my TODO list is to add logging to the vfio_ap module which will track all significant activity within the device driver. I plan to do that with a patch or set of patches specifically put together for that purpose. Having said that, the best place to log this would be in the in_use callback in the vfio_ap device driver (see next patch) where the APQNs that are in use can be identified. For now, I will log a message to the dmesg log indicating which APQNs are in use by the matrix mdev.

Sounds reasonable. My main issue was what an admin was supposed to do until logging was in place :)

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

drivers/s390/crypto/ap_bus.c +6 -0

+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}  

Quoted 19 lines · On Thu, 16 Apr 2020 11:37:21 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 16 lines · On Wed, 15 Apr 2020 13:10:10 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 7 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1224

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

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

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

One of the things on my TODO list is to add logging to the vfio_ap module which will track all significant activity within the device driver. I plan to do that with a patch or set of patches specifically put together for that purpose. Having said that, the best place to log this would be in the in_use callback in the vfio_ap device driver (see next patch) where the APQNs that are in use can be identified. For now, I will log a message to the dmesg log indicating which APQNs are in use by the matrix mdev.

Quoted 2 lines

Sounds reasonable. My main issue was what an admin was supposed to do until logging was in place :)

Logging may not be the right answer here. Imagine somebody wants to build a nice web-tool for managing this stuff at scale -- e.g. something HMC. I don't think the solution is to let this tool parse the kernel messages and try to relate that to its own transactions.

But I do agree, having a way to report why "won't do" to the end user is important for usability.

Regards,
Halil

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/s390/crypto/ap_bus.c +6 -0

+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}

Quoted 22 lines · Halil Pasic

Quoted 19 lines · On Thu, 16 Apr 2020 11:37:21 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 16 lines · On Wed, 15 Apr 2020 13:10:10 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 7 lines · Cornelia Huck

drivers/s390/crypto/ap_bus.c static ssize_t apmask_show(struct bus_type *bus, char *buf) · line 1224

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

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

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

One of the things on my TODO list is to add logging to the vfio_ap module which will track all significant activity within the device driver. I plan to do that with a patch or set of patches specifically put together for that purpose. Having said that, the best place to log this would be in the in_use callback in the vfio_ap device driver (see next patch) where the APQNs that are in use can be identified. For now, I will log a message to the dmesg log indicating which APQNs are in use by the matrix mdev.

Quoted 6 lines

Quoted 2 lines

Sounds reasonable. My main issue was what an admin was supposed to do until logging was in place :)

Logging may not be the right answer here. Imagine somebody wants to build a nice web-tool for managing this stuff at scale -- e.g. something HMC. I don't think the solution is to let this tool parse the kernel messages and try to relate that to its own transactions.

I don't believe there is no right or wrong answer here; I simply don't see the relevance of discussing a tool in this context. We are talking about a sysfs attribute interface here, so - correct me if I'm mistaken - our options for notifying the user that a queue is in use are limited to the return code from the sysfs interface and logging. I would expect that a tool would have to do something similar to the callback implemented in the vfio_ap device driver and check the APQNs removed against the APQNs assigned to the mdevs to determine which is in use.

Quoted 30 lines · On Fri, 24 Apr 2020 13:07:38 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 22 lines · Halil Pasic

Quoted 19 lines · On Thu, 16 Apr 2020 11:37:21 +0200 Cornelia Huck <[email protected]> wrote:

Quoted 16 lines · On Wed, 15 Apr 2020 13:10:10 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 7 lines · Cornelia Huck

Review

Quoted 3 lines · On Tue, 7 Apr 2020 15:20:03 -0400 Tony Krowiak <[email protected]> wrote:

(quoted diff) +3 -0

+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))

Can we log the offending apm somewhere, preferably with additional info that allows the admin to figure out why an error was returned?

One of the things on my TODO list is to add logging to the vfio_ap module which will track all significant activity within the device driver. I plan to do that with a patch or set of patches specifically put together for that purpose. Having said that, the best place to log this would be in the in_use callback in the vfio_ap device driver (see next patch) where the APQNs that are in use can be identified. For now, I will log a message to the dmesg log indicating which APQNs are in use by the matrix mdev.

Quoted 6 lines

Quoted 2 lines

Sounds reasonable. My main issue was what an admin was supposed to do until logging was in place :)

Logging may not be the right answer here. Imagine somebody wants to build a nice web-tool for managing this stuff at scale -- e.g. something HMC. I don't think the solution is to let this tool parse the kernel messages and try to relate that to its own transactions.

Quoted 10 lines

I don't believe there is no right or wrong answer here; I simply don't see the relevance of discussing a tool in this context. We are talking about a sysfs attribute interface here, so - correct me if I'm mistaken - our options for notifying the user that a queue is in use are limited to the return code from the sysfs interface and logging. I would expect that a tool would have to do something similar to the callback implemented in the vfio_ap device driver and check the APQNs removed against the APQNs assigned to the mdevs to determine which is in use.

We are talking interface design. The relevance of discussing a tool is that any userspace tool must come by with whatever interface we come up now. IMHO thinking about the usage (and the client code) is very helpful in avoiding broken interface designs. AFAIK this is one of the basic ideas behind test driven development.

Regards,
Halil

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it.

The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver.

The
vfio_ap device driver manages AP queues passed through to one or more guests

and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Quoted 5 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it.

How can we know, at this point if the guest uses or not the queue?

Do you want to say that this prevents to take away a queue when it is currently assigned to a VFIO device? and with a guest currently using this VFIO device?

Quoted 7 lines

The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

AFAIU you mean that Linux's driver's binding and unbinding mechanism is not sufficient to avoid this issue because unbind can not be refused by the driver.

The reason why we do not want a single queue to be removed from the VFIO driver is because the VFIO drivers works on a matrix, not on queues, and for the matrix to be consistent it needs to acquire all queues defined by the cross product of all APID and AQID assigned to the matrix.

This functionality is valid for the host as for the guests and is handled automatically by the firmware with the CRYCB. The AP bus uses QCI to retrieve the host CRYCB and build the hosts AP queues.

If instead to mix VFIO CRYCB matrix handling and queues at the same level inside the AP bus we separate these different firmware entities in two different software entities.

If we make the AP bus sit above a CRYCB/Matrix bus, and in the way virtualize the QCI and test AP queue instructions:
- we can directly pass a matrix device to the guest though a VFIO
matrix device
- the consistence will be automatic
- the VFIO device and parent device will be of the same kind which
would make the design much more clearer.
- there will be no need for these callback because the consistence of
the matrix will be guaranteed by firmware

Quoted 5 lines

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver.

You mean that the admin may take queues away from the "default driver", while the queue is in use, to give it to an other driver? Why is it to avoid in one way and not in the other way?

Quoted 4 lines

The
vfio_ap device driver manages AP queues passed through to one or more guests

I read this as if a queue may be passed to several guest... please, rephrase or explain.

Quoted 3 lines

and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

When you say "independently administered", you mean as a second admin inside the host, don't you?

Regards,
Pierre

Quoted 8 lines · Pierre Morel

Quoted 5 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it.

Quoted 6 lines

This functionality is valid for the host as for the guests and is handled automatically by the firmware with the CRYCB. The AP bus uses QCI to retrieve the host CRYCB and build the hosts AP queues.

If instead to mix VFIO CRYCB matrix handling and queues at the same level inside the AP bus we separate these different firmware entities in two different software entities.

Quoted 8 lines

If we make the AP bus sit above a CRYCB/Matrix bus, and in the way virtualize the QCI and test AP queue instructions:
- we can directly pass a matrix device to the guest though a VFIO
matrix device
- the consistence will be automatic
- the VFIO device and parent device will be of the same kind which
would make the design much more clearer.
- there will be no need for these callback because the consistence of
the matrix will be guaranteed by firmware

As stated in my response above, the issue here is not consistency. While the design you describe may be reasonable, it is a major departure from what is out in the field. In other words, that ship has sailed.

Quoted 10 lines · Pierre Morel

Quoted 5 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it.

How can we know, at this point if the guest uses or not the queue?

The struct ap_matrix_mdev has a field, struct kvm *kvm, which holds a pointer to KVM when the matrix mdev is in use by a guest. This patch series also introduces a shadow_crycb (soon to be shadow_apcb) which holds the AP configuration for the guest. Between those two things, the driver can detect when a queue is in use by a guest.

Quoted 2 lines

Do you want to say that this prevents to take away a queue when it is currently assigned to a VFIO device? and with a guest currently using this VFIO device?

No, I do not. The intent here is to enforce the proper procedure for giving up a queue so it is done deliberately. Before taking a queue away from the matrix mdev, its APQN should be unassigned from the matrix mdev. That is not to say that if there are major objections to this that we can't base in_use upon the queue being in use by a guest at the time. Maybe that is preferable to the community. I'll leave it to them to state their case.

Quoted 9 lines

Quoted 7 lines

The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

AFAIU you mean that Linux's driver's binding and unbinding mechanism is not sufficient to avoid this issue because unbind can not be refused by the driver.

Correct!

Quoted 3 lines

The reason why we do not want a single queue to be removed from the VFIO driver is because the VFIO drivers works on a matrix, not on queues, and for the matrix to be consistent it needs to acquire all queues defined by the cross product of all APID and AQID assigned to the matrix.

Not correct. The reason why is because we do not want a queue to be surreptitiously removed without the guest administrator being aware of its removal.

Quoted 11 lines

This functionality is valid for the host as for the guests and is handled automatically by the firmware with the CRYCB. The AP bus uses QCI to retrieve the host CRYCB and build the hosts AP queues.

If instead to mix VFIO CRYCB matrix handling and queues at the same level inside the AP bus we separate these different firmware entities in two different software entities.

If we make the AP bus sit above a CRYCB/Matrix bus, and in the way virtualize the QCI and test AP queue instructions:
- we can directly pass a matrix device to the guest though a VFIO matrix device
- the consistence will be automatic
- the VFIO device and parent device will be of the same kind which would make the design much more clearer.
- there will be no need for these callback because the consistence of the matrix will be guaranteed by firmware

As stated in my response above, the issue here is not consistency. While the design you describe may be reasonable, it is a major departure from what is out in the field. In other words, that ship has sailed.

Quoted 6 lines

Quoted 5 lines

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver.

Quoted 2 lines

You mean that the admin may take queues away from the "default driver", while the queue is in use, to give it to an other driver? Why is it to avoid in one way and not in the other way?

Because the default drivers have direct control over the queues and can ensure they are empty and reset before giving up control. The vfio driver does not have direct control over the queues because they have been passed through to the guest.

Quoted 8 lines

Quoted 5 lines

The
vfio_ap device driver manages AP queues passed through to one or more guests

I read this as if a queue may be passed to several guest... please, rephrase or explain.

AP queues is plural, so it is true that AP queues can be passed through to more than one guest. I see your point, however, so I'll reword that to be more clear.

Quoted 5 lines

Quoted 3 lines

and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

When you say "independently administered", you mean as a second admin inside the host, don't you?

I mean that a guest can be administered by a different person than the host administrator. Again, I'll try to clarify this.

Quoted 5 lines

Regards,
Pierre

Quoted 52 lines · Tony Krowiak

Quoted 8 lines · Pierre Morel

Quoted 5 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. The intent of this callback is to provide a driver with the means to prevent a root user from inadvertently taking a queue away from a guest and giving it to the host while the guest is still using it.

How can we know, at this point if the guest uses or not the queue?

The struct ap_matrix_mdev has a field, struct kvm *kvm, which holds a pointer to KVM when the matrix mdev is in use by a guest. This patch series also introduces a shadow_crycb (soon to be shadow_apcb) which holds the AP configuration for the guest. Between those two things, the driver can detect when a queue is in use by a guest.

Quoted 2 lines

Do you want to say that this prevents to take away a queue when it is currently assigned to a VFIO device? and with a guest currently using this VFIO device?

No, I do not. The intent here is to enforce the proper procedure for giving up a queue so it is done deliberately. Before taking a queue away from the matrix mdev, its APQN should be unassigned from the matrix mdev. That is not to say that if there are major objections to this that we can't base in_use upon the queue being in use by a guest at the time. Maybe that is preferable to the community. I'll leave it to them to state their case.

Quoted 7 lines

Quoted 6 lines

The callback will
be invoked whenever a change to the AP bus's sysfs apmask or aqmask attributes would result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

AFAIU you mean that Linux's driver's binding and unbinding mechanism is not sufficient to avoid this issue because unbind can not be refused by the driver.

Correct!

Quoted 2 lines

The reason why we do not want a single queue to be removed from the VFIO driver is because the VFIO drivers works on a matrix, not on queues, and for the matrix to be consistent it needs to acquire all queues defined by the cross product of all APID and AQID assigned to the matrix.

Not correct. The reason why is because we do not want a queue to be surreptitiously removed without the guest administrator being aware of its removal.

Quoted 10 lines

This functionality is valid for the host as for the guests and is handled automatically by the firmware with the CRYCB. The AP bus uses QCI to retrieve the host CRYCB and build the hosts AP queues.

If instead to mix VFIO CRYCB matrix handling and queues at the same level inside the AP bus we separate these different firmware entities in two different software entities.

If we make the AP bus sit above a CRYCB/Matrix bus, and in the way virtualize the QCI and test AP queue instructions:
- we can directly pass a matrix device to the guest though a VFIO matrix device
- the consistence will be automatic
- the VFIO device and parent device will be of the same kind which would make the design much more clearer.
- there will be no need for these callback because the consistence of the matrix will be guaranteed by firmware

As stated in my response above, the issue here is not consistency. While the design you describe may be reasonable, it is a major departure from what is out in the field. In other words, that ship has sailed.

Quoted 1 line

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver.

You mean that the admin may take queues away from the "default driver", while the queue is in use, to give it to an other driver? Why is it to avoid in one way and not in the other way?

Because the default drivers have direct control over the queues and can ensure they are empty and reset before giving up control. The vfio driver does not have direct control over the queues because they have been passed through to the guest.

No, that's not true. The 'default' drivers have no change to do anything with an APQN when it is removed from the driver. They get the very same notification which is the remove() callback as the vfio dd gets and have the very same change to do something here. The more interesting thing here is, that the remove() callback invocation is usually because a hardware HAS BEEN GONE AWAY. Neither the 'default' drivers nor the vfio dd can do a reset on a not-any-more existing APQN.
And it is also not true that the vfio dd has no direct control over the queue because they have been passed through to the guest. It's the job of the vfio dd to modify the guest's APM, AQM, ADM masks to disable the guest's access to the APQN and then the vfio can (try to) do a reset.

Quoted 19 lines

Quoted 5 lines

Quoted 3 lines

The
vfio_ap device driver manages AP queues passed through to one or more guests

I read this as if a queue may be passed to several guest... please, rephrase or explain.

AP queues is plural, so it is true that AP queues can be passed through to more than one guest. I see your point, however, so I'll reword that to be more clear.

Quoted 3 lines

Quoted 2 lines

and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

When you say "independently administered", you mean as a second admin inside the host, don't you?

I mean that a guest can be administered by a different person than the host administrator. Again, I'll try to clarify this.

Quoted 4 lines

Regards,
Pierre

Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is invoked whenever a change to the apmask or aqmask would result in one or more queue devices being removed from the driver. The vfio_ap device driver will indicate a resource is in use
if the APQN of any of the queue devices to be removed are assigned to any of the matrix mdevs under the driver's control.

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

---
 drivers/s390/crypto/vfio_ap_drv.c     |  1 +
 drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
 drivers/s390/crypto/vfio_ap_private.h |  2 ++
 3 files changed, 33 insertions(+), 17 deletions(-)

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

+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}

Quoted 15 lines · On Tue, 7 Apr 2020 15:20:04 -0400 Tony Krowiak <[email protected]> wrote:

Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is invoked whenever a change to the apmask or aqmask would result in one or more queue devices being removed from the driver. The vfio_ap device driver will indicate a resource is in use
if the APQN of any of the queue devices to be removed are assigned to any of the matrix mdevs under the driver's control.

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

---
  drivers/s390/crypto/vfio_ap_drv.c     |  1 +
  drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
  drivers/s390/crypto/vfio_ap_private.h |  2 ++
  3 files changed, 33 insertions(+), 17 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_queue *queue) · line 1380

Quoted 10 lines

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

@@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
  	kfree(q);
  	mutex_unlock(&matrix_dev->lock);
  }
+
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+	bool in_use;
+
+	mutex_lock(&matrix_dev->lock);
+	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;

Maybe

in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);

?

Quoted 5 lines

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

+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}

Quoted 33 lines · Cornelia Huck

Quoted 27 lines · On Tue,  7 Apr 2020 15:20:04 -0400 Tony Krowiak <[email protected]> wrote:

Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is invoked whenever a change to the apmask or aqmask would result in one or more queue devices being removed from the driver. The vfio_ap device driver will indicate a resource is in use
if the APQN of any of the queue devices to be removed are assigned to any of the matrix mdevs under the driver's control.

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

---
  drivers/s390/crypto/vfio_ap_drv.c     |  1 + drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++---------- drivers/s390/crypto/vfio_ap_private.h |  2 ++

  3 files changed, 33 insertions(+), 17 deletions(-)

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

@@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue 
 *queue)
       kfree(q);
       mutex_unlock(&matrix_dev->lock);

  }

(quoted diff) +2 -0

+
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long 

*aqm)

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

+{
+    bool in_use;
+
+    mutex_lock(&matrix_dev->lock);
+    in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : 

Quoted 1 line

false;

Maybe

Quoted 3 lines

in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);

?

To be honest, I find the !! expression very confusing. Every time I see it, I have to spend time thinking about what the result of !! is going to be. I think the statement should be left as-is because it more clearly expresses the intent.

Quoted 6 lines

Quoted 6 lines

+    mutex_unlock(&matrix_dev->lock);

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

+
+    return in_use;
+}

Quoted 32 lines · Cornelia Huck

Quoted 15 lines · On Tue, 7 Apr 2020 15:20:04 -0400 Tony Krowiak <[email protected]> wrote:

Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is invoked whenever a change to the apmask or aqmask would result in one or more queue devices being removed from the driver. The vfio_ap device driver will indicate a resource is in use
if the APQN of any of the queue devices to be removed are assigned to any of the matrix mdevs under the driver's control.

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

---
  drivers/s390/crypto/vfio_ap_drv.c     |  1 +
  drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
  drivers/s390/crypto/vfio_ap_private.h |  2 ++
  3 files changed, 33 insertions(+), 17 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_queue *queue) · line 1380

Quoted 10 lines

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

@@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
  	kfree(q);
  	mutex_unlock(&matrix_dev->lock);
  }
+
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+	bool in_use;
+
+	mutex_lock(&matrix_dev->lock);
+	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;

Maybe

in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);

Quoted 1 line

?

To be honest, I find the !! expression very confusing. Every time I see it, I have
to spend time thinking about what the result of !! is going to be. I think the statement should be left as-is because it more clearly expresses the intent.

Quoted 5 lines

Quoted 5 lines

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

+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}

Quoted 32 lines · On Thu, 16 Apr 2020 10:45:20 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 30 lines · Cornelia Huck

Quoted 15 lines · On Tue, 7 Apr 2020 15:20:04 -0400 Tony Krowiak <[email protected]> wrote:

Let's implement the callback to indicate when an APQN
is in use by the vfio_ap device driver. The callback is invoked whenever a change to the apmask or aqmask would result in one or more queue devices being removed from the driver. The vfio_ap device driver will indicate a resource is in use
if the APQN of any of the queue devices to be removed are assigned to any of the matrix mdevs under the driver's control.

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

---
   drivers/s390/crypto/vfio_ap_drv.c     |  1 +
   drivers/s390/crypto/vfio_ap_ops.c     | 47 +++++++++++++++++----------
   drivers/s390/crypto/vfio_ap_private.h |  2 ++
   3 files changed, 33 insertions(+), 17 deletions(-)

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_queue *queue) · line 1380

Quoted 10 lines

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

@@ -1369,3 +1371,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
   	kfree(q);
   	mutex_unlock(&matrix_dev->lock);
   }
+
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+	bool in_use;
+
+	mutex_lock(&matrix_dev->lock);
+	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;

Maybe

Quoted 9 lines

Quoted 3 lines

in_use = !!vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm);

?

To be honest, I find the !! expression very confusing. Every time I see it, I have
to spend time thinking about what the result of !! is going to be. I think the statement should be left as-is because it more clearly expresses the intent.

This is discussion is just about cosmetics, I believe. Just a piece of
advice: try to be sensitive about the community. In this community, and I believe in C general !! is the idiomatic way to convert number to boolean. Why would one want to do that is a bit longer story. The short version is in logic condition context the value 0 is false and any other value is true. !! keeps false value (0) false, and forces a true to the most true true value. If you keep getting confused every time you run across a !! that won't help with reading other peoples C.

Regards,
Halil

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

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

+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}