s390/vfio-ap: dynamic configuration support

Let's move the probe and remove callbacks into the vfio_ap_ops.c file to keep all code related to managing queues in a single file. This way, all functions related to queue management can be removed from the vfio_ap_private.h header file defining the public interfaces for the vfio_ap device driver.

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

Decrement the reference count to KVM when notified that KVM pointer is invalidated via the vfio group notifier.

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

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

---
 drivers/s390/crypto/vfio_ap_ops.c | 4 ++++
 1 file changed, 4 insertions(+)

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

index 66fd9784a156..31e39c1f6e56 100644
@@ -1095,7 +1095,11 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
 
 	if (!data) {
+		if (matrix_mdev->kvm)
+			kvm_put_kvm(matrix_mdev->kvm);
+
 		matrix_mdev->kvm = NULL;
+
 		return NOTIFY_OK;
 	}
 

On Tue, 24 Nov 2020 16:40:02 -0500
Tony Krowiak <[email protected]> wrote:

A nit: for all other patches the title prefix is s390/vfio-ap, here you have 390/vfio-ap.

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

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

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

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

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

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

index 8e6972495daa..dc699fd54505 100644
@@ -26,14 +26,6 @@
 
 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.
  * @matrix_mdev: the associated mediated matrix

@@ -1121,20 +1113,6 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-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);
-		vfio_ap_irq_disable(q);
-		put_device(dev);
-	}
-}
-
 static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
 				    unsigned int retry)
 {

@@ -1169,6 +1147,7 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 {
 	int ret;
 	int rc = 0;
+	struct vfio_ap_queue *q;
 	unsigned long apid, apqi;
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 

@@ -1184,7 +1163,10 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 			 */
 			if (ret)
 				rc = ret;
-			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
+
+			q = vfio_ap_get_queue(matrix_mdev, AP_MKQID(apid, apqi);
+			if (q)
+				vfio_ap_free_aqic_resources(q);
 		}
 	}
 

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

@@ -1155,6 +1243,11 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 			     matrix_mdev->matrix.apm_max + 1) {
 		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
 				     matrix_mdev->matrix.aqm_max + 1) {
+			q = vfio_ap_mdev_get_queue(matrix_mdev,
+						   AP_MKQID(apid, apqi));
+			if (!q)
+				continue;
+
 			ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
 			/*
 			 * Regardless whether a queue turns out to be busy, or

Quoted 16 lines · On Tue, 24 Nov 2020 16:40:04 -0500 Tony Krowiak <[email protected]> wrote:

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

@@ -1155,6 +1243,11 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			     matrix_mdev->matrix.apm_max + 1) {
  		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
  				     matrix_mdev->matrix.aqm_max + 1) {
+			q = vfio_ap_mdev_get_queue(matrix_mdev,
+						   AP_MKQID(apid, apqi));
+			if (!q)
+				continue;
+
  			ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
  			/*
  			 * Regardless whether a queue turns out to be busy, or

@@ -1164,9 +1257,7 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			if (ret)
  				rc = ret;
 

drivers/s390/crypto/vfio_ap_ops.c static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev) · line 1262

Quoted 6 lines

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

-			q = vfio_ap_get_queue(matrix_mdev, AP_MKQID(apid, apqi);
-			if (q)
-				vfio_ap_free_aqic_resources(q);
+			vfio_ap_free_aqic_resources(q);
  		}
  	}

During the review of v11 we discussed this. Introducing this the one way around, just to change it in the next patch, which should deal with something different makes no sense to me.

BTW I've provided a ton of feedback for '[PATCH v11 03/14] s390/vfio-ap: manage link between queue struct and matrix mdev', but I can't find your response to that. Some of the things resurface here, and I don't feel like repeating myself. Can you provide me an answer to the v11 version?

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

The links will be created as follows:

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

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

The links will be removed as follows:

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

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

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

---
 drivers/s390/crypto/vfio_ap_ops.c     | 161 +++++++++++++++++++++++---
 drivers/s390/crypto/vfio_ap_private.h |   3 +
 2 files changed, 146 insertions(+), 18 deletions(-)

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

index dc699fd54505..07caf871943c 100644
@@ -28,7 +28,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 
 /**
  * vfio_ap_get_queue: Retrieve a queue with a specific APQN.
- * @matrix_mdev: the associated mediated matrix
  * @apqn: The queue APQN
  *
  * Retrieve a queue with a specific APQN from the AP queue devices attached to

@@ -36,32 +35,36 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
  *
  * Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL.
  */
-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,
-					int apqn)
+static struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
 {
 	struct ap_queue *queue;
 	struct vfio_ap_queue *q = NULL;
 
-	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
-		return NULL;
-	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
-		return NULL;
-
 	queue = ap_get_qdev(apqn);
 	if (!queue)
 		return NULL;
 
 	put_device(&queue->ap_dev.device);
 
-	if (queue->ap_dev.device.driver == &matrix_dev->vfio_ap_drv->driver) {
+	if (queue->ap_dev.device.driver == &matrix_dev->vfio_ap_drv->driver)
 		q = dev_get_drvdata(&queue->ap_dev.device);
-		q->matrix_mdev = matrix_mdev;
-	}
 
 	return q;
 }
 
