[PATCH v2 0/7] s390: vfio-ap: dynamic configuration support

[PATCH v2 0/7] s390: vfio-ap: dynamic configuration support

[Tony Krowiak]

This patch series extends the crypto adapter pass-through support to 
provide safeguards against inadvertent sharing of AP resources between
guests and/or the host, and to implement more of the s390 AP
architecture related to provisioning and dynamic configuration of
AP resources.

Change log v2->v3:
-----------------
* Allow guest access to an AP queue only if the queue is bound to
  the vfio_ap device driver.

* Removed the patch to test CRYCB masks before taking the vCPUs
  out of SIE. Now checking the shadow CRYCB in the vfio_ap driver.

Change log v1->v2:
-----------------
* Removed patches preventing root user from unbinding AP queues from 
  the vfio_ap device driver
* Introduced a shadow CRYCB in the vfio_ap driver to manage dynamic 
  changes to the AP guest configuration due to root user interventions
  or hardware anomalies.

Tony Krowiak (7):
  s390: vfio-ap: wait for queue empty on queue reset
  s390: vfio-ap: maintain a shadow of the guest's CRYCB
  s390: vfio-ap: sysfs interface to display guest CRYCB
  s390: vfio-ap: allow assignment of unavailable AP resources to mdev
    device
  s390: vfio-ap: allow hot plug/unplug of AP resources using mdev device
  s390: vfio-ap: handle bind and unbind of AP queue device
  s390: vfio-ap: update documentation

 Documentation/s390/vfio-ap.txt        | 191 +++++++----
 drivers/s390/crypto/vfio_ap_drv.c     |  12 +-
 drivers/s390/crypto/vfio_ap_ops.c     | 612 ++++++++++++++++++++++------------
 drivers/s390/crypto/vfio_ap_private.h |   4 +
 4 files changed, 536 insertions(+), 283 deletions(-)

-- 
2.7.4

[PATCH v2 1/7] s390: vfio-ap: wait for queue empty on queue reset

[Tony Krowiak]

Refactors the AP queue reset function to wait until the queue is empty
after the PQAP(ZAPQ) instruction is executed to zero out the queue as
required by the AP architecture.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 900b9cf20ca5..b88a2a2ba075 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -271,6 +271,32 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 	return 0;
 }
 
+static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
+{
+	struct ap_queue_status status;
+	ap_qid_t qid = AP_MKQID(apid, apqi);
+	int retry = 5;
+
+	do {
+		status = ap_tapq(qid, NULL);
+		switch (status.response_code) {
+		case AP_RESPONSE_NORMAL:
+			if (status.queue_empty)
+				return;
+			msleep(20);
+			break;
+		case AP_RESPONSE_RESET_IN_PROGRESS:
+		case AP_RESPONSE_BUSY:
+			msleep(20);
+			break;
+		default:
+			pr_warn("%s: tapq err %02x: %04lx.%02lx may not be empty\n",
+				__func__, status.response_code, apid, apqi);
+			return;
+		}
+	} while (--retry);
+}
+
 /**
  * assign_adapter_store
  *
@@ -790,15 +816,18 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
-				    unsigned int retry)
+int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
 {
 	struct ap_queue_status status;
+	int retry = 5;
 
 	do {
 		status = ap_zapq(AP_MKQID(apid, apqi));
 		switch (status.response_code) {
 		case AP_RESPONSE_NORMAL:
+			vfio_ap_mdev_wait_for_qempty(apid, apqi);
+			return 0;
+		case AP_RESPONSE_DECONFIGURED:
 			return 0;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
@@ -824,7 +853,7 @@ 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) {
-			ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
+			ret = vfio_ap_mdev_reset_queue(apid, apqi);
 			/*
 			 * Regardless whether a queue turns out to be busy, or
 			 * is not operational, we need to continue resetting
-- 
2.7.4

[PATCH v2 2/7] s390: vfio-ap: maintain a shadow of the guest's CRYCB

[Tony Krowiak]

This patch introduces a shadow of the CRYCB being used by a guest. This
will enable to more effectively manage dynamic changes to the AP
resources installed on the host that may be assigned to an mdev device
and being used by a guest. For example:

* AP adapter cards can be dynamically added to and removed from the AP
  configuration via the SE or an SCLP command.

* AP resources that disappear and reappear due to hardware malfunctions.

* AP queues bound to and unbound from the vfio_ap device driver by a
  root user.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 91 ++++++++++++++++++++++++++++++++---
 drivers/s390/crypto/vfio_ap_private.h |  2 +
 2 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b88a2a2ba075..44a04b4aa9ae 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -297,6 +297,45 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
 	} while (--retry);
 }
 
+/*
+ * vfio_ap_mdev_update_crycb
+ *
+ * @matrix_mdev: the mediated matrix device
+ *
+ * Updates the AP matrix in the guest's CRYCB from it's shadow masks.
+ *
+ * Returns zero if the guest's CRYCB is successfully updated; otherwise,
+ * returns -ENODEV if a guest is not running or does not have a CRYCB.
+ */
+static int vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
+{
+	if (!matrix_mdev->kvm || !matrix_mdev->kvm->arch.crypto.crycbd)
+		return -ENODEV;
+
+	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
+				  matrix_mdev->shadow_crycb->apm,
+				  matrix_mdev->shadow_crycb->aqm,
+				  matrix_mdev->shadow_crycb->adm);
+
+	return 0;
+}
+
+static int match_apqn(struct device *dev, void *data)
+{
+	struct ap_queue *apq = to_ap_queue(dev);
+
+	return (apq->qid == *(unsigned long *)(data)) ? 1 : 0;
+}
+
+static struct device *vfio_ap_get_queue_dev(unsigned long apid,
+					     unsigned long apqi)
+{
+	unsigned long apqn = AP_MKQID(apid, apqi);
+
+	return driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				  &apqn, match_apqn);
+}
+
 /**
  * assign_adapter_store
  *
@@ -805,14 +844,9 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 	if (ret)
 		return NOTIFY_DONE;
 
-	/* If there is no CRYCB pointer, then we can't copy the masks */
-	if (!matrix_mdev->kvm->arch.crypto.crycbd)
+	if (vfio_ap_mdev_update_crycb(matrix_mdev))
 		return NOTIFY_DONE;
 
-	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
-				  matrix_mdev->matrix.aqm,
-				  matrix_mdev->matrix.adm);
-
 	return NOTIFY_OK;
 }
 
@@ -867,12 +901,55 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 	return rc;
 }
 
+static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev *matrix_mdev)
+{
+	unsigned long apid, apqi, domid;
+	struct device *dev;
+
+	matrix_mdev->shadow_crycb = kzalloc(sizeof(*matrix_mdev->shadow_crycb),
+					    GFP_KERNEL);
+	if (!matrix_mdev->shadow_crycb)
+		return -ENOMEM;
+
+	vfio_ap_matrix_init(&matrix_dev->info, matrix_mdev->shadow_crycb);
+
+	/*
+	 * Examine each APQN assigned to the mdev device. Set the APID and APQI
+	 * in the shadow CRYCB if and only if the queue device identified by
+	 * the APQN is in the configuration.
+	 */
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1) {
+			dev = vfio_ap_get_queue_dev(apid, apqi);
+			if (dev) {
+				set_bit_inv(apid,
+					    matrix_mdev->shadow_crycb->apm);
+				set_bit_inv(apqi,
+					    matrix_mdev->shadow_crycb->aqm);
+				put_device(dev);
+			}
+		}
+	}
+
+	/* Set all control domains assigned to the mdev in the shadow CRYCB */
+	for_each_set_bit_inv(domid, matrix_mdev->matrix.adm,
+			     matrix_mdev->matrix.adm_max + 1)
+		set_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
+
+	return 0;
+}
+
 static int vfio_ap_mdev_open(struct mdev_device *mdev)
 {
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 	unsigned long events;
 	int ret;
 
+	ret = vfio_ap_mdev_create_shadow_crycb(matrix_mdev);
+	if (ret)
+		return ret;
 
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
@@ -902,6 +979,8 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 				 &matrix_mdev->group_notifier);
 	matrix_mdev->kvm = NULL;
 	module_put(THIS_MODULE);
+	kfree(matrix_mdev->shadow_crycb);
+	matrix_mdev->shadow_crycb = NULL;
 }
 
 static int vfio_ap_mdev_get_device_info(unsigned long arg)
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 76b7f98e47e9..e8457aa61976 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -72,6 +72,7 @@ struct ap_matrix {
  * @list:	allows the ap_matrix_mdev struct to be added to a list
  * @matrix:	the adapters, usage domains and control domains assigned to the
  *		mediated matrix device.
+ * @shadow_crycb: a shadow copy of the crycb in use by a guest
  * @group_notifier: notifier block used for specifying callback function for
  *		    handling the VFIO_GROUP_NOTIFY_SET_KVM event
  * @kvm:	the struct holding guest's state
@@ -79,6 +80,7 @@ struct ap_matrix {
 struct ap_matrix_mdev {
 	struct list_head node;
 	struct ap_matrix matrix;
+	struct ap_matrix *shadow_crycb;
 	struct notifier_block group_notifier;
 	struct kvm *kvm;
 };
-- 
2.7.4

[PATCH v2 3/7] s390: vfio-ap: sysfs interface to display guest CRYCB

[Tony Krowiak]

Introduces a sysfs interface on the matrix mdev device to display the
contents of the shadow of the guest's CRYCB

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 59 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44a04b4aa9ae..1021466cb661 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -771,6 +771,64 @@ static ssize_t matrix_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(matrix);
 
+static ssize_t guest_matrix_show(struct device *dev,
+				 struct device_attribute *attr, char *buf)
+{
+	struct mdev_device *mdev = mdev_from_dev(dev);
+	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
+	char *bufpos = buf;
+	unsigned long apid;
+	unsigned long apqi;
+	unsigned long apid1;
+	unsigned long apqi1;
+	unsigned long napm_bits;
+	unsigned long naqm_bits;
+	int nchars = 0;
+	int n;
+
+	if (!matrix_mdev->shadow_crycb)
+		return -ENODEV;
+
+	mutex_lock(&matrix_dev->lock);
+	napm_bits = matrix_mdev->shadow_crycb->apm_max + 1;
+	naqm_bits = matrix_mdev->shadow_crycb->aqm_max + 1;
+	apid1 = find_first_bit_inv(matrix_mdev->shadow_crycb->apm, napm_bits);
+	apqi1 = find_first_bit_inv(matrix_mdev->shadow_crycb->aqm, naqm_bits);
+
+	if ((apid1 < napm_bits) && (apqi1 < naqm_bits)) {
+		for_each_set_bit_inv(apid, matrix_mdev->shadow_crycb->apm,
+				     napm_bits) {
+			for_each_set_bit_inv(apqi,
+					     matrix_mdev->shadow_crycb->aqm,
+					     naqm_bits) {
+				n = sprintf(bufpos, "%02lx.%04lx\n", apid,
+					    apqi);
+				bufpos += n;
+				nchars += n;
+			}
+		}
+	} else if (apid1 < napm_bits) {
+		for_each_set_bit_inv(apid, matrix_mdev->shadow_crycb->apm,
+				     napm_bits) {
+			n = sprintf(bufpos, "%02lx.\n", apid);
+			bufpos += n;
+			nchars += n;
+		}
+	} else if (apqi1 < naqm_bits) {
+		for_each_set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm,
+				     naqm_bits) {
+			n = sprintf(bufpos, ".%04lx\n", apqi);
+			bufpos += n;
+			nchars += n;
+		}
+	}
+
+	mutex_unlock(&matrix_dev->lock);
+
+	return nchars;
+}
+static DEVICE_ATTR_RO(guest_matrix);
+
 static struct attribute *vfio_ap_mdev_attrs[] = {
 	&dev_attr_assign_adapter.attr,
 	&dev_attr_unassign_adapter.attr,
@@ -780,6 +838,7 @@ static struct attribute *vfio_ap_mdev_attrs[] = {
 	&dev_attr_unassign_control_domain.attr,
 	&dev_attr_control_domains.attr,
 	&dev_attr_matrix.attr,
+	&dev_attr_guest_matrix.attr,
 	NULL,
 };
 
-- 
2.7.4

[PATCH v2 4/7] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device

[Tony Krowiak]

The AP architecture does not preclude assignment of AP resources that are
not yet in the AP configuration (i.e., not available or not online).
Let's go ahead and implement this facet of the AP architecture for linux
guests.

The current implementation does not allow assignment of AP resources to
an mdev device if the AP queue devices identified by the assignment are
not bound to the vfio_ap device driver. This patch allows assignment of AP
resources to the mdev device even if the AP queue devices are not bound to
the vfio_ap device driver, as long as the AP queue devices are not
reserved by the AP BUS for use by the zcrypt device drivers.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 231 ++++++++------------------------------
 1 file changed, 44 insertions(+), 187 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 1021466cb661..ea24caf17a16 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -113,122 +113,6 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
 	NULL,
 };
 
-struct vfio_ap_queue_reserved {
-	unsigned long *apid;
-	unsigned long *apqi;
-	bool reserved;
-};
-
-/**
- * vfio_ap_has_queue
- *
- * @dev: an AP queue device
- * @data: a struct vfio_ap_queue_reserved reference
- *
- * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
- * apid or apqi specified in @data:
- *
- * - If @data contains both an apid and apqi value, then @data will be flagged
- *   as reserved if the APID and APQI fields for the AP queue device matches
- *
- * - If @data contains only an apid value, @data will be flagged as
- *   reserved if the APID field in the AP queue device matches
- *
- * - If @data contains only an apqi value, @data will be flagged as
- *   reserved if the APQI field in the AP queue device matches
- *
- * Returns 0 to indicate the input to function succeeded. Returns -EINVAL if
- * @data does not contain either an apid or apqi.
- */
-static int vfio_ap_has_queue(struct device *dev, void *data)
-{
-	struct vfio_ap_queue_reserved *qres = data;
-	struct ap_queue *ap_queue = to_ap_queue(dev);
-	ap_qid_t qid;
-	unsigned long id;
-
-	if (qres->apid && qres->apqi) {
-		qid = AP_MKQID(*qres->apid, *qres->apqi);
-		if (qid == ap_queue->qid)
-			qres->reserved = true;
-	} else if (qres->apid && !qres->apqi) {
-		id = AP_QID_CARD(ap_queue->qid);
-		if (id == *qres->apid)
-			qres->reserved = true;
-	} else if (!qres->apid && qres->apqi) {
-		id = AP_QID_QUEUE(ap_queue->qid);
-		if (id == *qres->apqi)
-			qres->reserved = true;
-	} else {
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
-/**
- * vfio_ap_verify_queue_reserved
- *
- * @matrix_dev: a mediated matrix device
- * @apid: an AP adapter ID
- * @apqi: an AP queue index
- *
- * Verifies that the AP queue with @apid/@apqi is reserved by the VFIO AP device
- * driver according to the following rules:
- *
- * - If both @apid and @apqi are not NULL, then there must be an AP queue
- *   device bound to the vfio_ap driver with the APQN identified by @apid and
- *   @apqi
- *
- * - If only @apid is not NULL, then there must be an AP queue device bound
- *   to the vfio_ap driver with an APQN containing @apid
- *
- * - If only @apqi is not NULL, then there must be an AP queue device bound
- *   to the vfio_ap driver with an APQN containing @apqi
- *
- * Returns 0 if the AP queue is reserved; otherwise, returns -EADDRNOTAVAIL.
- */
-static int vfio_ap_verify_queue_reserved(unsigned long *apid,
-					 unsigned long *apqi)
-{
-	int ret;
-	struct vfio_ap_queue_reserved qres;
-
-	qres.apid = apid;
-	qres.apqi = apqi;
-	qres.reserved = false;
-
-	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				     &qres, vfio_ap_has_queue);
-	if (ret)
-		return ret;
-
-	if (qres.reserved)
-		return 0;
-
-	return -EADDRNOTAVAIL;
-}
-
-static int
-vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
-					     unsigned long apid)
-{
-	int ret;
-	unsigned long apqi;
-	unsigned long nbits = matrix_mdev->matrix.aqm_max + 1;
-
-	if (find_first_bit_inv(matrix_mdev->matrix.aqm, nbits) >= nbits)
-		return vfio_ap_verify_queue_reserved(&apid, NULL);
-
-	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, nbits) {
-		ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 /**
  * vfio_ap_mdev_verify_no_sharing
  *
@@ -236,18 +120,26 @@ vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
  * and AP queue indexes comprising the AP matrix are not configured for another
  * mediated device. AP queue sharing is not allowed.
  *
- * @matrix_mdev: the mediated matrix device
+ * @mdev_apm: the mask identifying the adapters assigned to mdev
+ * @mdev_apm: the mask identifying the adapters assigned to mdev
  *
  * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
  */
-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
 {
 	struct ap_matrix_mdev *lstdev;
 	DECLARE_BITMAP(apm, AP_DEVICES);
 	DECLARE_BITMAP(aqm, AP_DOMAINS);
 
 	list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
-		if (matrix_mdev == lstdev)
+		/*
+		 * If either of the input masks belongs to the mdev to which an
+		 * AP resource is being assigned, then we don't need to verify
+		 * that mdev's masks.
+		 */
+		if ((mdev_apm == lstdev->matrix.apm) ||
+		    (mdev_aqm == lstdev->matrix.aqm))
 			continue;
 
 		memset(apm, 0, sizeof(apm));
@@ -257,12 +149,10 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 		 * We work on full longs, as we can only exclude the leftover
 		 * bits in non-inverse order. The leftover is all zeros.
 		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
 			continue;
 
-		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-				lstdev->matrix.aqm, AP_DOMAINS))
+		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
 			continue;
 
 		return -EADDRINUSE;
@@ -336,6 +226,17 @@ static struct device *vfio_ap_get_queue_dev(unsigned long apid,
 				  &apqn, match_apqn);
 }
 
