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

[PATCH v2 0/8] 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 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 (8):
  s390: vfio-ap: maintain a shadow of the CRYCB in use by a guest
  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: wait for queue empty on queue reset
  s390: kvm: test CRYCB masks before setting them
  s390: vfio-ap: handle bind and unbind of AP queue device
  s390: vfio-ap: update documentation

 Documentation/s390/vfio-ap.txt        | 188 +++++++++++---
 arch/s390/kvm/kvm-s390.c              |  31 +++
 drivers/s390/crypto/vfio_ap_drv.c     |  16 +-
 drivers/s390/crypto/vfio_ap_ops.c     | 449 ++++++++++++++++++++--------------
 drivers/s390/crypto/vfio_ap_private.h |   4 +
 5 files changed, 467 insertions(+), 221 deletions(-)

-- 
2.7.4

[PATCH v2 1/8] s390: vfio-ap: maintain a shadow of the CRYCB in use by a guest

[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     | 69 ++++++++++++++++++++++++++++++++---
 drivers/s390/crypto/vfio_ap_private.h |  2 +
 2 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 900b9cf20ca5..b0453e6c20d0 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -271,6 +271,29 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 	return 0;
 }
 
+/*
+ * 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;
+}
+
 /**
  * assign_adapter_store
  *
@@ -340,6 +363,9 @@ static ssize_t assign_adapter_store(struct device *dev,
 	if (ret)
 		goto share_err;
 
+	if (matrix_mdev->shadow_crycb)
+		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+
 	ret = count;
 	goto done;
 
@@ -391,6 +417,9 @@ 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)
+		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -481,6 +510,9 @@ static ssize_t assign_domain_store(struct device *dev,
 	if (ret)
 		goto share_err;
 
+	if (matrix_mdev->shadow_crycb)
+		set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
+
 	ret = count;
 	goto done;
 
@@ -533,6 +565,10 @@ 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)
+		clear_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -582,6 +618,10 @@ 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)
+		set_bit_inv(id, matrix_mdev->shadow_crycb->adm);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -626,6 +666,10 @@ 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)
+		clear_bit_inv(domid, matrix_mdev->shadow_crycb->adm);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -779,14 +823,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;
 }
 
@@ -838,12 +877,28 @@ 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)
+{
+	matrix_mdev->shadow_crycb = kzalloc(sizeof(*matrix_mdev->shadow_crycb),
+					    GFP_KERNEL);
+	if (!matrix_mdev->shadow_crycb)
+		return -ENOMEM;
+
+	memcpy(matrix_mdev->shadow_crycb, &matrix_mdev->matrix,
+	       sizeof(matrix_mdev->matrix));
+
+	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;
@@ -873,6 +928,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 2/8] 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 | 58 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b0453e6c20d0..eae4ee24460a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -750,6 +750,63 @@ 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 = matrix_mdev->shadow_crycb->apm_max + 1;
+	unsigned long naqm_bits = matrix_mdev->shadow_crycb->aqm_max + 1;
+	int nchars = 0;
+	int n;
+
+	if (!matrix_mdev->shadow_crycb)
+		return -ENODEV;
+
+	apid1 = find_first_bit_inv(matrix_mdev->shadow_crycb->apm, napm_bits);
+	apqi1 = find_first_bit_inv(matrix_mdev->shadow_crycb->aqm, naqm_bits);
+
+	mutex_lock(&matrix_dev->lock);
+
+	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,
@@ -759,6 +816,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 3/8] 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.

Access to AP resources is controlled by bit masks in a guest's SIE
state description (i.e., the control block containing the state of a
guest). When an AP instruction is executed on the guest, the firmware
combines the masks from the guest's SIE state description with the masks
from the host by doing a logical bitwise AND of the masks to create what
are defined as effective masks. The effective masks determine which AP
resources can be accessed by the guest.

Effective masking allows the assignment of AP resources to a guest even
if the underlying device is not in the AP configuration. If the device
subsequently becomes configured, the guest will automatically have access
to the associated AP resources (e.g., AP queues).

The current implementation does not allow assignment of AP resources to
an mdev device if the AP queues 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 queues are not bound to the
vfio_ap device driver, as long as the AP queues are not reserved by the
AP BUS for use by the zcrypt device drivers. If the queues are subsequently
bound to the vfio_ap device driver while a running guest is using the mdev
device, the guest will automatically gain access to the queues. Effective
masking prevents access to queues until such time as they become available.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index eae4ee24460a..221491a9ba95 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
  *
@@ -359,9 +243,17 @@ static ssize_t assign_adapter_store(struct device *dev,
 
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
 
+	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+						 matrix_mdev->matrix.aqm);
+
+	/* If any APQN is owned by a default driver */
+	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
+	if (ret)
+		goto error;
+
 	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
 	if (ret)
-		goto share_err;
+		goto error;
 
 	if (matrix_mdev->shadow_crycb)
 		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
@@ -369,7 +261,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 	ret = count;
 	goto done;
 
-share_err:
+error:
 	clear_bit_inv(apid, matrix_mdev->matrix.apm);
 done:
 	mutex_unlock(&matrix_dev->lock);
@@ -426,26 +318,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
  *
@@ -500,12 +372,16 @@ static ssize_t assign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 
-	ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
-	if (ret)
-		goto done;
-
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
 
+	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+						 matrix_mdev->matrix.aqm);
+
+	/* If any APQN is owned by a default driver */
+	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
+	if (ret)
+		goto error;
+
 	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
 	if (ret)
 		goto share_err;
-- 
2.7.4

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

[Tony Krowiak]