+static struct vfio_ap_queue *
+vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
+{
+	struct vfio_ap_queue *q;
+
+	hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
+		if (q && (q->apqn == apqn))
+			return q;
+	}
+
+	return NULL;
+}
+
 /**
  * vfio_ap_wait_for_irqclear
  * @apqn: The AP Queue number

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

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

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

@@ -559,6 +562,87 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 	return 0;
 }
 
+enum qlink_action {
+	LINK_APID,
+	LINK_APQI,
+	UNLINK_APID,
+	UNLINK_APQI,
+};
+
+static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
+				    unsigned long apid, unsigned long apqi)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+	if (q) {
+		q->matrix_mdev = matrix_mdev;
+		hash_add(matrix_mdev->qtable,
+			 &q->mdev_qnode, q->apqn);
+	}
+}
+

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

+
+/**
+ * vfio_ap_mdev_manage_qlinks
+ *
+ * @matrix_mdev: The matrix mdev to link.
+ * @action:	 The action to take on @qlink_id.
+ * @qlink_id:	 The APID or APQI of the queues to link.
+ *
+ * Sets or clears the links between the queues with the specified @qlink_id
+ * and the @matrix_mdev:
+ *	@action == LINK_APID:	Set the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APID)
+ *	@action == LINK_APQI:	Set the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APQI)
+ *	@action == UNLINK_APID:	Clear the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APID)
+ *	@action == UNLINK_APQI:	Clear the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APQI)
+ */
+static void vfio_ap_mdev_manage_qlinks(struct ap_matrix_mdev *matrix_mdev,
+				       enum qlink_action action,
+				       unsigned long qlink_id)
+{
+	unsigned long id;
+
+	switch (action) {
+	case LINK_APID:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1)
+			vfio_ap_mdev_link_queue(matrix_mdev, qlink_id, id);
+		break;
+	case UNLINK_APID:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1)
+			vfio_ap_mdev_unlink_queue(qlink_id, id);
+		break;
+	case LINK_APQI:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.apm,
+				     matrix_mdev->matrix.apm_max + 1)
+			vfio_ap_mdev_link_queue(matrix_mdev, id, qlink_id);
+		break;
+	case UNLINK_APQI:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.apm,
+				     matrix_mdev->matrix.apm_max + 1)
+			vfio_ap_mdev_link_queue(matrix_mdev, id, qlink_id);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+	}
+}
+
 /**
  * assign_adapter_store
  *

@@ -628,6 +712,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 	if (ret)
 		goto share_err;
 
+	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APID, apid);
 	ret = count;
 	goto done;
 

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

@@ -769,6 +855,7 @@ static ssize_t assign_domain_store(struct device *dev,
 	if (ret)
 		goto share_err;
 
+	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APQI, apqi);
 	ret = count;
 	goto done;
 

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

@@ -1155,6 +1243,11 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 			     matrix_mdev->matrix.apm_max + 1) {
 		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
 				     matrix_mdev->matrix.aqm_max + 1) {
+			q = vfio_ap_mdev_get_queue(matrix_mdev,
+						   AP_MKQID(apid, apqi));
+			if (!q)
+				continue;
+
 			ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
 			/*
 			 * Regardless whether a queue turns out to be busy, or

@@ -1164,9 +1257,7 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 			if (ret)
 				rc = ret;
 
-			q = vfio_ap_get_queue(matrix_mdev, AP_MKQID(apid, apqi);
-			if (q)
-				vfio_ap_free_aqic_resources(q);
+			vfio_ap_free_aqic_resources(q);
 		}
 	}
 

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

@@ -1305,9 +1419,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
 	q = kzalloc(sizeof(*q), GFP_KERNEL);
 	if (!q)
 		return -ENOMEM;
+	mutex_lock(&matrix_dev->lock);
 	dev_set_drvdata(&apdev->device, q);
 	q->apqn = to_ap_queue(&apdev->device)->qid;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_queue_link_mdev(q);
+	mutex_unlock(&matrix_dev->lock);
+
 	return 0;
 }
 

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

+	}
 	kfree(q);
 	mutex_unlock(&matrix_dev->lock);
 }

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

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

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

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

Quoted 23 lines · On Tue, 24 Nov 2020 16:40:04 -0500 Tony Krowiak <[email protected]> wrote:

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

The links will be created as follows:

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

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

The links will be removed as follows:

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

Quoted 6 lines

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

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

Actually some aspects of this look much better than last time, but I'm afraid there one new issue that must be corrected -- see below.

Quoted 114 lines

---
  drivers/s390/crypto/vfio_ap_ops.c     | 161 +++++++++++++++++++++++---
  drivers/s390/crypto/vfio_ap_private.h |   3 +
  2 files changed, 146 insertions(+), 18 deletions(-)

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

index dc699fd54505..07caf871943c 100644
@@ -28,7 +28,6 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 
  /**
   * vfio_ap_get_queue: Retrieve a queue with a specific APQN.
- * @matrix_mdev: the associated mediated matrix
   * @apqn: The queue APQN
   *
   * Retrieve a queue with a specific APQN from the AP queue devices attached to

@@ -36,32 +35,36 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
   *
   * Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL.
   */
-static struct vfio_ap_queue *vfio_ap_get_queue(
-					struct ap_matrix_mdev *matrix_mdev,
-					int apqn)
+static struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
  {
  	struct ap_queue *queue;
  	struct vfio_ap_queue *q = NULL;
 
-	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
-		return NULL;
-	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
-		return NULL;
-
  	queue = ap_get_qdev(apqn);
  	if (!queue)
  		return NULL;
 
  	put_device(&queue->ap_dev.device);
 
-	if (queue->ap_dev.device.driver == &matrix_dev->vfio_ap_drv->driver) {
+	if (queue->ap_dev.device.driver == &matrix_dev->vfio_ap_drv->driver)
  		q = dev_get_drvdata(&queue->ap_dev.device);
-		q->matrix_mdev = matrix_mdev;
-	}
 
  	return q;
  }
 