+static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
+{
+	int ret;
+
+	ret = ap_apqn_in_matrix_owned_by_def_drv(apm, aqm);
+	if (ret)
+		return (ret == 1) ? -EADDRNOTAVAIL : ret;
+
+	return vfio_ap_mdev_verify_no_sharing(apm, aqm);
+}
+
 /**
  * assign_adapter_store
  *
@@ -374,6 +275,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 {
 	int ret;
 	unsigned long apid;
+	DECLARE_BITMAP(apm, AP_DEVICES);
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
@@ -388,32 +290,19 @@ static ssize_t assign_adapter_store(struct device *dev,
 	if (apid > matrix_mdev->matrix.apm_max)
 		return -ENODEV;
 
-	/*
-	 * Set the bit in the AP mask (APM) corresponding to the AP adapter
-	 * number (APID). The bits in the mask, from most significant to least
-	 * significant bit, correspond to APIDs 0-255.
-	 */
-	mutex_lock(&matrix_dev->lock);
-
-	ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
-	if (ret)
-		goto done;
+	memset(apm, 0, ARRAY_SIZE(apm) * sizeof(*apm));
+	set_bit_inv(apid, apm);
 
+	mutex_lock(&matrix_dev->lock);
+	ret = vfio_ap_mdev_validate_masks(apm, matrix_mdev->matrix.aqm);
+	if (ret) {
+		mutex_unlock(&matrix_dev->lock);
+		return ret;
+	}
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
-
-	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
-	if (ret)
-		goto share_err;
-
-	ret = count;
-	goto done;
-
-share_err:
-	clear_bit_inv(apid, matrix_mdev->matrix.apm);
-done:
 	mutex_unlock(&matrix_dev->lock);
 
-	return ret;
+	return count;
 }
 static DEVICE_ATTR_WO(assign_adapter);
 
@@ -462,26 +351,6 @@ static ssize_t unassign_adapter_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(unassign_adapter);
 
-static int
-vfio_ap_mdev_verify_queues_reserved_for_apqi(struct ap_matrix_mdev *matrix_mdev,
-					     unsigned long apqi)
-{
-	int ret;
-	unsigned long apid;
-	unsigned long nbits = matrix_mdev->matrix.apm_max + 1;
-
-	if (find_first_bit_inv(matrix_mdev->matrix.apm, nbits) >= nbits)
-		return vfio_ap_verify_queue_reserved(NULL, &apqi);
-
-	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm, nbits) {
-		ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
-}
-
 /**
  * assign_domain_store
  *
@@ -520,6 +389,7 @@ static ssize_t assign_domain_store(struct device *dev,
 {
 	int ret;
 	unsigned long apqi;
+	DECLARE_BITMAP(aqm, AP_DEVICES);
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 	unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
@@ -534,27 +404,19 @@ static ssize_t assign_domain_store(struct device *dev,
 	if (apqi > max_apqi)
 		return -ENODEV;
 
-	mutex_lock(&matrix_dev->lock);
-
-	ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
-	if (ret)
-		goto done;
+	memset(aqm, 0, ARRAY_SIZE(aqm) * sizeof(*aqm));
+	set_bit_inv(apqi, aqm);
 
+	mutex_lock(&matrix_dev->lock);
+	ret = vfio_ap_mdev_validate_masks(matrix_mdev->matrix.apm, aqm);
+	if (ret) {
+		mutex_unlock(&matrix_dev->lock);
+		return ret;
+	}
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
-
-	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
-	if (ret)
-		goto share_err;
-
-	ret = count;
-	goto done;
-
-share_err:
-	clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
-done:
 	mutex_unlock(&matrix_dev->lock);
 
-	return ret;
+	return count;
 }
 static DEVICE_ATTR_WO(assign_domain);
 
@@ -640,11 +502,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	if (id > matrix_mdev->matrix.adm_max)
 		return -ENODEV;
 
-	/* Set the bit in the ADM (bitmask) corresponding to the AP control
-	 * domain number (id). The bits in the mask, from most significant to
-	 * least significant, correspond to IDs 0 up to the one less than the
-	 * number of control domains that can be assigned.
-	 */
 	mutex_lock(&matrix_dev->lock);
 	set_bit_inv(id, matrix_mdev->matrix.adm);
 	mutex_unlock(&matrix_dev->lock);
-- 
2.7.4

[PATCH v2 5/7] s390: vfio-ap: allow hot plug/unplug of AP resources using mdev device

[Tony Krowiak]

Let's allow AP resources to be assigned to or unassigned from an AP matrix
mdev device while it is in use by a guest. If a guest is using the mdev
device while assignment is taking place, the guest will be granted access
to the resource as long as the guest will not be given access to an AP
queue device that is not bound to the vfio_ap device driver. If a guest is
using the mdev device while unassignment is taking place, access to the
resource will be taken from the guest.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 116 ++++++++++++++++++++++++++++----------
 1 file changed, 86 insertions(+), 30 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ea24caf17a16..ede45184eb67 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -226,6 +226,8 @@ static struct device *vfio_ap_get_queue_dev(unsigned long apid,
 				  &apqn, match_apqn);
 }
 
+
+
 static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
 {
 	int ret;
@@ -237,6 +239,26 @@ static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
 	return vfio_ap_mdev_verify_no_sharing(apm, aqm);
 }
 
+static bool vfio_ap_queues_on_drv(unsigned long *apm, unsigned long *aqm)
+{
+	unsigned long apid, apqi, apqn;
+	struct device *dev;
+
+	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
+		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
+			apqn = AP_MKQID(apid, apqi);
+
+			dev = vfio_ap_get_queue_dev(apid, apqi);
+			if (!dev)
+				return false;
+
+			put_device(dev);
+		}
+	}
+
+	return true;
+}
+
 /**
  * assign_adapter_store
  *
@@ -247,7 +269,10 @@ static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
  * @count:	the number of bytes in @buf
  *
  * Parses the APID from @buf and sets the corresponding bit in the mediated
- * matrix device's APM.
+ * matrix device's APM. If a guest is using the mediated matrix device and each
+ * new APQN formed as a result of the assignment identifies an AP queue device
+ * that is bound to the vfio_ap device driver, the guest will be granted access
+ * to the adapter with the specified APID.
  *
  * Returns the number of bytes processed if the APID is valid; otherwise,
  * returns one of the following errors:
@@ -279,10 +304,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;
@@ -300,6 +321,14 @@ static ssize_t assign_adapter_store(struct device *dev,
 		return ret;
 	}
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
+
+	if (matrix_mdev->shadow_crycb) {
+		if (vfio_ap_queues_on_drv(apm,
+					  matrix_mdev->shadow_crycb->aqm)) {
+			set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -315,7 +344,9 @@ static DEVICE_ATTR_WO(assign_adapter);
  * @count:	the number of bytes in @buf
  *
  * Parses the APID from @buf and clears the corresponding bit in the mediated
- * matrix device's APM.
+ * matrix device's APM. If a guest is using the mediated matrix device and has
+ * access to the AP adapter with the specified APID, access to the adapter will
+ * be taken from the guest.
  *
  * Returns the number of bytes processed if the APID is valid; otherwise,
  * returns one of the following errors:
@@ -332,10 +363,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;
@@ -345,6 +372,13 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
+
+	if (matrix_mdev->shadow_crycb) {
+		if (test_bit_inv(apid, matrix_mdev->shadow_crycb->apm)) {
+			clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -361,7 +395,10 @@ static DEVICE_ATTR_WO(unassign_adapter);
  * @count:	the number of bytes in @buf
  *
  * Parses the APQI from @buf and sets the corresponding bit in the mediated
- * matrix device's AQM.
+ * matrix device's AQM. If a guest is using the mediated matrix device and each
+ * new APQN formed as a result of the assignment identifies an AP queue device
+ * that is bound to the vfio_ap device driver, the guest will be given access
+ * to the AP queue(s) with the specified APQI.
  *
  * Returns the number of bytes processed if the APQI is valid; otherwise returns
  * one of the following errors:
@@ -394,10 +431,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;
@@ -414,6 +447,14 @@ static ssize_t assign_domain_store(struct device *dev,
 		return ret;
 	}
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
+
+	if (matrix_mdev->shadow_crycb) {
+		if (vfio_ap_queues_on_drv(matrix_mdev->shadow_crycb->apm,
+					  aqm)) {
+			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -431,7 +472,9 @@ static DEVICE_ATTR_WO(assign_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the APQI from @buf and clears the corresponding bit in the
- * mediated matrix device's AQM.
+ * mediated matrix device's AQM. If a guest is using the mediated matrix device
+ * and has access to queue(s) with the specified domain APQI, access to
+ * the queue(s) will be taken away from the guest.
  *
  * Returns the number of bytes processed if the APQI is valid; otherwise,
  * returns one of the following errors:
@@ -447,10 +490,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;
@@ -460,6 +499,13 @@ static ssize_t unassign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
+
+	if (matrix_mdev->shadow_crycb) {
+		if (test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm)) {
+			clear_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -475,7 +521,9 @@ static DEVICE_ATTR_WO(unassign_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the domain ID from @buf and sets the corresponding bit in the mediated
- * matrix device's ADM.
+ * matrix device's ADM. If a guest is using the mediated matrix device and the
+ * guest does not have access to the control domain with the specified ID, the
+ * guest will be granted access to it.
  *
  * Returns the number of bytes processed if the domain ID is valid; otherwise,
  * returns one of the following errors:
@@ -491,10 +539,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;
@@ -504,6 +548,13 @@ static ssize_t assign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	set_bit_inv(id, matrix_mdev->matrix.adm);
+
+	if (matrix_mdev->shadow_crycb) {
+		if (!test_bit_inv(id, matrix_mdev->shadow_crycb->adm)) {
+			set_bit_inv(id, matrix_mdev->shadow_crycb->adm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -519,7 +570,9 @@ static DEVICE_ATTR_WO(assign_control_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the domain ID from @buf and clears the corresponding bit in the
- * mediated matrix device's ADM.
+ * mediated matrix device's ADM. If a guest is using the mediated matrix device
+ * and has access to control domain with the specified domain ID, access to
+ * the control domain will be taken from the guest.
  *
  * Returns the number of bytes processed if the domain ID is valid; otherwise,
  * returns one of the following errors:
@@ -536,10 +589,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;
@@ -548,6 +597,13 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv(domid, matrix_mdev->matrix.adm);
+
+	if (matrix_mdev->shadow_crycb) {
+		if (test_bit_inv(domid, matrix_mdev->shadow_crycb->adm)) {
+			clear_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
-- 
2.7.4

[PATCH v2 6/7] s390: vfio-ap: handle bind and unbind of AP queue device

[Tony Krowiak]

There is nothing preventing a root user from inadvertently unbinding an
AP queue device that is in use by a guest from the vfio_ap device driver
and binding it to a zcrypt driver. This can result in a queue being
accessible from both the host and a guest.

This patch introduces safeguards that prevent sharing of an AP queue
between the host when a queue device is unbound from the vfio_ap device
driver. In addition, this patch restores guest access to AP queue devices
bound to the vfio_ap driver if the queue's APQN is assigned to an mdev
device in use by a guest.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     |  12 +++-
 drivers/s390/crypto/vfio_ap_ops.c     | 100 +++++++++++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |   2 +
 3 files changed, 111 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index e9824c35c34f..c215978daf39 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -42,12 +42,22 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
 
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_probe_queue(queue);
+	mutex_unlock(&matrix_dev->lock);
+
 	return 0;
 }
 
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-	/* Nothing to do yet */
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_remove_queue(queue);
+	mutex_unlock(&matrix_dev->lock);
 }
 
 static void vfio_ap_matrix_dev_release(struct device *dev)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ede45184eb67..40324951bd37 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -226,8 +226,6 @@ static struct device *vfio_ap_get_queue_dev(unsigned long apid,
 				  &apqn, match_apqn);
 }
 
-
-
 static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
 {
 	int ret;
@@ -259,6 +257,27 @@ static bool vfio_ap_queues_on_drv(unsigned long *apm, unsigned long *aqm)
 	return true;
 }
 