Let's allow adapters, domains and control domains to be assigned to or
unassigned from an AP matrix mdev device while it is in use by a guest.
When an adapter, domain or control domain is assigned to or unassigned
from an mdev device while a guest is using it, the guest's CRYCB will be
updated thus giving access to the resource assigned, or taking access away
from the resource unassigned for the guest.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 221491a9ba95..091804317c5e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -219,10 +219,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;
@@ -237,10 +233,6 @@ static ssize_t assign_adapter_store(struct device *dev,
 	 */
 	mutex_lock(&matrix_dev->lock);
 
-	ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
-	if (ret)
-		goto done;
-
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
 
 	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
@@ -258,6 +250,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 	if (matrix_mdev->shadow_crycb)
 		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
 
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	ret = count;
 	goto done;
 
@@ -296,10 +289,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;
@@ -312,6 +301,8 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
 	if (matrix_mdev->shadow_crycb)
 		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -360,10 +351,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;
@@ -384,15 +371,16 @@ static ssize_t assign_domain_store(struct device *dev,
 
 	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
 	if (ret)
-		goto share_err;
+		goto error;
 
 	if (matrix_mdev->shadow_crycb)
 		set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
 
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	ret = count;
 	goto done;
 
-share_err:
+error:
 	clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
 done:
 	mutex_unlock(&matrix_dev->lock);
@@ -428,10 +416,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;
@@ -445,6 +429,7 @@ static ssize_t unassign_domain_store(struct device *dev,
 	if (matrix_mdev->shadow_crycb)
 		clear_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
 
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -476,10 +461,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;
@@ -487,10 +468,12 @@ 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.
+	/*
+	 * Set the bits in the ADM (bitmask) corresponding to the AP control
+	 * domain numbers in dommask. The bits in the mask, from left to right,
+	 * 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);
@@ -498,6 +481,7 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	if (matrix_mdev->shadow_crycb)
 		set_bit_inv(id, matrix_mdev->shadow_crycb->adm);
 
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -530,10 +514,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;
@@ -546,6 +526,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 	if (matrix_mdev->shadow_crycb)
 		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 5/8] 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 091804317c5e..31ce30ee784d 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -744,15 +744,44 @@ 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)
+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: 0x%04x may not be empty\n",
+				__func__, status.response_code, q->apqn);
+			return;
+		}
+	} while (--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:
@@ -778,7 +807,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 6/8] s390: kvm: test CRYCB masks before setting them

[Tony Krowiak]

There is no sense in blocking all vCPUs to set the masks in the guest's
CRYCB if the mask values will not be changed, so let's verify

Let's verify that mask values will be changed before blocking all vCPUs
in order to set the crypto masks in the guest's CRYCB.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/kvm/kvm-s390.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 4638303ba6a8..38835717c815 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2217,12 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
 		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
 }
 
+static int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+				      unsigned long *aqm, unsigned long *adm)
+{
+	int ret;
+	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
+
+	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
+		ret &= bitmap_equal(aqm,
+				    (unsigned long *)crycb->apcb1.aqm, 256);
+		ret &= bitmap_equal(adm,
+				    (unsigned long *)crycb->apcb1.adm, 256);
+		break;
+	case CRYCB_FORMAT1:
+	case CRYCB_FORMAT0: /* Fall through both use APCB0 */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb0.apm, 64);
+		ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb0.aqm, 16);
+		ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb0.adm, 16);
+		break;
+	default:	/* Can not happen */
+		ret = 0;
+		break;
+	}
+	return ret;
+}
+
 void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 			       unsigned long *aqm, unsigned long *adm)
 {
 	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
 
 	mutex_lock(&kvm->lock);
+	if (kvm_arch_crypto_test_masks(kvm, apm, aqm, adm)) {
+		mutex_unlock(&kvm->lock);
+		return;
+	}
 	kvm_s390_vcpu_block_all(kvm);
 
 	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
-- 
2.7.4

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

[Tony Krowiak]

There is an implied guarantee that when an AP queue device is bound to a
device driver, that driver will have exclusive control over the device.
When an AP queue device is unbound from the vfio_ap driver while the
queue is in use by a guest and subsquently bound to a zcrypt driver, the
guarantee that the zcrypt driver has exclusive control of the queue
device will be violated. Likewise, when an AP queue device is bound to
the vfio_ap device driver and its APQN is assigned to an mdev device in
use by a guest, the expectation is that the guest will have access to
the queue.

The purpose of this patch is to ensure three things:

1. When an AP queue device in use by a guest is unbound, the guest shall
   no longer access the queue. Due to the limitations of the
   architecture, access to a single queue can not be denied to a guest,
   so access to the AP card to which the queue is connected will be
   denied to the guest.

2. When an AP queue device with an APQN assigned to an mdev device is
   bound to the vfio_ap driver while the mdev device is in use by a guest,
   the guest shall be granted access to the queue.

3. When a guest is started, each APQN assigned to the guest's mdev device
   must be owned by the vfio_ap driver.

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

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index e9824c35c34f..0f5dafddf5b1 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -42,12 +42,26 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
 
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
+	struct ap_queue *apq = to_ap_queue(&apdev->device);
+	unsigned long apid = AP_QID_CARD(apq->qid);
+	unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_probe_queue(apid, apqi);
+	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 *apq = to_ap_queue(&apdev->device);
+	unsigned long apid = AP_QID_CARD(apq->qid);
+	unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_remove_queue(apid, apqi);
+	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 31ce30ee784d..3f9506516545 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -763,8 +763,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
 			msleep(20);
 			break;
 		default:
-			pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
-				__func__, status.response_code, q->apqn);
+			pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
+				__func__, status.response_code, apid, apqi);
 			return;
 		}
 	} while (--retry);
@@ -823,6 +823,14 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 
 static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev *matrix_mdev)
 {
+	/*
+	 * If an AP resource is not owned by the vfio_ap driver - e.g., it was
+	 * unbound from the driver while still assigned to the mdev device
+	 */
+	if (ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+					       matrix_mdev->matrix.aqm))
+		return -EADDRNOTAVAIL;
+
 	matrix_mdev->shadow_crycb = kzalloc(sizeof(*matrix_mdev->shadow_crycb),
 					    GFP_KERNEL);
 	if (!matrix_mdev->shadow_crycb)
@@ -847,6 +855,18 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 	if (!try_module_get(THIS_MODULE))
 		return -ENODEV;
 
+	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
+						 matrix_mdev->matrix.aqm);
+
+	/*
+	 * If any APQN is owned by a default driver, it can not be used by the
+	 * guest. This can happen if a queue is unbound from the vfio_ap
+	 * driver but not unassigned from the mdev device.
+	 */
+	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
+	if (ret)
+		return ret;
+
 	matrix_mdev->group_notifier.notifier_call = vfio_ap_mdev_group_notifier;
 	events = VFIO_GROUP_NOTIFY_SET_KVM;
 
@@ -938,3 +958,61 @@ 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_remove_queue(unsigned long apid, unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	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
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		/*
+		 * Unplug the adapter from the guest but don't unassign it
+		 * from the mdev device
+		 */
+		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+	}
+
+	vfio_ap_mdev_reset_queue(apid, apqi);
+}
+
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	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
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		/* Plug the adapter into the guest */
+		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
+
+		/* Make sure the queue is also plugged in to the guest */
+		if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
+			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
+
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+	}
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index e8457aa61976..00eaae3b853f 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(unsigned long apid, unsigned long apqi);
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
 
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

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

[Tony Krowiak]

Updates to the documentation to reflect changes introduced by
this patch series. This patch also clarifies the section on
configuring the apmask and aqmask.

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

diff --git a/Documentation/s390/vfio-ap.txt b/Documentation/s390/vfio-ap.txt
index 65167cfe4485..66e05e56ac45 100644
--- a/Documentation/s390/vfio-ap.txt
+++ b/Documentation/s390/vfio-ap.txt
@@ -351,6 +351,9 @@ matrix device.
     * 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.