+static struct vfio_ap_queue *
+vfio_ap_mdev_get_queue(struct ap_matrix_mdev *matrix_mdev, unsigned long apqn)
+{
+	struct vfio_ap_queue *q;
+
+	hash_for_each_possible(matrix_mdev->qtable, q, mdev_qnode, apqn) {
+		if (q && (q->apqn == apqn))
+			return q;
+	}
+
+	return NULL;
+}
+
  /**
   * vfio_ap_wait_for_irqclear
   * @apqn: The AP Queue number

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

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

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

@@ -559,6 +562,87 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
  	return 0;
  }
 
+enum qlink_action {
+	LINK_APID,
+	LINK_APQI,
+	UNLINK_APID,
+	UNLINK_APQI,
+};
+
+static void vfio_ap_mdev_link_queue(struct ap_matrix_mdev *matrix_mdev,
+				    unsigned long apid, unsigned long apqi)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+	if (q) {
+		q->matrix_mdev = matrix_mdev;
+		hash_add(matrix_mdev->qtable,
+			 &q->mdev_qnode, q->apqn);
+	}
+}
+

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

Quoted 10 lines

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

+static void vfio_ap_mdev_unlink_queue(unsigned long apid, unsigned long apqi)
+{
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+	if (q) {
+		q->matrix_mdev = NULL;
+		hash_del(&q->mdev_qnode);
+	}
+}

I would do

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

+static void vfio_ap_mdev_unlink_queue(struct vfio_ap_queue *q)
+{
+	if (!q)
+		return;

Quoted 154 lines

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

+
+/**
+ * vfio_ap_mdev_manage_qlinks
+ *
+ * @matrix_mdev: The matrix mdev to link.
+ * @action:	 The action to take on @qlink_id.
+ * @qlink_id:	 The APID or APQI of the queues to link.
+ *
+ * Sets or clears the links between the queues with the specified @qlink_id
+ * and the @matrix_mdev:
+ *	@action == LINK_APID:	Set the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APID)
+ *	@action == LINK_APQI:	Set the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APQI)
+ *	@action == UNLINK_APID:	Clear the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APID)
+ *	@action == UNLINK_APQI:	Clear the links between the @matrix_mdev and the
+ *				queues with the specified @qlink_id (APQI)
+ */
+static void vfio_ap_mdev_manage_qlinks(struct ap_matrix_mdev *matrix_mdev,
+				       enum qlink_action action,
+				       unsigned long qlink_id)
+{
+	unsigned long id;
+
+	switch (action) {
+	case LINK_APID:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1)
+			vfio_ap_mdev_link_queue(matrix_mdev, qlink_id, id);
+		break;
+	case UNLINK_APID:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1)
+			vfio_ap_mdev_unlink_queue(qlink_id, id);
+		break;
+	case LINK_APQI:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.apm,
+				     matrix_mdev->matrix.apm_max + 1)
+			vfio_ap_mdev_link_queue(matrix_mdev, id, qlink_id);
+		break;
+	case UNLINK_APQI:
+		for_each_set_bit_inv(id, matrix_mdev->matrix.apm,
+				     matrix_mdev->matrix.apm_max + 1)
+			vfio_ap_mdev_link_queue(matrix_mdev, id, qlink_id);
+		break;
+	default:
+		WARN_ON_ONCE(1);
+	}
+}
+
  /**
   * assign_adapter_store
   *

@@ -628,6 +712,7 @@ static ssize_t assign_adapter_store(struct device *dev,
  	if (ret)
  		goto share_err;
 
+	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APID, apid);
  	ret = count;
  	goto done;
 

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

@@ -769,6 +855,7 @@ static ssize_t assign_domain_store(struct device *dev,
  	if (ret)
  		goto share_err;
 
+	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APQI, apqi);
  	ret = count;
  	goto done;
 

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

@@ -1155,6 +1243,11 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			     matrix_mdev->matrix.apm_max + 1) {
  		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
  				     matrix_mdev->matrix.aqm_max + 1) {
+			q = vfio_ap_mdev_get_queue(matrix_mdev,
+						   AP_MKQID(apid, apqi));
+			if (!q)
+				continue;
+
  			ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
  			/*
  			 * Regardless whether a queue turns out to be busy, or

@@ -1164,9 +1257,7 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			if (ret)
  				rc = ret;
 
-			q = vfio_ap_get_queue(matrix_mdev, AP_MKQID(apid, apqi);
-			if (q)
-				vfio_ap_free_aqic_resources(q);
+			vfio_ap_free_aqic_resources(q);
  		}
  	}
 

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

@@ -1305,9 +1419,13 @@ int vfio_ap_mdev_probe_queue(struct ap_device *apdev)
  	q = kzalloc(sizeof(*q), GFP_KERNEL);
  	if (!q)
  		return -ENOMEM;
+	mutex_lock(&matrix_dev->lock);
  	dev_set_drvdata(&apdev->device, q);
  	q->apqn = to_ap_queue(&apdev->device)->qid;
  	q->saved_isc = VFIO_AP_ISC_INVALID;
+	vfio_ap_queue_link_mdev(q);
+	mutex_unlock(&matrix_dev->lock);
+
  	return 0;
  }
 

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_device *apdev) · line 1448

Quoted 4 lines

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

@@ -1328,7 +1446,14 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
  	apid = AP_QID_CARD(q->apqn);
  	apqi = AP_QID_QUEUE(q->apqn);
  	vfio_ap_mdev_reset_queue(apid, apqi, 1);

Does it make sense to reset if !q->matrix_dev?

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_device *apdev) · line 1451

Quoted 5 lines

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

-	vfio_ap_irq_disable(q);
+	if (q->matrix_mdev) {
+		if (q->matrix_mdev->kvm) {
+			vfio_ap_free_aqic_resources(q);

Again this belongs to the previous patch.

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_device *apdev) · line 1452

Quoted 2 lines

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

+			kvm_put_kvm(q->matrix_mdev->kvm);

This kvm_put_kvm() makes no sense to me! Please explain. Where is the corresponding kvm_get_kvm()?

drivers/s390/crypto/vfio_ap_ops.c void vfio_ap_mdev_remove_queue(struct ap_device *apdev) · line 1455

Quoted 4 lines

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

+		}
+		hash_del(&q->mdev_qnode);
+		q->matrix_mdev = NULL;

This shouuld be an unlink_queue(q).

Quoted 33 lines

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

+	}
  	kfree(q);
  	mutex_unlock(&matrix_dev->lock);
  }

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

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

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

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

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.

There is potential for a deadlock condition between the matrix_dev->lock used to lock the matrix device during assignment of adapters and domains and the ap_perms_mutex locked by the AP bus when changes are made to the sysfs apmask/aqmask attributes.

Consider following scenario (courtesy of Halil Pasic):
1) apmask_store() takes ap_perms_mutex
2) assign_adapter_store() takes matrix_dev->lock
3) apmask_store() calls vfio_ap_mdev_resource_in_use() which tries to take matrix_dev->lock
4) assign_adapter_store() calls ap_apqn_in_matrix_owned_by_def_drv which tries to take ap_perms_mutex

BANG!

To resolve this issue, instead of using the mutex_lock(&matrix_dev->lock) function to lock the matrix device during assignment of an adapter or domain to a matrix_mdev as well as during the in_use callback, the mutex_trylock(&matrix_dev->lock) function will be used. If the lock is not obtained, then the assignment and in_use functions will terminate with -EBUSY.

Quoted 21 lines · On Tue, 24 Nov 2020 16:40:06 -0500 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.

There is potential for a deadlock condition between the matrix_dev->lock used to lock the matrix device during assignment of adapters and domains and the ap_perms_mutex locked by the AP bus when changes are made to the sysfs apmask/aqmask attributes.

Consider following scenario (courtesy of Halil Pasic):
1) apmask_store() takes ap_perms_mutex
2) assign_adapter_store() takes matrix_dev->lock
3) apmask_store() calls vfio_ap_mdev_resource_in_use() which tries

    to take matrix_dev->lock

4) assign_adapter_store() calls ap_apqn_in_matrix_owned_by_def_drv which tries to take ap_perms_mutex

Quoted 8 lines

BANG!

To resolve this issue, instead of using the mutex_lock(&matrix_dev->lock) function to lock the matrix device during assignment of an adapter or domain to a matrix_mdev as well as during the in_use callback, the mutex_trylock(&matrix_dev->lock) function will be used. If the lock is not obtained, then the assignment and in_use functions will terminate with -EBUSY.

Good news is: the final product is OK with regards to in_use(). Bad news
is: this patch does not do enough. At this stage we are still racy.

The problem is that the assign operations don't bother to take the ap_perms_mutex lock under the matrix_dev->lock.

The scenario is the following:
1) apmask_store() takes ap_perms_mutex
2) apmask_store() calls vfio_ap_mdev_resource_in_use() which

      takes matrix_dev->lock

3) vfio_ap_mdev_resource_in_use() releases matrix_dev->lock and returns 0
4) assign_adapter_store() takes matrix_dev->lock does the assign (the queues are still bound to vfio_ap) and releases matrix_dev->lock
5) apmask_store() carries on, does the update to apask and releases ap_perms_mutex
6) The queues get 'stolen' from vfio ap while used.

This gets fixed with "s390/vfio-ap: allow assignment of unavailable AP queues to mdev device". Maybe we can reorder these patches. I didn't look into that.

We could also just ignore the problem, because it is just for a couple of commits, but I would prefer it gone.

Regards,
Halil

The APCB is a field within the CRYCB that provides the AP configuration to a KVM guest. Let's introduce a shadow copy of the KVM guest's APCB and maintain it for the lifespan of the guest.

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

Reviewed-by Halil Pasic <[email protected]>

The matrix of adapters and domains configured in a guest's APCB may differ from the matrix of adapters and domains assigned to the matrix mdev, so this patch introduces a sysfs attribute to display the matrix of adapters and domains that are or will be assigned to the APCB of a guest that is or will be using the matrix mdev. For a matrix mdev denoted by $uuid, the guest matrix can be displayed as follows:

cat /sys/devices/vfio_ap/matrix/$uuid/guest_matrix

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

Quoted 10 lines · On Tue, 24 Nov 2020 16:40:08 -0500 Tony Krowiak <[email protected]> wrote:

The matrix of adapters and domains configured in a guest's APCB may differ from the matrix of adapters and domains assigned to the matrix mdev, so this patch introduces a sysfs attribute to display the matrix of adapters and domains that are or will be assigned to the APCB of a guest that is or will be using the matrix mdev. For a matrix mdev denoted by $uuid, the guest matrix can be displayed as follows:

cat /sys/devices/vfio_ap/matrix/$uuid/guest_matrix

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

Code looks good, but it may be a little early, since the treatment of guset_matrix is changed by the following patches.

The APCB is a control block containing the masks that specify the adapters, domains and control domains to which a KVM guest is granted access. When the vfio_ap device driver is notified that the KVM pointer has been set, the guest's APCB is initialized from the AP configuration of adapters, domains and control domains assigned to the matrix mdev. The linux device model, however, precludes passing through to a guest any devices that are not bound to the device driver facilitating the pass-through. Consequently, APQNs assigned to the matrix mdev that do not reference AP queue devices must be filtered before assigning them to the KVM guest's APCB; however, the AP architecture precludes filtering individual APQNs, so the APQNs will be filtered by APID. That is, if a given APQN does not reference a queue device bound to the vfio_ap driver, its APID will not get assigned to the guest's APCB. For example:

Queues bound to vfio_ap:
04.0004
04.0022
04.0035
05.0004
05.0022

Adapters/domains assigned to the matrix mdev:
04 0004
0022
0035
05 0004
0022
0035

APQNs assigned to APCB:
04.0004
04.0022
04.0035

The APID 05 was filtered from the matrix mdev's matrix because queue device 05.0035 is not bound to the vfio_ap device driver.

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

Quoted 37 lines · On Tue, 24 Nov 2020 16:40:09 -0500 Tony Krowiak <[email protected]> wrote:

The APCB is a control block containing the masks that specify the adapters, domains and control domains to which a KVM guest is granted access. When the vfio_ap device driver is notified that the KVM pointer has been set, the guest's APCB is initialized from the AP configuration of adapters, domains and control domains assigned to the matrix mdev. The linux device model, however, precludes passing through to a guest any devices that are not bound to the device driver facilitating the pass-through. Consequently, APQNs assigned to the matrix mdev that do not reference AP queue devices must be filtered before assigning them to the KVM guest's APCB; however, the AP architecture precludes filtering individual APQNs, so the APQNs will be filtered by APID. That is, if a given APQN does not reference a queue device bound to the vfio_ap driver, its APID will not get assigned to the guest's APCB. For example:

Queues bound to vfio_ap:
04.0004
04.0022
04.0035
05.0004
05.0022

Adapters/domains assigned to the matrix mdev:
04 0004

    0022
    0035
05 0004
    0022
    0035

APQNs assigned to APCB:
04.0004
04.0022
04.0035

The APID 05 was filtered from the matrix mdev's matrix because queue device 05.0035 is not bound to the vfio_ap device driver.

Quoted 1 line

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

This adds filtering. So from here guest_matrix may be different than matrix also for an mdev that is associated with a guest. I'm still grappling with the big picture. Have you thought about testability? How is a testcase supposed to figure out which behavior is to be deemed correct?

I don't like the title line. It implies that guest apcb was uninitialized before. Which is not the case.

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

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

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

Quoted 15 lines · On Tue, 24 Nov 2020 16:40:10 -0500 Tony Krowiak <[email protected]> wrote:

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

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

Quoted 1 line

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

Again code looks good. I'm still worried about all the incremental changes (good for review) and their testability.

Quoted 18 lines · Halil Pasic

Quoted 15 lines · On Tue, 24 Nov 2020 16:40:10 -0500 Tony Krowiak <[email protected]> wrote:

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

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

Quoted 3 lines

Quoted 1 line

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

Again code looks good. I'm still worried about all the incremental changes (good for review) and their testability.

I'm not sure what your concern is here. Is there an expectation that each patch needs to be testable by itself, or whether the functionality in each patch can be easily tested en masse?

I'm not sure some of these changes can be tested with an automated test because the test code would have to be able to dynamically change the host's AP configuration and I don't know if there is currently a way to do this programmatically. In order to test the effects of dynamic host crypto configuration manually, one needs access to an SE or HMC with DPM.

Let's hot plug/unplug adapters, domains and control domains assigned to or unassigned from an AP matrix mdev device while it is in use by a guest per the following rules:

* Assign an adapter to mdev's matrix:

The adapter will be hot plugged into the guest under the following conditions:
1. The adapter is not yet assigned to the guest's matrix
2. At least one domain is assigned to the guest's matrix
3. Each APQN derived from the APID of the newly assigned adapter and the APQIs of the domains already assigned to the guest's matrix references a queue device bound to the vfio_ap device driver.

The adapter and each domain assigned to the mdev's matrix will be hot plugged into the guest under the following conditions:
1. The adapter is not yet assigned to the guest's matrix
2. No domains are assigned to the guest's matrix
3 At least one domain is assigned to the mdev's matrix
4. Each APQN derived from the APID of the newly assigned adapter and the APQIs of the domains assigned to the mdev's matrix references a queue device bound to the vfio_ap device driver.

* Unassign an adapter from mdev's matrix:

The adapter will be hot unplugged from the KVM guest if it is assigned to the guest's matrix.

* Assign a domain to mdev's matrix:

The domain will be hot plugged into the guest under the following conditions:
1. The domain is not yet assigned to the guest's matrix
2. At least one adapter is assigned to the guest's matrix
3. Each APQN derived from the APQI of the newly assigned domain and the APIDs of the adapters already assigned to the guest's matrix references a queue device bound to the vfio_ap device driver.

The domain and each adapter assigned to the mdev's matrix will be hot plugged into the guest under the following conditions:
1. The domain is not yet assigned to the guest's matrix
2. No adapters are assigned to the guest's matrix
3 At least one adapter is assigned to the mdev's matrix
4. Each APQN derived from the APQI of the newly assigned domain and the APIDs of the adapters assigned to the mdev's matrix references a queue device bound to the vfio_ap device driver.

* Unassign adapter from mdev's matrix:

The domain will be hot unplugged from the KVM guest if it is assigned to the guest's matrix.

* Assign a control domain:

The control domain will be hot plugged into the KVM guest if it is not assigned to the guest's APCB. The AP architecture ensures a guest will only get access to the control domain if it is in the host's AP configuration, so there is no risk in hot plugging it; however, it will become automatically available to the guest when it is added to the host configuration.

* Unassign a control domain:

The control domain will be hot unplugged from the KVM guest if it is assigned to the guest's APCB.

Note: Now that hot plug/unplug is implemented, there is the possibility
that an assignment/unassignment of an adapter, domain or control domain could be initiated while the guest is starting, so the matrix device lock will be taken for the group notification callback that initializes the guest's APCB when the KVM pointer is made available to the vfio_ap device driver.

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

---
 drivers/s390/crypto/vfio_ap_ops.c | 190 +++++++++++++++++++++++++-----
 1 file changed, 159 insertions(+), 31 deletions(-)

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

index 586ec5776693..4f96b7861607 100644
@@ -631,6 +631,60 @@ static void vfio_ap_mdev_manage_qlinks(struct ap_matrix_mdev *matrix_mdev,
 	}
 }
 
+static bool vfio_ap_assign_apid_to_apcb(struct ap_matrix_mdev *matrix_mdev,
+					unsigned long apid)
+{
+	unsigned long apqi, apqn;
+	unsigned long *aqm = matrix_mdev->shadow_apcb.aqm;
+
+	/*
+	 * If the APID is already assigned to the guest's shadow APCB, there is
+	 * no need to assign it.
+	 */
+	if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
+		return false;
+
+	/*
+	 * If no domains have yet been assigned to the shadow APCB and one or
+	 * more domains have been assigned to the matrix mdev, then use
+	 * the domains assigned to the matrix mdev; otherwise, there is nothing
+	 * to assign to the shadow APCB.
+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.aqm, AP_DOMAINS)) {
+		if (bitmap_empty(matrix_mdev->matrix.aqm, AP_DOMAINS))
+			return false;
+
+		aqm = matrix_mdev->matrix.aqm;
+	}
+
+	/* Make sure all APQNs are bound to the vfio_ap driver */
+	for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
+		apqn = AP_MKQID(apid, apqi);
+
+		if (vfio_ap_mdev_get_queue(matrix_mdev, apqn) == NULL)
+			return false;
+	}
+
+	set_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
+