+static bool vfio_ap_card_on_drv(struct ap_queue *queue, unsigned long *aqm)
+{
+	unsigned long apid, apqi;
+	struct device *dev;
+
+	apid = AP_QID_CARD(queue->qid);
+
+	for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
+		if (queue->qid == AP_MKQID(apid, apqi))
+			continue;
+
+		dev = vfio_ap_get_queue_dev(apid, apqi);
+		if (!dev)
+			return false;
+
+		put_device(dev);
+	}
+
+	return true;
+}
+
 /**
  * assign_adapter_store
  *
@@ -1017,3 +1036,80 @@ void vfio_ap_mdev_unregister(void)
 {
 	mdev_unregister_device(&matrix_dev->device);
 }
+
+static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
+							    unsigned long apqi)
+{
+	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))
+			return matrix_mdev;
+	}
+
+	return NULL;
+}
+
+void vfio_ap_mdev_probe_queue(struct ap_queue *queue)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+	unsigned long *shadow_apm, *shadow_aqm;
+	unsigned long apid = AP_QID_CARD(queue->qid);
+	unsigned long apqi = AP_QID_QUEUE(queue->qid);
+
+	/*
+	 * Find the mdev device to which the APQN of the queue device being
+	 * probed is assigned
+	 */
+	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+	/* Check whether we found an mdev device and it is in use by a guest */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		shadow_apm = matrix_mdev->shadow_crycb->apm;
+		shadow_aqm = matrix_mdev->shadow_crycb->aqm;
+		/*
+		 * If the guest already has access to the adapter card
+		 * referenced by APID or does not have access to the queues
+		 * referenced by APQI, there is nothing to do here.
+		 */
+		if (test_bit_inv(apid, shadow_apm) ||
+		    !test_bit_inv(apqi, shadow_aqm))
+			return;
+
+		/*
+		 * If each APQN with the APID of the queue being probed and an
+		 * APQI in the shadow CRYCB references a queue device that is
+		 * bound to the vfio_ap driver, then plug the adapter into the
+		 * guest.
+		 */
+		if (vfio_ap_card_on_drv(queue, shadow_aqm)) {
+			set_bit_inv(apid, shadow_apm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
+}
+
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+	unsigned long apid = AP_QID_CARD(queue->qid);
+	unsigned long apqi = AP_QID_QUEUE(queue->qid);
+
+	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+	/*
+	 * If the queue is assigned to the mdev device and the mdev device
+	 * is in use by a guest, unplug the adapter referred to by the APID
+	 * of the APQN of the queue being removed.
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		if (!test_bit_inv(apid, matrix_mdev->shadow_crycb->apm))
+			return;
+
+		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+	}
+
+	vfio_ap_mdev_reset_queue(apid, apqi);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index e8457aa61976..6b1f7df5b979 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -87,5 +87,7 @@ struct ap_matrix_mdev {
 
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
+void vfio_ap_mdev_probe_queue(struct ap_queue *queue);
 
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v2 7/7] s390: vfio-ap: update documentation

[Tony Krowiak]

This patch updates the vfio-ap documentation to include the information
below.

Changes made to the mdev matrix assignment interfaces:

* We now allow assignment of APQNs that are not bound to the vfio-ap
  device driver

* We now use assignment interfaces to hot plug AP resources into a
  a running guest using the mdev matrix device assignment interfaces

* We now use unassignment interfaces to hot unplug AP resources from
  a running guest using the mdev matrix device unassignment interfaces

Changes made to the vfio_ap device driver probe and remove callbacks:

* We now plug the queue being bound to the vfio_ap into the running
  guest if the APQN of the queue is assigned to the mdev matrix device
  in use by the guest.

* If a guest is using the queue being unbound from the vfio_ap device
  driver, we now unplug the adapter card with the APID of the queue being
  unbound from the guest.

This patch also clarifies the section on configuring the AP bus's apmask
and aqmask.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 Documentation/s390/vfio-ap.txt | 191 +++++++++++++++++++++++++++--------------
 1 file changed, 127 insertions(+), 64 deletions(-)

diff --git a/Documentation/s390/vfio-ap.txt b/Documentation/s390/vfio-ap.txt
index 65167cfe4485..ca6aa8eeff8a 100644
--- a/Documentation/s390/vfio-ap.txt
+++ b/Documentation/s390/vfio-ap.txt
@@ -81,10 +81,19 @@ definitions:
   which the AP command is to be sent for processing.
 
   The AP bus will create a sysfs device for each APQN that can be derived from
-  the cross product of the AP adapter and usage domain numbers detected when the
-  AP bus module is loaded. For example, if adapters 4 and 10 (0x0a) and usage
-  domains 6 and 71 (0x47) are assigned to the LPAR, the AP bus will create the
-  following sysfs entries:
+  the Cartesian product of the AP adapter and usage domain numbers detected when
+  the AP bus module is loaded. For example, if adapters 4 and 10 (0x0a) and
+  usage domains 6 and 71 (0x47) are assigned to the LPAR, the Cartesian product
+  would be defined by the following table:
+
+		        06           71
+		   +-----------+-----------+
+		04 |  (04,06)  |  (04,47)  |
+		   +-----------|-----------+
+		10 |  (0a,06)  |  (0a,47)  |
+		   +-----------|-----------+
+
+  The AP bus will create the following sysfs entries:
 
     /sys/devices/ap/card04/04.0006
     /sys/devices/ap/card04/04.0047
@@ -146,10 +155,20 @@ If you recall from the description of an AP Queue, AP instructions include
 an APQN to identify the AP queue to which an AP command-request message is to be
 sent (NQAP and PQAP instructions), or from which a command-reply message is to
 be received (DQAP instruction). The validity of an APQN is defined by the matrix
-calculated from the APM and AQM; it is the cross product of all assigned adapter
-numbers (APM) with all assigned queue indexes (AQM). For example, if adapters 1
-and 2 and usage domains 5 and 6 are assigned to a guest, the APQNs (1,5), (1,6),
-(2,5) and (2,6) will be valid for the guest.
+calculated from the APM and AQM; it is the Cartesian product of all assigned
+adapter numbers (APM) with all assigned queue indexes (AQM). For example, if
+adapters 1 and 2 and usage domains 5 and 6 are assigned to a guest:
+
+
+		        05           06
+		   +-----------+-----------+
+		01 |  (01,05)  |  (01,06)  |
+		   +-----------|-----------+
+		02 |  (02,05)  |  (02,06)  |
+		   +-----------|-----------+
+
+
+APQNs (01,05), (01,06), (02,05) and (02,06) will be valid for the guest.
 
 The APQNs can provide secure key functionality - i.e., a private key is stored
 on the adapter card for each of its domains - so each APQN must be assigned to
@@ -349,8 +368,9 @@ matrix device.
       number of the the usage domain is echoed to the respective attribute
       file.
     * matrix:
-      A read-only file for displaying the APQNs derived from the cross product
-      of the adapter and domain numbers assigned to the mediated matrix device.
+      A read-only file for displaying the APQNs derived from the Caresian
+      product of the adapter and domain numbers assigned to the mediated matrix
+      device.
     * assign_control_domain:
     * unassign_control_domain:
       Write-only attributes for assigning/unassigning an AP control domain
@@ -513,35 +533,44 @@ These are the steps:
    /sys/bus/ap/aqmask
 
    The 'apmask' is a 256-bit mask that identifies a set of AP adapter IDs
-   (APID). Each bit in the mask, from left to right (i.e., from most significant
-   to least significant bit in big endian order), corresponds to an APID from
-   0-255. If a bit is set, the APID is marked as usable only by the default AP
-   queue device drivers; otherwise, the APID is usable by the vfio_ap
-   device driver.
+   (APID). Each bit in the mask, from left to right, corresponds to an APID from
+   0-255.
 
    The 'aqmask' is a 256-bit mask that identifies a set of AP queue indexes
-   (APQI). Each bit in the mask, from left to right (i.e., from most significant
-   to least significant bit in big endian order), corresponds to an APQI from
-   0-255. If a bit is set, the APQI is marked as usable only by the default AP
-   queue device drivers; otherwise, the APQI is usable by the vfio_ap device
-   driver.
+   (APQI). Each bit in the mask, from left to right, corresponds to an APQI from
+   0-255.
 
-   Take, for example, the following mask:
+   The Cartesian product of the APIDs set in the apmask and the APQIs set in
+   the aqmask identify the APQNs of AP queue devices owned by the zcrypt
+   device drivers.
 
-      0x7dffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
+   Take, for example, the following masks:
 
-    It indicates:
+     apmask: 0x0700000000000000000000000000000000000000000000000000000000000000
 
-      1, 2, 3, 4, 5, and 7-255 belong to the default drivers' pool, and 0 and 6
-      belong to the vfio_ap device driver's pool.
+     aqmask: 0x0180000000000000000000000000000000000000000000000000000000000000
 
-   The APQN of each AP queue device assigned to the linux host is checked by the
-   AP bus against the set of APQNs derived from the cross product of APIDs
-   and APQIs marked as usable only by the default AP queue device drivers. If a
-   match is detected,  only the default AP queue device drivers will be probed;
-   otherwise, the vfio_ap device driver will be probed.
+   The bits set in apmask are bits 1, 2 and 3. The bits set in aqmask are bits
+   7 and 8. The Cartesian product of the bits set in the two masks is:
 
-   By default, the two masks are set to reserve all APQNs for use by the default
+             07           08
+        +-----------+-----------+
+     01 |  (01,07)  |  (01,08)  |
+        +-----------|-----------+
+     02 |  (02,07)  |  (02,08)  |
+        +-----------|-----------+
+     03 |  (03,07)  |  (03,08)  |
+        +-----------|-----------+
+
+   The masks indicate that the queues with APQNs (01,07), (01,08), (02,07),
+   (02,08), (03,07) and (03,08) are owned by the zcrypt drivers. When the AP bus
+   detects an AP queue device, its APQN is checked against the set of APQNs
+   derived from the apmask and aqmask. If a match is detected, the zcrypt
+   device driver registered for the device type of the queue will be probed. If
+   a match is not detected and the device type of the queue is CEX4 or newer,
+   the vfio_ap device driver will be probed.
+
+   By default, the two masks are set to reserve all APQNs for use by the zcrypt
    AP queue device drivers. There are two ways the default masks can be changed:
 
    1. The sysfs mask files can be edited by echoing a string into the
@@ -554,8 +583,7 @@ These are the steps:
 
            0x4100000000000000000000000000000000000000000000000000000000000000
 
-        Keep in mind that the mask reads from left to right (i.e., most
-        significant to least significant bit in big endian order), so the mask
+        Keep in mind that the mask reads from left to right, so the mask
         above identifies device numbers 1 and 7 (01000001).
 
         If the string is longer than the mask, the operation is terminated with
@@ -563,7 +591,7 @@ These are the steps:
 
       * Individual bits in the mask can be switched on and off by specifying
         each bit number to be switched in a comma separated list. Each bit
-        number string must be prepended with a ('+') or minus ('-') to indicate
+        number string must be prefixed with a ('+') or minus ('-') to indicate
         the corresponding bit is to be switched on ('+') or off ('-'). Some
         valid values are:
 
@@ -594,11 +622,6 @@ These are the steps:
             aqmask:
             0x4000000000000000000000000000000000000000000000000000000000000000
 
-         Resulting in these two pools:
-
-            default drivers pool:    adapter 0-15, domain 1
-            alternate drivers pool:  adapter 16-255, domains 0, 2-255
-
    Securing the APQNs for our example:
    ----------------------------------
    To secure the AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004, 06.0047,
@@ -747,14 +770,13 @@ These are the steps:
      higher than the maximum is specified, the operation will terminate with
      an error (ENODEV).
 
-   * All APQNs that can be derived from the adapter ID and the IDs of
-     the previously assigned domains must be bound to the vfio_ap device
-     driver. If no domains have yet been assigned, then there must be at least
-     one APQN with the specified APID bound to the vfio_ap driver. If no such
-     APQNs are bound to the driver, the operation will terminate with an
-     error (EADDRNOTAVAIL).
+   * Each APQN that can be derived from the adapter ID and the IDs of
+     the previously assigned domains must not be reserved for use by the
+     zcrypt device drivers as specified by the /sys/bus/ap/apmask and
+     /sys/bus/ap/aqmask syfs interfaces. If any APQN is reserved, the operation
+     will terminate with an error (EADDRNOTAVAIL).
 
-     No APQN that can be derived from the adapter ID and the IDs of the
+   * No APQN that can be derived from the adapter ID and the IDs of the
      previously assigned domains can be assigned to another mediated matrix
      device. If an APQN is assigned to another mediated matrix device, the
      operation will terminate with an error (EADDRINUSE).
@@ -766,14 +788,13 @@ These are the steps:
      higher than the maximum is specified, the operation will terminate with
      an error (ENODEV).
 
-   * All APQNs that can be derived from the domain ID and the IDs of
-     the previously assigned adapters must be bound to the vfio_ap device
-     driver. If no domains have yet been assigned, then there must be at least
-     one APQN with the specified APQI bound to the vfio_ap driver. If no such
-     APQNs are bound to the driver, the operation will terminate with an
-     error (EADDRNOTAVAIL).
+   * Each APQN that can be derived from the domain ID and the IDs of
+     the previously assigned adapters must not be reserved for use by the
+     zcrypt device drivers as specified by the /sys/bus/ap/apmask and
+     /sys/bus/ap/aqmask syfs interfaces. If any APQN is reserved, the operation
+     will terminate with an error (EADDRNOTAVAIL).
 
-     No APQN that can be derived from the domain ID and the IDs of the
+   * No APQN that can be derived from the domain ID and the IDs of the
      previously assigned adapters can be assigned to another mediated matrix
      device. If an APQN is assigned to another mediated matrix device, the
      operation will terminate with an error (EADDRINUSE).
@@ -822,16 +843,58 @@ Using our example again, to remove the mediated matrix device $uuid1:
    host. If the mdev matrix device is removed, one may want to also reconfigure
    the pool of adapters and queues reserved for use by the default drivers.
 
-Limitations
-===========
-* The KVM/kernel interfaces do not provide a way to prevent restoring an APQN
-  to the default drivers pool of a queue that is still assigned to a mediated
-  device in use by a guest. It is incumbent upon the administrator to
-  ensure there is no mediated device in use by a guest to which the APQN is
-  assigned lest the host be given access to the private data of the AP queue
-  device such as a private key configured specifically for the guest.
+Hot plug/unplug via mdev matrix device sysfs interfaces:
+=======================================================
+If an mdev matrix device is in use by a running guest, AP resources can be
+plugged into or unplugged from the guest via the mdev device's sysfs
+assignment interfaces. Below are some examples.
+
+   To plug adapter 10 into a running guest:
+
+      echo 0xa > assign_adapter
+
+   To unplug domain 5 from a running guest:
+
+      echo 5 > unassign_domain
+
+To display the matrix of a guest using the mdev matrix device:
+
+   cat guest_matrix
+
+If you attempt to display the guest matrix when a guest is not using the
+mdev matrix device, an error will be displayed (ENODEV).
+
+Considerations for binding and unbinding AP queue devices:
+=========================================================
+There are considerations for binding AP queue devices to and unbinding AP queue
+devices from the vfio_ap device driver. Keep in mind that binding/unbinding of
+AP queue devices is controlled by the two AP bus sysfs interfaces:
+
+   /sys/bus/ap/apmask
+   /sys/bus/ap/aqmask
 
-* Dynamically modifying the AP matrix for a running guest (which would amount to
-  hot(un)plug of AP devices for the guest) is currently not supported
+Binding considerations:
+----------------------
+When an AP queue device is bound to the vfio_ap device driver and its APQN is
+assigned to an mdev matrix device in use by a running guest, the driver will
+hot plug the queue into the guest as long as each APQN that can be derived from
+the APID of the queue device and the APQIs of the queues already in use by the
+guest are bound to the vfio_ap device driver.
+
+Unbinding considerations:
+------------------------
+When an AP queue device being used by a running guest is unbound from the
+vfio_ap device driver, the adapter card to which the queue is connected will
+be unplugged from the guest.
+
+Note: The AP architecture does not provide a way to unplug an individual queue.
+
+Live migration:
+==============
+Live guest migration is not supported for guests using AP devices. All AP
+devices in use by the guest must be unplugged prior to initiating live
+migration (see "Hot plug/unplug via mdev matrix device sysfs interfaces" section
+above). If you are using QEMU to run your guest and it supports hot plug/unplug
+of the vfio-ap device, this would be another option (consult the QEMU
+documentation for details).
 
-* Live guest migration is not supported for guests using AP devices.
-- 
2.7.4

Re: [PATCH v2 1/7] s390: vfio-ap: wait for queue empty on queue reset

[Pierre Morel]

On 03/05/2019 23:14, Tony Krowiak wrote:
> Refactors the AP queue reset function to wait until the queue is empty
> after the PQAP(ZAPQ) instruction is executed to zero out the queue as
> required by the AP architecture.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 35 ++++++++++++++++++++++++++++++++---
>   1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 900b9cf20ca5..b88a2a2ba075 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -271,6 +271,32 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
>   	return 0;
>   }
>   
> +static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
> +{
> +	struct ap_queue_status status;
> +	ap_qid_t qid = AP_MKQID(apid, apqi);
> +	int retry = 5;
> +
> +	do {
> +		status = ap_tapq(qid, NULL);
> +		switch (status.response_code) {
> +		case AP_RESPONSE_NORMAL:
> +			if (status.queue_empty)
> +				return;
> +			msleep(20);

NIT: 	Fall through ?

> +			break;
> +		case AP_RESPONSE_RESET_IN_PROGRESS:
> +		case AP_RESPONSE_BUSY:
> +			msleep(20);
> +			break;
> +		default:
> +			pr_warn("%s: tapq err %02x: %04lx.%02lx may not be empty\n",
> +				__func__, status.response_code, apid, apqi);

I do not thing the warning sentence is appropriate:
The only possible errors here are if the AP is not available due to AP 
checkstop, deconfigured AP or invalid APQN.


> +			return;
> +		}
> +	} while (--retry);
> +}
> +
>   /**
>    * assign_adapter_store
>    *
> @@ -790,15 +816,18 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>   	return NOTIFY_OK;
>   }
>   
> -static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
> -				    unsigned int retry)
> +int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
>   {
>   	struct ap_queue_status status;
> +	int retry = 5;
>   
>   	do {
>   		status = ap_zapq(AP_MKQID(apid, apqi));
>   		switch (status.response_code) {
>   		case AP_RESPONSE_NORMAL:
> +			vfio_ap_mdev_wait_for_qempty(apid, apqi);
> +			return 0;
> +		case AP_RESPONSE_DECONFIGURED:

Since you modify the switch, you can return for all the following cases:
AP_RESPONSE_DECONFIGURE
..._CHECKSTOP
..._INVALID_APQN


And you should wait for qempty on AP_RESET_IN_PROGRESS along with 
AP_RESPONSE_NORMAL

>   			return 0;
>   		case AP_RESPONSE_RESET_IN_PROGRESS:
>   		case AP_RESPONSE_BUSY:

While at modifying this function, the AP_RESPONSE_BUSY is not a valid 
code for ZAPQ, you can remove this.

> @@ -824,7 +853,7 @@ 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) {
> -			ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
> +			ret = vfio_ap_mdev_reset_queue(apid, apqi);

IMHO, since you are at changing this call, passing the apqn as parameter 
would be a good simplification.



>   			/*
>   			 * Regardless whether a queue turns out to be busy, or
>   			 * is not operational, we need to continue resetting

Depends on why the reset failed, but this is out of scope.

> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 2/7] s390: vfio-ap: maintain a shadow of the guest's CRYCB

[Pierre Morel]

On 03/05/2019 23:14, Tony Krowiak wrote:
> This patch introduces a shadow of the CRYCB being used by a guest. This
> will enable to more effectively manage dynamic changes to the AP
> resources installed on the host that may be assigned to an mdev device
> and being used by a guest. For example:
> 
> * AP adapter cards can be dynamically added to and removed from the AP
>    configuration via the SE or an SCLP command.
> 
> * AP resources that disappear and reappear due to hardware malfunctions.
> 
> * AP queues bound to and unbound from the vfio_ap device driver by a
>    root user.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c     | 91 ++++++++++++++++++++++++++++++++---
>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>   2 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index b88a2a2ba075..44a04b4aa9ae 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -297,6 +297,45 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
>   	} while (--retry);
>   }
>   
> +/*
> + * vfio_ap_mdev_update_crycb
> + *
> + * @matrix_mdev: the mediated matrix device
> + *
> + * Updates the AP matrix in the guest's CRYCB from it's shadow masks.
> + *
> + * Returns zero if the guest's CRYCB is successfully updated; otherwise,
> + * returns -ENODEV if a guest is not running or does not have a CRYCB.
> + */
> +static int vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	if (!matrix_mdev->kvm || !matrix_mdev->kvm->arch.crypto.crycbd)
> +		return -ENODEV;
> +
> +	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
> +				  matrix_mdev->shadow_crycb->apm,
> +				  matrix_mdev->shadow_crycb->aqm,
> +				  matrix_mdev->shadow_crycb->adm);
> +
> +	return 0;
> +}
> +
> +static int match_apqn(struct device *dev, void *data)
> +{
> +	struct ap_queue *apq = to_ap_queue(dev);
> +
> +	return (apq->qid == *(unsigned long *)(data)) ? 1 : 0;
> +}
> +
> +static struct device *vfio_ap_get_queue_dev(unsigned long apid,
> +					     unsigned long apqi)
> +{
> +	unsigned long apqn = AP_MKQID(apid, apqi);
> +
> +	return driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> +				  &apqn, match_apqn);
> +}
> +
>   /**
>    * assign_adapter_store
>    *
> @@ -805,14 +844,9 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>   	if (ret)
>   		return NOTIFY_DONE;
>   
> -	/* If there is no CRYCB pointer, then we can't copy the masks */
> -	if (!matrix_mdev->kvm->arch.crypto.crycbd)
> +	if (vfio_ap_mdev_update_crycb(matrix_mdev))
>   		return NOTIFY_DONE;
>   
> -	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
> -				  matrix_mdev->matrix.aqm,
> -				  matrix_mdev->matrix.adm);
> -
>   	return NOTIFY_OK;
>   }
>   
> @@ -867,12 +901,55 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>   	return rc;
>   }
>   
> +static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	unsigned long apid, apqi, domid;
> +	struct device *dev;
> +
> +	matrix_mdev->shadow_crycb = kzalloc(sizeof(*matrix_mdev->shadow_crycb),
> +					    GFP_KERNEL);
> +	if (!matrix_mdev->shadow_crycb)
> +		return -ENOMEM;
> +
> +	vfio_ap_matrix_init(&matrix_dev->info, matrix_mdev->shadow_crycb);
> +
> +	/*
> +	 * Examine each APQN assigned to the mdev device. Set the APID and APQI
> +	 * in the shadow CRYCB if and only if the queue device identified by
> +	 * the APQN is in the configuration.
> +	 */
> +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
> +			     matrix_mdev->matrix.apm_max + 1) {
> +		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
> +				     matrix_mdev->matrix.aqm_max + 1) {
> +			dev = vfio_ap_get_queue_dev(apid, apqi);
> +			if (dev) {
> +				set_bit_inv(apid,
> +					    matrix_mdev->shadow_crycb->apm);
> +				set_bit_inv(apqi,
> +					    matrix_mdev->shadow_crycb->aqm);
> +				put_device(dev);
> +			}

I think that if we do not find a device here we have a problem.
Don't we?


> +		}
> +	}
> +
> +	/* Set all control domains assigned to the mdev in the shadow CRYCB */
> +	for_each_set_bit_inv(domid, matrix_mdev->matrix.adm,
> +			     matrix_mdev->matrix.adm_max + 1)
> +		set_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
> +
> +	return 0;
> +}
> +
>   static int vfio_ap_mdev_open(struct mdev_device *mdev)
>   {
>   	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>   	unsigned long events;
>   	int ret;
>   
> +	ret = vfio_ap_mdev_create_shadow_crycb(matrix_mdev);
> +	if (ret)
> +		return ret;
>   
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
> @@ -902,6 +979,8 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>   				 &matrix_mdev->group_notifier);
>   	matrix_mdev->kvm = NULL;
>   	module_put(THIS_MODULE);
> +	kfree(matrix_mdev->shadow_crycb);
> +	matrix_mdev->shadow_crycb = NULL;
>   }
>   
>   static int vfio_ap_mdev_get_device_info(unsigned long arg)
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 76b7f98e47e9..e8457aa61976 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -72,6 +72,7 @@ struct ap_matrix {
>    * @list:	allows the ap_matrix_mdev struct to be added to a list
>    * @matrix:	the adapters, usage domains and control domains assigned to the
>    *		mediated matrix device.
> + * @shadow_crycb: a shadow copy of the crycb in use by a guest
>    * @group_notifier: notifier block used for specifying callback function for
>    *		    handling the VFIO_GROUP_NOTIFY_SET_KVM event
>    * @kvm:	the struct holding guest's state
> @@ -79,6 +80,7 @@ struct ap_matrix {
>   struct ap_matrix_mdev {
>   	struct list_head node;
>   	struct ap_matrix matrix;
> +	struct ap_matrix *shadow_crycb;
>   	struct notifier_block group_notifier;
>   	struct kvm *kvm;
>   };
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 3/7] s390: vfio-ap: sysfs interface to display guest CRYCB

[Pierre Morel]

On 03/05/2019 23:14, Tony Krowiak wrote:
> Introduces a sysfs interface on the matrix mdev device to display the
> contents of the shadow of the guest's CRYCB
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 59 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44a04b4aa9ae..1021466cb661 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -771,6 +771,64 @@ static ssize_t matrix_show(struct device *dev, struct device_attribute *attr,
>   }
>   static DEVICE_ATTR_RO(matrix);
>   
> +static ssize_t guest_matrix_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct mdev_device *mdev = mdev_from_dev(dev);
> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> +	char *bufpos = buf;
> +	unsigned long apid;
> +	unsigned long apqi;
> +	unsigned long apid1;
> +	unsigned long apqi1;
> +	unsigned long napm_bits;
> +	unsigned long naqm_bits;
> +	int nchars = 0;
> +	int n;
> +
> +	if (!matrix_mdev->shadow_crycb)
> +		return -ENODEV;
> +
> +	mutex_lock(&matrix_dev->lock);
> +	napm_bits = matrix_mdev->shadow_crycb->apm_max + 1;
> +	naqm_bits = matrix_mdev->shadow_crycb->aqm_max + 1;
> +	apid1 = find_first_bit_inv(matrix_mdev->shadow_crycb->apm, napm_bits);
> +	apqi1 = find_first_bit_inv(matrix_mdev->shadow_crycb->aqm, naqm_bits);
> +
> +	if ((apid1 < napm_bits) && (apqi1 < naqm_bits)) {
> +		for_each_set_bit_inv(apid, matrix_mdev->shadow_crycb->apm,
> +				     napm_bits) {
> +			for_each_set_bit_inv(apqi,
> +					     matrix_mdev->shadow_crycb->aqm,
> +					     naqm_bits) {
> +				n = sprintf(bufpos, "%02lx.%04lx\n", apid,
> +					    apqi);
> +				bufpos += n;
> +				nchars += n;
> +			}
> +		}
> +	} else if (apid1 < napm_bits) {
> +		for_each_set_bit_inv(apid, matrix_mdev->shadow_crycb->apm,
> +				     napm_bits) {
> +			n = sprintf(bufpos, "%02lx.\n", apid);
> +			bufpos += n;
> +			nchars += n;
> +		}
> +	} else if (apqi1 < naqm_bits) {
> +		for_each_set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm,
> +				     naqm_bits) {
> +			n = sprintf(bufpos, ".%04lx\n", apqi);
> +			bufpos += n;
> +			nchars += n;
> +		}
> +	}
> +
> +	mutex_unlock(&matrix_dev->lock);
> +
> +	return nchars;
> +}
> +static DEVICE_ATTR_RO(guest_matrix);
> +
>   static struct attribute *vfio_ap_mdev_attrs[] = {
>   	&dev_attr_assign_adapter.attr,
>   	&dev_attr_unassign_adapter.attr,
> @@ -780,6 +838,7 @@ static struct attribute *vfio_ap_mdev_attrs[] = {
>   	&dev_attr_unassign_control_domain.attr,
>   	&dev_attr_control_domains.attr,
>   	&dev_attr_matrix.attr,
> +	&dev_attr_guest_matrix.attr,
>   	NULL,
>   };
>   
> 

Code seems very similar to matrix_show, can't you share the code?





-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 4/7] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device