+    * guest_matrix:
+      A read-only file for displaying the APQNs derived from the cross product
+      of adapter and domain numbers assigned to the guest matrix (i.e., CRYCB)
     * assign_control_domain:
     * unassign_control_domain:
       Write-only attributes for assigning/unassigning an AP control domain
@@ -438,7 +441,33 @@ guest use.
 Example:
 =======
 Let's now provide an example to illustrate how KVM guests may be given
-access to AP facilities. For this example, we will show how to configure
+access to AP facilities. Let's assume that the following AP devices are
+configured for the host:
+
+/sys/bus/ap/devices
+... 04.0004
+... 04.0005
+... 04.0006
+... 04.0047
+... 04.00ab
+... 04.00ff
+... 05.0004
+... 05.0005
+... 05.0006
+... 05.0047
+... 05.00ab
+... 05.00ff
+... 06.0004
+... 06.0005
+... 06.0006
+... 06.0047
+... 06.00ab
+... 06.00ff
+... card04
+... card05
+... card06
+
+For this example, we will show how to configure
 three guests such that executing the lszcrypt command on the guests would
 look like this:
 
@@ -461,7 +490,7 @@ CARD.DOMAIN TYPE  MODE
 05.0047     CEX5A Accelerator
 05.00ff     CEX5A Accelerator
 
-Guest2
+Guest3
 ------
 CARD.DOMAIN TYPE  MODE
 ------------------------------
@@ -538,7 +567,7 @@ These are the steps:
    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;
+   match is detected, only the default AP queue device drivers will be probed;
    otherwise, the vfio_ap device driver will be probed.
 
    By default, the two masks are set to reserve all APQNs for use by the default
@@ -599,32 +628,75 @@ These are the steps:
             default drivers pool:    adapter 0-15, domain 1
             alternate drivers pool:  adapter 16-255, domains 0, 2-255
 
+   Note:
+
+   * Clearing a bit from the apmask renders all queues associated with
+     the corresponding adapter inaccessible to the host.
+
+   * Clearing a bit from the aqmask renders that queue inaccessible on all
+     adapters reserved for the host.
+
+   * When either mask is changed, the AP bus will reprobe all of the AP devices
+     to ensure each adapter card and queue is bound to the appropriate device
+     driver as specified by the apmask and aqmask. If the change results in
+     unbinding an AP queue with an APQN that is in use by a guest, the adapter
+     card with the specified APID will be hot unplugged from the guest. If the
+     change results in binding an AP queue with an APQN that is assigned to an
+     mdev device which is in use by a guest, the queue will be hot plugged into
+     the guest.
+
    Securing the APQNs for our example:
    ----------------------------------
    To secure the AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004, 06.0047,
-   06.00ab, and 06.00ff for use by the vfio_ap device driver, the corresponding
-   APQNs can either be removed from the default masks:
+   06.00ab, and 06.00ff for use by the vfio_ap device driver, either mask can
+   be edited.
+
+   To reserve all queues for adapters 05 and 06:
 
       echo -5,-6 > /sys/bus/ap/apmask
+      or
+      echo 0xf9ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff \
+      > apmask
 
-      echo -4,-0x47,-0xab,-0xff > /sys/bus/ap/aqmask
+   This would result in binding all available queues for adapters 05 and 06 to
+   the vfio_ap device driver:
 
-   Or the masks can be set as follows:
+   /sys/bus/ap
+   ... [drivers]
+   ...... [vfio_ap]
+   ......... [05.0004]
+   ......... [05.0005]
+   ......... [05.0006]
+   ......... [05.0047]
+   ......... [05.00ab]
+   ......... [05.00ff]
+   ......... [06.0004]
+   ......... [06.0005]
+   ......... [06.0006]
+   ......... [06.0047]
+   ......... [06.00ab]
+   ......... [06.00ff]
 
-      echo 0xf9ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff \
-      > apmask
+   To reserve only the queues (thus allowing the adapters to be shared by the
+   host and guests):
 
+      echo -4,-0x47,-0xab,-0xff > /sys/bus/ap/aqmask
+      or
       echo 0xf7fffffffffffffffeffffffffffffffffffffffffeffffffffffffffffffffe \
       > aqmask
 
-   This will result in AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004,
-   06.0047, 06.00ab, and 06.00ff getting bound to the vfio_ap device driver. The
-   sysfs directory for the vfio_ap device driver will now contain symbolic links
-   to the AP queue devices bound to it:
+   Or the masks can be set as follows:
+
+   This would result in binding only queues 4, 71 (0x47), 171 (0xab), and
+   255 (0xff) all available adapters to the vfio_ap device driver:
 
    /sys/bus/ap
    ... [drivers]
    ...... [vfio_ap]
+   ......... [04.0004]
+   ......... [04.0047]
+   ......... [04.00ab]
+   ......... [04.00ff]
    ......... [05.0004]
    ......... [05.0047]
    ......... [05.00ab]
@@ -709,6 +781,16 @@ These are the steps:
 
 4. The administrator now needs to configure the matrixes for the mediated
    devices $uuid1 (for Guest1), $uuid2 (for Guest2) and $uuid3 (for Guest3).
+   The matrix is configured by assigning adapters, domains, and control domains
+   to the mediated matrix device using its sysfs assignment interfaces.
+
+   For example, to assign adapters 4, 5, 6, 22 and 23:
+
+	echo 4 > assign_adapter
+	echo 5 > assign_adapter
+	echo 6 > assign_adapter
+	echo 22 > assign_adapter
+	echo 23 > assign_adapter
 
    This is how the matrix is configured for Guest1:
 
@@ -748,11 +830,9 @@ These are the steps:
      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).
+     the previously assigned domains must be available for use by the vfio_ap
+     device driver as specified by the bus's apmask and aqmask sysfs interfaces;
+     otherwise, the operation will terminate with an error (EADDRNOTAVAIL).
 
      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
@@ -767,11 +847,9 @@ These are the steps:
      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).
+     the previously assigned adapters must be available for use by the vfio_ap
+     device driver as specified by the bus's apmask and aqmask sysfs interfaces;
+     otherwise, the operation will terminate with an error (EADDRNOTAVAIL).
 
      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
@@ -788,7 +866,7 @@ These are the steps:
    /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
       -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid1 ...
 
-7. Start Guest2:
+6. Start Guest2:
 
    /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
       -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid2 ...
@@ -798,6 +876,16 @@ These are the steps:
    /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
       -device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid3 ...
 
+Note that prior to starting a guest, if one or more of the the queues assigned
+to the guest's mdev device is unbound from the vfio_ap device driver, the
+guest will fail to start with error EADDRNOTAVAIL (cannot assign requested
+address). To display the guest's matrix:
+
+   cat /sys/devices/vfio-ap/matrix/$uuid/guest_matrix
+
+   Attempting to display the guest matrix when a guest is not using the matrix
+   mdev device will return an ENODEV (No such device) error.
+
 When the guest is shut down, the mediated matrix devices may be removed.
 
 Using our example again, to remove the mediated matrix device $uuid1:
@@ -822,16 +910,46 @@ 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 using mdev device:
+=================================
+If a guest is using a matrix mdev device, AP resources can be plugged into and
+unplugged from the running guest by using the mdev device sysfs interfaces.
+For example:
+
+   * If adapter 5 is assigned to the matrix mdev device, it can be
+     unplugged from the running guest as follows:
 
-* 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
+        echo 5 > /sys/devices/vfio-ap/matrix/$uuid/unassign_adapter
 
-* Live guest migration is not supported for guests using AP devices.
+   * If domain 71 is not reserved for use by a zcrypt driver nor assigned to
+     another matrix mdev device, it can be plugged into a running guest as
+     follows:
+
+        echo 0x47 > /sys/devices/vfio-ap/matrix/$uuid/assign_domain
+
+If a queue that is in use by a guest is unbound from the vfio_ap device driver,
+the adapter to which the queue is connected will be unplugged from the
+guest. Likewise, if a queue is bound to a matrix mdev device to which the queue
+is assigned abd the mdev device is being used by a running guest, the queue
+will be plugged into the guest.
+
+Limitations
+===========
+* Live guest migration is not supported for guests using AP devices. The
+  AP devices being used by the guest must be unplugged before live migration
+  can be initiated. Hot plug/unplug of adapters, domains and control domains can
+  be done using the mediated matrix device sysfs assignment interfaces. To
+  unplug an adapter from a running guest, simply unassign it. For example, if
+  a guest is using adapters 0 through 15, they can be unplugged as follows:
+
+	echo 0 > unassign_adapter
+	echo 1 > unassign_adapter
+	echo 2 > unassign_adapter
+	...
+	echo 15 > unassign_adapter
+
+  Note that if you unassign an adapter, all of the associated domains will also
+  become inaccessible on the guest, so it is only necessary to unassign
+  the adapters before live migration. After the live migration is complete,
+  the AP resources will have to be re-assigned to the mediated matrix device
+  on the target system.
-- 
2.7.4

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

[Pierre Morel]

On 20/04/2019 23:49, 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.
> 
> Access to AP resources is controlled by bit masks in a guest's SIE
> state description (i.e., the control block containing the state of a
> guest). When an AP instruction is executed on the guest, the firmware
> combines the masks from the guest's SIE state description with the masks
> from the host by doing a logical bitwise AND of the masks to create what
> are defined as effective masks. The effective masks determine which AP
> resources can be accessed by the guest.
> 
> Effective masking allows the assignment of AP resources to a guest even
> if the underlying device is not in the AP configuration. If the device
> subsequently becomes configured, the guest will automatically have access
> to the associated AP resources (e.g., AP queues).
> 
> The current implementation does not allow assignment of AP resources to
> an mdev device if the AP queues 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 queues are not bound to the
> vfio_ap device driver, as long as the AP queues are not reserved by the
> AP BUS for use by the zcrypt device drivers. If the queues are subsequently
> bound to the vfio_ap device driver while a running guest is using the mdev
> device, the guest will automatically gain access to the queues. Effective
> masking prevents access to queues until such time as they become available.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 160 +++++---------------------------------
>   1 file changed, 18 insertions(+), 142 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index eae4ee24460a..221491a9ba95 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)

You suppress this function but it is still in use in assign_adapter_store...


> -{
> -	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
>    *
> @@ -359,9 +243,17 @@ static ssize_t assign_adapter_store(struct device *dev,
>   
>   	set_bit_inv(apid, matrix_mdev->matrix.apm);
>   
> +	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
> +						 matrix_mdev->matrix.aqm);
> +
> +	/* If any APQN is owned by a default driver */
> +	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
> +	if (ret)
> +		goto error;
> +
>   	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
>   	if (ret)
> -		goto share_err;
> +		goto error;
>   
>   	if (matrix_mdev->shadow_crycb)
>   		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> @@ -369,7 +261,7 @@ static ssize_t assign_adapter_store(struct device *dev,
>   	ret = count;
>   	goto done;
>   
> -share_err:
> +error:
>   	clear_bit_inv(apid, matrix_mdev->matrix.apm);
>   done:
>   	mutex_unlock(&matrix_dev->lock);
> @@ -426,26 +318,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
>    *
> @@ -500,12 +372,16 @@ static ssize_t assign_domain_store(struct device *dev,
>   
>   	mutex_lock(&matrix_dev->lock);
>   
> -	ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
> -	if (ret)
> -		goto done;
> -
>   	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
>   
> +	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
> +						 matrix_mdev->matrix.aqm);
> +
> +	/* If any APQN is owned by a default driver */
> +	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
> +	if (ret)
> +		goto error;

forget to add the error label ?

> +
>   	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
>   	if (ret)
>   		goto share_err;
> 


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

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

[Pierre Morel]

On 20/04/2019 23:49, 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 091804317c5e..31ce30ee784d 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -744,15 +744,44 @@ 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)
> +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: 0x%04x may not be empty\n",
> +				__func__, status.response_code, q->apqn);

q is not defined. use qid ?