drivers/s390/crypto/vfio_ap_ops.c +113 -31

+
+static void vfio_ap_mdev_hot_plug_adapter(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long apid)
+{
+	if (vfio_ap_assign_apid_to_apcb(matrix_mdev, apid))
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+}
+
 /**
  * assign_adapter_store
  *

@@ -673,10 +727,6 @@ static ssize_t assign_adapter_store(struct device *dev,
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow assignment of adapter */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
 	ret = kstrtoul(buf, 0, &apid);
 	if (ret)
 		return ret;

@@ -698,12 +748,22 @@ static ssize_t assign_adapter_store(struct device *dev,
 	}
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
 	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APID, apid);
+	vfio_ap_mdev_hot_plug_adapter(matrix_mdev, apid);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
 }
 static DEVICE_ATTR_WO(assign_adapter);
 
+static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
+					    unsigned long apid)
+{
+	if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
+		clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
+
 /**
  * unassign_adapter_store
  *

@@ -730,10 +790,6 @@ static ssize_t unassign_adapter_store(struct device *dev,
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow un-assignment of adapter */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
 	ret = kstrtoul(buf, 0, &apid);
 	if (ret)
 		return ret;

@@ -744,12 +800,67 @@ static ssize_t unassign_adapter_store(struct device *dev,
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
 	vfio_ap_mdev_manage_qlinks(matrix_mdev, UNLINK_APID, apid);
+	vfio_ap_mdev_hot_unplug_adapter(matrix_mdev, apid);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
 }
 static DEVICE_ATTR_WO(unassign_adapter);
 
+static bool vfio_ap_assign_apqi_to_apcb(struct ap_matrix_mdev *matrix_mdev,
+					unsigned long apqi)
+{
+	unsigned long apid, apqn;
+	unsigned long *apm = matrix_mdev->shadow_apcb.apm;
+
+	/*
+	 * If the APQI is already assigned to the guest's shadow APCB, there is
+	 * no need to assign it.
+	 */
+	if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
+		return false;
+
+	/*
+	 * If no adapters have yet been assigned to the shadow APCB and one or
+	 * more adapters have been assigned to the matrix mdev, then use
+	 * the adapters assigned to the matrix mdev; otherwise, there is nothing
+	 * to assign to the shadow APCB.
+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.apm, AP_DEVICES)) {
+		if (bitmap_empty(matrix_mdev->matrix.apm, AP_DEVICES))
+			return false;
+
+		apm = matrix_mdev->matrix.apm;
+	}
+
+	/* Make sure all APQNs are bound to the vfio_ap driver */
+	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
+		apqn = AP_MKQID(apid, apqi);
+
+		if (vfio_ap_mdev_get_queue(matrix_mdev, apqn) == NULL)
+			return false;
+	}
+
+	set_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
+
+	/*
+	 * If we verified APQNs using the adapters assigned to the matrix mdev,
+	 * then copy the APIDs of those adapters into the guest's APCB
+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.apm, AP_DEVICES))
+		bitmap_copy(matrix_mdev->shadow_apcb.apm,
+			    matrix_mdev->matrix.apm, AP_DEVICES);
+
+	return true;
+}
+
+static void vfio_ap_mdev_hot_plug_domain(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apqi)
+{
+	if (vfio_ap_assign_apqi_to_apcb(matrix_mdev, apqi))
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+}
+
 /**
  * assign_domain_store
  *

@@ -793,10 +904,6 @@ static ssize_t assign_domain_store(struct device *dev,
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 	unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
 
-	/* If the guest is running, disallow assignment of domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
 	ret = kstrtoul(buf, 0, &apqi);
 	if (ret)
 		return ret;

@@ -817,12 +924,21 @@ static ssize_t assign_domain_store(struct device *dev,
 	}
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
 	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APQI, apqi);
+	vfio_ap_mdev_hot_plug_domain(matrix_mdev, apqi);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
 }
 static DEVICE_ATTR_WO(assign_domain);
 
+static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
+					   unsigned long apqi)
+{
+	if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) {
+		clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
 
 /**
  * unassign_domain_store

@@ -850,10 +966,6 @@ static ssize_t unassign_domain_store(struct device *dev,
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow un-assignment of domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
 	ret = kstrtoul(buf, 0, &apqi);
 	if (ret)
 		return ret;

@@ -864,12 +976,22 @@ static ssize_t unassign_domain_store(struct device *dev,
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
 	vfio_ap_mdev_manage_qlinks(matrix_mdev, UNLINK_APQI, apqi);
+	vfio_ap_mdev_hot_unplug_domain(matrix_mdev, apqi);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
 }
 static DEVICE_ATTR_WO(unassign_domain);
 
+static void vfio_ap_mdev_hot_plug_ctl_domain(struct ap_matrix_mdev *matrix_mdev,
+					     unsigned long domid)
+{
+	if (!test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
+		set_bit_inv(domid, matrix_mdev->shadow_apcb.adm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
+
 /**
  * assign_control_domain_store
  *

@@ -895,10 +1017,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow assignment of control domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
 	ret = kstrtoul(buf, 0, &id);
 	if (ret)
 		return ret;

@@ -914,12 +1032,23 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	if (!mutex_trylock(&matrix_dev->lock))
 		return -EBUSY;
 	set_bit_inv(id, matrix_mdev->matrix.adm);
+	vfio_ap_mdev_hot_plug_ctl_domain(matrix_mdev, id);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
 }
 static DEVICE_ATTR_WO(assign_control_domain);
 
+static void
+vfio_ap_mdev_hot_unplug_ctl_domain(struct ap_matrix_mdev *matrix_mdev,
+				   unsigned long domid)
+{
+	if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
+		clear_bit_inv(domid, matrix_mdev->shadow_apcb.adm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
+
 /**
  * unassign_control_domain_store
  *

@@ -946,10 +1075,6 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 	unsigned long max_domid =  matrix_mdev->matrix.adm_max;
 
-	/* If the guest is running, disallow un-assignment of control domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
 	ret = kstrtoul(buf, 0, &domid);
 	if (ret)
 		return ret;

@@ -958,6 +1083,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv(domid, matrix_mdev->matrix.adm);
+	vfio_ap_mdev_hot_unplug_ctl_domain(matrix_mdev, domid);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;

@@ -1099,8 +1225,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 {
 	struct ap_matrix_mdev *m;
 
-	mutex_lock(&matrix_dev->lock);
-
 	list_for_each_entry(m, &matrix_dev->mdev_list, node) {
 		if ((m != matrix_mdev) && (m->kvm == kvm)) {
 			mutex_unlock(&matrix_dev->lock);

@@ -1111,7 +1235,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	matrix_mdev->kvm = kvm;
 	kvm_get_kvm(kvm);
 	kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
-	mutex_unlock(&matrix_dev->lock);
 
 	return 0;
 }

@@ -1148,7 +1271,7 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
 static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 				       unsigned long action, void *data)
 {
-	int ret;
+	int ret = NOTIFY_DONE;
 	struct ap_matrix_mdev *matrix_mdev;
 
 	if (action != VFIO_GROUP_NOTIFY_SET_KVM)

@@ -1156,23 +1279,28 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 
 	matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
 
+	mutex_lock(&matrix_dev->lock);
+
 	if (!data) {
 		if (matrix_mdev->kvm)
 			kvm_put_kvm(matrix_mdev->kvm);
 
 		matrix_mdev->kvm = NULL;
 
-		return NOTIFY_OK;
+		ret = NOTIFY_OK;
+		goto done;
 	}
 
 	ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
 	if (ret)
-		return NOTIFY_DONE;
+		goto done;
 
 	vfio_ap_mdev_init_apcb(matrix_mdev);
 	vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
 
-	return NOTIFY_OK;
+done:
+	mutex_unlock(&matrix_dev->lock);
+	return ret;
 }
 
 static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,

Quoted 61 lines · On Tue, 24 Nov 2020 16:40:11 -0500 Tony Krowiak <[email protected]> wrote:

Let's hot plug/unplug adapters, domains and control domains assigned to or unassigned from an AP matrix mdev device while it is in use by a guest per the following rules:

* Assign an adapter to mdev's matrix:

The adapter will be hot plugged into the guest under the following conditions:
1. The adapter is not yet assigned to the guest's matrix
2. At least one domain is assigned to the guest's matrix
3. Each APQN derived from the APID of the newly assigned adapter and the APQIs of the domains already assigned to the guest's matrix references a queue device bound to the vfio_ap device driver.

The adapter and each domain assigned to the mdev's matrix will be hot plugged into the guest under the following conditions:
1. The adapter is not yet assigned to the guest's matrix
2. No domains are assigned to the guest's matrix
3 At least one domain is assigned to the mdev's matrix
4. Each APQN derived from the APID of the newly assigned adapter and the APQIs of the domains assigned to the mdev's matrix references a queue device bound to the vfio_ap device driver.

* Unassign an adapter from mdev's matrix:

The adapter will be hot unplugged from the KVM guest if it is assigned to the guest's matrix.

* Assign a domain to mdev's matrix:

The domain will be hot plugged into the guest under the following conditions:
1. The domain is not yet assigned to the guest's matrix
2. At least one adapter is assigned to the guest's matrix
3. Each APQN derived from the APQI of the newly assigned domain and the APIDs of the adapters already assigned to the guest's matrix references a queue device bound to the vfio_ap device driver.

The domain and each adapter assigned to the mdev's matrix will be hot plugged into the guest under the following conditions:
1. The domain is not yet assigned to the guest's matrix
2. No adapters are assigned to the guest's matrix
3 At least one adapter is assigned to the mdev's matrix
4. Each APQN derived from the APQI of the newly assigned domain and the APIDs of the adapters assigned to the mdev's matrix references a queue device bound to the vfio_ap device driver.

* Unassign adapter from mdev's matrix:

The domain will be hot unplugged from the KVM guest if it is assigned to the guest's matrix.

* Assign a control domain:

The control domain will be hot plugged into the KVM guest if it is not assigned to the guest's APCB. The AP architecture ensures a guest will only get access to the control domain if it is in the host's AP configuration, so there is no risk in hot plugging it; however, it will become automatically available to the guest when it is added to the host configuration.

Quoted 4 lines

* Unassign a control domain:

The control domain will be hot unplugged from the KVM guest if it is assigned to the guest's APCB.

This is where things start getting tricky. E.g. do we need to revise filtering after an unassign? (For example an assign_adapter X didn't change the shadow, because queue XY was missing, but now we unplug domain Y. Should the adapter X pop up? I guess it should.)

Quoted 61 lines

Note: Now that hot plug/unplug is implemented, there is the possibility
that an assignment/unassignment of an adapter, domain or control domain could be initiated while the guest is starting, so the matrix device lock will be taken for the group notification callback that initializes the guest's APCB when the KVM pointer is made available to the vfio_ap device driver.

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

---
  drivers/s390/crypto/vfio_ap_ops.c | 190 +++++++++++++++++++++++++-----
  1 file changed, 159 insertions(+), 31 deletions(-)

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

index 586ec5776693..4f96b7861607 100644
@@ -631,6 +631,60 @@ static void vfio_ap_mdev_manage_qlinks(struct ap_matrix_mdev *matrix_mdev,
  	}
  }
 
+static bool vfio_ap_assign_apid_to_apcb(struct ap_matrix_mdev *matrix_mdev,
+					unsigned long apid)
+{
+	unsigned long apqi, apqn;
+	unsigned long *aqm = matrix_mdev->shadow_apcb.aqm;
+
+	/*
+	 * If the APID is already assigned to the guest's shadow APCB, there is
+	 * no need to assign it.
+	 */
+	if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
+		return false;
+
+	/*
+	 * If no domains have yet been assigned to the shadow APCB and one or
+	 * more domains have been assigned to the matrix mdev, then use
+	 * the domains assigned to the matrix mdev; otherwise, there is nothing
+	 * to assign to the shadow APCB.
+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.aqm, AP_DOMAINS)) {
+		if (bitmap_empty(matrix_mdev->matrix.aqm, AP_DOMAINS))
+			return false;
+
+		aqm = matrix_mdev->matrix.aqm;
+	}
+
+	/* Make sure all APQNs are bound to the vfio_ap driver */
+	for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
+		apqn = AP_MKQID(apid, apqi);
+
+		if (vfio_ap_mdev_get_queue(matrix_mdev, apqn) == NULL)
+			return false;
+	}
+
+	set_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
+
+	/*
+	 * If we verified APQNs using the domains assigned to the matrix mdev,
+	 * then copy the APQIs of those domains into the guest's APCB

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

Quoted 7 lines

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

+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.aqm, AP_DOMAINS))
+		bitmap_copy(matrix_mdev->shadow_apcb.aqm,
+			    matrix_mdev->matrix.aqm, AP_DOMAINS);
+
+	return true;
+}

What is the rationale behind the shadow aqm empty special handling?

I.e.
why not simply:

static bool vfio_ap_assign_apid_to_apcb(struct ap_matrix_mdev *matrix_mdev,
unsigned long apid)

{
         unsigned long apqi, apqn;
         unsigned long *aqm = matrix_mdev->shadow_apcb.aqm;
                                                                                 
         /*
          * If the APID is already assigned to the guest's shadow APCB, there is
          * no need to assign it.
          */
         if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm))
                 return false;
                                                                                 
         /* Make sure all APQNs are bound to the vfio_ap driver */
         for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
                 apqn = AP_MKQID(apid, apqi);
                                                                                 
                 if (vfio_ap_mdev_get_queue(matrix_mdev, apqn) == NULL)
                         return false;
         }
                                                                                 
         set_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
                                                                                 
         return true;
}