[Pierre Morel]

On 03/05/2019 23:14, Tony Krowiak wrote:
> The AP architecture does not preclude assignment of AP resources that are
> not yet in the AP configuration (i.e., not available or not online).
> Let's go ahead and implement this facet of the AP architecture for linux
> guests.
> 
> The current implementation does not allow assignment of AP resources to
> an mdev device if the AP queue devices identified by the assignment are
> not bound to the vfio_ap device driver. This patch allows assignment of AP
> resources to the mdev device even if the AP queue devices are not bound to
> the vfio_ap device driver, as long as the AP queue devices are not
> reserved by the AP BUS for use by the zcrypt device drivers.

or another mediated device.


> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 231 ++++++++------------------------------
>   1 file changed, 44 insertions(+), 187 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 1021466cb661..ea24caf17a16 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -113,122 +113,6 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
>   	NULL,
>   };
>   
> -struct vfio_ap_queue_reserved {
> -	unsigned long *apid;
> -	unsigned long *apqi;
> -	bool reserved;
> -};
> -
> -/**
> - * vfio_ap_has_queue
> - *
> - * @dev: an AP queue device
> - * @data: a struct vfio_ap_queue_reserved reference
> - *
> - * Flags whether the AP queue device (@dev) has a queue ID containing the APQN,
> - * apid or apqi specified in @data:
> - *
> - * - If @data contains both an apid and apqi value, then @data will be flagged
> - *   as reserved if the APID and APQI fields for the AP queue device matches
> - *
> - * - If @data contains only an apid value, @data will be flagged as
> - *   reserved if the APID field in the AP queue device matches
> - *
> - * - If @data contains only an apqi value, @data will be flagged as
> - *   reserved if the APQI field in the AP queue device matches
> - *
> - * Returns 0 to indicate the input to function succeeded. Returns -EINVAL if
> - * @data does not contain either an apid or apqi.
> - */
> -static int vfio_ap_has_queue(struct device *dev, void *data)
> -{
> -	struct vfio_ap_queue_reserved *qres = data;
> -	struct ap_queue *ap_queue = to_ap_queue(dev);
> -	ap_qid_t qid;
> -	unsigned long id;
> -
> -	if (qres->apid && qres->apqi) {
> -		qid = AP_MKQID(*qres->apid, *qres->apqi);
> -		if (qid == ap_queue->qid)
> -			qres->reserved = true;
> -	} else if (qres->apid && !qres->apqi) {
> -		id = AP_QID_CARD(ap_queue->qid);
> -		if (id == *qres->apid)
> -			qres->reserved = true;
> -	} else if (!qres->apid && qres->apqi) {
> -		id = AP_QID_QUEUE(ap_queue->qid);
> -		if (id == *qres->apqi)
> -			qres->reserved = true;
> -	} else {
> -		return -EINVAL;
> -	}
> -
> -	return 0;
> -}
> -
> -/**
> - * vfio_ap_verify_queue_reserved
> - *
> - * @matrix_dev: a mediated matrix device
> - * @apid: an AP adapter ID
> - * @apqi: an AP queue index
> - *
> - * Verifies that the AP queue with @apid/@apqi is reserved by the VFIO AP device
> - * driver according to the following rules:
> - *
> - * - If both @apid and @apqi are not NULL, then there must be an AP queue
> - *   device bound to the vfio_ap driver with the APQN identified by @apid and
> - *   @apqi
> - *
> - * - If only @apid is not NULL, then there must be an AP queue device bound
> - *   to the vfio_ap driver with an APQN containing @apid
> - *
> - * - If only @apqi is not NULL, then there must be an AP queue device bound
> - *   to the vfio_ap driver with an APQN containing @apqi
> - *
> - * Returns 0 if the AP queue is reserved; otherwise, returns -EADDRNOTAVAIL.
> - */
> -static int vfio_ap_verify_queue_reserved(unsigned long *apid,
> -					 unsigned long *apqi)
> -{
> -	int ret;
> -	struct vfio_ap_queue_reserved qres;
> -
> -	qres.apid = apid;
> -	qres.apqi = apqi;
> -	qres.reserved = false;
> -
> -	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> -				     &qres, vfio_ap_has_queue);
> -	if (ret)
> -		return ret;
> -
> -	if (qres.reserved)
> -		return 0;
> -
> -	return -EADDRNOTAVAIL;
> -}
> -
> -static int
> -vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
> -					     unsigned long apid)
> -{
> -	int ret;
> -	unsigned long apqi;
> -	unsigned long nbits = matrix_mdev->matrix.aqm_max + 1;
> -
> -	if (find_first_bit_inv(matrix_mdev->matrix.aqm, nbits) >= nbits)
> -		return vfio_ap_verify_queue_reserved(&apid, NULL);
> -
> -	for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, nbits) {
> -		ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
> -}
> -
>   /**
>    * vfio_ap_mdev_verify_no_sharing
>    *
> @@ -236,18 +120,26 @@ vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
>    * and AP queue indexes comprising the AP matrix are not configured for another
>    * mediated device. AP queue sharing is not allowed.
>    *
> - * @matrix_mdev: the mediated matrix device
> + * @mdev_apm: the mask identifying the adapters assigned to mdev
> + * @mdev_apm: the mask identifying the adapters assigned to mdev
>    *
>    * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
>    */
> -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
> +static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
> +					  unsigned long *mdev_aqm)
>   {
>   	struct ap_matrix_mdev *lstdev;
>   	DECLARE_BITMAP(apm, AP_DEVICES);
>   	DECLARE_BITMAP(aqm, AP_DOMAINS);
>   
>   	list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
> -		if (matrix_mdev == lstdev)
> +		/*
> +		 * If either of the input masks belongs to the mdev to which an
> +		 * AP resource is being assigned, then we don't need to verify
> +		 * that mdev's masks.
> +		 */
> +		if ((mdev_apm == lstdev->matrix.apm) ||
> +		    (mdev_aqm == lstdev->matrix.aqm))
>   			continue;

Is it possible that mdev_apm and mdev_aqm do not belong to the same 
mediated device?

...snip...


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 5/7] s390: vfio-ap: allow hot plug/unplug of AP resources using mdev device

[Pierre Morel]

On 03/05/2019 23:14, Tony Krowiak wrote:
> Let's allow AP resources to be assigned to or unassigned from an AP matrix
> mdev device while it is in use by a guest. If a guest is using the mdev
> device while assignment is taking place, the guest will be granted access
> to the resource as long as the guest will not be given access to an AP
> queue device that is not bound to the vfio_ap device driver. If a guest is
> using the mdev device while unassignment is taking place, access to the
> resource will be taken from the guest.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 116 ++++++++++++++++++++++++++++----------
>   1 file changed, 86 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ea24caf17a16..ede45184eb67 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -226,6 +226,8 @@ static struct device *vfio_ap_get_queue_dev(unsigned long apid,
>   				  &apqn, match_apqn);
>   }
>   
> +
> +

two white lines