> +			return;
> +		}
> +	} while (--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:
> @@ -778,7 +807,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
> 


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

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

[Pierre Morel]

On 20/04/2019 23:49, Tony Krowiak wrote:
> There is an implied guarantee that when an AP queue device is bound to a
> device driver, that driver will have exclusive control over the device.
> When an AP queue device is unbound from the vfio_ap driver while the
> queue is in use by a guest and subsquently bound to a zcrypt driver, the
> guarantee that the zcrypt driver has exclusive control of the queue
> device will be violated. Likewise, when an AP queue device is bound to
> the vfio_ap device driver and its APQN is assigned to an mdev device in
> use by a guest, the expectation is that the guest will have access to
> the queue.
> 
> The purpose of this patch is to ensure three things:
> 
> 1. When an AP queue device in use by a guest is unbound, the guest shall
>     no longer access the queue. Due to the limitations of the
>     architecture, access to a single queue can not be denied to a guest,
>     so access to the AP card to which the queue is connected will be
>     denied to the guest.
> 
> 2. When an AP queue device with an APQN assigned to an mdev device is
>     bound to the vfio_ap driver while the mdev device is in use by a guest,
>     the guest shall be granted access to the queue.
> 
> 3. When a guest is started, each APQN assigned to the guest's mdev device
>     must be owned by the vfio_ap driver.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 16 ++++++-
>   drivers/s390/crypto/vfio_ap_ops.c     | 82 ++++++++++++++++++++++++++++++++++-
>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>   3 files changed, 97 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index e9824c35c34f..0f5dafddf5b1 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -42,12 +42,26 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>   
>   static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   {
> +	struct ap_queue *apq = to_ap_queue(&apdev->device);
> +	unsigned long apid = AP_QID_CARD(apq->qid);
> +	unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	vfio_ap_mdev_probe_queue(apid, apqi);
> +	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 *apq = to_ap_queue(&apdev->device);
> +	unsigned long apid = AP_QID_CARD(apq->qid);
> +	unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	vfio_ap_mdev_remove_queue(apid, apqi);
> +	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 31ce30ee784d..3f9506516545 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -763,8 +763,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
>   			msleep(20);
>   			break;
>   		default:
> -			pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
> -				__func__, status.response_code, q->apqn);
> +			pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
> +				__func__, status.response_code, apid, apqi);
>   			return;
>   		}
>   	} while (--retry);
> @@ -823,6 +823,14 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>   
>   static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev *matrix_mdev)
>   {
> +	/*
> +	 * If an AP resource is not owned by the vfio_ap driver - e.g., it was
> +	 * unbound from the driver while still assigned to the mdev device
> +	 */
> +	if (ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
> +					       matrix_mdev->matrix.aqm))
> +		return -EADDRNOTAVAIL;
> +
>   	matrix_mdev->shadow_crycb = kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>   					    GFP_KERNEL);
>   	if (!matrix_mdev->shadow_crycb)
> @@ -847,6 +855,18 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
>   
> +	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
> +						 matrix_mdev->matrix.aqm);
> +
> +	/*
> +	 * If any APQN is owned by a default driver, it can not be used by the
> +	 * guest. This can happen if a queue is unbound from the vfio_ap
> +	 * driver but not unassigned from the mdev device.
> +	 */
> +	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
> +	if (ret)
> +		return ret;
> +
>   	matrix_mdev->group_notifier.notifier_call = vfio_ap_mdev_group_notifier;
>   	events = VFIO_GROUP_NOTIFY_SET_KVM;
>   
> @@ -938,3 +958,61 @@ 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_remove_queue(unsigned long apid, unsigned long apqi)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +
> +	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
> +	 */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		/*
> +		 * Unplug the adapter from the guest but don't unassign it
> +		 * from the mdev device
> +		 */
> +		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +		vfio_ap_mdev_update_crycb(matrix_mdev);
> +	}
> +
> +	vfio_ap_mdev_reset_queue(apid, apqi);
> +}
> +
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +
> +	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
> +	 */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		/* Plug the adapter into the guest */
> +		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +
> +		/* Make sure the queue is also plugged in to the guest */
> +		if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
> +			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);

Why are you testing the domain before setting it and not the adapter?

Eventually you do not need to test at all or if, then both.

NIT


> +
> +		vfio_ap_mdev_update_crycb(matrix_mdev);
> +	}
> +}
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index e8457aa61976..00eaae3b853f 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(unsigned long apid, unsigned long apqi);
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
>   
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 


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

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

[Tony Krowiak]

On 4/23/19 8:46 AM, Pierre Morel wrote:
> On 20/04/2019 23:49, 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.
>>
>> Access to AP resources is controlled by bit masks in a guest's SIE
>> state description (i.e., the control block containing the state of a
>> guest). When an AP instruction is executed on the guest, the firmware
>> combines the masks from the guest's SIE state description with the masks
>> from the host by doing a logical bitwise AND of the masks to create what
>> are defined as effective masks. The effective masks determine which AP
>> resources can be accessed by the guest.
>>
>> Effective masking allows the assignment of AP resources to a guest even
>> if the underlying device is not in the AP configuration. If the device
>> subsequently becomes configured, the guest will automatically have access
>> to the associated AP resources (e.g., AP queues).
>>
>> The current implementation does not allow assignment of AP resources to
>> an mdev device if the AP queues 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 queues are not bound to the
>> vfio_ap device driver, as long as the AP queues are not reserved by the
>> AP BUS for use by the zcrypt device drivers. If the queues are 
>> subsequently
>> bound to the vfio_ap device driver while a running guest is using the 
>> mdev
>> device, the guest will automatically gain access to the queues. Effective
>> masking prevents access to queues until such time as they become 
>> available.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 160 
>> +++++---------------------------------
>>   1 file changed, 18 insertions(+), 142 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index eae4ee24460a..221491a9ba95 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)
> 
> You suppress this function but it is still in use in 
> assign_adapter_store...

Right you are. I must have been removed in a subsequent patch, because
this code was compiled and tested. It should have been removed here
though.

> 
> 
>> -{
>> -    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
>>    *
>> @@ -359,9 +243,17 @@ static ssize_t assign_adapter_store(struct device 
>> *dev,
>>       set_bit_inv(apid, matrix_mdev->matrix.apm);
>> +    ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
>> +                         matrix_mdev->matrix.aqm);
>> +
>> +    /* If any APQN is owned by a default driver */
>> +    ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
>> +    if (ret)
>> +        goto error;
>> +
>>       ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
>>       if (ret)
>> -        goto share_err;
>> +        goto error;
>>       if (matrix_mdev->shadow_crycb)
>>           set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> @@ -369,7 +261,7 @@ static ssize_t assign_adapter_store(struct device 
>> *dev,
>>       ret = count;
>>       goto done;
>> -share_err:
>> +error:
>>       clear_bit_inv(apid, matrix_mdev->matrix.apm);
>>   done:
>>       mutex_unlock(&matrix_dev->lock);
>> @@ -426,26 +318,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
>>    *
>> @@ -500,12 +372,16 @@ static ssize_t assign_domain_store(struct device 
>> *dev,
>>       mutex_lock(&matrix_dev->lock);
>> -    ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, 
>> apqi);
>> -    if (ret)
>> -        goto done;
>> -
>>       set_bit_inv(apqi, matrix_mdev->matrix.aqm);
>> +    ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
>> +                         matrix_mdev->matrix.aqm);
>> +
>> +    /* If any APQN is owned by a default driver */
>> +    ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
>> +    if (ret)
>> +        goto error;
> 
> forget to add the error label ?

Apparently I did. I'm not sure what happened here since I compiled
and tested the code. I must have messed something up when formatting
the patches.

> 
>> +
>>       ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
>>       if (ret)
>>           goto share_err;
>>
> 
> 

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

[Tony Krowiak]

On 4/23/19 8:50 AM, Pierre Morel wrote:
> On 20/04/2019 23:49, 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 091804317c5e..31ce30ee784d 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -744,15 +744,44 @@ 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)
>> +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: 0x%04x may not be empty\n",
>> +                __func__, status.response_code, q->apqn);
> 
> q is not defined. use qid ?

You are right, it is not.