Please answer the questions I've asked, and note that I will have to return to this patch, later.

Regards,
Halil

Quoted 303 lines

drivers/s390/crypto/vfio_ap_ops.c +113 -31

+
+static void vfio_ap_mdev_hot_plug_adapter(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long apid)
+{
+	if (vfio_ap_assign_apid_to_apcb(matrix_mdev, apid))
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+}
+
  /**
   * assign_adapter_store
   *

@@ -673,10 +727,6 @@ static ssize_t assign_adapter_store(struct device *dev,
  	struct mdev_device *mdev = mdev_from_dev(dev);
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow assignment of adapter */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
  	ret = kstrtoul(buf, 0, &apid);
  	if (ret)
  		return ret;

@@ -698,12 +748,22 @@ static ssize_t assign_adapter_store(struct device *dev,
  	}
  	set_bit_inv(apid, matrix_mdev->matrix.apm);
  	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APID, apid);
+	vfio_ap_mdev_hot_plug_adapter(matrix_mdev, apid);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;
  }
  static DEVICE_ATTR_WO(assign_adapter);
 
+static void vfio_ap_mdev_hot_unplug_adapter(struct ap_matrix_mdev *matrix_mdev,
+					    unsigned long apid)
+{
+	if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm)) {
+		clear_bit_inv(apid, matrix_mdev->shadow_apcb.apm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
+
  /**
   * unassign_adapter_store
   *

@@ -730,10 +790,6 @@ static ssize_t unassign_adapter_store(struct device *dev,
  	struct mdev_device *mdev = mdev_from_dev(dev);
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow un-assignment of adapter */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
  	ret = kstrtoul(buf, 0, &apid);
  	if (ret)
  		return ret;

@@ -744,12 +800,67 @@ static ssize_t unassign_adapter_store(struct device *dev,
  	mutex_lock(&matrix_dev->lock);
  	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
  	vfio_ap_mdev_manage_qlinks(matrix_mdev, UNLINK_APID, apid);
+	vfio_ap_mdev_hot_unplug_adapter(matrix_mdev, apid);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;
  }
  static DEVICE_ATTR_WO(unassign_adapter);
 
+static bool vfio_ap_assign_apqi_to_apcb(struct ap_matrix_mdev *matrix_mdev,
+					unsigned long apqi)
+{
+	unsigned long apid, apqn;
+	unsigned long *apm = matrix_mdev->shadow_apcb.apm;
+
+	/*
+	 * If the APQI is already assigned to the guest's shadow APCB, there is
+	 * no need to assign it.
+	 */
+	if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
+		return false;
+
+	/*
+	 * If no adapters have yet been assigned to the shadow APCB and one or
+	 * more adapters have been assigned to the matrix mdev, then use
+	 * the adapters assigned to the matrix mdev; otherwise, there is nothing
+	 * to assign to the shadow APCB.
+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.apm, AP_DEVICES)) {
+		if (bitmap_empty(matrix_mdev->matrix.apm, AP_DEVICES))
+			return false;
+
+		apm = matrix_mdev->matrix.apm;
+	}
+
+	/* Make sure all APQNs are bound to the vfio_ap driver */
+	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
+		apqn = AP_MKQID(apid, apqi);
+
+		if (vfio_ap_mdev_get_queue(matrix_mdev, apqn) == NULL)
+			return false;
+	}
+
+	set_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
+
+	/*
+	 * If we verified APQNs using the adapters assigned to the matrix mdev,
+	 * then copy the APIDs of those adapters into the guest's APCB
+	 */
+	if (bitmap_empty(matrix_mdev->shadow_apcb.apm, AP_DEVICES))
+		bitmap_copy(matrix_mdev->shadow_apcb.apm,
+			    matrix_mdev->matrix.apm, AP_DEVICES);
+
+	return true;
+}
+
+static void vfio_ap_mdev_hot_plug_domain(struct ap_matrix_mdev *matrix_mdev,
+					 unsigned long apqi)
+{
+	if (vfio_ap_assign_apqi_to_apcb(matrix_mdev, apqi))
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+}
+
  /**
   * assign_domain_store
   *

@@ -793,10 +904,6 @@ static ssize_t assign_domain_store(struct device *dev,
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
  	unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
 
-	/* If the guest is running, disallow assignment of domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
  	ret = kstrtoul(buf, 0, &apqi);
  	if (ret)
  		return ret;

@@ -817,12 +924,21 @@ static ssize_t assign_domain_store(struct device *dev,
  	}
  	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
  	vfio_ap_mdev_manage_qlinks(matrix_mdev, LINK_APQI, apqi);
+	vfio_ap_mdev_hot_plug_domain(matrix_mdev, apqi);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;
  }
  static DEVICE_ATTR_WO(assign_domain);
 
+static void vfio_ap_mdev_hot_unplug_domain(struct ap_matrix_mdev *matrix_mdev,
+					   unsigned long apqi)
+{
+	if (test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm)) {
+		clear_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
 
  /**
   * unassign_domain_store

@@ -850,10 +966,6 @@ static ssize_t unassign_domain_store(struct device *dev,
  	struct mdev_device *mdev = mdev_from_dev(dev);
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow un-assignment of domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
  	ret = kstrtoul(buf, 0, &apqi);
  	if (ret)
  		return ret;

@@ -864,12 +976,22 @@ static ssize_t unassign_domain_store(struct device *dev,
  	mutex_lock(&matrix_dev->lock);
  	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
  	vfio_ap_mdev_manage_qlinks(matrix_mdev, UNLINK_APQI, apqi);
+	vfio_ap_mdev_hot_unplug_domain(matrix_mdev, apqi);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;
  }
  static DEVICE_ATTR_WO(unassign_domain);
 
+static void vfio_ap_mdev_hot_plug_ctl_domain(struct ap_matrix_mdev *matrix_mdev,
+					     unsigned long domid)
+{
+	if (!test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
+		set_bit_inv(domid, matrix_mdev->shadow_apcb.adm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
+
  /**
   * assign_control_domain_store
   *

@@ -895,10 +1017,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
  	struct mdev_device *mdev = mdev_from_dev(dev);
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	/* If the guest is running, disallow assignment of control domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
  	ret = kstrtoul(buf, 0, &id);
  	if (ret)
  		return ret;

@@ -914,12 +1032,23 @@ static ssize_t assign_control_domain_store(struct device *dev,
  	if (!mutex_trylock(&matrix_dev->lock))
  		return -EBUSY;
  	set_bit_inv(id, matrix_mdev->matrix.adm);
+	vfio_ap_mdev_hot_plug_ctl_domain(matrix_mdev, id);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;
  }
  static DEVICE_ATTR_WO(assign_control_domain);
 
+static void
+vfio_ap_mdev_hot_unplug_ctl_domain(struct ap_matrix_mdev *matrix_mdev,
+				   unsigned long domid)
+{
+	if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
+		clear_bit_inv(domid, matrix_mdev->shadow_apcb.adm);
+		vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
+	}
+}
+
  /**
   * unassign_control_domain_store
   *

@@ -946,10 +1075,6 @@ static ssize_t unassign_control_domain_store(struct device *dev,
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
  	unsigned long max_domid =  matrix_mdev->matrix.adm_max;
 
-	/* If the guest is running, disallow un-assignment of control domain */
-	if (matrix_mdev->kvm)
-		return -EBUSY;
-
  	ret = kstrtoul(buf, 0, &domid);
  	if (ret)
  		return ret;

@@ -958,6 +1083,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 
  	mutex_lock(&matrix_dev->lock);
  	clear_bit_inv(domid, matrix_mdev->matrix.adm);
+	vfio_ap_mdev_hot_unplug_ctl_domain(matrix_mdev, domid);
  	mutex_unlock(&matrix_dev->lock);
 
  	return count;

@@ -1099,8 +1225,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
  {
  	struct ap_matrix_mdev *m;
 
-	mutex_lock(&matrix_dev->lock);
-
  	list_for_each_entry(m, &matrix_dev->mdev_list, node) {
  		if ((m != matrix_mdev) && (m->kvm == kvm)) {
  			mutex_unlock(&matrix_dev->lock);

@@ -1111,7 +1235,6 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
  	matrix_mdev->kvm = kvm;
  	kvm_get_kvm(kvm);
  	kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
-	mutex_unlock(&matrix_dev->lock);
 
  	return 0;
  }

@@ -1148,7 +1271,7 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
  static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
  				       unsigned long action, void *data)
  {
-	int ret;
+	int ret = NOTIFY_DONE;
  	struct ap_matrix_mdev *matrix_mdev;
 
  	if (action != VFIO_GROUP_NOTIFY_SET_KVM)

@@ -1156,23 +1279,28 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 
  	matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
 
+	mutex_lock(&matrix_dev->lock);
+
  	if (!data) {
  		if (matrix_mdev->kvm)
  			kvm_put_kvm(matrix_mdev->kvm);
 
  		matrix_mdev->kvm = NULL;
 
-		return NOTIFY_OK;
+		ret = NOTIFY_OK;
+		goto done;
  	}
 
  	ret = vfio_ap_mdev_set_kvm(matrix_mdev, data);
  	if (ret)
-		return NOTIFY_DONE;
+		goto done;
 
  	vfio_ap_mdev_init_apcb(matrix_mdev);
  	vfio_ap_mdev_commit_shadow_apcb(matrix_mdev);
 
-	return NOTIFY_OK;
+done:
+	mutex_unlock(&matrix_dev->lock);
+	return ret;
  }
 
  static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,