>   static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
>   {
>   	int ret;
> @@ -237,6 +239,26 @@ static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
>   	return vfio_ap_mdev_verify_no_sharing(apm, aqm);
>   }
>   
> +static bool vfio_ap_queues_on_drv(unsigned long *apm, unsigned long *aqm)
> +{
> +	unsigned long apid, apqi, apqn;
> +	struct device *dev;
> +
> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> +			apqn = AP_MKQID(apid, apqi);

You do not use apqn in the function.

> +
> +			dev = vfio_ap_get_queue_dev(apid, apqi);
> +			if (!dev)
> +				return false;
> +
> +			put_device(dev);
> +		}
> +	}
> +
> +	return true;
> +}
> +
>   /**
>    * assign_adapter_store
>    *
> @@ -247,7 +269,10 @@ static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
>    * @count:	the number of bytes in @buf
>    *
>    * Parses the APID from @buf and sets the corresponding bit in the mediated
> - * matrix device's APM.
> + * matrix device's APM. If a guest is using the mediated matrix device and each
> + * new APQN formed as a result of the assignment identifies an AP queue device
> + * that is bound to the vfio_ap device driver, the guest will be granted access
> + * to the adapter with the specified APID.
>    *
>    * Returns the number of bytes processed if the APID is valid; otherwise,
>    * returns one of the following errors:
> @@ -279,10 +304,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;
> @@ -300,6 +321,14 @@ static ssize_t assign_adapter_store(struct device *dev,
>   		return ret;
>   	}
>   	set_bit_inv(apid, matrix_mdev->matrix.apm);
> +
> +	if (matrix_mdev->shadow_crycb) {
> +		if (vfio_ap_queues_on_drv(apm,
> +					  matrix_mdev->shadow_crycb->aqm)) {
> +			set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
>   	mutex_unlock(&matrix_dev->lock);
>   
>   	return count;
> @@ -315,7 +344,9 @@ static DEVICE_ATTR_WO(assign_adapter);
>    * @count:	the number of bytes in @buf
>    *
>    * Parses the APID from @buf and clears the corresponding bit in the mediated
> - * matrix device's APM.
> + * matrix device's APM. If a guest is using the mediated matrix device and has
> + * access to the AP adapter with the specified APID, access to the adapter will
> + * be taken from the guest.
>    *
>    * Returns the number of bytes processed if the APID is valid; otherwise,
>    * returns one of the following errors:
> @@ -332,10 +363,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;
> @@ -345,6 +372,13 @@ static ssize_t unassign_adapter_store(struct device *dev,
>   
>   	mutex_lock(&matrix_dev->lock);
>   	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
> +
> +	if (matrix_mdev->shadow_crycb) {
> +		if (test_bit_inv(apid, matrix_mdev->shadow_crycb->apm)) {
> +			clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
>   	mutex_unlock(&matrix_dev->lock);
>   
>   	return count;
> @@ -361,7 +395,10 @@ static DEVICE_ATTR_WO(unassign_adapter);
>    * @count:	the number of bytes in @buf
>    *
>    * Parses the APQI from @buf and sets the corresponding bit in the mediated
> - * matrix device's AQM.
> + * matrix device's AQM. If a guest is using the mediated matrix device and each
> + * new APQN formed as a result of the assignment identifies an AP queue device
> + * that is bound to the vfio_ap device driver, the guest will be given access
> + * to the AP queue(s) with the specified APQI.
>    *
>    * Returns the number of bytes processed if the APQI is valid; otherwise returns
>    * one of the following errors:
> @@ -394,10 +431,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;
> @@ -414,6 +447,14 @@ static ssize_t assign_domain_store(struct device *dev,
>   		return ret;
>   	}
>   	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
> +
> +	if (matrix_mdev->shadow_crycb) {
> +		if (vfio_ap_queues_on_drv(matrix_mdev->shadow_crycb->apm,
> +					  aqm)) {
> +			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
>   	mutex_unlock(&matrix_dev->lock);
>   
>   	return count;
> @@ -431,7 +472,9 @@ static DEVICE_ATTR_WO(assign_domain);
>    * @count:	the number of bytes in @buf
>    *
>    * Parses the APQI from @buf and clears the corresponding bit in the
> - * mediated matrix device's AQM.
> + * mediated matrix device's AQM. If a guest is using the mediated matrix device
> + * and has access to queue(s) with the specified domain APQI, access to
> + * the queue(s) will be taken away from the guest.
>    *
>    * Returns the number of bytes processed if the APQI is valid; otherwise,
>    * returns one of the following errors:
> @@ -447,10 +490,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;
> @@ -460,6 +499,13 @@ static ssize_t unassign_domain_store(struct device *dev,
>   
>   	mutex_lock(&matrix_dev->lock);
>   	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
> +
> +	if (matrix_mdev->shadow_crycb) {
> +		if (test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm)) {
> +			clear_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
>   	mutex_unlock(&matrix_dev->lock);
>   
>   	return count;
> @@ -475,7 +521,9 @@ static DEVICE_ATTR_WO(unassign_domain);
>    * @count:	the number of bytes in @buf
>    *
>    * Parses the domain ID from @buf and sets the corresponding bit in the mediated
> - * matrix device's ADM.
> + * matrix device's ADM. If a guest is using the mediated matrix device and the
> + * guest does not have access to the control domain with the specified ID, the
> + * guest will be granted access to it.
>    *
>    * Returns the number of bytes processed if the domain ID is valid; otherwise,
>    * returns one of the following errors:
> @@ -491,10 +539,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;
> @@ -504,6 +548,13 @@ static ssize_t assign_control_domain_store(struct device *dev,
>   
>   	mutex_lock(&matrix_dev->lock);
>   	set_bit_inv(id, matrix_mdev->matrix.adm);
> +
> +	if (matrix_mdev->shadow_crycb) {
> +		if (!test_bit_inv(id, matrix_mdev->shadow_crycb->adm)) {
> +			set_bit_inv(id, matrix_mdev->shadow_crycb->adm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
>   	mutex_unlock(&matrix_dev->lock);
>   
>   	return count;
> @@ -519,7 +570,9 @@ static DEVICE_ATTR_WO(assign_control_domain);
>    * @count:	the number of bytes in @buf
>    *
>    * Parses the domain ID from @buf and clears the corresponding bit in the
> - * mediated matrix device's ADM.
> + * mediated matrix device's ADM. If a guest is using the mediated matrix device
> + * and has access to control domain with the specified domain ID, access to
> + * the control domain will be taken from the guest.
>    *
>    * Returns the number of bytes processed if the domain ID is valid; otherwise,
>    * returns one of the following errors:
> @@ -536,10 +589,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;
> @@ -548,6 +597,13 @@ static ssize_t unassign_control_domain_store(struct device *dev,
>   
>   	mutex_lock(&matrix_dev->lock);
>   	clear_bit_inv(domid, matrix_mdev->matrix.adm);
> +
> +	if (matrix_mdev->shadow_crycb) {
> +		if (test_bit_inv(domid, matrix_mdev->shadow_crycb->adm)) {
> +			clear_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
>   	mutex_unlock(&matrix_dev->lock);
>   
>   	return count;
> 

beside the two NITs, look good to me.
Still need to test.



-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 6/7] s390: vfio-ap: handle bind and unbind of AP queue device

[Pierre Morel]

On 03/05/2019 23:14, Tony Krowiak wrote:
> There is nothing preventing a root user from inadvertently unbinding an
> AP queue device that is in use by a guest from the vfio_ap device driver
> and binding it to a zcrypt driver. This can result in a queue being
> accessible from both the host and a guest.
> 
> This patch introduces safeguards that prevent sharing of an AP queue
> between the host when a queue device is unbound from the vfio_ap device
> driver. In addition, this patch restores guest access to AP queue devices
> bound to the vfio_ap driver if the queue's APQN is assigned to an mdev
> device in use by a guest.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     |  12 +++-
>   drivers/s390/crypto/vfio_ap_ops.c     | 100 +++++++++++++++++++++++++++++++++-
>   drivers/s390/crypto/vfio_ap_private.h |   2 +
>   3 files changed, 111 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index e9824c35c34f..c215978daf39 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -42,12 +42,22 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>   
>   static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   {
> +	struct ap_queue *queue = to_ap_queue(&apdev->device);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	vfio_ap_mdev_probe_queue(queue);
> +	mutex_unlock(&matrix_dev->lock);
> +
>   	return 0;
>   }
>   
>   static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>   {
> -	/* Nothing to do yet */
> +	struct ap_queue *queue = to_ap_queue(&apdev->device);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	vfio_ap_mdev_remove_queue(queue);
> +	mutex_unlock(&matrix_dev->lock);
>   }
>   
>   static void vfio_ap_matrix_dev_release(struct device *dev)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ede45184eb67..40324951bd37 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -226,8 +226,6 @@ static struct device *vfio_ap_get_queue_dev(unsigned long apid,
>   				  &apqn, match_apqn);
>   }
>   
> -
> -
>   static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned long *aqm)
>   {
>   	int ret;
> @@ -259,6 +257,27 @@ static bool vfio_ap_queues_on_drv(unsigned long *apm, unsigned long *aqm)
>   	return true;
>   }
>   
> +static bool vfio_ap_card_on_drv(struct ap_queue *queue, unsigned long *aqm)
> +{
> +	unsigned long apid, apqi;
> +	struct device *dev;
> +
> +	apid = AP_QID_CARD(queue->qid);
> +
> +	for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> +		if (queue->qid == AP_MKQID(apid, apqi))
> +			continue;
> +
> +		dev = vfio_ap_get_queue_dev(apid, apqi);
> +		if (!dev)
> +			return false;
> +
> +		put_device(dev);
> +	}
> +
> +	return true;
> +}
> +
>   /**
>    * assign_adapter_store
>    *
> @@ -1017,3 +1036,80 @@ void vfio_ap_mdev_unregister(void)
>   {
>   	mdev_unregister_device(&matrix_dev->device);
>   }
> +
> +static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
> +							    unsigned long apqi)
> +{
> +	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))
> +			return matrix_mdev;
> +	}
> +
> +	return NULL;
> +}
> +
> +void vfio_ap_mdev_probe_queue(struct ap_queue *queue)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +	unsigned long *shadow_apm, *shadow_aqm;
> +	unsigned long apid = AP_QID_CARD(queue->qid);
> +	unsigned long apqi = AP_QID_QUEUE(queue->qid);
> +
> +	/*
> +	 * Find the mdev device to which the APQN of the queue device being
> +	 * probed is assigned
> +	 */
> +	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
> +
> +	/* Check whether we found an mdev device and it is in use by a guest */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		shadow_apm = matrix_mdev->shadow_crycb->apm;
> +		shadow_aqm = matrix_mdev->shadow_crycb->aqm;
> +		/*
> +		 * If the guest already has access to the adapter card
> +		 * referenced by APID or does not have access to the queues
> +		 * referenced by APQI, there is nothing to do here.
> +		 */
> +		if (test_bit_inv(apid, shadow_apm) ||
> +		    !test_bit_inv(apqi, shadow_aqm))
> +			return;
> +
> +		/*
> +		 * If each APQN with the APID of the queue being probed and an
> +		 * APQI in the shadow CRYCB references a queue device that is
> +		 * bound to the vfio_ap driver, then plug the adapter into the
> +		 * guest.
> +		 */
> +		if (vfio_ap_card_on_drv(queue, shadow_aqm)) {
> +			set_bit_inv(apid, shadow_apm);
> +			vfio_ap_mdev_update_crycb(matrix_mdev);
> +		}
> +	}
> +}
> +
> +void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +	unsigned long apid = AP_QID_CARD(queue->qid);
> +	unsigned long apqi = AP_QID_QUEUE(queue->qid);
> +
> +	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
> +
> +	/*
> +	 * If the queue is assigned to the mdev device and the mdev device
> +	 * is in use by a guest, unplug the adapter referred to by the APID
> +	 * of the APQN of the queue being removed.
> +	 */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		if (!test_bit_inv(apid, matrix_mdev->shadow_crycb->apm))
> +			return;
> +
> +		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +		vfio_ap_mdev_update_crycb(matrix_mdev);
> +	}
> +
> +	vfio_ap_mdev_reset_queue(apid, apqi);
> +}
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index e8457aa61976..6b1f7df5b979 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -87,5 +87,7 @@ struct ap_matrix_mdev {
>   
>   extern int vfio_ap_mdev_register(void);
>   extern void vfio_ap_mdev_unregister(void);
> +void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
> +void vfio_ap_mdev_probe_queue(struct ap_queue *queue);
>   
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 


AFAIU the apmask/aqmask of the AP_BUS are replacing bind/unbind for the 
admin. Don't they?
Then why not suppress bind/unbind for ap_queues?

Otherwise, it seems to me to handle correctly the disappearance of a 
card, which is the only thing that can happen from out of the firmware 
queue change requires configuration change and re-IPL.

Even still need testing, LGTM


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 1/7] s390: vfio-ap: wait for queue empty on queue reset

[Tony Krowiak]