> 
> 
>> +            return;
>> +        }
>> +    } while (--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:
>> @@ -778,7 +807,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
>>
> 
> 

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

[Tony Krowiak]

On 4/23/19 9:08 AM, Pierre Morel wrote:
> On 20/04/2019 23:49, Tony Krowiak wrote:
>> There is an implied guarantee that when an AP queue device is bound to a
>> device driver, that driver will have exclusive control over the device.
>> When an AP queue device is unbound from the vfio_ap driver while the
>> queue is in use by a guest and subsquently bound to a zcrypt driver, the
>> guarantee that the zcrypt driver has exclusive control of the queue
>> device will be violated. Likewise, when an AP queue device is bound to
>> the vfio_ap device driver and its APQN is assigned to an mdev device in
>> use by a guest, the expectation is that the guest will have access to
>> the queue.
>>
>> The purpose of this patch is to ensure three things:
>>
>> 1. When an AP queue device in use by a guest is unbound, the guest shall
>>     no longer access the queue. Due to the limitations of the
>>     architecture, access to a single queue can not be denied to a guest,
>>     so access to the AP card to which the queue is connected will be
>>     denied to the guest.
>>
>> 2. When an AP queue device with an APQN assigned to an mdev device is
>>     bound to the vfio_ap driver while the mdev device is in use by a 
>> guest,
>>     the guest shall be granted access to the queue.
>>
>> 3. When a guest is started, each APQN assigned to the guest's mdev device
>>     must be owned by the vfio_ap driver.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 16 ++++++-
>>   drivers/s390/crypto/vfio_ap_ops.c     | 82 
>> ++++++++++++++++++++++++++++++++++-
>>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>>   3 files changed, 97 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index e9824c35c34f..0f5dafddf5b1 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -42,12 +42,26 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>   static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>>   {
>> +    struct ap_queue *apq = to_ap_queue(&apdev->device);
>> +    unsigned long apid = AP_QID_CARD(apq->qid);
>> +    unsigned long apqi = AP_QID_QUEUE(apq->qid);
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    vfio_ap_mdev_probe_queue(apid, apqi);
>> +    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 *apq = to_ap_queue(&apdev->device);
>> +    unsigned long apid = AP_QID_CARD(apq->qid);
>> +    unsigned long apqi = AP_QID_QUEUE(apq->qid);
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    vfio_ap_mdev_remove_queue(apid, apqi);
>> +    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 31ce30ee784d..3f9506516545 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -763,8 +763,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned 
>> long apid, unsigned long apqi)
>>               msleep(20);
>>               break;
>>           default:
>> -            pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
>> -                __func__, status.response_code, q->apqn);
>> +            pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
>> +                __func__, status.response_code, apid, apqi);
>>               return;
>>           }
>>       } while (--retry);
>> @@ -823,6 +823,14 @@ static int vfio_ap_mdev_reset_queues(struct 
>> mdev_device *mdev)
>>   static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev 
>> *matrix_mdev)
>>   {
>> +    /*
>> +     * If an AP resource is not owned by the vfio_ap driver - e.g., 
>> it was
>> +     * unbound from the driver while still assigned to the mdev device
>> +     */
>> +    if (ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
>> +                           matrix_mdev->matrix.aqm))
>> +        return -EADDRNOTAVAIL;
>> +
>>       matrix_mdev->shadow_crycb = 
>> kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>>                           GFP_KERNEL);
>>       if (!matrix_mdev->shadow_crycb)
>> @@ -847,6 +855,18 @@ static int vfio_ap_mdev_open(struct mdev_device 
>> *mdev)
>>       if (!try_module_get(THIS_MODULE))
>>           return -ENODEV;
>> +    ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
>> +                         matrix_mdev->matrix.aqm);
>> +
>> +    /*
>> +     * If any APQN is owned by a default driver, it can not be used 
>> by the
>> +     * guest. This can happen if a queue is unbound from the vfio_ap
>> +     * driver but not unassigned from the mdev device.
>> +     */
>> +    ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
>> +    if (ret)
>> +        return ret;
>> +
>>       matrix_mdev->group_notifier.notifier_call = 
>> vfio_ap_mdev_group_notifier;
>>       events = VFIO_GROUP_NOTIFY_SET_KVM;
>> @@ -938,3 +958,61 @@ 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_remove_queue(unsigned long apid, unsigned long apqi)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev;
>> +
>> +    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
>> +     */
>> +    if (matrix_mdev && matrix_mdev->kvm) {
>> +        /*
>> +         * Unplug the adapter from the guest but don't unassign it
>> +         * from the mdev device
>> +         */
>> +        clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +        vfio_ap_mdev_update_crycb(matrix_mdev);
>> +    }
>> +
>> +    vfio_ap_mdev_reset_queue(apid, apqi);
>> +}
>> +
>> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev;
>> +
>> +    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
>> +     */
>> +    if (matrix_mdev && matrix_mdev->kvm) {
>> +        /* Plug the adapter into the guest */
>> +        set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +
>> +        /* Make sure the queue is also plugged in to the guest */
>> +        if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
>> +            set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
> 
> Why are you testing the domain before setting it and not the adapter?
> 
> Eventually you do not need to test at all or if, then both.

My thinking was that when a queue is removed, we clear only the APID
from the APM, but do not clear the APQI from the AQM. I think you are
correct in saying we do not need to test either mask since we are
plugging the queue in regardless.

> 
> NIT
> 
> 
>> +
>> +        vfio_ap_mdev_update_crycb(matrix_mdev);
>> +    }
>> +}
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h 
>> b/drivers/s390/crypto/vfio_ap_private.h
>> index e8457aa61976..00eaae3b853f 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(unsigned long apid, unsigned long apqi);
>> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
>>   #endif /* _VFIO_AP_PRIVATE_H_ */
>>
> 
> 

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

[Pierre Morel]

On 20/04/2019 23:49, Tony Krowiak wrote:
> There is an implied guarantee that when an AP queue device is bound to a
> device driver, that driver will have exclusive control over the device.
> When an AP queue device is unbound from the vfio_ap driver while the
> queue is in use by a guest and subsquently bound to a zcrypt driver, the
> guarantee that the zcrypt driver has exclusive control of the queue
> device will be violated. Likewise, when an AP queue device is bound to
> the vfio_ap device driver and its APQN is assigned to an mdev device in
> use by a guest, the expectation is that the guest will have access to
> the queue.
> 
> The purpose of this patch is to ensure three things:
> 
> 1. When an AP queue device in use by a guest is unbound, the guest shall
>     no longer access the queue. Due to the limitations of the
>     architecture, access to a single queue can not be denied to a guest,
>     so access to the AP card to which the queue is connected will be
>     denied to the guest.
> 
> 2. When an AP queue device with an APQN assigned to an mdev device is
>     bound to the vfio_ap driver while the mdev device is in use by a guest,
>     the guest shall be granted access to the queue.
> 
> 3. When a guest is started, each APQN assigned to the guest's mdev device
>     must be owned by the vfio_ap driver.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 16 ++++++-
>   drivers/s390/crypto/vfio_ap_ops.c     | 82 ++++++++++++++++++++++++++++++++++-
>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>   3 files changed, 97 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index e9824c35c34f..0f5dafddf5b1 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -42,12 +42,26 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>   
>   static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   {
> +	struct ap_queue *apq = to_ap_queue(&apdev->device);
> +	unsigned long apid = AP_QID_CARD(apq->qid);
> +	unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	vfio_ap_mdev_probe_queue(apid, apqi);
> +	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 *apq = to_ap_queue(&apdev->device);
> +	unsigned long apid = AP_QID_CARD(apq->qid);
> +	unsigned long apqi = AP_QID_QUEUE(apq->qid);
> +
> +	mutex_lock(&matrix_dev->lock);
> +	vfio_ap_mdev_remove_queue(apid, apqi);
> +	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 31ce30ee784d..3f9506516545 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -763,8 +763,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
>   			msleep(20);
>   			break;
>   		default:
> -			pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
> -				__func__, status.response_code, q->apqn);
> +			pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
> +				__func__, status.response_code, apid, apqi);
>   			return;
>   		}
>   	} while (--retry);
> @@ -823,6 +823,14 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>   
>   static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev *matrix_mdev)
>   {
> +	/*
> +	 * If an AP resource is not owned by the vfio_ap driver - e.g., it was
> +	 * unbound from the driver while still assigned to the mdev device
> +	 */
> +	if (ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
> +					       matrix_mdev->matrix.aqm))
> +		return -EADDRNOTAVAIL;
> +
>   	matrix_mdev->shadow_crycb = kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>   					    GFP_KERNEL);
>   	if (!matrix_mdev->shadow_crycb)
> @@ -847,6 +855,18 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>   	if (!try_module_get(THIS_MODULE))
>   		return -ENODEV;
>   
> +	ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
> +						 matrix_mdev->matrix.aqm);
> +
> +	/*
> +	 * If any APQN is owned by a default driver, it can not be used by the
> +	 * guest. This can happen if a queue is unbound from the vfio_ap
> +	 * driver but not unassigned from the mdev device.
> +	 */
> +	ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
> +	if (ret)
> +		return ret;
> +
>   	matrix_mdev->group_notifier.notifier_call = vfio_ap_mdev_group_notifier;
>   	events = VFIO_GROUP_NOTIFY_SET_KVM;
>   
> @@ -938,3 +958,61 @@ 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_remove_queue(unsigned long apid, unsigned long apqi)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +
> +	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
> +	 */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		/*
> +		 * Unplug the adapter from the guest but don't unassign it
> +		 * from the mdev device
> +		 */
> +		clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +		vfio_ap_mdev_update_crycb(matrix_mdev);
> +	}
> +
> +	vfio_ap_mdev_reset_queue(apid, apqi);
> +}
> +
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +
> +	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
> +	 */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		/* Plug the adapter into the guest */
> +		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);

Before you set the bit in the shadow...

> +
> +		/* Make sure the queue is also plugged in to the guest */

I Think we must take care in the use of queues and domains to avoid 
being confusing.

> +		if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
> +			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
> +
> +		vfio_ap_mdev_update_crycb(matrix_mdev);

...and you update the real CRYCB,

don't you need to verify that all ap queues which verify APID and any 
already pre-existing APQI are bound to the driver?

Same for pre-existing APID if you set the APQI.


> +	}
> +}
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index e8457aa61976..00eaae3b853f 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(unsigned long apid, unsigned long apqi);
> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
>   
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 


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

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

[Halil Pasic]

On Sat, 20 Apr 2019 17:49:39 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
> +{
> +	struct ap_matrix_mdev *matrix_mdev;
> +
> +	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
> +	 */
> +	if (matrix_mdev && matrix_mdev->kvm) {
> +		/* Plug the adapter into the guest */
> +		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> +
> +		/* Make sure the queue is also plugged in to the guest */
> +		if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
> +			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
> +
> +		vfio_ap_mdev_update_crycb(matrix_mdev);

With this you effectively grant access to all the assigned domains on
the AP identified by the apid, not only to the domain identified by
apqi! But some of these queues may still not be bound to the vfio_ap
driver.

IMHO you should only set the apid-th bit in apm if all queues (apid, q)
such that q-th bit is set in aqm are bound to the vfio_ap driver.

BTW a 'shadow' (or effective) apm would perfectly suffice. I don't think
you fiddle with shadow_crycb->a[qd]m, and if you do, I don't think that's
a good idea.

Regards,
Halil

> +	}
> +}

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

[Tony Krowiak]