On 5/6/19 2:41 AM, Pierre Morel wrote:
> On 03/05/2019 23:14, Tony Krowiak wrote:
>> Refactors the AP queue reset function to wait until the queue is empty
>> after the PQAP(ZAPQ) instruction is executed to zero out the queue as
>> required by the AP architecture.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 35 
>> ++++++++++++++++++++++++++++++++---
>>   1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 900b9cf20ca5..b88a2a2ba075 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -271,6 +271,32 @@ static int vfio_ap_mdev_verify_no_sharing(struct 
>> ap_matrix_mdev *matrix_mdev)
>>       return 0;
>>   }
>> +static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned 
>> long apqi)
>> +{
>> +    struct ap_queue_status status;
>> +    ap_qid_t qid = AP_MKQID(apid, apqi);
>> +    int retry = 5;
>> +
>> +    do {
>> +        status = ap_tapq(qid, NULL);
>> +        switch (status.response_code) {
>> +        case AP_RESPONSE_NORMAL:
>> +            if (status.queue_empty)
>> +                return;
>> +            msleep(20);
> 
> NIT:     Fall through ?

Yes

> 
>> +            break;
>> +        case AP_RESPONSE_RESET_IN_PROGRESS:
>> +        case AP_RESPONSE_BUSY:
>> +            msleep(20);
>> +            break;
>> +        default:
>> +            pr_warn("%s: tapq err %02x: %04lx.%02lx may not be empty\n",
>> +                __func__, status.response_code, apid, apqi);
> 
> I do not thing the warning sentence is appropriate:
> The only possible errors here are if the AP is not available due to AP 
> checkstop, deconfigured AP or invalid APQN.

Right you are! I'll work on a new message.

> 
> 
>> +            return;
>> +        }
>> +    } while (--retry);
>> +}
>> +
>>   /**
>>    * assign_adapter_store
>>    *
>> @@ -790,15 +816,18 @@ static int vfio_ap_mdev_group_notifier(struct 
>> notifier_block *nb,
>>       return NOTIFY_OK;
>>   }
>> -static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int 
>> apqi,
>> -                    unsigned int retry)
>> +int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
>>   {
>>       struct ap_queue_status status;
>> +    int retry = 5;
>>       do {
>>           status = ap_zapq(AP_MKQID(apid, apqi));
>>           switch (status.response_code) {
>>           case AP_RESPONSE_NORMAL:
>> +            vfio_ap_mdev_wait_for_qempty(apid, apqi);
>> +            return 0;
>> +        case AP_RESPONSE_DECONFIGURED:
> 
> Since you modify the switch, you can return for all the following cases:
> AP_RESPONSE_DECONFIGURE
> ..._CHECKSTOP
> ..._INVALID_APQN
> 
> 
> And you should wait for qempty on AP_RESET_IN_PROGRESS along with 
> AP_RESPONSE_NORMAL

If a queue reset is in progress, we retry the zapq. Are you saying we
should wait for qempty then reissue the zapq?

> 
>>               return 0;
>>           case AP_RESPONSE_RESET_IN_PROGRESS:
>>           case AP_RESPONSE_BUSY:
> 
> While at modifying this function, the AP_RESPONSE_BUSY is not a valid 
> code for ZAPQ, you can remove this.

Okay

> 
>> @@ -824,7 +853,7 @@ 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) {
>> -            ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
>> +            ret = vfio_ap_mdev_reset_queue(apid, apqi);
> 
> IMHO, since you are at changing this call, passing the apqn as parameter 
> would be a good simplification.

Okay.

> 
> 
> 
>>               /*
>>                * Regardless whether a queue turns out to be busy, or
>>                * is not operational, we need to continue resetting
> 
> Depends on why the reset failed, but this is out of scope.

I'm not sure what you mean by out of scope here, but you do make a valid
point. If the response code for the zapq is AP_RESPONSE_DECONFIGURED,
there is probably no sense in continuing to reset queues for that
particular adapter. I'll consider a change here.

> 
>>
> 
> 

Re: [PATCH v2 2/7] s390: vfio-ap: maintain a shadow of the guest's CRYCB

[Tony Krowiak]

On 5/6/19 2:49 AM, Pierre Morel wrote:
> On 03/05/2019 23:14, Tony Krowiak wrote:
>> This patch introduces a shadow of the CRYCB being used by a guest. This
>> will enable to more effectively manage dynamic changes to the AP
>> resources installed on the host that may be assigned to an mdev device
>> and being used by a guest. For example:
>>
>> * AP adapter cards can be dynamically added to and removed from the AP
>>    configuration via the SE or an SCLP command.
>>
>> * AP resources that disappear and reappear due to hardware malfunctions.
>>
>> * AP queues bound to and unbound from the vfio_ap device driver by a
>>    root user.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c     | 91 
>> ++++++++++++++++++++++++++++++++---
>>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>>   2 files changed, 87 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index b88a2a2ba075..44a04b4aa9ae 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -297,6 +297,45 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned 
>> long apid, unsigned long apqi)
>>       } while (--retry);
>>   }
>> +/*
>> + * vfio_ap_mdev_update_crycb
>> + *
>> + * @matrix_mdev: the mediated matrix device
>> + *
>> + * Updates the AP matrix in the guest's CRYCB from it's shadow masks.
>> + *
>> + * Returns zero if the guest's CRYCB is successfully updated; otherwise,
>> + * returns -ENODEV if a guest is not running or does not have a CRYCB.
>> + */
>> +static int vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +    if (!matrix_mdev->kvm || !matrix_mdev->kvm->arch.crypto.crycbd)
>> +        return -ENODEV;
>> +
>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>> +                  matrix_mdev->shadow_crycb->apm,
>> +                  matrix_mdev->shadow_crycb->aqm,
>> +                  matrix_mdev->shadow_crycb->adm);
>> +
>> +    return 0;
>> +}
>> +
>> +static int match_apqn(struct device *dev, void *data)
>> +{
>> +    struct ap_queue *apq = to_ap_queue(dev);
>> +
>> +    return (apq->qid == *(unsigned long *)(data)) ? 1 : 0;
>> +}
>> +
>> +static struct device *vfio_ap_get_queue_dev(unsigned long apid,
>> +                         unsigned long apqi)
>> +{
>> +    unsigned long apqn = AP_MKQID(apid, apqi);
>> +
>> +    return driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> +                  &apqn, match_apqn);
>> +}
>> +
>>   /**
>>    * assign_adapter_store
>>    *
>> @@ -805,14 +844,9 @@ static int vfio_ap_mdev_group_notifier(struct 
>> notifier_block *nb,
>>       if (ret)
>>           return NOTIFY_DONE;
>> -    /* If there is no CRYCB pointer, then we can't copy the masks */
>> -    if (!matrix_mdev->kvm->arch.crypto.crycbd)
>> +    if (vfio_ap_mdev_update_crycb(matrix_mdev))
>>           return NOTIFY_DONE;
>> -    kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
>> -                  matrix_mdev->matrix.aqm,
>> -                  matrix_mdev->matrix.adm);
>> -
>>       return NOTIFY_OK;
>>   }
>> @@ -867,12 +901,55 @@ static int vfio_ap_mdev_reset_queues(struct 
>> mdev_device *mdev)
>>       return rc;
>>   }
>> +static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev 
>> *matrix_mdev)
>> +{
>> +    unsigned long apid, apqi, domid;
>> +    struct device *dev;
>> +
>> +    matrix_mdev->shadow_crycb = 
>> kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>> +                        GFP_KERNEL);
>> +    if (!matrix_mdev->shadow_crycb)
>> +        return -ENOMEM;
>> +
>> +    vfio_ap_matrix_init(&matrix_dev->info, matrix_mdev->shadow_crycb);
>> +
>> +    /*
>> +     * Examine each APQN assigned to the mdev device. Set the APID 
>> and APQI
>> +     * in the shadow CRYCB if and only if the queue device identified by
>> +     * the APQN is in the configuration.
>> +     */
>> +    for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>> +                 matrix_mdev->matrix.apm_max + 1) {
>> +        for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>> +                     matrix_mdev->matrix.aqm_max + 1) {
>> +            dev = vfio_ap_get_queue_dev(apid, apqi);
>> +            if (dev) {
>> +                set_bit_inv(apid,
>> +                        matrix_mdev->shadow_crycb->apm);
>> +                set_bit_inv(apqi,
>> +                        matrix_mdev->shadow_crycb->aqm);
>> +                put_device(dev);
>> +            }
> 
> I think that if we do not find a device here we have a problem.
> Don't we?

Other than the fact that the guest will not have any AP devices,
what would be the problem? What would you suggest?

> 
> 
>> +        }
>> +    }
>> +
>> +    /* Set all control domains assigned to the mdev in the shadow 
>> CRYCB */
>> +    for_each_set_bit_inv(domid, matrix_mdev->matrix.adm,
>> +                 matrix_mdev->matrix.adm_max + 1)
>> +        set_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
>> +
>> +    return 0;
>> +}
>> +
>>   static int vfio_ap_mdev_open(struct mdev_device *mdev)
>>   {
>>       struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>       unsigned long events;
>>       int ret;
>> +    ret = vfio_ap_mdev_create_shadow_crycb(matrix_mdev);
>> +    if (ret)
>> +        return ret;
>>       if (!try_module_get(THIS_MODULE))
>>           return -ENODEV;
>> @@ -902,6 +979,8 @@ static void vfio_ap_mdev_release(struct 
>> mdev_device *mdev)
>>                    &matrix_mdev->group_notifier);
>>       matrix_mdev->kvm = NULL;
>>       module_put(THIS_MODULE);
>> +    kfree(matrix_mdev->shadow_crycb);
>> +    matrix_mdev->shadow_crycb = NULL;
>>   }
>>   static int vfio_ap_mdev_get_device_info(unsigned long arg)
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h 
>> b/drivers/s390/crypto/vfio_ap_private.h
>> index 76b7f98e47e9..e8457aa61976 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -72,6 +72,7 @@ struct ap_matrix {
>>    * @list:    allows the ap_matrix_mdev struct to be added to a list
>>    * @matrix:    the adapters, usage domains and control domains 
>> assigned to the
>>    *        mediated matrix device.
>> + * @shadow_crycb: a shadow copy of the crycb in use by a guest
>>    * @group_notifier: notifier block used for specifying callback 
>> function for
>>    *            handling the VFIO_GROUP_NOTIFY_SET_KVM event
>>    * @kvm:    the struct holding guest's state
>> @@ -79,6 +80,7 @@ struct ap_matrix {
>>   struct ap_matrix_mdev {
>>       struct list_head node;
>>       struct ap_matrix matrix;
>> +    struct ap_matrix *shadow_crycb;
>>       struct notifier_block group_notifier;
>>       struct kvm *kvm;
>>   };
>>
> 
> 

Re: [PATCH v2 3/7] s390: vfio-ap: sysfs interface to display guest CRYCB

[Tony Krowiak]

On 5/6/19 2:54 AM, Pierre Morel wrote:
> On 03/05/2019 23:14, Tony Krowiak wrote:
>> Introduces a sysfs interface on the matrix mdev device to display the
>> contents of the shadow of the guest's CRYCB
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 59 
>> +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 44a04b4aa9ae..1021466cb661 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -771,6 +771,64 @@ static ssize_t matrix_show(struct device *dev, 
>> struct device_attribute *attr,
>>   }
>>   static DEVICE_ATTR_RO(matrix);
>> +static ssize_t guest_matrix_show(struct device *dev,
>> +                 struct device_attribute *attr, char *buf)
>> +{
>> +    struct mdev_device *mdev = mdev_from_dev(dev);
>> +    struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +    char *bufpos = buf;
>> +    unsigned long apid;
>> +    unsigned long apqi;
>> +    unsigned long apid1;
>> +    unsigned long apqi1;
>> +    unsigned long napm_bits;
>> +    unsigned long naqm_bits;
>> +    int nchars = 0;
>> +    int n;
>> +
>> +    if (!matrix_mdev->shadow_crycb)
>> +        return -ENODEV;
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    napm_bits = matrix_mdev->shadow_crycb->apm_max + 1;
>> +    naqm_bits = matrix_mdev->shadow_crycb->aqm_max + 1;
>> +    apid1 = find_first_bit_inv(matrix_mdev->shadow_crycb->apm, 
>> napm_bits);
>> +    apqi1 = find_first_bit_inv(matrix_mdev->shadow_crycb->aqm, 
>> naqm_bits);
>> +
>> +    if ((apid1 < napm_bits) && (apqi1 < naqm_bits)) {
>> +        for_each_set_bit_inv(apid, matrix_mdev->shadow_crycb->apm,
>> +                     napm_bits) {
>> +            for_each_set_bit_inv(apqi,
>> +                         matrix_mdev->shadow_crycb->aqm,
>> +                         naqm_bits) {
>> +                n = sprintf(bufpos, "%02lx.%04lx\n", apid,
>> +                        apqi);
>> +                bufpos += n;
>> +                nchars += n;
>> +            }
>> +        }
>> +    } else if (apid1 < napm_bits) {
>> +        for_each_set_bit_inv(apid, matrix_mdev->shadow_crycb->apm,
>> +                     napm_bits) {
>> +            n = sprintf(bufpos, "%02lx.\n", apid);
>> +            bufpos += n;
>> +            nchars += n;
>> +        }
>> +    } else if (apqi1 < naqm_bits) {
>> +        for_each_set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm,
>> +                     naqm_bits) {
>> +            n = sprintf(bufpos, ".%04lx\n", apqi);
>> +            bufpos += n;
>> +            nchars += n;
>> +        }
>> +    }
>> +
>> +    mutex_unlock(&matrix_dev->lock);
>> +
>> +    return nchars;
>> +}
>> +static DEVICE_ATTR_RO(guest_matrix);
>> +
>>   static struct attribute *vfio_ap_mdev_attrs[] = {
>>       &dev_attr_assign_adapter.attr,
>>       &dev_attr_unassign_adapter.attr,
>> @@ -780,6 +838,7 @@ static struct attribute *vfio_ap_mdev_attrs[] = {
>>       &dev_attr_unassign_control_domain.attr,
>>       &dev_attr_control_domains.attr,
>>       &dev_attr_matrix.attr,
>> +    &dev_attr_guest_matrix.attr,
>>       NULL,
>>   };
>>
> 
> Code seems very similar to matrix_show, can't you share the code?

It is, I suppose I could write a function that both can call.

> 
> 
> 
> 
> 

Re: [PATCH v2 4/7] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device

[Tony Krowiak]

On 5/6/19 3:05 AM, Pierre Morel wrote:
> On 03/05/2019 23:14, Tony Krowiak wrote:
>> The AP architecture does not preclude assignment of AP resources that are
>> not yet in the AP configuration (i.e., not available or not online).
>> Let's go ahead and implement this facet of the AP architecture for linux
>> guests.
>>
>> The current implementation does not allow assignment of AP resources to
>> an mdev device if the AP queue devices identified by the assignment are
>> not bound to the vfio_ap device driver. This patch allows assignment 
>> of AP
>> resources to the mdev device even if the AP queue devices are not 
>> bound to
>> the vfio_ap device driver, as long as the AP queue devices are not
>> reserved by the AP BUS for use by the zcrypt device drivers.
> 
> or another mediated device.

Right you are!!

> 
> 
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 231 
>> ++++++++------------------------------
>>   1 file changed, 44 insertions(+), 187 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 1021466cb661..ea24caf17a16 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -113,122 +113,6 @@ static struct attribute_group 
>> *vfio_ap_mdev_type_groups[] = {
>>       NULL,
>>   };
>> -struct vfio_ap_queue_reserved {
>> -    unsigned long *apid;
>> -    unsigned long *apqi;
>> -    bool reserved;
>> -};
>> -
>> -/**
>> - * vfio_ap_has_queue
>> - *
>> - * @dev: an AP queue device
>> - * @data: a struct vfio_ap_queue_reserved reference
>> - *
>> - * Flags whether the AP queue device (@dev) has a queue ID containing 
>> the APQN,
>> - * apid or apqi specified in @data:
>> - *
>> - * - If @data contains both an apid and apqi value, then @data will 
>> be flagged
>> - *   as reserved if the APID and APQI fields for the AP queue device 
>> matches
>> - *
>> - * - If @data contains only an apid value, @data will be flagged as
>> - *   reserved if the APID field in the AP queue device matches
>> - *
>> - * - If @data contains only an apqi value, @data will be flagged as
>> - *   reserved if the APQI field in the AP queue device matches
>> - *
>> - * Returns 0 to indicate the input to function succeeded. Returns 
>> -EINVAL if
>> - * @data does not contain either an apid or apqi.
>> - */
>> -static int vfio_ap_has_queue(struct device *dev, void *data)
>> -{
>> -    struct vfio_ap_queue_reserved *qres = data;
>> -    struct ap_queue *ap_queue = to_ap_queue(dev);
>> -    ap_qid_t qid;
>> -    unsigned long id;
>> -
>> -    if (qres->apid && qres->apqi) {
>> -        qid = AP_MKQID(*qres->apid, *qres->apqi);
>> -        if (qid == ap_queue->qid)
>> -            qres->reserved = true;
>> -    } else if (qres->apid && !qres->apqi) {
>> -        id = AP_QID_CARD(ap_queue->qid);
>> -        if (id == *qres->apid)
>> -            qres->reserved = true;
>> -    } else if (!qres->apid && qres->apqi) {
>> -        id = AP_QID_QUEUE(ap_queue->qid);
>> -        if (id == *qres->apqi)
>> -            qres->reserved = true;
>> -    } else {
>> -        return -EINVAL;
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>> -/**
>> - * vfio_ap_verify_queue_reserved
>> - *
>> - * @matrix_dev: a mediated matrix device
>> - * @apid: an AP adapter ID
>> - * @apqi: an AP queue index
>> - *
>> - * Verifies that the AP queue with @apid/@apqi is reserved by the 
>> VFIO AP device
>> - * driver according to the following rules:
>> - *
>> - * - If both @apid and @apqi are not NULL, then there must be an AP 
>> queue
>> - *   device bound to the vfio_ap driver with the APQN identified by 
>> @apid and
>> - *   @apqi
>> - *
>> - * - If only @apid is not NULL, then there must be an AP queue device 
>> bound
>> - *   to the vfio_ap driver with an APQN containing @apid
>> - *
>> - * - If only @apqi is not NULL, then there must be an AP queue device 
>> bound
>> - *   to the vfio_ap driver with an APQN containing @apqi
>> - *
>> - * Returns 0 if the AP queue is reserved; otherwise, returns 
>> -EADDRNOTAVAIL.
>> - */
>> -static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>> -                     unsigned long *apqi)
>> -{
>> -    int ret;
>> -    struct vfio_ap_queue_reserved qres;
>> -
>> -    qres.apid = apid;
>> -    qres.apqi = apqi;
>> -    qres.reserved = false;
>> -
>> -    ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> -                     &qres, vfio_ap_has_queue);
>> -    if (ret)
>> -        return ret;
>> -
>> -    if (qres.reserved)
>> -        return 0;
>> -
>> -    return -EADDRNOTAVAIL;
>> -}
>> -
>> -static int
>> -vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev 
>> *matrix_mdev,
>> -                         unsigned long apid)
>> -{
>> -    int ret;
>> -    unsigned long apqi;
>> -    unsigned long nbits = matrix_mdev->matrix.aqm_max + 1;
>> -
>> -    if (find_first_bit_inv(matrix_mdev->matrix.aqm, nbits) >= nbits)
>> -        return vfio_ap_verify_queue_reserved(&apid, NULL);
>> -
>> -    for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm, nbits) {
>> -        ret = vfio_ap_verify_queue_reserved(&apid, &apqi);
>> -        if (ret)
>> -            return ret;
>> -    }
>> -
>> -    return 0;
>> -}
>> -
>>   /**
>>    * vfio_ap_mdev_verify_no_sharing
>>    *
>> @@ -236,18 +120,26 @@ 
>> vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev 
>> *matrix_mdev,
>>    * and AP queue indexes comprising the AP matrix are not configured 
>> for another
>>    * mediated device. AP queue sharing is not allowed.
>>    *
>> - * @matrix_mdev: the mediated matrix device
>> + * @mdev_apm: the mask identifying the adapters assigned to mdev
>> + * @mdev_apm: the mask identifying the adapters assigned to mdev
>>    *
>>    * Returns 0 if the APQNs are not shared, otherwise; returns 
>> -EADDRINUSE.
>>    */
>> -static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev 
>> *matrix_mdev)
>> +static int vfio_ap_mdev_verify_no_sharing(unsigned long *mdev_apm,
>> +                      unsigned long *mdev_aqm)
>>   {
>>       struct ap_matrix_mdev *lstdev;
>>       DECLARE_BITMAP(apm, AP_DEVICES);
>>       DECLARE_BITMAP(aqm, AP_DOMAINS);
>>       list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
>> -        if (matrix_mdev == lstdev)
>> +        /*
>> +         * If either of the input masks belongs to the mdev to which an
>> +         * AP resource is being assigned, then we don't need to verify
>> +         * that mdev's masks.
>> +         */
>> +        if ((mdev_apm == lstdev->matrix.apm) ||
>> +            (mdev_aqm == lstdev->matrix.aqm))
>>               continue;
> 
> Is it possible that mdev_apm and mdev_aqm do not belong to the same 
> mediated device?

The mdev_apm and the mdev_aqm will not both belong to the same mdev
device. Either the mdev_apm OR the mdev_aqm will belong to the mdev
device to which the adapter or domain is being assigned.

When an adapter is assigned, the mdev_apm is allocated in the
assign_adapter_store function setting only the bit corresponding to
the APID of the adapter being assigned. The mdev_aqm address is the
address of the matrix_mdev->matrix.aqm.

When a domain is assigned, the mdev_aqm is allocated in the
assign_adapter_store function setting only the bit corresponding to
the APQI of the domain being assigned. The mdev_apm address is the
address of the matrix_mdev->matrix.apm.

> 
> ...snip...
> 
> 

Re: [PATCH v2 5/7] s390: vfio-ap: allow hot plug/unplug of AP resources using mdev device

[Tony Krowiak]

On 5/6/19 6:42 AM, Pierre Morel wrote:
> On 03/05/2019 23:14, Tony Krowiak wrote:
>> Let's allow AP resources to be assigned to or unassigned from an AP 
>> matrix
>> mdev device while it is in use by a guest. If a guest is using the mdev
>> device while assignment is taking place, the guest will be granted access
>> to the resource as long as the guest will not be given access to an AP
>> queue device that is not bound to the vfio_ap device driver. If a 
>> guest is
>> using the mdev device while unassignment is taking place, access to the
>> resource will be taken from the guest.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 116 
>> ++++++++++++++++++++++++++++----------
>>   1 file changed, 86 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index ea24caf17a16..ede45184eb67 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -226,6 +226,8 @@ static struct device 
>> *vfio_ap_get_queue_dev(unsigned long apid,
>>                     &apqn, match_apqn);
>>   }
>> +
>> +
> 
> two white lines

I will fix it.