On 4/23/19 9:38 AM, Pierre Morel wrote:
> On 20/04/2019 23:49, Tony Krowiak wrote:
>> There is an implied guarantee that when an AP queue device is bound to a
>> device driver, that driver will have exclusive control over the device.
>> When an AP queue device is unbound from the vfio_ap driver while the
>> queue is in use by a guest and subsquently bound to a zcrypt driver, the
>> guarantee that the zcrypt driver has exclusive control of the queue
>> device will be violated. Likewise, when an AP queue device is bound to
>> the vfio_ap device driver and its APQN is assigned to an mdev device in
>> use by a guest, the expectation is that the guest will have access to
>> the queue.
>>
>> The purpose of this patch is to ensure three things:
>>
>> 1. When an AP queue device in use by a guest is unbound, the guest shall
>>     no longer access the queue. Due to the limitations of the
>>     architecture, access to a single queue can not be denied to a guest,
>>     so access to the AP card to which the queue is connected will be
>>     denied to the guest.
>>
>> 2. When an AP queue device with an APQN assigned to an mdev device is
>>     bound to the vfio_ap driver while the mdev device is in use by a 
>> guest,
>>     the guest shall be granted access to the queue.
>>
>> 3. When a guest is started, each APQN assigned to the guest's mdev device
>>     must be owned by the vfio_ap driver.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 16 ++++++-
>>   drivers/s390/crypto/vfio_ap_ops.c     | 82 
>> ++++++++++++++++++++++++++++++++++-
>>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>>   3 files changed, 97 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index e9824c35c34f..0f5dafddf5b1 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -42,12 +42,26 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
>>   static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>>   {
>> +    struct ap_queue *apq = to_ap_queue(&apdev->device);
>> +    unsigned long apid = AP_QID_CARD(apq->qid);
>> +    unsigned long apqi = AP_QID_QUEUE(apq->qid);
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    vfio_ap_mdev_probe_queue(apid, apqi);
>> +    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 *apq = to_ap_queue(&apdev->device);
>> +    unsigned long apid = AP_QID_CARD(apq->qid);
>> +    unsigned long apqi = AP_QID_QUEUE(apq->qid);
>> +
>> +    mutex_lock(&matrix_dev->lock);
>> +    vfio_ap_mdev_remove_queue(apid, apqi);
>> +    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 31ce30ee784d..3f9506516545 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -763,8 +763,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned 
>> long apid, unsigned long apqi)
>>               msleep(20);
>>               break;
>>           default:
>> -            pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
>> -                __func__, status.response_code, q->apqn);
>> +            pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
>> +                __func__, status.response_code, apid, apqi);
>>               return;
>>           }
>>       } while (--retry);
>> @@ -823,6 +823,14 @@ static int vfio_ap_mdev_reset_queues(struct 
>> mdev_device *mdev)
>>   static int vfio_ap_mdev_create_shadow_crycb(struct ap_matrix_mdev 
>> *matrix_mdev)
>>   {
>> +    /*
>> +     * If an AP resource is not owned by the vfio_ap driver - e.g., 
>> it was
>> +     * unbound from the driver while still assigned to the mdev device
>> +     */
>> +    if (ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
>> +                           matrix_mdev->matrix.aqm))
>> +        return -EADDRNOTAVAIL;
>> +
>>       matrix_mdev->shadow_crycb = 
>> kzalloc(sizeof(*matrix_mdev->shadow_crycb),
>>                           GFP_KERNEL);
>>       if (!matrix_mdev->shadow_crycb)
>> @@ -847,6 +855,18 @@ static int vfio_ap_mdev_open(struct mdev_device 
>> *mdev)
>>       if (!try_module_get(THIS_MODULE))
>>           return -ENODEV;
>> +    ret = ap_apqn_in_matrix_owned_by_def_drv(matrix_mdev->matrix.apm,
>> +                         matrix_mdev->matrix.aqm);
>> +
>> +    /*
>> +     * If any APQN is owned by a default driver, it can not be used 
>> by the
>> +     * guest. This can happen if a queue is unbound from the vfio_ap
>> +     * driver but not unassigned from the mdev device.
>> +     */
>> +    ret = (ret == 1) ? -EADDRNOTAVAIL : ret;
>> +    if (ret)
>> +        return ret;
>> +
>>       matrix_mdev->group_notifier.notifier_call = 
>> vfio_ap_mdev_group_notifier;
>>       events = VFIO_GROUP_NOTIFY_SET_KVM;
>> @@ -938,3 +958,61 @@ 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_remove_queue(unsigned long apid, unsigned long apqi)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev;
>> +
>> +    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
>> +     */
>> +    if (matrix_mdev && matrix_mdev->kvm) {
>> +        /*
>> +         * Unplug the adapter from the guest but don't unassign it
>> +         * from the mdev device
>> +         */
>> +        clear_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +        vfio_ap_mdev_update_crycb(matrix_mdev);
>> +    }
>> +
>> +    vfio_ap_mdev_reset_queue(apid, apqi);
>> +}
>> +
>> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
>> +{
>> +    struct ap_matrix_mdev *matrix_mdev;
>> +
>> +    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
>> +     */
>> +    if (matrix_mdev && matrix_mdev->kvm) {
>> +        /* Plug the adapter into the guest */
>> +        set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
> 
> Before you set the bit in the shadow...

yes

> 
>> +
>> +        /* Make sure the queue is also plugged in to the guest */
> 
> I Think we must take care in the use of queues and domains to avoid 
> being confusing.

Duly noted.

> 
>> +        if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
>> +            set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
>> +
>> +        vfio_ap_mdev_update_crycb(matrix_mdev);
> 
> ...and you update the real CRYCB,
> 
> don't you need to verify that all ap queues which verify APID and any 
> already pre-existing APQI are bound to the driver?
> 
> Same for pre-existing APID if you set the APQI.

Since I last responded to your comment, I've been doing some testing
and discovered some scenarios that need to be considered. There is
definitely some additional checking that needs to be done here. It is
not necessary to verify all queues are bound to the vfio_ap driver, but
it we must assure that no queues are reserved for use
by the zcrypt drivers (i.e., APQN set in the apmask/aqmask).

> 
> 
>> +    }
>> +}
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h 
>> b/drivers/s390/crypto/vfio_ap_private.h
>> index e8457aa61976..00eaae3b853f 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(unsigned long apid, unsigned long apqi);
>> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
>>   #endif /* _VFIO_AP_PRIVATE_H_ */
>>
> 
> 

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

[Tony Krowiak]

On 4/23/19 9:54 AM, Halil Pasic wrote:
> On Sat, 20 Apr 2019 17:49:39 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> +void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
>> +{
>> +	struct ap_matrix_mdev *matrix_mdev;
>> +
>> +	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
>> +	 */
>> +	if (matrix_mdev && matrix_mdev->kvm) {
>> +		/* Plug the adapter into the guest */
>> +		set_bit_inv(apid, matrix_mdev->shadow_crycb->apm);
>> +
>> +		/* Make sure the queue is also plugged in to the guest */
>> +		if (!test_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm))
>> +			set_bit_inv(apqi, matrix_mdev->shadow_crycb->aqm);
>> +
>> +		vfio_ap_mdev_update_crycb(matrix_mdev);
> 
> With this you effectively grant access to all the assigned domains on
> the AP identified by the apid, not only to the domain identified by
> apqi! But some of these queues may still not be bound to the vfio_ap
> driver.

I have been doing some more testing since I last visited and discovered
that there needs to be additional checking here.

> 
> IMHO you should only set the apid-th bit in apm if all queues (apid, q)
> such that q-th bit is set in aqm are bound to the vfio_ap driver.

This is partially correct. It is not necessary to verify that the
affected queues are bound to the vfio_ap driver. It is only necessary
to verify they are not reserved for use by a zcrypt driver since we
are allowing assignment of APQNs for queues that are not available (see
patch 3/8).

> 
> BTW a 'shadow' (or effective) apm would perfectly suffice. I don't think
> you fiddle with shadow_crycb->a[qd]m, and if you do, I don't think that's
> a good idea.

I do not think it is accurate to refer to the APM in the shadow CRYCB as
an effective mask. Effective masking is a firmware construct. The CRYCB
of the guest may be configured with APQNs that are not available.

The shadow CRYCB is in fact a copy of the guest CRYCB. Whenever the
masks in the guest CRYCB are set, they are set from the masks in the
shadow CRYCB. The lifespan of the shadow CRYCB is synonymous with the
lifespan of the guest.

Each of the mdev device sysfs assignment/unassignment interfaces does
fiddle with the masks in the shadow CRYCB if a guest is using the mdev
device. This allows us to hot plug/unplug AP resources for the guest.
Recall that an adapter or domain can not be assigned unless each
new APQN created is NOT reserved for use by the zcrypt drivers via
the AP bus's apmask/aqmask sysfs interfaces, and the APQN is not 
assigned to any other mdev device. That is how protection is
provided against inadvertently sharing AP queues between guests or
the guest and the host. I do have to add that verification to
the vfio_ap_mdev_probe_queue function though.



> 
> Regards,
> Halil
> 
>> +	}
>> +}
>