> 
>>   static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned 
>> long *aqm)
>>   {
>>       int ret;
>> @@ -237,6 +239,26 @@ static int vfio_ap_mdev_validate_masks(unsigned 
>> long *apm, unsigned long *aqm)
>>       return vfio_ap_mdev_verify_no_sharing(apm, aqm);
>>   }
>> +static bool vfio_ap_queues_on_drv(unsigned long *apm, unsigned long 
>> *aqm)
>> +{
>> +    unsigned long apid, apqi, apqn;
>> +    struct device *dev;
>> +
>> +    for_each_set_bit_inv(apid, apm, AP_DEVICES) {
>> +        for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
>> +            apqn = AP_MKQID(apid, apqi);
> 
> You do not use apqn in the function.

Must be a remnant of another iteration. I'll remove it.

> 
>> +
>> +            dev = vfio_ap_get_queue_dev(apid, apqi);
>> +            if (!dev)
>> +                return false;
>> +
>> +            put_device(dev);
>> +        }
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   /**
>>    * assign_adapter_store
>>    *
>> @@ -247,7 +269,10 @@ static int vfio_ap_mdev_validate_masks(unsigned 
>> long *apm, unsigned long *aqm)
>>    * @count:    the number of bytes in @buf
>>    *
>>    * Parses the APID from @buf and sets the corresponding bit in the 
>> mediated
>> - * matrix device's APM.
>> + * matrix device's APM. If a guest is using the mediated matrix 
>> device and each
>> + * new APQN formed as a result of the assignment identifies an AP 
>> queue device
>> + * that is bound to the vfio_ap device driver, the guest will be 
>> granted access
>> + * to the adapter with the specified APID.
>>    *
>>    * Returns the number of bytes processed if the APID is valid; 
>> otherwise,
>>    * returns one of the following errors:
>> @@ -279,10 +304,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;
>> @@ -300,6 +321,14 @@ static ssize_t assign_adapter_store(struct device 
>> *dev,
>>           return ret;
>>       }
>>       set_bit_inv(apid, matrix_mdev->matrix.apm);
>> +
>> +    if (matrix_mdev->shadow_crycb) {
>> +        if (vfio_ap_queues_on_drv(apm,
>> +                      matrix_mdev->shadow_crycb->aqm)) {
>> +            set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>>       mutex_unlock(&matrix_dev->lock);
>>       return count;
>> @@ -315,7 +344,9 @@ static DEVICE_ATTR_WO(assign_adapter);
>>    * @count:    the number of bytes in @buf
>>    *
>>    * Parses the APID from @buf and clears the corresponding bit in the 
>> mediated
>> - * matrix device's APM.
>> + * matrix device's APM. If a guest is using the mediated matrix 
>> device and has
>> + * access to the AP adapter with the specified APID, access to the 
>> adapter will
>> + * be taken from the guest.
>>    *
>>    * Returns the number of bytes processed if the APID is valid; 
>> otherwise,
>>    * returns one of the following errors:
>> @@ -332,10 +363,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;
>> @@ -345,6 +372,13 @@ static ssize_t unassign_adapter_store(struct 
>> device *dev,
>>       mutex_lock(&matrix_dev->lock);
>>       clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
>> +
>> +    if (matrix_mdev->shadow_crycb) {
>> +        if (test_bit_inv(apid, matrix_mdev->shadow_crycb->apm)) {
>> +            clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>>       mutex_unlock(&matrix_dev->lock);
>>       return count;
>> @@ -361,7 +395,10 @@ static DEVICE_ATTR_WO(unassign_adapter);
>>    * @count:    the number of bytes in @buf
>>    *
>>    * Parses the APQI from @buf and sets the corresponding bit in the 
>> mediated
>> - * matrix device's AQM.
>> + * matrix device's AQM. If a guest is using the mediated matrix 
>> device and each
>> + * new APQN formed as a result of the assignment identifies an AP 
>> queue device
>> + * that is bound to the vfio_ap device driver, the guest will be 
>> given access
>> + * to the AP queue(s) with the specified APQI.
>>    *
>>    * Returns the number of bytes processed if the APQI is valid; 
>> otherwise returns
>>    * one of the following errors:
>> @@ -394,10 +431,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;
>> @@ -414,6 +447,14 @@ static ssize_t assign_domain_store(struct device 
>> *dev,
>>           return ret;
>>       }
>>       set_bit_inv(apqi, matrix_mdev->matrix.aqm);
>> +
>> +    if (matrix_mdev->shadow_crycb) {
>> +        if (vfio_ap_queues_on_drv(matrix_mdev->shadow_crycb->apm,
>> +                      aqm)) {
>> +            set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>>       mutex_unlock(&matrix_dev->lock);
>>       return count;
>> @@ -431,7 +472,9 @@ static DEVICE_ATTR_WO(assign_domain);
>>    * @count:    the number of bytes in @buf
>>    *
>>    * Parses the APQI from @buf and clears the corresponding bit in the
>> - * mediated matrix device's AQM.
>> + * mediated matrix device's AQM. If a guest is using the mediated 
>> matrix device
>> + * and has access to queue(s) with the specified domain APQI, access to
>> + * the queue(s) will be taken away from the guest.
>>    *
>>    * Returns the number of bytes processed if the APQI is valid; 
>> otherwise,
>>    * returns one of the following errors:
>> @@ -447,10 +490,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;
>> @@ -460,6 +499,13 @@ static ssize_t unassign_domain_store(struct 
>> device *dev,
>>       mutex_lock(&matrix_dev->lock);
>>       clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
>> +
>> +    if (matrix_mdev->shadow_crycb) {
>> +        if (test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm)) {
>> +            clear_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>>       mutex_unlock(&matrix_dev->lock);
>>       return count;
>> @@ -475,7 +521,9 @@ static DEVICE_ATTR_WO(unassign_domain);
>>    * @count:    the number of bytes in @buf
>>    *
>>    * Parses the domain ID from @buf and sets the corresponding bit in 
>> the mediated
>> - * matrix device's ADM.
>> + * matrix device's ADM. If a guest is using the mediated matrix 
>> device and the
>> + * guest does not have access to the control domain with the 
>> specified ID, the
>> + * guest will be granted access to it.
>>    *
>>    * Returns the number of bytes processed if the domain ID is valid; 
>> otherwise,
>>    * returns one of the following errors:
>> @@ -491,10 +539,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;
>> @@ -504,6 +548,13 @@ static ssize_t assign_control_domain_store(struct 
>> device *dev,
>>       mutex_lock(&matrix_dev->lock);
>>       set_bit_inv(id, matrix_mdev->matrix.adm);
>> +
>> +    if (matrix_mdev->shadow_crycb) {
>> +        if (!test_bit_inv(id, matrix_mdev->shadow_crycb->adm)) {
>> +            set_bit_inv(id, matrix_mdev->shadow_crycb->adm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>>       mutex_unlock(&matrix_dev->lock);
>>       return count;
>> @@ -519,7 +570,9 @@ static DEVICE_ATTR_WO(assign_control_domain);
>>    * @count:    the number of bytes in @buf
>>    *
>>    * Parses the domain ID from @buf and clears the corresponding bit 
>> in the
>> - * mediated matrix device's ADM.
>> + * mediated matrix device's ADM. If a guest is using the mediated 
>> matrix device
>> + * and has access to control domain with the specified domain ID, 
>> access to
>> + * the control domain will be taken from the guest.
>>    *
>>    * Returns the number of bytes processed if the domain ID is valid; 
>> otherwise,
>>    * returns one of the following errors:
>> @@ -536,10 +589,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;
>> @@ -548,6 +597,13 @@ static ssize_t 
>> unassign_control_domain_store(struct device *dev,
>>       mutex_lock(&matrix_dev->lock);
>>       clear_bit_inv(domid, matrix_mdev->matrix.adm);
>> +
>> +    if (matrix_mdev->shadow_crycb) {
>> +        if (test_bit_inv(domid, matrix_mdev->shadow_crycb->adm)) {
>> +            clear_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>>       mutex_unlock(&matrix_dev->lock);
>>       return count;
>>
> 
> beside the two NITs, look good to me.
> Still need to test.
> 
> 
> 

Re: [PATCH v2 6/7] s390: vfio-ap: handle bind and unbind of AP queue device

[Tony Krowiak]

On 5/6/19 6:55 AM, Pierre Morel wrote:
> On 03/05/2019 23:14, Tony Krowiak wrote:
>> There is nothing preventing a root user from inadvertently unbinding an
>> AP queue device that is in use by a guest from the vfio_ap device driver
>> and binding it to a zcrypt driver. This can result in a queue being
>> accessible from both the host and a guest.
>>
>> This patch introduces safeguards that prevent sharing of an AP queue
>> between the host when a queue device is unbound from the vfio_ap device
>> driver. In addition, this patch restores guest access to AP queue devices
>> bound to the vfio_ap driver if the queue's APQN is assigned to an mdev
>> device in use by a guest.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     |  12 +++-
>>   drivers/s390/crypto/vfio_ap_ops.c     | 100 
>> +++++++++++++++++++++++++++++++++-
>>   drivers/s390/crypto/vfio_ap_private.h |   2 +
>>   3 files changed, 111 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index e9824c35c34f..c215978daf39 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -42,12 +42,22 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>   static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>>   {
>> +    struct ap_queue *queue = to_ap_queue(&apdev->device);
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    vfio_ap_mdev_probe_queue(queue);
>> +    mutex_unlock(&matrix_dev->lock);
>> +
>>       return 0;
>>   }
>>   static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>>   {
>> -    /* Nothing to do yet */
>> +    struct ap_queue *queue = to_ap_queue(&apdev->device);
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    vfio_ap_mdev_remove_queue(queue);
>> +    mutex_unlock(&matrix_dev->lock);
>>   }
>>   static void vfio_ap_matrix_dev_release(struct device *dev)
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index ede45184eb67..40324951bd37 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -226,8 +226,6 @@ static struct device 
>> *vfio_ap_get_queue_dev(unsigned long apid,
>>                     &apqn, match_apqn);
>>   }
>> -
>> -
>>   static int vfio_ap_mdev_validate_masks(unsigned long *apm, unsigned 
>> long *aqm)
>>   {
>>       int ret;
>> @@ -259,6 +257,27 @@ static bool vfio_ap_queues_on_drv(unsigned long 
>> *apm, unsigned long *aqm)
>>       return true;
>>   }
>> +static bool vfio_ap_card_on_drv(struct ap_queue *queue, unsigned long 
>> *aqm)
>> +{
>> +    unsigned long apid, apqi;
>> +    struct device *dev;
>> +
>> +    apid = AP_QID_CARD(queue->qid);
>> +
>> +    for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
>> +        if (queue->qid == AP_MKQID(apid, apqi))
>> +            continue;
>> +
>> +        dev = vfio_ap_get_queue_dev(apid, apqi);
>> +        if (!dev)
>> +            return false;
>> +
>> +        put_device(dev);
>> +    }
>> +
>> +    return true;
>> +}
>> +
>>   /**
>>    * assign_adapter_store
>>    *
>> @@ -1017,3 +1036,80 @@ void vfio_ap_mdev_unregister(void)
>>   {
>>       mdev_unregister_device(&matrix_dev->device);
>>   }
>> +
>> +static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned 
>> long apid,
>> +                                unsigned long apqi)
>> +{
>> +    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))
>> +            return matrix_mdev;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +void vfio_ap_mdev_probe_queue(struct ap_queue *queue)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev;
>> +    unsigned long *shadow_apm, *shadow_aqm;
>> +    unsigned long apid = AP_QID_CARD(queue->qid);
>> +    unsigned long apqi = AP_QID_QUEUE(queue->qid);
>> +
>> +    /*
>> +     * Find the mdev device to which the APQN of the queue device being
>> +     * probed is assigned
>> +     */
>> +    matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
>> +
>> +    /* Check whether we found an mdev device and it is in use by a 
>> guest */
>> +    if (matrix_mdev && matrix_mdev->kvm) {
>> +        shadow_apm = matrix_mdev->shadow_crycb->apm;
>> +        shadow_aqm = matrix_mdev->shadow_crycb->aqm;
>> +        /*
>> +         * If the guest already has access to the adapter card
>> +         * referenced by APID or does not have access to the queues
>> +         * referenced by APQI, there is nothing to do here.
>> +         */
>> +        if (test_bit_inv(apid, shadow_apm) ||
>> +            !test_bit_inv(apqi, shadow_aqm))
>> +            return;
>> +
>> +        /*
>> +         * If each APQN with the APID of the queue being probed and an
>> +         * APQI in the shadow CRYCB references a queue device that is
>> +         * bound to the vfio_ap driver, then plug the adapter into the
>> +         * guest.
>> +         */
>> +        if (vfio_ap_card_on_drv(queue, shadow_aqm)) {
>> +            set_bit_inv(apid, shadow_apm);
>> +            vfio_ap_mdev_update_crycb(matrix_mdev);
>> +        }
>> +    }
>> +}
>> +
>> +void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev;
>> +    unsigned long apid = AP_QID_CARD(queue->qid);
>> +    unsigned long apqi = AP_QID_QUEUE(queue->qid);
>> +
>> +    matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
>> +
>> +    /*
>> +     * If the queue is assigned to the mdev device and the mdev device
>> +     * is in use by a guest, unplug the adapter referred to by the APID
>> +     * of the APQN of the queue being removed.
>> +     */
>> +    if (matrix_mdev && matrix_mdev->kvm) {
>> +        if (!test_bit_inv(apid, matrix_mdev->shadow_crycb->apm))
>> +            return;
>> +
>> +        clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +        vfio_ap_mdev_update_crycb(matrix_mdev);
>> +    }
>> +
>> +    vfio_ap_mdev_reset_queue(apid, apqi);
>> +}
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h 
>> b/drivers/s390/crypto/vfio_ap_private.h
>> index e8457aa61976..6b1f7df5b979 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -87,5 +87,7 @@ struct ap_matrix_mdev {
>>   extern int vfio_ap_mdev_register(void);
>>   extern void vfio_ap_mdev_unregister(void);
>> +void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
>> +void vfio_ap_mdev_probe_queue(struct ap_queue *queue);
>>   #endif /* _VFIO_AP_PRIVATE_H_ */
>>
> 
> 
> AFAIU the apmask/aqmask of the AP_BUS are replacing bind/unbind for the 
> admin. Don't they?

Yes, these interfaces are used to bind/unbind.

> Then why not suppress bind/unbind for ap_queues?

I did suppress them in a previous version, but I believe Harald
objected. I don't recall the reason. If any other maintainers
agree with this, I can reinstate that change. I personally would
prefer that. I think leaving the bind/unbind interfaces confuses
the issue.

> 
> Otherwise, it seems to me to handle correctly the disappearance of a 
> card, which is the only thing that can happen from out of the firmware 
> queue change requires configuration change and re-IPL.

You are correct.

> 
> Even still need testing, LGTM

I would welcome and appreciate additional testing, thanks in advance.

> 
> 

Re: [PATCH v2 1/7] s390: vfio-ap: wait for queue empty on queue reset

[Pierre Morel]

On 06/05/2019 21:37, Tony Krowiak wrote:
> On 5/6/19 2:41 AM, Pierre Morel wrote:
>> On 03/05/2019 23:14, Tony Krowiak wrote:
>>> Refactors the AP queue reset function to wait until the queue is empty
>>> after the PQAP(ZAPQ) instruction is executed to zero out the queue as
>>> required by the AP architecture.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_ops.c | 35 
>>> ++++++++++++++++++++++++++++++++---
>>>   1 file changed, 32 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 900b9cf20ca5..b88a2a2ba075 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -271,6 +271,32 @@ static int vfio_ap_mdev_verify_no_sharing(struct 
>>> ap_matrix_mdev *matrix_mdev)
>>>       return 0;
>>>   }
>>> +static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, 
>>> unsigned long apqi)
>>> +{
>>> +    struct ap_queue_status status;
>>> +    ap_qid_t qid = AP_MKQID(apid, apqi);
>>> +    int retry = 5;
>>> +
>>> +    do {
>>> +        status = ap_tapq(qid, NULL);
>>> +        switch (status.response_code) {
>>> +        case AP_RESPONSE_NORMAL:
>>> +            if (status.queue_empty)
>>> +                return;
>>> +            msleep(20);
>>
>> NIT:     Fall through ?
> 
> Yes
> 
>>
>>> +            break;
>>> +        case AP_RESPONSE_RESET_IN_PROGRESS:
>>> +        case AP_RESPONSE_BUSY:
>>> +            msleep(20);
>>> +            break;
>>> +        default:
>>> +            pr_warn("%s: tapq err %02x: %04lx.%02lx may not be 
>>> empty\n",
>>> +                __func__, status.response_code, apid, apqi);
>>
>> I do not thing the warning sentence is appropriate:
>> The only possible errors here are if the AP is not available due to AP 
>> checkstop, deconfigured AP or invalid APQN.
> 
> Right you are! I'll work on a new message.
> 
>>
>>
>>> +            return;
>>> +        }
>>> +    } while (--retry);
>>> +}
>>> +
>>>   /**
>>>    * assign_adapter_store
>>>    *
>>> @@ -790,15 +816,18 @@ static int vfio_ap_mdev_group_notifier(struct 
>>> notifier_block *nb,
>>>       return NOTIFY_OK;
>>>   }
>>> -static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int 
>>> apqi,
>>> -                    unsigned int retry)
>>> +int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
>>>   {
>>>       struct ap_queue_status status;
>>> +    int retry = 5;
>>>       do {
>>>           status = ap_zapq(AP_MKQID(apid, apqi));
>>>           switch (status.response_code) {
>>>           case AP_RESPONSE_NORMAL:
>>> +            vfio_ap_mdev_wait_for_qempty(apid, apqi);
>>> +            return 0;
>>> +        case AP_RESPONSE_DECONFIGURED:
>>
>> Since you modify the switch, you can return for all the following cases:
>> AP_RESPONSE_DECONFIGURE
>> ..._CHECKSTOP
>> ..._INVALID_APQN
>>
>>
>> And you should wait for qempty on AP_RESET_IN_PROGRESS along with 
>> AP_RESPONSE_NORMAL
> 
> If a queue reset is in progress, we retry the zapq. Are you saying we
> should wait for qempty then reissue the zapq?


Yes, I fear that if we reissue the zapq while RESET is in progress we 
could fall in a loop depending on the reset hardware time and the 
software retry .

> 
>>
>>>               return 0;
>>>           case AP_RESPONSE_RESET_IN_PROGRESS:
>>>           case AP_RESPONSE_BUSY:
>>
>> While at modifying this function, the AP_RESPONSE_BUSY is not a valid 
>> code for ZAPQ, you can remove this.
> 
> Okay
> 
>>
>>> @@ -824,7 +853,7 @@ 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) {
>>> -            ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
>>> +            ret = vfio_ap_mdev_reset_queue(apid, apqi);
>>
>> IMHO, since you are at changing this call, passing the apqn as 
>> parameter would be a good simplification.
> 
> Okay.

Sorry, I should have add: NIT.

> 
>>
>>
>>
>>>               /*
>>>                * Regardless whether a queue turns out to be busy, or
>>>                * is not operational, we need to continue resetting
>>
>> Depends on why the reset failed, but this is out of scope.
> 
> I'm not sure what you mean by out of scope here, but you do make a valid
> point. If the response code for the zapq is AP_RESPONSE_DECONFIGURED,
> there is probably no sense in continuing to reset queues for that
> particular adapter. I'll consider a change here.

Yes, this was the point, but I consider this as a enhancement, trying a 
reset on bad queues AFAIK do no arm.

> 
>>
>>>
>>
>>
> 


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 2/7] s390: vfio-ap: maintain a shadow of the guest's CRYCB

[Pierre Morel]

On 06/05/2019 21:53, Tony Krowiak wrote:
> On 5/6/19 2:49 AM, Pierre Morel wrote:
>> On 03/05/2019 23:14, Tony Krowiak wrote:
>>> This patch introduces a shadow of the CRYCB being used by a guest. This
>>> will enable to more effectively manage dynamic changes to the AP
>>> resources installed on the host that may be assigned to an mdev device
>>> and being used by a guest. For example:
>>>
>>> * AP adapter cards can be dynamically added to and removed from the AP
>>>    configuration via the SE or an SCLP command.
>>>
>>> * AP resources that disappear and reappear due to hardware malfunctions.
>>>
>>> * AP queues bound to and unbound from the vfio_ap device driver by a
>>>    root user.
>>>
>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_ops.c     | 91 
>>> ++++++++++++++++++++++++++++++++---
>>>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>>>   2 files changed, 87 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>> index b88a2a2ba075..44a04b4aa9ae 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -297,6 +297,45 @@ static void 
>>> vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
>>>       } while (--retry);
>>>   }
>>> +/*
>>> + * vfio_ap_mdev_update_crycb
>>> + *
>>> + * @matrix_mdev: the mediated matrix device
>>> + *
>>> + * Updates the AP matrix in the guest's CRYCB from it's shadow masks.
>>> + *
>>> + * Returns zero if the guest's CRYCB is successfully updated; 
>>> otherwise,
>>> + * returns -ENODEV if a guest is not running or does not have a CRYCB.
>>> + */
>>> +static int vfio_ap_mdev_update_crycb(struct ap_matrix_mdev 
>>> *matrix_mdev)
>>> +{
>>> +    if (!matrix_mdev->kvm || !matrix_mdev->kvm->arch.crypto.crycbd)
>>> +        return -ENODEV;
>>> +
>>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>>> +                  matrix_mdev->shadow_crycb->apm,
>>> +                  matrix_mdev->shadow_crycb->aqm,
>>> +                  matrix_mdev->shadow_crycb->adm);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int match_apqn(struct device *dev, void *data)
>>> +{
>>> +    struct ap_queue *apq = to_ap_queue(dev);
>>> +
>>> +    return (apq->qid == *(unsigned long *)(data)) ? 1 : 0;
>>> +}
>>> +
>>> +static struct device *vfio_ap_get_queue_dev(unsigned long apid,
>>> +                         unsigned long apqi)
>>> +{
>>> +    unsigned long apqn = AP_MKQID(apid, apqi);
>>> +
>>> +    return driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>>> +                  &apqn, match_apqn);
>>> +}
>>> +
>>>   /**
>>>    * assign_adapter_store
>>>    *
>>> @@ -805,14 +844,9 @@ static int vfio_ap_mdev_group_notifier(struct 
>>> notifier_block *nb,
>>>       if (ret)
>>>           return NOTIFY_DONE;
>>> -    /* If there is no CRYCB pointer, then we can't copy the masks */
>>> -    if (!matrix_mdev->kvm->arch.crypto.crycbd)
>>> +    if (vfio_ap_mdev_update_crycb(matrix_mdev))
>>>           return NOTIFY_DONE;
>>> -    kvm_arch_crypto_set_masks(matrix_mdev->kvm, 
>>> matrix_mdev->matrix.apm,
>>> -                  matrix_mdev->matrix.aqm,
>>> -                  matrix_mdev->matrix.adm);
>>> -
>>>       return NOTIFY_OK;
>>>   }
>>> @@ -867,12 +901,55 @@ static int vfio_ap_mdev_reset_queues(struct 
>>> mdev_device *mdev)
>>>       return rc;
>>>   }
>>> +static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev 
>>> *matrix_mdev)
>>> +{
>>> +    unsigned long apid, apqi, domid;
>>> +    struct device *dev;
>>> +
>>> +    matrix_mdev->shadow_crycb = 
>>> kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>>> +                        GFP_KERNEL);
>>> +    if (!matrix_mdev->shadow_crycb)
>>> +        return -ENOMEM;
>>> +
>>> +    vfio_ap_matrix_init(&matrix_dev->info, matrix_mdev->shadow_crycb);
>>> +
>>> +    /*
>>> +     * Examine each APQN assigned to the mdev device. Set the APID 
>>> and APQI
>>> +     * in the shadow CRYCB if and only if the queue device 
>>> identified by
>>> +     * the APQN is in the configuration.
>>> +     */
>>> +    for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>>> +                 matrix_mdev->matrix.apm_max + 1) {
>>> +        for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>>> +                     matrix_mdev->matrix.aqm_max + 1) {
>>> +            dev = vfio_ap_get_queue_dev(apid, apqi);
>>> +            if (dev) {
>>> +                set_bit_inv(apid,
>>> +                        matrix_mdev->shadow_crycb->apm);
>>> +                set_bit_inv(apqi,
>>> +                        matrix_mdev->shadow_crycb->aqm);
>>> +                put_device(dev);
>>> +            }
>>
>> I think that if we do not find a device here we have a problem.
>> Don't we?
> 
> Other than the fact that the guest will not have any AP devices,
> what would be the problem? What would you suggest?
> 

Suppose we have in matrix_mdev->matrix:
1-2
1-3
2-2
2-3

We set the shadow_crycb with:
we find 1-2 we set 1 2
we find 1-3 we se 1 3
we find 2-2 we set 2 2
we do not find 2-3

we have set apm(1,2) aqm(2,3)
the guest can access 2-3 but we do not have the device.

Pierre

-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany

Re: [PATCH v2 1/7] s390: vfio-ap: wait for queue empty on queue reset

[Tony Krowiak]

On 5/7/19 4:10 AM, Pierre Morel wrote:
> On 06/05/2019 21:37, Tony Krowiak wrote:
>> On 5/6/19 2:41 AM, Pierre Morel wrote:
>>> On 03/05/2019 23:14, Tony Krowiak wrote:
>>>> Refactors the AP queue reset function to wait until the queue is empty
>>>> after the PQAP(ZAPQ) instruction is executed to zero out the queue as
>>>> required by the AP architecture.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>>>   drivers/s390/crypto/vfio_ap_ops.c | 35 
>>>> ++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 32 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>>> index 900b9cf20ca5..b88a2a2ba075 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>> @@ -271,6 +271,32 @@ static int 
>>>> vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
>>>>       return 0;
>>>>   }
>>>> +static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, 
>>>> unsigned long apqi)
>>>> +{
>>>> +    struct ap_queue_status status;
>>>> +    ap_qid_t qid = AP_MKQID(apid, apqi);
>>>> +    int retry = 5;
>>>> +
>>>> +    do {
>>>> +        status = ap_tapq(qid, NULL);
>>>> +        switch (status.response_code) {
>>>> +        case AP_RESPONSE_NORMAL:
>>>> +            if (status.queue_empty)
>>>> +                return;
>>>> +            msleep(20);
>>>
>>> NIT:     Fall through ?
>>
>> Yes
>>
>>>
>>>> +            break;
>>>> +        case AP_RESPONSE_RESET_IN_PROGRESS:
>>>> +        case AP_RESPONSE_BUSY:
>>>> +            msleep(20);
>>>> +            break;
>>>> +        default:
>>>> +            pr_warn("%s: tapq err %02x: %04lx.%02lx may not be 
>>>> empty\n",
>>>> +                __func__, status.response_code, apid, apqi);
>>>
>>> I do not thing the warning sentence is appropriate:
>>> The only possible errors here are if the AP is not available due to 
>>> AP checkstop, deconfigured AP or invalid APQN.
>>
>> Right you are! I'll work on a new message.
>>
>>>
>>>
>>>> +            return;
>>>> +        }
>>>> +    } while (--retry);
>>>> +}
>>>> +
>>>>   /**
>>>>    * assign_adapter_store
>>>>    *
>>>> @@ -790,15 +816,18 @@ static int vfio_ap_mdev_group_notifier(struct 
>>>> notifier_block *nb,
>>>>       return NOTIFY_OK;
>>>>   }
>>>> -static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int 
>>>> apqi,
>>>> -                    unsigned int retry)
>>>> +int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
>>>>   {
>>>>       struct ap_queue_status status;
>>>> +    int retry = 5;
>>>>       do {
>>>>           status = ap_zapq(AP_MKQID(apid, apqi));
>>>>           switch (status.response_code) {
>>>>           case AP_RESPONSE_NORMAL:
>>>> +            vfio_ap_mdev_wait_for_qempty(apid, apqi);
>>>> +            return 0;
>>>> +        case AP_RESPONSE_DECONFIGURED:
>>>
>>> Since you modify the switch, you can return for all the following cases:
>>> AP_RESPONSE_DECONFIGURE
>>> ..._CHECKSTOP
>>> ..._INVALID_APQN
>>>
>>>
>>> And you should wait for qempty on AP_RESET_IN_PROGRESS along with 
>>> AP_RESPONSE_NORMAL
>>
>> If a queue reset is in progress, we retry the zapq. Are you saying we
>> should wait for qempty then reissue the zapq?
> 
> 
> Yes, I fear that if we reissue the zapq while RESET is in progress we 
> could fall in a loop depending on the reset hardware time and the 
> software retry .

I already did this in the forthcoming v4 series.

> 
>>
>>>
>>>>               return 0;
>>>>           case AP_RESPONSE_RESET_IN_PROGRESS:
>>>>           case AP_RESPONSE_BUSY:
>>>
>>> While at modifying this function, the AP_RESPONSE_BUSY is not a valid 
>>> code for ZAPQ, you can remove this.
>>
>> Okay
>>
>>>
>>>> @@ -824,7 +853,7 @@ 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) {
>>>> -            ret = vfio_ap_mdev_reset_queue(apid, apqi, 1);
>>>> +            ret = vfio_ap_mdev_reset_queue(apid, apqi);
>>>
>>> IMHO, since you are at changing this call, passing the apqn as 
>>> parameter would be a good simplification.
>>
>> Okay.
> 
> Sorry, I should have add: NIT.
> 
>>
>>>
>>>
>>>
>>>>               /*
>>>>                * Regardless whether a queue turns out to be busy, or
>>>>                * is not operational, we need to continue resetting
>>>
>>> Depends on why the reset failed, but this is out of scope.
>>
>> I'm not sure what you mean by out of scope here, but you do make a valid
>> point. If the response code for the zapq is AP_RESPONSE_DECONFIGURED,
>> there is probably no sense in continuing to reset queues for that
>> particular adapter. I'll consider a change here.
> 
> Yes, this was the point, but I consider this as a enhancement, trying a 
> reset on bad queues AFAIK do no arm.

I included the enhancement in the forthcoming v4 series.

> 
>>
>>>
>>>>
>>>
>>>
>>
> 
> 

Re: [PATCH v2 2/7] s390: vfio-ap: maintain a shadow of the guest's CRYCB

[Tony Krowiak]

On 5/7/19 4:22 AM, Pierre Morel wrote:
> On 06/05/2019 21:53, Tony Krowiak wrote:
>> On 5/6/19 2:49 AM, Pierre Morel wrote:
>>> On 03/05/2019 23:14, Tony Krowiak wrote:
>>>> This patch introduces a shadow of the CRYCB being used by a guest. This
>>>> will enable to more effectively manage dynamic changes to the AP
>>>> resources installed on the host that may be assigned to an mdev device
>>>> and being used by a guest. For example:
>>>>
>>>> * AP adapter cards can be dynamically added to and removed from the AP
>>>>    configuration via the SE or an SCLP command.
>>>>
>>>> * AP resources that disappear and reappear due to hardware 
>>>> malfunctions.
>>>>
>>>> * AP queues bound to and unbound from the vfio_ap device driver by a
>>>>    root user.
>>>>
>>>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>>>> ---
>>>>   drivers/s390/crypto/vfio_ap_ops.c     | 91 
>>>> ++++++++++++++++++++++++++++++++---
>>>>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>>>>   2 files changed, 87 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>>>> b/drivers/s390/crypto/vfio_ap_ops.c
>>>> index b88a2a2ba075..44a04b4aa9ae 100644
>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>> @@ -297,6 +297,45 @@ static void 
>>>> vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
>>>>       } while (--retry);
>>>>   }
>>>> +/*
>>>> + * vfio_ap_mdev_update_crycb
>>>> + *
>>>> + * @matrix_mdev: the mediated matrix device
>>>> + *
>>>> + * Updates the AP matrix in the guest's CRYCB from it's shadow masks.
>>>> + *
>>>> + * Returns zero if the guest's CRYCB is successfully updated; 
>>>> otherwise,
>>>> + * returns -ENODEV if a guest is not running or does not have a CRYCB.
>>>> + */
>>>> +static int vfio_ap_mdev_update_crycb(struct ap_matrix_mdev 
>>>> *matrix_mdev)
>>>> +{
>>>> +    if (!matrix_mdev->kvm || !matrix_mdev->kvm->arch.crypto.crycbd)
>>>> +        return -ENODEV;
>>>> +
>>>> +    kvm_arch_crypto_set_masks(matrix_mdev->kvm,
>>>> +                  matrix_mdev->shadow_crycb->apm,
>>>> +                  matrix_mdev->shadow_crycb->aqm,
>>>> +                  matrix_mdev->shadow_crycb->adm);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int match_apqn(struct device *dev, void *data)
>>>> +{
>>>> +    struct ap_queue *apq = to_ap_queue(dev);
>>>> +
>>>> +    return (apq->qid == *(unsigned long *)(data)) ? 1 : 0;
>>>> +}
>>>> +
>>>> +static struct device *vfio_ap_get_queue_dev(unsigned long apid,
>>>> +                         unsigned long apqi)
>>>> +{
>>>> +    unsigned long apqn = AP_MKQID(apid, apqi);
>>>> +
>>>> +    return driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>>>> +                  &apqn, match_apqn);
>>>> +}
>>>> +
>>>>   /**
>>>>    * assign_adapter_store
>>>>    *
>>>> @@ -805,14 +844,9 @@ static int vfio_ap_mdev_group_notifier(struct 
>>>> notifier_block *nb,
>>>>       if (ret)
>>>>           return NOTIFY_DONE;
>>>> -    /* If there is no CRYCB pointer, then we can't copy the masks */
>>>> -    if (!matrix_mdev->kvm->arch.crypto.crycbd)
>>>> +    if (vfio_ap_mdev_update_crycb(matrix_mdev))
>>>>           return NOTIFY_DONE;
>>>> -    kvm_arch_crypto_set_masks(matrix_mdev->kvm, 
>>>> matrix_mdev->matrix.apm,
>>>> -                  matrix_mdev->matrix.aqm,
>>>> -                  matrix_mdev->matrix.adm);
>>>> -
>>>>       return NOTIFY_OK;
>>>>   }
>>>> @@ -867,12 +901,55 @@ static int vfio_ap_mdev_reset_queues(struct 
>>>> mdev_device *mdev)
>>>>       return rc;
>>>>   }
>>>> +static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev 
>>>> *matrix_mdev)
>>>> +{
>>>> +    unsigned long apid, apqi, domid;
>>>> +    struct device *dev;
>>>> +
>>>> +    matrix_mdev->shadow_crycb = 
>>>> kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>>>> +                        GFP_KERNEL);
>>>> +    if (!matrix_mdev->shadow_crycb)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    vfio_ap_matrix_init(&matrix_dev->info, matrix_mdev->shadow_crycb);
>>>> +
>>>> +    /*
>>>> +     * Examine each APQN assigned to the mdev device. Set the APID 
>>>> and APQI
>>>> +     * in the shadow CRYCB if and only if the queue device 
>>>> identified by
>>>> +     * the APQN is in the configuration.
>>>> +     */
>>>> +    for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>>>> +                 matrix_mdev->matrix.apm_max + 1) {
>>>> +        for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>>>> +                     matrix_mdev->matrix.aqm_max + 1) {
>>>> +            dev = vfio_ap_get_queue_dev(apid, apqi);
>>>> +            if (dev) {
>>>> +                set_bit_inv(apid,
>>>> +                        matrix_mdev->shadow_crycb->apm);
>>>> +                set_bit_inv(apqi,
>>>> +                        matrix_mdev->shadow_crycb->aqm);
>>>> +                put_device(dev);
>>>> +            }
>>>
>>> I think that if we do not find a device here we have a problem.
>>> Don't we?
>>
>> Other than the fact that the guest will not have any AP devices,
>> what would be the problem? What would you suggest?
>>
> 
> Suppose we have in matrix_mdev->matrix:
> 1-2
> 1-3
> 2-2
> 2-3
> 
> We set the shadow_crycb with:
> we find 1-2 we set 1 2
> we find 1-3 we se 1 3
> we find 2-2 we set 2 2
> we do not find 2-3
> 
> we have set apm(1,2) aqm(2,3)
> the guest can access 2-3 but we do not have the device.

Good point. I'll fix this for v4.

> 
> Pierre
>