kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support
@ 2019-09-13 21:26 Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 01/10] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks Tony Krowiak
                   ` (10 more replies)
  0 siblings, 11 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

The current design for AP pass-through does not support making dynamic
changes to the AP matrix of a running guest resulting in three deficiencies
this patch series is intended to mitigate:

1. Adapters, domains and control domains can not be added to or removed
   from a running guest. In order to modify a guest's AP configuration,
   the guest must be terminated; only then can AP resources be assigned
   to or unassigned from the guest's matrix mdev. The new AP configuration
   becomes available to the guest when it is subsequently restarted.

2. The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask interfaces can
   be modified by a root user without any restrictions. A change to either
   mask can result in AP queue devices being unbound from the vfio_ap
   device driver and bound to a zcrypt device driver even if a guest is
   using the queues, thus giving the host access to the guest's private
   crypto data and vice versa.

3. The APQNs derived from the Cartesian product of the APIDs of the
   adapters and APQIs of the domains assigned to a matrix mdev must
   reference an AP queue device bound to the vfio_ap device driver. 

This patch series introduces the following changes to the current design
to alleviate the shortcomings described above as well as to implement more
of the AP architecture:

1. A root user will be prevented from making changes to the AP bus's
   /sys/bus/ap/apmask or /sys/bus/ap/aqmask if the ownership of an APQN
   changes from the vfio_ap device driver to a zcrypt driver when the APQN
   is assigned to a matrix mdev.

2. The sysfs bind/unbind interfaces will be disabled for the vfio_ap
   device driver.

3. Allow AP resources to be assigned to or removed from a matrix mdev
   while a guest is using it and hot plug the resource into or hot unplug
   the resource from the running guest.

4. Allow assignment of an AP adapter or domain to a matrix mdev even if it
   results in assignment of an APQN that does not reference an AP queue
   device bound to the vfio_ap device driver, as long as the APQN is owned
   by the vfio_ap driver. Allowing over-provisioning of AP resources
   better models the architecture which does not preclude assigning AP
   resources that are not yet available in the system. If/when the queue
   becomes available to the host, it will immediately also become available
   to the guest.

1. Rationale for changes to AP bus's apmask/aqmask interfaces:
----------------------------------------------------------
Due to the extremely sensitive nature of cryptographic data, it is
imperative that great care be taken to ensure that such data is secured.
Allowing a root user, either inadvertently or maliciously, to configure
these masks such that a queue is shared between the host and a guest is
not only avoidable, it is advisable. It was suggested that this scenario
is better handled in user space with management software, but that does
not preclude a malicious administrator from using the sysfs interfaces
to gain access to a guest's crypto data. It was also suggested that this
scenario could be avoided by taking access to the adapter away from the
guest and zeroing out the queues prior to the vfio_ap driver releasing the
device; however, stealing an adapter in use from a guest as a by-product
of an operation is bad and will likely cause problems for the guest
unnecessarily. It was decided that the most effective solution with the
least number of negative side effects is to prevent the situation at the
source.

2. Rationale for disabling bind/unbind interfaces for vfio_ap driver:
-----------------------------------------------------------------
By disabling the bind/unbind interfaces for the vfio_ap device driver, 
the user is forced to use the AP bus's apmask/aqmask interfaces to control
the probing and removing of AP queues. There are two primary reasons for
disabling the bind/unbind interfaces for the vfio_ap device driver:

* The device architecture does not provide a means to prevent unbinding
  a device from a device driver, so an AP queue device can be unbound
  from the vfio_ap driver even when the queue is in use by a guest. By
  disabling the unbind interface, the user is forced to use the AP bus's
  apmask/aqmask interfaces which will prevent this.

* Binding of AP queues is controlled by the AP bus /sys/bus/ap/apmask and
  /sys/bus/ap/aqmask interfaces. If the masks indicate that an APQN is
  owned by zcrypt, trying to bind it to the vfio_ap device driver will
  fail; therefore, the bind interface is somewhat redundant and certainly
  unnecessary.        
  
3. Rationale for hot plug/unplug using matrix mdev sysfs interfaces:
----------------------------------------------------------------
Allowing a user to hot plug/unplug AP resources using the matrix mdev
sysfs interfaces circumvents the need to terminate the guest in order to
modify its AP configuration. Allowing dynamic configuration makes 
reconfiguring a guest's AP matrix much less disruptive.

4. Rationale for allowing over-provisioning of AP resources:
----------------------------------------------------------- 
Allowing assignment of AP resources to a matrix mdev and ultimately to a
guest better models the AP architecture. The architecture does not
preclude assignment of unavailable AP resources. If a queue subsequently
becomes available while a guest using the matrix mdev to which its APQN
is assigned, the guest will automatically acquire access to it. If an APQN
is dynamically unassigned from the underlying host system, it will 
automatically become unavailable to the guest.

Change log v5-v6:
----------------
* Fixed a bug in ap_bus.c introduced with patch 2/7 of the v5 
  series. Harald Freudenberer pointed out that the mutex lock
  for ap_perms_mutex in the apmask_store and aqmask_store functions
  was not being freed. 

* Removed patch 6/7 which added logging to the vfio_ap driver
  to expedite acceptance of this series. The logging will be introduced
  with a separate patch series to allow more time to explore options
  such as DBF logging vs. tracepoints.

* Added 3 patches related to ensuring that APQNs that do not reference
  AP queue devices bound to the vfio_ap device driver are not assigned
  to the guest CRYCB:

  Patch 4: Filter CRYCB bits for unavailable queue devices
  Patch 5: sysfs attribute to display the guest CRYCB
  Patch 6: update guest CRYCB in vfio_ap probe and remove callbacks

* Added a patch (Patch 9) to version the vfio_ap module.

* Reshuffled patches to allow the in_use callback implementation to
  invoke the vfio_ap_mdev_verify_no_sharing() function introduced in
  patch 2.  

Change log v4-v5:
----------------
* Added a patch to provide kernel s390dbf debug logs for VFIO AP

Change log v3->v4:
-----------------
* Restored patches preventing root user from changing ownership of
  APQNs from zcrypt drivers to the vfio_ap driver if the APQN is
  assigned to an mdev.

* No longer enforcing requirement restricting guest access to
  queues represented by a queue device bound to the vfio_ap
  device driver.

* Removed shadow CRYCB and now directly updating the guest CRYCB
  from the matrix mdev's matrix.

* Rebased the patch series on top of 'vfio: ap: AP Queue Interrupt
  Control' patches.

* Disabled bind/unbind sysfs interfaces for vfio_ap driver

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

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

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

Tony Krowiak (10):
  s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks
  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: filter CRYCB bits for unavailable queue devices
  s390: vfio-ap: sysfs attribute to display the guest CRYCB
  s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove
    callbacks
  s390: zcrypt: driver callback to indicate resource in use
  s390: vfio-ap: implement in-use callback for vfio_ap driver
  s390: vfio-ap: added versioning to vfio_ap module
  s390: vfio-ap: update documentation

 Documentation/s390/vfio-ap.rst        | 899 +++++++++++++++++++++++++---------
 drivers/s390/crypto/ap_bus.c          | 144 +++++-
 drivers/s390/crypto/ap_bus.h          |   4 +
 drivers/s390/crypto/vfio_ap_drv.c     |  27 +-
 drivers/s390/crypto/vfio_ap_ops.c     | 610 ++++++++++++++---------
 drivers/s390/crypto/vfio_ap_private.h |  12 +-
 6 files changed, 1200 insertions(+), 496 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v6 01/10] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 02/10] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device Tony Krowiak
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

In order to limit the number of private mdev functions called from the
vfio_ap device driver as well as to provide a landing spot for dynamic
configuration code related to binding/unbinding AP queue devices to/from
the vfio_ap driver, the following changes are being introduced:

* Move code from the vfio_ap driver's probe callback into a function
  defined in the mdev private operations file.

* Move code from the vfio_ap driver's remove callback into a function
  defined in the mdev private operations file.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 23 +++--------
 drivers/s390/crypto/vfio_ap_ops.c     | 74 ++++++++++++++++++++++++++++++-----
 drivers/s390/crypto/vfio_ap_private.h |  6 +--
 3 files changed, 72 insertions(+), 31 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 003662aa8060..9e61d4c6e6b5 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -49,15 +49,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
  */
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
-	struct vfio_ap_queue *q;
-
-	q = kzalloc(sizeof(*q), GFP_KERNEL);
-	if (!q)
-		return -ENOMEM;
-	dev_set_drvdata(&apdev->device, q);
-	q->apqn = to_ap_queue(&apdev->device)->qid;
-	q->saved_isc = VFIO_AP_ISC_INVALID;
-	return 0;
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+	return vfio_ap_mdev_probe_queue(queue);
 }
 
 /**
@@ -68,17 +62,10 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
  */
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-	struct vfio_ap_queue *q;
-	int apid, apqi;
+	struct ap_queue *queue = to_ap_queue(&apdev->device);
 
 	mutex_lock(&matrix_dev->lock);
-	q = dev_get_drvdata(&apdev->device);
-	dev_set_drvdata(&apdev->device, NULL);
-	apid = AP_QID_CARD(q->apqn);
-	apqi = AP_QID_QUEUE(q->apqn);
-	vfio_ap_mdev_reset_queue(apid, apqi, 1);
-	vfio_ap_irq_disable(q);
-	kfree(q);
+	vfio_ap_mdev_remove_queue(queue);
 	mutex_unlock(&matrix_dev->lock);
 }
 
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 0604b49a4d32..75a413fada4f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -144,7 +144,7 @@ static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
  * Returns if ap_aqic function failed with invalid, deconfigured or
  * checkstopped AP.
  */
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
+static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 {
 	struct ap_qirq_ctrl aqic_gisa = {};
 	struct ap_queue_status status;
@@ -1128,23 +1128,49 @@ static void vfio_ap_irq_disable_apqn(int apqn)
 	}
 }
 
-int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
-			     unsigned int retry)
+static void vfio_ap_mdev_wait_for_qempty(ap_qid_t qid)
+{
+	struct ap_queue_status status;
+	int retry = 5;
+
+	do {
+		status = ap_tapq(qid, NULL);
+		switch (status.response_code) {
+		case AP_RESPONSE_NORMAL:
+			if (status.queue_empty)
+				return;
+			break;
+		case AP_RESPONSE_RESET_IN_PROGRESS:
+		case AP_RESPONSE_BUSY:
+			msleep(20);
+			break;
+		default:
+			pr_warn("%s: tapq response %02x waiting for queue %02x.%04x empty\n",
+				  __func__, status.response_code,
+				  AP_QID_CARD(qid), AP_QID_QUEUE(qid));
+			return;
+		}
+	} while (--retry);
+
+	WARN_ON_ONCE(retry <= 0);
+}
+
+static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi)
 {
 	struct ap_queue_status status;
-	int retry2 = 2;
 	int apqn = AP_MKQID(apid, apqi);
+	int retry = 5;
 
 	do {
 		status = ap_zapq(apqn);
 		switch (status.response_code) {
 		case AP_RESPONSE_NORMAL:
-			while (!status.queue_empty && retry2--) {
-				msleep(20);
-				status = ap_tapq(apqn, NULL);
-			}
-			WARN_ON_ONCE(retry <= 0);
+			if (!status.queue_empty)
+				vfio_ap_mdev_wait_for_qempty(AP_MKQID(apid,
+								      apqi));
 			return 0;
+		case AP_RESPONSE_DECONFIGURED:
+			return -ENODEV;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
 			msleep(20);
@@ -1169,12 +1195,12 @@ 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);
 			/*
 			 * Regardless whether a queue turns out to be busy, or
 			 * is not operational, we need to continue resetting
 			 * the remaining queues.
 			 */
+			ret = vfio_ap_mdev_reset_queue(apid, apqi);
 			if (ret)
 				rc = ret;
 			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
@@ -1302,3 +1328,31 @@ void vfio_ap_mdev_unregister(void)
 {
 	mdev_unregister_device(&matrix_dev->device);
 }
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
+{
+	struct vfio_ap_queue *q;
+
+	q = kzalloc(sizeof(*q), GFP_KERNEL);
+	if (!q)
+		return -ENOMEM;
+	dev_set_drvdata(&queue->ap_dev.device, q);
+	q->apqn = queue->qid;
+	q->saved_isc = VFIO_AP_ISC_INVALID;
+
+	return 0;
+}
+
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
+{
+	struct vfio_ap_queue *q;
+	int apid, apqi;
+
+	q = dev_get_drvdata(&queue->ap_dev.device);
+	dev_set_drvdata(&queue->ap_dev.device, NULL);
+	apid = AP_QID_CARD(q->apqn);
+	apqi = AP_QID_QUEUE(q->apqn);
+	vfio_ap_mdev_reset_queue(apid, apqi);
+	vfio_ap_irq_disable(q);
+	kfree(q);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index f46dde56b464..5cc3c2ebf151 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -90,8 +90,6 @@ struct ap_matrix_mdev {
 
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
-int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
-			     unsigned int retry);
 
 struct vfio_ap_queue {
 	struct ap_matrix_mdev *matrix_mdev;
@@ -100,5 +98,7 @@ struct vfio_ap_queue {
 #define VFIO_AP_ISC_INVALID 0xff
 	unsigned char saved_isc;
 };
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue);
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 02/10] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 01/10] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 03/10] s390: vfio-ap: allow hot plug/unplug of AP resources using " Tony Krowiak
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

The AP architecture does not preclude assignment of AP resources that are
not available. Let's go ahead and implement this facet of the AP
architecture for linux guests.

The current implementation does not allow assignment of an AP adapter or
domain to an mdev device if the APQNs resulting from the assignment
do not reference AP queue devices that are bound to the vfio_ap device
driver. This patch allows assignment of AP resources to the mdev device as
long as the APQNs resulting from the assignment:

1. Are not reserved by the AP BUS for use by the zcrypt device drivers.

2. Are not assigned to another mdev device.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 75a413fada4f..18da9ab2b2fb 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -406,122 +406,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
  *
@@ -530,16 +414,25 @@ vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
  * mediated device. AP queue sharing is not allowed.
  *
  * @matrix_mdev: the mediated matrix device
+ * @mdev_apm: the mask identifying the adapters assigned to mdev
+ * @mdev_aqm: the mask identifying the domains (queues) assigned to mdev
  *
  * Returns 0 if the APQNs are not shared, otherwise; returns -EADDRINUSE.
  */
-static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
+					  unsigned long *mdev_apm,
+					  unsigned long *mdev_aqm)
 {
 	struct ap_matrix_mdev *lstdev;
 	DECLARE_BITMAP(apm, AP_DEVICES);
 	DECLARE_BITMAP(aqm, AP_DOMAINS);
 
 	list_for_each_entry(lstdev, &matrix_dev->mdev_list, node) {
+		/*
+		 * If either of the input masks belongs to the mdev to which an
+		 * AP resource is being assigned, then we don't need to verify
+		 * that mdev's masks.
+		 */
 		if (matrix_mdev == lstdev)
 			continue;
 
@@ -550,12 +443,10 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 		 * We work on full longs, as we can only exclude the leftover
 		 * bits in non-inverse order. The leftover is all zeros.
 		 */
-		if (!bitmap_and(apm, matrix_mdev->matrix.apm,
-				lstdev->matrix.apm, AP_DEVICES))
+		if (!bitmap_and(apm, mdev_apm, lstdev->matrix.apm, AP_DEVICES))
 			continue;
 
-		if (!bitmap_and(aqm, matrix_mdev->matrix.aqm,
-				lstdev->matrix.aqm, AP_DOMAINS))
+		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
 			continue;
 
 		return -EADDRINUSE;
@@ -564,6 +455,20 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev)
 	return 0;
 }
 
+static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
+				       unsigned long *mdev_apm,
+				       unsigned long *mdev_aqm)
+{
+	DECLARE_BITMAP(apm, AP_DEVICES);
+	DECLARE_BITMAP(aqm, AP_DOMAINS);
+
+	if (bitmap_and(apm, mdev_apm, ap_perms.apm, AP_DEVICES) &&
+	    bitmap_and(aqm, mdev_aqm, ap_perms.aqm, AP_DOMAINS))
+		return -EADDRNOTAVAIL;
+
+	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, mdev_apm, mdev_aqm);
+}
+
 /**
  * assign_adapter_store
  *
@@ -602,6 +507,7 @@ static ssize_t assign_adapter_store(struct device *dev,
 {
 	int ret;
 	unsigned long apid;
+	DECLARE_BITMAP(apm, AP_DEVICES);
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
@@ -616,32 +522,20 @@ static ssize_t assign_adapter_store(struct device *dev,
 	if (apid > matrix_mdev->matrix.apm_max)
 		return -ENODEV;
 
-	/*
-	 * Set the bit in the AP mask (APM) corresponding to the AP adapter
-	 * number (APID). The bits in the mask, from most significant to least
-	 * significant bit, correspond to APIDs 0-255.
-	 */
-	mutex_lock(&matrix_dev->lock);
-
-	ret = vfio_ap_mdev_verify_queues_reserved_for_apid(matrix_mdev, apid);
-	if (ret)
-		goto done;
+	memset(apm, 0, sizeof(apm));
+	set_bit_inv(apid, apm);
 
+	mutex_lock(&matrix_dev->lock);
+	ret = vfio_ap_mdev_validate_masks(matrix_mdev, apm,
+					  matrix_mdev->matrix.aqm);
+	if (ret) {
+		mutex_unlock(&matrix_dev->lock);
+		return ret;
+	}
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
-
-	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
-	if (ret)
-		goto share_err;
-
-	ret = count;
-	goto done;
-
-share_err:
-	clear_bit_inv(apid, matrix_mdev->matrix.apm);
-done:
 	mutex_unlock(&matrix_dev->lock);
 
-	return ret;
+	return count;
 }
 static DEVICE_ATTR_WO(assign_adapter);
 
@@ -690,26 +584,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
  *
@@ -748,6 +622,7 @@ static ssize_t assign_domain_store(struct device *dev,
 {
 	int ret;
 	unsigned long apqi;
+	DECLARE_BITMAP(aqm, AP_DOMAINS);
 	struct mdev_device *mdev = mdev_from_dev(dev);
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 	unsigned long max_apqi = matrix_mdev->matrix.aqm_max;
@@ -762,27 +637,20 @@ static ssize_t assign_domain_store(struct device *dev,
 	if (apqi > max_apqi)
 		return -ENODEV;
 
-	mutex_lock(&matrix_dev->lock);
-
-	ret = vfio_ap_mdev_verify_queues_reserved_for_apqi(matrix_mdev, apqi);
-	if (ret)
-		goto done;
+	memset(aqm, 0, sizeof(aqm));
+	set_bit_inv(apqi, aqm);
 
+	mutex_lock(&matrix_dev->lock);
+	ret = vfio_ap_mdev_validate_masks(matrix_mdev, matrix_mdev->matrix.apm,
+					  aqm);
+	if (ret) {
+		mutex_unlock(&matrix_dev->lock);
+		return ret;
+	}
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
-
-	ret = vfio_ap_mdev_verify_no_sharing(matrix_mdev);
-	if (ret)
-		goto share_err;
-
-	ret = count;
-	goto done;
-
-share_err:
-	clear_bit_inv(apqi, matrix_mdev->matrix.aqm);
-done:
 	mutex_unlock(&matrix_dev->lock);
 
-	return ret;
+	return count;
 }
 static DEVICE_ATTR_WO(assign_domain);
 
@@ -868,11 +736,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	if (id > matrix_mdev->matrix.adm_max)
 		return -ENODEV;
 
-	/* Set the bit in the ADM (bitmask) corresponding to the AP control
-	 * domain number (id). The bits in the mask, from most significant to
-	 * least significant, correspond to IDs 0 up to the one less than the
-	 * number of control domains that can be assigned.
-	 */
 	mutex_lock(&matrix_dev->lock);
 	set_bit_inv(id, matrix_mdev->matrix.adm);
 	mutex_unlock(&matrix_dev->lock);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 03/10] s390: vfio-ap: allow hot plug/unplug of AP resources using mdev device
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 01/10] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 02/10] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices Tony Krowiak
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

Let's allow AP resources - i.e., adapters, domains and control domains -
to be assigned to or unassigned from an AP matrix mdev while it is in use
by a guest. If an AP resource is assigned while a guest is using the
matrix mdev, the guest's CRYCB will be dynamically updated as follows:

* When assigning an adapter or domain, each APQN that is derived from the
  Cartesian product of the adapter numbers (APID) and domain numbers
  (APQI) assigned must reference an AP queue device that is bound to the
  vfio_ap device driver. If a particular APQN does not reference a queue
  bound to vfio_ap, the bits corresponding to the APID and APQI will not
  be set in the guest's CRYCB.

* When assigning a control domain, the bit corresponding to the domain
  number of the control domain will be set in the guest's CRYCB.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 18da9ab2b2fb..18270f286dec 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -511,10 +511,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;
@@ -565,10 +561,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;
@@ -627,10 +619,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;
@@ -681,10 +669,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;
@@ -725,10 +709,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;
@@ -770,10 +750,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;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (2 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 03/10] s390: vfio-ap: allow hot plug/unplug of AP resources using " Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-18 17:04   ` Halil Pasic
  2019-09-19 10:34   ` Halil Pasic
  2019-09-13 21:26 ` [PATCH v6 05/10] s390: vfio-ap: sysfs attribute to display the guest CRYCB Tony Krowiak
                   ` (6 subsequent siblings)
  10 siblings, 2 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

Even though APQNs for queues that are not in the AP configuration can be
assigned to a matrix mdev, we do not want to set bits in the guest's CRYCB
for APQNs that do not reference AP queue devices bound to the vfio_ap
device driver. The reason is because we have no idea whether a queue with
a given APQN will have a valid type (i.e., type 10 or newer) for the
vfio_ap device driver if/when it is subsequently added to the AP
configuration. If not, then it will not get bound to the vfio_ap device
driver. This patch filters out such APQNs before setting the bits in the
guest's CRYCB.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 18270f286dec..fec07f912916 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -33,6 +33,22 @@ static int match_apqn(struct device *dev, const void *data)
 	return (q->apqn == *(int *)(data)) ? 1 : 0;
 }
 
+static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
+{
+	struct device *dev;
+	struct vfio_ap_queue *q;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				 &apqn, match_apqn);
+	if (!dev)
+		return NULL;
+
+	q = dev_get_drvdata(dev);
+	put_device(dev);
+
+	return q;
+}
+
 /**
  * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
  * @matrix_mdev: the associated mediated matrix
@@ -49,20 +65,15 @@ static struct vfio_ap_queue *vfio_ap_get_queue(
 					int apqn)
 {
 	struct vfio_ap_queue *q;
-	struct device *dev;
 
 	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
 		return NULL;
 	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
 		return NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (!dev)
-		return NULL;
-	q = dev_get_drvdata(dev);
-	q->matrix_mdev = matrix_mdev;
-	put_device(dev);
+	q = vfio_ap_find_queue(apqn);
+	if (q)
+		q->matrix_mdev = matrix_mdev;
 
 	return q;
 }
@@ -336,6 +347,7 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 
 	matrix_mdev->mdev = mdev;
 	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
+	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->crycb);
 	mdev_set_drvdata(mdev, matrix_mdev);
 	matrix_mdev->pqap_hook.hook = handle_pqap;
 	matrix_mdev->pqap_hook.owner = THIS_MODULE;
@@ -469,6 +481,44 @@ static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
 	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, mdev_apm, mdev_aqm);
 }
 
+static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev *matrix_mdev)
+{
+	unsigned long apid, apqi;
+	unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
+			       sizeof(unsigned long);
+
+	memset(matrix_mdev->crycb.apm, 0, masksz);
+	memset(matrix_mdev->crycb.apm, 0, masksz);
+	memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1) {
+			if (vfio_ap_find_queue(AP_MKQID(apid, apqi))) {
+				if (!test_bit_inv(apid, matrix_mdev->crycb.apm))
+					set_bit_inv(apid,
+						    matrix_mdev->crycb.apm);
+				if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm))
+					set_bit_inv(apqi,
+						    matrix_mdev->crycb.aqm);
+			}
+		}
+	}
+}
+
+static bool vfio_ap_mdev_has_crycb(struct ap_matrix_mdev *matrix_mdev)
+{
+	return matrix_mdev->kvm && matrix_mdev->kvm->arch.crypto.crycbd;
+}
+
+static void vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
+{
+	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->crycb.apm,
+				  matrix_mdev->crycb.aqm,
+				  matrix_mdev->crycb.adm);
+}
+
 /**
  * assign_adapter_store
  *
@@ -479,7 +529,11 @@ static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
  * @count:	the number of bytes in @buf
  *
  * Parses the APID from @buf and sets the corresponding bit in the mediated
- * matrix device's APM.
+ * matrix device's APM. If a guest is using the matrix mdev, for each new APQN
+ * resulting from the assignment that identifies an AP queue device bound to the
+ * vfio_ap device driver, the bits corresponding to the APID and APQI will be
+ * set in the APM and AQM of the guest's CRYCB respectively, effectively
+ * plugging the queues into the guest.
  *
  * Returns the number of bytes processed if the APID is valid; otherwise,
  * returns one of the following errors:
@@ -529,6 +583,12 @@ static ssize_t assign_adapter_store(struct device *dev,
 		return ret;
 	}
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
+
+	vfio_ap_mdev_get_crycb_matrix(matrix_mdev);
+
+	if (vfio_ap_mdev_has_crycb(matrix_mdev))
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -544,7 +604,9 @@ static DEVICE_ATTR_WO(assign_adapter);
  * @count:	the number of bytes in @buf
  *
  * Parses the APID from @buf and clears the corresponding bit in the mediated
- * matrix device's APM.
+ * matrix device's APM. If a guest is using the matrix mdev, the APID will also
+ * be cleared from the APM in the guest's CRYCB effectively unplugging the
+ * adapter from the guest.
  *
  * Returns the number of bytes processed if the APID is valid; otherwise,
  * returns one of the following errors:
@@ -570,6 +632,11 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
+	clear_bit_inv((unsigned long)apid, matrix_mdev->crycb.apm);
+
+	if (vfio_ap_mdev_has_crycb(matrix_mdev))
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -586,7 +653,11 @@ static DEVICE_ATTR_WO(unassign_adapter);
  * @count:	the number of bytes in @buf
  *
  * Parses the APQI from @buf and sets the corresponding bit in the mediated
- * matrix device's AQM.
+ * matrix device's AQM. If a guest is using the matrix mdev, for each new APQN
+ * resulting from the assignment that identifies an AP queue device bound to the
+ * vfio_ap device driver, the bits corresponding to its APID and APQI will be
+ * set in the APM and AQM of the guest's CRYCB respectively, effectively
+ * plugging the queues into the guest.
  *
  * Returns the number of bytes processed if the APQI is valid; otherwise returns
  * one of the following errors:
@@ -636,6 +707,11 @@ static ssize_t assign_domain_store(struct device *dev,
 		return ret;
 	}
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
+	vfio_ap_mdev_get_crycb_matrix(matrix_mdev);
+
+	if (vfio_ap_mdev_has_crycb(matrix_mdev))
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -653,7 +729,9 @@ static DEVICE_ATTR_WO(assign_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the APQI from @buf and clears the corresponding bit in the
- * mediated matrix device's AQM.
+ * mediated matrix device's AQM. If a guest is using the matrix mdev, the APQI
+ * will also be cleared from the AQM in the guest's CRYCB effectively unplugging
+ * all queues with the specified APQI from the guest.
  *
  * Returns the number of bytes processed if the APQI is valid; otherwise,
  * returns one of the following errors:
@@ -678,6 +756,11 @@ static ssize_t unassign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
+	clear_bit_inv((unsigned long)apqi, matrix_mdev->crycb.aqm);
+
+	if (vfio_ap_mdev_has_crycb(matrix_mdev))
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -693,7 +776,9 @@ static DEVICE_ATTR_WO(unassign_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the domain ID from @buf and sets the corresponding bit in the mediated
- * matrix device's ADM.
+ * matrix device's ADM. If a guest is using the matrix mdev, the domain ID
+ * will also be set in the ADM of the guest's CRYCB giving the guest control
+ * over the domain.
  *
  * Returns the number of bytes processed if the domain ID is valid; otherwise,
  * returns one of the following errors:
@@ -718,6 +803,11 @@ static ssize_t assign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	set_bit_inv(id, matrix_mdev->matrix.adm);
+	set_bit_inv(id, matrix_mdev->crycb.adm);
+
+	if (vfio_ap_mdev_has_crycb(matrix_mdev))
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -733,7 +823,9 @@ static DEVICE_ATTR_WO(assign_control_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the domain ID from @buf and clears the corresponding bit in the
- * mediated matrix device's ADM.
+ * mediated matrix device's ADM. If a guest is using the matrix mdev, the domain
+ * ID will also be cleared from the ADM of the guest's CRYCB taking control
+ * over the domain away from the guest.
  *
  * Returns the number of bytes processed if the domain ID is valid; otherwise,
  * returns one of the following errors:
@@ -942,13 +1034,10 @@ 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)
-		return NOTIFY_DONE;
-
-	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
-				  matrix_mdev->matrix.aqm,
-				  matrix_mdev->matrix.adm);
+	if (vfio_ap_mdev_has_crycb(matrix_mdev)) {
+		vfio_ap_mdev_get_crycb_matrix(matrix_mdev);
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+	}
 
 	return NOTIFY_OK;
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5cc3c2ebf151..4a4e2c11fdf2 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -74,6 +74,8 @@ 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.
+ * @crycb:	the adapters, usage domains and control domains configured for
+ *		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
@@ -81,6 +83,7 @@ struct ap_matrix {
 struct ap_matrix_mdev {
 	struct list_head node;
 	struct ap_matrix matrix;
+	struct ap_matrix crycb;
 	struct notifier_block group_notifier;
 	struct notifier_block iommu_notifier;
 	struct kvm *kvm;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 05/10] s390: vfio-ap: sysfs attribute to display the guest CRYCB
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (3 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 06/10] s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove callbacks Tony Krowiak
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

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

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

If a guest is not using the matrix mdev at the time the crycb is dispalyed
an error (ENODEV) will be returned.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index fec07f912916..14f221b7426b 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -930,6 +930,59 @@ 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->crycb.apm_max + 1;
+	unsigned long naqm_bits = matrix_mdev->crycb.aqm_max + 1;
+	int nchars = 0;
+	int n;
+
+	if (!vfio_ap_mdev_has_crycb(matrix_mdev))
+		return -ENODEV;
+
+	apid1 = find_first_bit_inv(matrix_mdev->crycb.apm, napm_bits);
+	apqi1 = find_first_bit_inv(matrix_mdev->crycb.aqm, naqm_bits);
+
+	mutex_lock(&matrix_dev->lock);
+
+	if ((apid1 < napm_bits) && (apqi1 < naqm_bits)) {
+		for_each_set_bit_inv(apid, matrix_mdev->crycb.apm, napm_bits) {
+			for_each_set_bit_inv(apqi, matrix_mdev->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->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->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,
@@ -939,6 +992,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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 06/10] s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove callbacks
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (4 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 05/10] s390: vfio-ap: sysfs attribute to display the guest CRYCB Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 07/10] s390: zcrypt: driver callback to indicate resource in use Tony Krowiak
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

When the vfio_ap device driver's probe callback is invoked to bind a
an AP queue device, if its APQN has been assigned to a matrix mdev that is
in use by a guest, the queue will be hot plugged into the guest's AP
configuration if:

1. Each APQN derived from the APID and the APQIs already set in the
   guest's CRYCB references an AP queue device bound to the vfio_ap
   device driver.

2. Each APQN derived from the APQI and the APIDs already set in the
   guest's CRYCB references an AP queue device bound to the vfio_ap
   device driver.

When an AP adapter is removed from the AP configuration via the SE or an
'SCLP Deconfigure AP Adapter' instruction, each queue device associated
with the adapter will be unbound from device driver to which it is bound.
When the vfio_ap device driver's remove callback is invoked to unbind an
AP queue device, if the queue's APQN is assigned to a matrix mdev in use
by a guest, the adapter to which the queue is connected will be hot
unplugged from the guest's configuration. The reason for this is because
an adapter of a different type can be added back to the AP configuration
with the APID corresponding to the adapter being removed. If the new
adapter is not of the appropriate type, it will not get bound to the
vfio_ap driver. If we did not unplug it from the guest, both the guest
and the host would have access to all of the queues associated with the
adapter which is a no-no.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 14f221b7426b..f3332424670f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -507,6 +507,63 @@ static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev *matrix_mdev)
 	}
 }
 
+static bool vfio_ap_mdev_update_crycb_matrix(struct ap_matrix_mdev *matrix_mdev,
+					     struct vfio_ap_queue *q)
+{
+	unsigned long crycb_apid, crycb_apqi;
+	unsigned long apid = AP_QID_CARD(q->apqn);
+	unsigned long apqi = AP_QID_QUEUE(q->apqn);
+
+	/*
+	 * If the APID of the input queue is not already set in the guest's
+	 * CRYCB, then verify that all APQNs derived from Cartesian product of
+	 * the APID and each APQI set in the guest's CRYCB references a queue
+	 * that is bound to the vfio_ap driver.
+	 */
+	if (!test_bit_inv(apid, matrix_mdev->crycb.apm)) {
+		for_each_set_bit_inv(crycb_apqi, matrix_mdev->crycb.aqm,
+				     matrix_mdev->crycb.aqm_max + 1) {
+			/*
+			 * The APQN formulated from the APID and APQI of the
+			 * input queue is in the process of being bound to the
+			 * vfio_ap driver so there is no need to verify it.
+			 */
+			if (apqi == crycb_apqi)
+				continue;
+
+			if (!vfio_ap_find_queue(AP_MKQID(apid, crycb_apqi)))
+				return false;
+		}
+	}
+
+	/*
+	 * If the APQI of the input queue is not already set in the guest's
+	 * CRYCB, then verify that all APQNs derived from Cartesian product of
+	 * the APQI and each APID set in the guest's CRYCB references a queue
+	 * that is bound to the vfio_ap driver.
+	 */
+	if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm)) {
+		for_each_set_bit_inv(crycb_apid, matrix_mdev->crycb.apm,
+				     matrix_mdev->crycb.apm_max + 1) {
+			/*
+			 * The APQN formulated from the APID and APQI of the
+			 * input queue is in the process of being bound to the
+			 * vfio_ap driver so there is no need to verify it.
+			 */
+			if (apid == crycb_apid)
+				continue;
+
+			if (!vfio_ap_find_queue(AP_MKQID(crycb_apid, apqi)))
+				return false;
+		}
+	}
+
+	set_bit_inv(apid, matrix_mdev->crycb.apm);
+	set_bit_inv(apqi, matrix_mdev->crycb.aqm);
+
+	return true;
+}
+
 static bool vfio_ap_mdev_has_crycb(struct ap_matrix_mdev *matrix_mdev)
 {
 	return matrix_mdev->kvm && matrix_mdev->kvm->arch.crypto.crycbd;
@@ -1311,9 +1368,23 @@ void vfio_ap_mdev_unregister(void)
 	mdev_unregister_device(&matrix_dev->device);
 }
 
+static struct ap_matrix_mdev *vfio_ap_mdev_for_queue(int apqn)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		if (test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm) &&
+		    test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
+			return matrix_mdev;
+	}
+
+	return NULL;
+}
+
 int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
 {
 	struct vfio_ap_queue *q;
+	struct ap_matrix_mdev *matrix_mdev;
 
 	q = kzalloc(sizeof(*q), GFP_KERNEL);
 	if (!q)
@@ -1322,18 +1393,40 @@ int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
 	q->apqn = queue->qid;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
 
+	/*
+	 * If the APQN for the queue is assigned to a matrix mdev that is in
+	 * use by a guest, then plug the queue into the guest.
+	 */
+	matrix_mdev = vfio_ap_mdev_for_queue(queue->qid);
+	if (matrix_mdev && vfio_ap_mdev_has_crycb(matrix_mdev)) {
+		vfio_ap_mdev_update_crycb_matrix(matrix_mdev, q);
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+	}
+
 	return 0;
 }
 
 void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
 {
 	struct vfio_ap_queue *q;
+	struct ap_matrix_mdev *matrix_mdev;
 	int apid, apqi;
 
 	q = dev_get_drvdata(&queue->ap_dev.device);
 	dev_set_drvdata(&queue->ap_dev.device, NULL);
 	apid = AP_QID_CARD(q->apqn);
 	apqi = AP_QID_QUEUE(q->apqn);
+
+	/*
+	 * If the APQN for the queue is assigned to a matrix mdev that is in
+	 * use by a guest, then unplug the adapter from the guest.
+	 */
+	matrix_mdev = vfio_ap_mdev_for_queue(queue->qid);
+	if (matrix_mdev && vfio_ap_mdev_has_crycb(matrix_mdev)) {
+		clear_bit_inv(apid, matrix_mdev->crycb.apm);
+		vfio_ap_mdev_update_crycb(matrix_mdev);
+	}
+
 	vfio_ap_mdev_reset_queue(apid, apqi);
 	vfio_ap_irq_disable(q);
 	kfree(q);
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 07/10] s390: zcrypt: driver callback to indicate resource in use
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (5 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 06/10] s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove callbacks Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 08/10] s390: vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding
an AP queue from its device driver if the queue is in use. This prevents
a root user from inadvertently taking a queue away from a guest and
giving it to the host, or vice versa. The callback will be invoked
whenever a change to the AP bus's apmask or aqmask sysfs interfaces may
result in one or more AP queues being removed from its driver. If the
callback responds in the affirmative for any driver queried, the change
to the apmask or aqmask will be rejected with a device in use error.

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

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

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index a76b8a8bcbbb..6c79e5c36bdf 100644
--- a/drivers/s390/crypto/ap_bus.c
+++ b/drivers/s390/crypto/ap_bus.c
@@ -35,6 +35,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/debugfs.h>
 #include <linux/ctype.h>
+#include <linux/module.h>
 
 #include "ap_bus.h"
 #include "ap_debug.h"
@@ -997,9 +998,11 @@ int ap_parse_mask_str(const char *str,
 	newmap = kmalloc(size, GFP_KERNEL);
 	if (!newmap)
 		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}
 	}
 
 	if (*str == '+' || *str == '-') {
@@ -1011,7 +1014,10 @@ int ap_parse_mask_str(const char *str,
 	}
 	if (rc == 0)
 		memcpy(bitmap, newmap, size);
-	mutex_unlock(lock);
+
+	if (lock)
+		mutex_unlock(lock);
+
 	kfree(newmap);
 	return rc;
 }
@@ -1198,12 +1204,75 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+int __verify_card_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newapm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify cards reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.
+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+
+static int apmask_commit(unsigned long *newapm)
+{
+	int rc;
+	unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
+
+	/*
+	 * Check if any bits in the apmask have been set which will
+	 * result in queues being removed from non-default drivers
+	 */
+	if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
+		rc = bus_for_each_drv(&ap_bus_type, NULL, reserved,
+				      __verify_card_reservations);
+		if (rc)
+			return rc;
+	}
+
+	memcpy(ap_perms.apm, newapm, APMASKSIZE);
+
+	return 0;
+}
+
 static ssize_t apmask_store(struct bus_type *bus, const char *buf,
 			    size_t count)
 {
 	int rc;
+	unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
+
+	if (mutex_lock_interruptible(&ap_perms_mutex))
+		return -ERESTARTSYS;
+
+	memcpy(newapm, ap_perms.apm, APMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
+	if (rc)
+		goto done;
+
+	rc = apmask_commit(newapm);
+	if (rc)
+		goto done;
 
-	rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
+done:
+	mutex_unlock(&ap_perms_mutex);
 	if (rc)
 		return rc;
 
@@ -1229,12 +1298,75 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+int __verify_queue_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newaqm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify queues reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.
+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* The non-default driver's module must be loaded */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(ap_perms.apm, newaqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+
+static int aqmask_commit(unsigned long *newaqm)
+{
+	int rc;
+	unsigned long reserved[BITS_TO_LONGS(AP_DOMAINS)];
+
+	/*
+	 * Check if any bits in the aqmask have been set which will
+	 * result in queues being removed from non-default drivers
+	 */
+	if (bitmap_andnot(reserved, newaqm, ap_perms.aqm, AP_DOMAINS)) {
+		rc = bus_for_each_drv(&ap_bus_type, NULL, reserved,
+				      __verify_queue_reservations);
+		if (rc)
+			return rc;
+	}
+
+	memcpy(ap_perms.aqm, newaqm, AQMASKSIZE);
+
+	return 0;
+}
+
 static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
 			    size_t count)
 {
 	int rc;
+	unsigned long newaqm[BITS_TO_LONGS(AP_DEVICES)];
 
-	rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
+	if (mutex_lock_interruptible(&ap_perms_mutex))
+		return -ERESTARTSYS;
+
+	memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
+	if (rc)
+		goto done;
+
+	rc = aqmask_commit(newaqm);
+	if (rc)
+		goto done;
+
+done:
+	mutex_unlock(&ap_perms_mutex);
 	if (rc)
 		return rc;
 
diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index 6f3cf37776ca..2836e674d00d 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -137,6 +137,7 @@ struct ap_driver {
 	void (*remove)(struct ap_device *);
 	void (*suspend)(struct ap_device *);
 	void (*resume)(struct ap_device *);
+	bool (*in_use)(unsigned long *apm, unsigned long *aqm);
 };
 
 #define to_ap_drv(x) container_of((x), struct ap_driver, driver)
@@ -265,6 +266,9 @@ void ap_queue_reinit_state(struct ap_queue *aq);
 struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
 			       int comp_device_type, unsigned int functions);
 
+#define APMASKSIZE (BITS_TO_LONGS(AP_DEVICES) * sizeof(unsigned long))
+#define AQMASKSIZE (BITS_TO_LONGS(AP_DOMAINS) * sizeof(unsigned long))
+
 struct ap_perms {
 	unsigned long ioctlm[BITS_TO_LONGS(AP_IOCTLS)];
 	unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 08/10] s390: vfio-ap: implement in-use callback for vfio_ap driver
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (6 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 07/10] s390: zcrypt: driver callback to indicate resource in use Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 09/10] s390: vfio-ap: added versioning to vfio_ap module Tony Krowiak
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

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

To ensure that the AP bus apmask/aqmask interfaces are used to control
which AP queues get manually bound to or unbound from the
vfio_ap device driver, the bind/unbind sysfs interfaces will
be disabled for the vfio_ap device driver. The reasons for this are:

* To prevent unbinding an AP queue device from the vfio_ap device
  driver representing a queue that is assigned to an mdev device.

* To enforce the policy that the the AP resources must first be
  unassigned from the mdev device - which will hot unplug them from a
  guest using the mdev device - before changing ownership of APQNs
  from the vfio_ap driver to a zcrypt driver. This ensures that private
  crypto data intended for the guest will never be accessible from the
  host.

* It takes advantage of the AP architecture to prevent dynamic changes
  to the LPAR configuration using the SE or SCLP commands from
  compromising the guest crypto devices. For example:

  * Even if an adapter is configured off, if and when it is configured
    back on, the queue devices associated with the adapter will be bound
    back to the vfio_ap driver and the queues will automatically be
    available to a guest using the mdev to which the APQN of the queue
    device is assigned.

  * If adapters or domains are dynamically unassigned from the LPAR
    in which the linux guest is running, effective masking will
    prevent access to the AP resources by a guest using them.

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

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 9e61d4c6e6b5..477218e39289 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -173,7 +173,9 @@ static int __init vfio_ap_init(void)
 	memset(&vfio_ap_drv, 0, sizeof(vfio_ap_drv));
 	vfio_ap_drv.probe = vfio_ap_queue_dev_probe;
 	vfio_ap_drv.remove = vfio_ap_queue_dev_remove;
+	vfio_ap_drv.in_use = vfio_ap_mdev_resource_in_use;
 	vfio_ap_drv.ids = ap_queue_ids;
+	vfio_ap_drv.driver.suppress_bind_attrs = true;
 
 	ret = ap_driver_register(&vfio_ap_drv, THIS_MODULE, VFIO_AP_DRV_NAME);
 	if (ret) {
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index f3332424670f..d06fd9f0db23 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1431,3 +1431,14 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
 	vfio_ap_irq_disable(q);
 	kfree(q);
 }
+
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+	bool in_use;
+
+	mutex_lock(&matrix_dev->lock);
+	in_use = vfio_ap_mdev_verify_no_sharing(NULL, apm, aqm) ? true : false;
+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 4a4e2c11fdf2..21546bb90240 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -104,4 +104,6 @@ struct vfio_ap_queue {
 int vfio_ap_mdev_probe_queue(struct ap_queue *queue);
 void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
 
+bool vfio_ap_mdev_resource_in_use(unsigned long *apm, unsigned long *aqm);
+
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 09/10] s390: vfio-ap: added versioning to vfio_ap module
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (7 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 08/10] s390: vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-09-13 21:26 ` [PATCH v6 10/10] s390: vfio-ap: update documentation Tony Krowiak
  2019-10-08 10:48 ` [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Halil Pasic
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

Added versioning to vfio_ap module. The introduction of hot plug/unplug
and over-provisioning of AP resources require a different set of
regression tests to be run. Version checking provides a means for the
regression test software to determine the appropriate set of tests to run.

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

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 477218e39289..d9051cf7fd5c 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -21,6 +21,8 @@
 MODULE_AUTHOR("IBM Corporation");
 MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
 MODULE_LICENSE("GPL v2");
+MODULE_VERSION(VFIO_AP_MODULE_VERSION);
+
 
 static struct ap_driver vfio_ap_drv;
 
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 21546bb90240..8d2099d222fa 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -22,6 +22,7 @@
 #include "ap_bus.h"
 
 #define VFIO_AP_MODULE_NAME "vfio_ap"
+#define VFIO_AP_MODULE_VERSION "1.2.0"
 #define VFIO_AP_DRV_NAME "vfio_ap"
 
 /**
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v6 10/10] s390: vfio-ap: update documentation
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (8 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 09/10] s390: vfio-ap: added versioning to vfio_ap module Tony Krowiak
@ 2019-09-13 21:26 ` Tony Krowiak
  2019-10-08 10:48 ` [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Halil Pasic
  10 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-13 21:26 UTC (permalink / raw)
  To: linux-s390, linux-kernel, kvm
  Cc: freude, borntraeger, cohuck, mjrosato, pmorel, pasic,
	alex.williamson, kwankhede, jjherne, Tony Krowiak

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

Changes made to the mdev matrix assignment interfaces:

* Allow assignment of APQNs that are not bound to the vfio-ap device
  driver as long as they are not owned by a zcrypt driver as identified
  in the AP bus sysfs apmask and aqmask interfaces.

* Allow assignment of an AP resource to a mediated device which is in use
  by a guest to hot plug an adapter, domain and control domain into a
  running guest.

* Allow unassignment of an AP resource from a mediated device which is in
  use by a guest to hot unplug an adapter, domain and control domain from
  a running guest.

This patch also:

* Clarifies the section on configuring the AP bus's apmask and aqmask.

* Adds sections on dynamic configuration using the SE or SCLP command.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 Documentation/s390/vfio-ap.rst | 899 ++++++++++++++++++++++++++++++-----------
 1 file changed, 665 insertions(+), 234 deletions(-)

diff --git a/Documentation/s390/vfio-ap.rst b/Documentation/s390/vfio-ap.rst
index b5c51f7c748d..3304135f3d0b 100644
--- a/Documentation/s390/vfio-ap.rst
+++ b/Documentation/s390/vfio-ap.rst
@@ -85,11 +85,20 @@ definitions:
   instructions include a field containing the APQN to identify the AP queue to
   which the AP command is to be sent for processing.
 
-  The AP bus will create a sysfs device for each APQN that can be derived from
-  the cross product of the AP adapter and usage domain numbers detected when the
-  AP bus module is loaded. For example, if adapters 4 and 10 (0x0a) and usage
-  domains 6 and 71 (0x47) are assigned to the LPAR, the AP bus will create the
-  following sysfs entries::
+  The AP bus creates an AP queue device in sysfs for each APQN that can be
+  derived from the Cartesian product of the adapter and usage domain numbers
+  of the adapters and domains detected when the AP bus is initialized. For
+  example, if adapters 4 and 10 (0x0a), and usage domains 6 and 71 (0x47) are
+  detected, the Cartesian product would be defined by the following table:
+
+		        04           10
+		   +-----------+-----------+
+		06 |  (04,06)  |  (0a,06)  |
+		   +-----------|-----------+
+		71 |  (04,47)  |  (0a,47)  |
+		   +-----------|-----------+
+
+  The AP bus will create the following sysfs entries:
 
     /sys/devices/ap/card04/04.0006
     /sys/devices/ap/card04/04.0047
@@ -151,14 +160,22 @@ If you recall from the description of an AP Queue, AP instructions include
 an APQN to identify the AP queue to which an AP command-request message is to be
 sent (NQAP and PQAP instructions), or from which a command-reply message is to
 be received (DQAP instruction). The validity of an APQN is defined by the matrix
-calculated from the APM and AQM; it is the cross product of all assigned adapter
-numbers (APM) with all assigned queue indexes (AQM). For example, if adapters 1
-and 2 and usage domains 5 and 6 are assigned to a guest, the APQNs (1,5), (1,6),
-(2,5) and (2,6) will be valid for the guest.
+calculated from the APM and AQM; it is the Cartesian product of all assigned
+adapter numbers (APM) with all assigned queue indexes (AQM). For example, if
+adapters 1 and 2 and usage domains 5 and 6 are assigned to a guest:
+
+		        01           02
+		   +-----------+-----------+
+		05 |  (01,05)  |  (02,05)  |
+		   +-----------|-----------+
+		06 |  (01,06)  |  (02,06)  |
+		   +-----------|-----------+
+
+APQNs (01,05), (01,06), (02,05) and (02,06) will be valid for the guest.
 
 The APQNs can provide secure key functionality - i.e., a private key is stored
 on the adapter card for each of its domains - so each APQN must be assigned to
-at most one guest or to the linux host::
+at most one guest or to the linux host:
 
    Example 1: Valid configuration:
    ------------------------------
@@ -212,8 +229,8 @@ Reserve APQNs for exclusive use of KVM guests
 The following block diagram illustrates the mechanism by which APQNs are
 reserved::
 
-				+------------------+
-		 7 remove       |                  |
+				            +------------------+
+		 7 remove           |                  |
 	   +--------------------> cex4queue driver |
 	   |                    |                  |
 	   |                    +------------------+
@@ -248,7 +265,7 @@ reserved::
 	 +-------------------------------------------------------------+
 		     10  assign adapter/domain/control domain
 
-The process for reserving an AP queue for use by a KVM guest is:
+The process for partitioning AP queues for use by a KVM guest is:
 
 1. The administrator loads the vfio_ap device driver
 2. The vfio-ap driver during its initialization will register a single 'matrix'
@@ -409,12 +426,12 @@ VFIO_GROUP_NOTIFY_SET_KVM notifier callback is invoked. The notifier
 function is called when QEMU connects to KVM. The guest's AP matrix is
 configured via it's CRYCB by:
 
-* Setting the bits in the APM corresponding to the APIDs assigned to the
-  mediated matrix device via its 'assign_adapter' interface.
-* Setting the bits in the AQM corresponding to the domains assigned to the
-  mediated matrix device via its 'assign_domain' interface.
-* Setting the bits in the ADM corresponding to the domain dIDs assigned to the
-  mediated matrix device via its 'assign_control_domains' interface.
+* For each APQN assigned to the mediated matrix device via its 'assign_adapter'
+  and 'assign_domain' interfaces, if the APQN references an AP queue device
+  bound to the vfio_ap device driver, the bit corresponding to the APID will be
+  set in the APM and the bit corresponding to the APQI will be set in the AQM.
+* The bits corresponding to the domain IDs assigned to the mediated matrix
+  device via its 'assign_control_domains' interface will be set in the ADM.
 
 The CPU model features for AP
 -----------------------------
@@ -454,45 +471,540 @@ the APFT facility is not installed on the guest, then the probe of device
 drivers will fail since only type 10 and newer devices can be configured for
 guest use.
 
-Example
+Creating mediated matrix devices:
+================================
+When the vfio_ap module is initialized, it creates a /sys/devices/vfio_ap/matrix
+device then registers the matrix device with the VFIO mediated device core.
+This results in creation of the following sysfs structures:
+
+   /sys/devices/vfio_ap/matrix/
+   --- [mdev_supported_types]
+   ------ [vfio_ap-passthrough]
+   --------- create
+   --------- [devices]
+
+To create a mediated matrix device that can be used by a guest to gain access
+to AP queues, a UUID must be written into the 'create' attribute interface; for
+example:
+
+   uuidgen > create
+      or
+   echo $uuid > create
+
+This will result in the creation of a subdirectory for the mediated matrix
+device in the [devices] subdirectory:
+
+   /sys/devices/vfio_ap/matrix/
+   --- [mdev_supported_types]
+   ------ [vfio_ap-passthrough]
+   --------- create
+   --------- [devices]
+   ------------ [$uuid]
+   --------------- assign_adapter
+   --------------- assign_control_domain
+   --------------- assign_domain
+   --------------- control_domains
+   --------------- guest_matrix
+   --------------- matrix
+   --------------- remove
+   --------------- unassign_adapter
+   ----------------unassign_control_domain
+   ----------------unassign_domain
+
+The mediated matrix device is used to provision AP queues for a guest. When a
+guest using the mediated device is started, it will be granted access to the AP
+queues provisioned for the mediated device.
+
+Provisioning AP queues for the host and its guests:
+==================================================
+Sharing of AP queues between the host and a guest is prohibited. To manage the
+partitioning of queues between the host and its guests, the AP bus provides two
+sysfs interfaces that specify the APQNs of the AP queues owned by the host's
+zcrypt drivers. The location of the sysfs files containing the masks are:
+
+   /sys/bus/ap/apmask
+   /sys/bus/ap/aqmask
+
+The 'apmask' is a 256-bit mask that identifies a set of AP adapter IDs (APID).
+Each bit in the mask, from left to right, corresponds to an APID from 0-255.
+
+The 'aqmask' is a 256-bit mask that identifies a set of AP queue indexes (APQI).
+Each bit in the mask, from left to right, corresponds to an APQI from 0-255.
+
+The Cartesian product of the APIDs set in the apmask and the APQIs set in the
+aqmask identify the APQNs owned by the zcrypt device
+drivers.
+
+Take, for example, the following masks:
+
+   apmask: 0x7000000000000000000000000000000000000000000000000000000000000000
+
+   aqmask: 0x0180000000000000000000000000000000000000000000000000000000000000
+
+The bits set in apmask are bits 1, 2 and 3. The bits set in aqmask are bits 7
+and 8. The Cartesian product of the bits set in the two masks is:
+
+           01           02          03
+      +-----------+-----------+-----------+
+   07 |  (01,07)  |  (02,07)  |  (03,07)  |
+      +-----------|-----------+-----------+
+   08 |  (01,08)  |  (02,08)  |  (03,08)  |
+      +-----------|-----------+-----------+
+
+The masks indicate that the queues with APQNs (01,07), (01,08), (02,07),
+(02,08), (03,07) and (03,08) are owned by zcrypt. When the AP bus
+detects an AP queue device, its APQN is checked against the set of APQNs
+derived from the apmask and aqmask. If a match is detected, the zcrypt
+device driver registered for the device type of the queue will be probed. If
+a match is not detected and the device type of the queue is CEX4 or newer,
+the vfio_ap device driver will be probed.
+
+By default, the two masks are set to give ownership of all APQNs to zcrypt.
+There are two ways the default masks can be changed:
+
+1. The sysfs mask files can be edited by echoing a string into the
+   respective sysfs mask file in one of two formats:
+
+   * An absolute hex string starting with 0x - like "0x12345678" - sets
+     the mask. If the given string is shorter than the mask, it is padded
+	 with 0s on the right; for example, specifying a mask value of 0x41 is
+	 the same as specifying::
+
+	    0x4100000000000000000000000000000000000000000000000000000000000000
+
+        Keep in mind that the mask reads from left to right, so the mask
+        above identifies device numbers 1 and 7 (01000001).
+
+	    If the string is longer than the mask, the operation is terminated with
+	    an error (EINVAL).
+
+   * Individual bits in the mask can be switched on and off by specifying
+     each bit number to be switched in a comma separated list. Each bit
+     number string must be prefixed with a ('+') or minus ('-') to indicate
+     the corresponding bit is to be switched on ('+') or off ('-'). Some
+     valid values are:
+
+        - "+0"    switches bit 0 on
+	    - "-13"   switches bit 13 off
+	    - "+0x41" switches bit 65 on
+	    - "-0xff" switches bit 255 off
+
+	 The following example:
+
+	    +0,-6,+0x47,-0xf0
+
+	 Switches bits 0 and 71 (0x47) on
+
+	 Switches bits 6 and 240 (0xf0) off
+
+	 Note that the bits not specified in the list remain as they were before
+	 the operation.
+
+2. The masks can also be changed at boot time via parameters on the kernel
+   command line like this:
+
+      ap.apmask=0xffff ap.aqmask=0x40
+
+   This would create the following masks::
+
+	   apmask:
+	   0xffff000000000000000000000000000000000000000000000000000000000000
+
+	   aqmask:
+	   0x4000000000000000000000000000000000000000000000000000000000000000
+
+A couple of nuances of this model to keep in mind are:
+
+   * All APQNs containing the APID corresponding to the unset bits in the apmask
+     will be unavailable to the host, but available to its guests. Clearing a
+     bit in the apmask gives ownership of all queues of the corresponding
+     adapter to the vfio_ap device driver.
+
+   * All APQNs containing the APQI corresponding to the unset bits in the aqmask
+     will be unavailable to the host, but available to its guests. Clearing a
+     bit in the aqmask gives ownership of the corresponding queue index for all
+     adapters to the vfio_ap device driver.
+
+   * Only APQNs derived from the Cartesian product of the bits set in the
+     apmask and aqmask are available to the host.
+
+Provisioning of AP queues for each guest:
+========================================
+The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask sysfs interfaces only
+specify the APQNs that are owned by the host; all other APQNs are available for
+its guests. Since sharing of AP queues is prohibited, this pool of available
+APQNs must be further provisioned amongst guests. A mediated matrix device
+is created for each guest that will require access to one or more AP queues.
+To provision APQNs for a guest, a mediated matrix device provides two sysfs
+attribute interfaces in the mediated device's subdirectory:
+
+   /sys/devices/vfio_ap/matrix/
+   --- [mdev_supported_types]
+   ------ [vfio_ap-passthrough]
+   --------- create
+   --------- [devices]
+   ------------ [$uuid]
+   --------------- assign_adapter
+   --------------- assign_domain
+   --------------- matrix
+   --------------- guest_matrix
+
+   1. assign_adapter
+
+      An adapter is assigned to a mediated matrix device by echoing an adapter
+      number (APID) into the the 'assign_adapter' interface; for example, to
+      assign adapter 12:
+
+         echo 12 > assign_adapter
+         or
+         echo 0xc > assign_adapter
+
+      In order to successfully assign an adapter:
+
+      * The adapter number specified must represent a value from 0 up to the
+        maximum adapter number configured for the system. If an adapter number
+        higher than the maximum is specified, the operation will terminate with
+        an error (ENODEV).
+
+      * Each APQN that can be derived from the adapter ID and the IDs of
+        the previously assigned domains must not be reserved for use by the
+        zcrypt device drivers as specified by the /sys/bus/ap/apmask and
+        /sys/bus/ap/aqmask syfs interfaces. If any APQN is reserved, the
+        operation will terminate with an error (EADDRNOTAVAIL).
+
+      * No APQN that can be derived from the adapter ID and the IDs of the
+        previously assigned domains can be assigned to another mediated matrix
+        device. If an APQN is assigned to another mediated matrix device, the
+        operation will terminate with an error (EADDRINUSE).
+
+      When an adapter is assigned to a mediated matrix device, new APQNs will
+      be provisioned as a side effect. The APQNs are derived from the Cartesian
+      product of the APID of the adapter and the APQIs of the domains previously
+      assigned. If a guest is using the mediated device at the time of adapter
+      assignment, for each new APQN that references an AP queue device bound to
+      to the vfio_ap device driver, the queue will be hot plugged into the
+      guest.
+
+   2. assign_domain
+
+      A domain is assigned to a mediated matrix device by echoing a domain
+      number into the the 'assign_domain' interface; for example, to assign
+      domain 10:
+
+         echo 10 > assign_domain
+         or
+         echo 0xa > assign_domain
+
+      In order to successfully assign a domain:
+
+      * The domain number specified must represent a value from 0 up to the
+        maximum domain number configured for the system. If a domain number
+        higher than the maximum is specified, the operation will terminate with
+        an error (ENODEV). The maximum domain number can be determined by
+        printing the sysfs /sys/bus/ap/ap_max_domain_id attribute:
+
+           cat /sys/bus/ap/ap_max_domain_id
+
+      * Each APQN that can be derived from the domain ID and the IDs of
+        the previously assigned adapters must not be reserved for use by the
+        zcrypt device drivers as specified by the /sys/bus/ap/apmask and
+        /sys/bus/ap/aqmask syfs interfaces. If any APQN is reserved, the
+        operation will terminate with an error (EADDRNOTAVAIL).
+
+      * No APQN that can be derived from the domain ID and the IDs of the
+        previously assigned adapters can be assigned to another mediated matrix
+        device. If an APQN is assigned to another mediated matrix device, the
+        operation will terminate with an error (EADDRINUSE).
+
+      When a domain is assigned to a mediated matrix device, new APQNs will
+      be provisioned as a side effect. The APQNs are derived from the Cartesian
+      product of the APQI of the domain and the APIDs of the adapters previously
+      assigned. If a guest is using the mediated device at the time of domain
+      assignment, for each new APQN that references an AP queue device bound to
+      to the vfio_ap device driver, the queue will be hot plugged into the
+      guest.
+
+   3. matrix
+
+      Lists the APQNs assigned to the mediated matrix device. To view the APQNs
+      print the 'matrix' attribute to stdout:
+
+           cat matrix
+
+   4. guest_matrix
+
+      Lists the APQNs configured for a running guest. Note that APQNS may be
+      assigned to the mediated matrix device even if the queues referenced are
+      not available, either because they have yet to be installed or have been
+      taken out of the configuration via the SE or an SCLP Deconfigure Adjunct
+      Processor instruction. This is known as over-provisioning of AP resources.
+      If an AP adapter subsequently becomes available while a guest is using the
+      mediated device, the adapter will be hot plugged into the guest. To view
+      the APQNs of the queues in use by the guest, print the 'guest_matrix'
+      attribute to stdout:
+
+           cat guest_matrix
+
+      If a guest is not using the mediated device at the time the 'guest_matrix'
+      is displayed, an error (ENODEV) will be returned.
+
+Changing ownership of APQNs:
+===========================
+A change to the AP bus's /sys/bus/ap/apmask or /sys/bus/ap/aqmask always results
+in a change in ownership of one or more AP queues. Consider the following
+example:
+
+   * AP queues installed in the host:
+
+      /sys/bus/ap/devices
+      --- 01.0004
+      --- 01.0005
+      --- 01.0006
+      --- 01.0007
+
+      --- 02.0004
+      --- 02.0005
+      --- 02.0006
+      --- 02.0007
+
+      --- 03.0004
+      --- 03.0005
+      --- 03.0006
+      --- 03.0007
+
+   * The apmask and aqmask are configured as follows:
+
+      apmask: 0x7000000000000000000000000000000000000000000000000000000000000000
+      aqmask: 0x0c00000000000000000000000000000000000000000000000000000000000000
+
+      The masks specify the following:
+
+      owned by zcrypt:
+         01.0004
+         01.0005
+         02.0004
+         02.0005
+         03.0004
+         03.0005
+
+   * The matrix assigned to the mediated matrix device $uuid is:
+
+      01.0006
+      01.0007
+      02.0006
+      02.0007
+      03.0006
+      03.0007
+
+If we execute the following:
+
+   echo +8 > aqmask
+
+This will result in the following new masks:
+
+   apmask: 0x7000000000000000000000000000000000000000000000000000000000000000
+   aqmask: 0x0c80000000000000000000000000000000000000000000000000000000000000
+
+   owned by zcrypt:
+      01.0004
+      01.0005
+      01.0008
+      02.0004
+      02.0005
+      02.0008
+      03.0004
+      03.0005
+      03.0008
+
+From this, we can see that ownership of APQNs 01.0008, 02.0008, and 03.0008
+changed from the vfio_ap device driver to the host zcrypt driver. In order to
+change ownership of one or more APQNs from the vfio_ap device driver to a zcrypt
+driver, none of the APQNs can be assigned to a mediated matrix device. If even
+one of the APQNs is assigned to a mediated matrix device, the operation to
+change the mask will fail with an error (EADDRINUSE). In this example,
+APQNs 01.0007, 02.0007 and 03.0007 are assigned to mediated device $uuid, so the
+following aqmask operation would fail if attempted:
+
+   echo +7 > aqmask
+
+In order to free up APQNs 01.0007, 02.0007 and 03.0007 to make them available to
+zcrypt, the APQNs must first be unassigned from mediated matrix device $uuid.
+Unfortunately, the AP architecture precludes unassignment of individual APQNs,
+so we are left with the choice of either unassigning adapters 1, 2 and 3, or
+unassigning domain 7 from mediated device $uuid. Note that if an adapter is
+unassigned, then all domains within the adapter will become unavailable to the
+guest using the mediated device. In our example, unassigning adapters 1,2 and 3
+would leave a guest using mediated device $uuid without any adapters. If a
+domain is unassigned, then access to that domain within each adapter assigned to
+the mediated matrix device will become unavailable to the guest. For our
+example, if domain 7 is unassigned, that would remove access to AP queues
+01.0007, 02.0007 and 03.0007. It would, however, leave the guest with access to
+queues 01.0006, 02.0006 and 03.0006, so it would probably be better to unassign
+domain 7 lest the guest be left without access to any queues.
+
+To unassign adapters and domains from a mediated matrix device, two sysfs
+attribute interfaces are provided in the mediated device's subdirectory:
+
+   /sys/devices/vfio_ap/matrix/
+   --- [mdev_supported_types]
+   ------ [vfio_ap-passthrough]
+   --------- create
+   --------- [devices]
+   ------------ [$uuid]
+   --------------- unassign_adapter
+   --------------- unassign_domain
+
+   1. unassign_adapter
+
+      An adapter is unassigned from a mediated matrix device by echoing its
+      adapter number into the the 'unassign_adapter' interface; for example, to
+      unassign adapter 12:
+
+         echo 12 > unassign_adapter
+         or
+         echo 0xc > unassign_adapter
+
+      In order to successfully unassign an adapter:
+
+      * The adapter number specified must represent a value from 0 up to the
+        maximum adapter number configured for the system. If an adapter number
+        higher than the maximum is specified, the operation will terminate with
+        an error (ENODEV).
+
+      Note that if an adapter is unassigned while a guest is using the mediated
+      matrix device, the adapter will be hot unplugged from the running guest.
+
+   2. unassign_domain
+
+      A domain is unassigned from a mediated matrix device by echoing a domain
+      number into the the 'unassign_domain' interface; for example, to unassign
+      domain 10:
+
+         echo 10 > unassign_domain
+         or
+         echo 0xa > unassign_domain
+
+      In order to successfully unassign a domain:
+
+      * The domain number specified must represent a value from 0 up to the
+        maximum domain number configured for the system. If a domain number
+        higher than the maximum is specified, the operation will terminate with
+        an error (ENODEV). The maximum domain number can be determined by
+        printing the sysfs /sys/bus/ap/ap_max_domain_id attribute:
+
+           cat /sys/bus/ap/ap_max_domain_id
+
+      Note that if a domain is unassigned while a guest is using the mediated
+      matrix device, the domain will be hot unplugged from the running guest.
+
+Configuring control domains for guest access:
+============================================
+Recall that control domains are domains that are changed by an AP command sent
+to a usage domain; for example, to set the secure private key for a domain.
+
+Three sysfs attribute interfaces are provided in the mediated device's
+subdirectory to assign, unassign and display control domains:
+
+   /sys/devices/vfio_ap/matrix/
+   --- [mdev_supported_types]
+   ------ [vfio_ap-passthrough]
+   --------- create
+   --------- [devices]
+   ------------ [$uuid]
+   --------------- assign_control_domain
+   --------------- unassign_control_domain
+   --------------- control_domains
+
+   1. assign_control_domain
+
+      A control domain is assigned to a mediated matrix device by echoing a
+      domain number into the the 'assign_control_domain' interface; for
+      example, to assign control domain 10:
+
+         echo 10 > assign_control_domain
+         or
+         echo 0xa > assign_control_domain
+
+      In order to successfully assign a control domain:
+
+      * The domain number specified must represent a value from 0 up to the
+        maximum domain number configured for the system. If a domain number
+        higher than the maximum is specified, the operation will terminate with
+        an error (ENODEV). The maximum domain number can be determined by
+        printing the sysfs /sys/bus/ap/ap_max_domain_id attribute:
+
+           cat /sys/bus/ap/ap_max_domain_id
+
+      Note that if a control domain is assigned while a guest is using the
+      mediated matrix device, the control domain will be hot plugged into the
+      running guest.
+
+   2. unassign_control_domain
+
+      A control domain is unassigned from a mediated matrix device by echoing a
+      domain number into the the 'unassign_control_domain' interface; for
+      example, to unassign control domain 10:
+
+         echo 10 > unassign_control_domain
+         or
+         echo 0xa > unassign_control_domain
+
+      In order to successfully unassign a control domain:
+
+      * The domain number specified must represent a value from 0 up to the
+        maximum domain number configured for the system. If a domain number
+        higher than the maximum is specified, the operation will terminate with
+        an error (ENODEV). The maximum domain number can be determined by
+        printing the sysfs /sys/bus/ap/ap_max_domain_id attribute:
+
+           cat /sys/bus/ap/ap_max_domain_id
+
+      Note that if a control domain is unassigned while a guest is using the
+      mediated matrix device, the control domain will be hot unplugged from the
+      running guest.
+
+   3. control_domains
+
+      The list of control domains assigned to the mediated matrix device. Note
+      that the guest will have access to all control domains assigned. To
+      display the list:
+
+           cat control_domains
+
+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
-three guests such that executing the lszcrypt command on the guests would
-look like this:
+access to AP facilities. For this example, we will assume that adapters 4, 5
+and 6 and domains 4, 71 (0x47), 171 (0xab) and 255 (0xff) are assigned to the
+LPAR and are online. We will show how to configure three guests such that
+executing the lszcrypt command on the guests would look like this:
 
 Guest1
 ------
-=========== ===== ============
 CARD.DOMAIN TYPE  MODE
-=========== ===== ============
+------------------------------
 05          CEX5C CCA-Coproc
 05.0004     CEX5C CCA-Coproc
 05.00ab     CEX5C CCA-Coproc
 06          CEX5A Accelerator
 06.0004     CEX5A Accelerator
 06.00ab     CEX5C CCA-Coproc
-=========== ===== ============
 
 Guest2
 ------
-=========== ===== ============
 CARD.DOMAIN TYPE  MODE
-=========== ===== ============
+------------------------------
 05          CEX5A Accelerator
 05.0047     CEX5A Accelerator
 05.00ff     CEX5A Accelerator
-=========== ===== ============
 
 Guest2
 ------
-=========== ===== ============
 CARD.DOMAIN TYPE  MODE
-=========== ===== ============
+------------------------------
 06          CEX5A Accelerator
 06.0047     CEX5A Accelerator
 06.00ff     CEX5A Accelerator
-=========== ===== ============
 
 These are the steps:
 
@@ -517,124 +1029,28 @@ These are the steps:
    * VFIO_MDEV_DEVICE
    * KVM
 
-   If using make menuconfig select the following to build the vfio_ap module::
-
-     -> Device Drivers
-	-> IOMMU Hardware Support
-	   select S390 AP IOMMU Support
-	-> VFIO Non-Privileged userspace driver framework
-	   -> Mediated device driver frramework
-	      -> VFIO driver for Mediated devices
-     -> I/O subsystem
-	-> VFIO support for AP devices
+   If using make menuconfig select the following to build the vfio_ap module:
+   -> Device Drivers
+      -> IOMMU Hardware Support
+         select S390 AP IOMMU Support
+      -> VFIO Non-Privileged userspace driver framework
+         -> Mediated device driver frramework
+            -> VFIO driver for Mediated devices
+   -> I/O subsystem
+      -> VFIO support for AP devices
 
 2. Secure the AP queues to be used by the three guests so that the host can not
-   access them. To secure them, there are two sysfs files that specify
-   bitmasks marking a subset of the APQN range as 'usable by the default AP
-   queue device drivers' or 'not usable by the default device drivers' and thus
-   available for use by the vfio_ap device driver'. The location of the sysfs
-   files containing the masks are::
-
-     /sys/bus/ap/apmask
-     /sys/bus/ap/aqmask
-
-   The 'apmask' is a 256-bit mask that identifies a set of AP adapter IDs
-   (APID). Each bit in the mask, from left to right (i.e., from most significant
-   to least significant bit in big endian order), corresponds to an APID from
-   0-255. If a bit is set, the APID is marked as usable only by the default AP
-   queue device drivers; otherwise, the APID is usable by the vfio_ap
-   device driver.
-
-   The 'aqmask' is a 256-bit mask that identifies a set of AP queue indexes
-   (APQI). Each bit in the mask, from left to right (i.e., from most significant
-   to least significant bit in big endian order), corresponds to an APQI from
-   0-255. If a bit is set, the APQI is marked as usable only by the default AP
-   queue device drivers; otherwise, the APQI is usable by the vfio_ap device
-   driver.
-
-   Take, for example, the following mask::
-
-      0x7dffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff
-
-    It indicates:
-
-      1, 2, 3, 4, 5, and 7-255 belong to the default drivers' pool, and 0 and 6
-      belong to the vfio_ap device driver's pool.
-
-   The APQN of each AP queue device assigned to the linux host is checked by the
-   AP bus against the set of APQNs derived from the cross product of APIDs
-   and APQIs marked as usable only by the default AP queue device drivers. If a
-   match is detected,  only the default AP queue device drivers will be probed;
-   otherwise, the vfio_ap device driver will be probed.
-
-   By default, the two masks are set to reserve all APQNs for use by the default
-   AP queue device drivers. There are two ways the default masks can be changed:
-
-   1. The sysfs mask files can be edited by echoing a string into the
-      respective sysfs mask file in one of two formats:
-
-      * An absolute hex string starting with 0x - like "0x12345678" - sets
-	the mask. If the given string is shorter than the mask, it is padded
-	with 0s on the right; for example, specifying a mask value of 0x41 is
-	the same as specifying::
-
-	   0x4100000000000000000000000000000000000000000000000000000000000000
-
-	Keep in mind that the mask reads from left to right (i.e., most
-	significant to least significant bit in big endian order), so the mask
-	above identifies device numbers 1 and 7 (01000001).
-
-	If the string is longer than the mask, the operation is terminated with
-	an error (EINVAL).
-
-      * Individual bits in the mask can be switched on and off by specifying
-	each bit number to be switched in a comma separated list. Each bit
-	number string must be prepended with a ('+') or minus ('-') to indicate
-	the corresponding bit is to be switched on ('+') or off ('-'). Some
-	valid values are:
-
-	   - "+0"    switches bit 0 on
-	   - "-13"   switches bit 13 off
-	   - "+0x41" switches bit 65 on
-	   - "-0xff" switches bit 255 off
-
-	The following example:
-
-	      +0,-6,+0x47,-0xf0
-
-	Switches bits 0 and 71 (0x47) on
-
-	Switches bits 6 and 240 (0xf0) off
-
-	Note that the bits not specified in the list remain as they were before
-	the operation.
-
-   2. The masks can also be changed at boot time via parameters on the kernel
-      command line like this:
+   access them.  There is no way to secure the specific AP queues 05.0004,
+   05.0047, 05.00ab, 05.00ff, 06.0004, 06.0047, 06.00ab, and 06.00ff for use by
+   the guests, so we are left with either securing all queues on adapters 05 and
+   06, or queues 0004, 0047, 00ab and 00ff can be secured on all adapters.
 
-	 ap.apmask=0xffff ap.aqmask=0x40
-
-	 This would create the following masks::
-
-	    apmask:
-	    0xffff000000000000000000000000000000000000000000000000000000000000
-
-	    aqmask:
-	    0x4000000000000000000000000000000000000000000000000000000000000000
-
-	 Resulting in these two pools::
-
-	    default drivers pool:    adapter 0-15, domain 1
-	    alternate drivers pool:  adapter 16-255, domains 0, 2-255
-
-Securing the APQNs for our example
-----------------------------------
-   To secure the AP queues 05.0004, 05.0047, 05.00ab, 05.00ff, 06.0004, 06.0047,
-   06.00ab, and 06.00ff for use by the vfio_ap device driver, the corresponding
-   APQNs can either be removed from the default masks::
+   To secure all queues on adapters 05 and 06:
 
       echo -5,-6 > /sys/bus/ap/apmask
 
+   To secure queues 0004, 0047, 00ab, and 00ff on all adapters:
+
       echo -4,-0x47,-0xab,-0xff > /sys/bus/ap/aqmask
 
    Or the masks can be set as follows::
@@ -645,22 +1061,28 @@ Securing the APQNs for our example
       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::
-
-     /sys/bus/ap
-     ... [drivers]
-     ...... [vfio_ap]
-     ......... [05.0004]
-     ......... [05.0047]
-     ......... [05.00ab]
-     ......... [05.00ff]
-     ......... [06.0004]
-     ......... [06.0047]
-     ......... [06.00ab]
-     ......... [06.00ff]
+   For this example, we will choose to secure queues 0004, 0047, 00ab, and 00ff
+   on all adapters. This will result in AP queues 04.0004, 04.0047, 04.00ab,
+   04.00ff, 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:
+
+   /sys/bus/ap
+   ... [drivers]
+   ...... [vfio_ap]
+   ......... [04.0004]
+   ......... [04.0047]
+   ......... [04.00ab]
+   ......... [04.00ff]
+   ......... [05.0004]
+   ......... [05.0047]
+   ......... [05.00ab]
+   ......... [05.00ff]
+   ......... [06.0004]
+   ......... [06.0047]
+   ......... [06.00ab]
+   ......... [06.00ff]
 
    Keep in mind that only type 10 and newer adapters (i.e., CEX4 and later)
    can be bound to the vfio_ap device driver. The reason for this is to
@@ -669,36 +1091,30 @@ Securing the APQNs for our example
    future and for which there are few older systems on which to test.
 
    The administrator, therefore, must take care to secure only AP queues that
-   can be bound to the vfio_ap device driver. The device type for a given AP
-   queue device can be read from the parent card's sysfs directory. For example,
-   to see the hardware type of the queue 05.0004:
+   can be bound to the vfio_ap device driver, or those queues will not get bound
+   to any driver. The device type for a given AP queue device can be read from
+   the parent card's sysfs directory. For example, to see the hardware type of
+   the queue 05.0004:
 
-     cat /sys/bus/ap/devices/card05/hwtype
+      cat /sys/bus/ap/devices/card05/hwtype
 
    The hwtype must be 10 or higher (CEX4 or newer) in order to be bound to the
    vfio_ap device driver.
 
 3. Create the mediated devices needed to configure the AP matrixes for the
-   three guests and to provide an interface to the vfio_ap driver for
-   use by the guests::
-
-     /sys/devices/vfio_ap/matrix/
-     --- [mdev_supported_types]
-     ------ [vfio_ap-passthrough] (passthrough mediated matrix device type)
-     --------- create
-     --------- [devices]
+   three guests. To create the mediated devices:
 
-   To create the mediated devices for the three guests::
+      cd /sys/devices/vfio_ap/matrix/mdev_supported_types/vfio_ap-passthrough
 
-	uuidgen > create
-	uuidgen > create
-	uuidgen > create
+      uuidgen > create
+      uuidgen > create
+      uuidgen > create
 
 	or
 
-	echo $uuid1 > create
-	echo $uuid2 > create
-	echo $uuid3 > create
+      echo $uuid1 > create
+      echo $uuid2 > create
+      echo $uuid3 > create
 
    This will create three mediated devices in the [devices] subdirectory named
    after the UUID written to the create attribute file. We call them $uuid1,
@@ -768,50 +1184,7 @@ Securing the APQNs for our example
       echo 0x47 > assign_domain
       echo 0xff > assign_domain
 
-   In order to successfully assign an adapter:
-
-   * The adapter number specified must represent a value from 0 up to the
-     maximum adapter number configured for the system. If an adapter number
-     higher than the maximum is specified, the operation will terminate with
-     an error (ENODEV).
-
-   * All APQNs that can be derived from the adapter ID and the IDs of
-     the previously assigned domains must be bound to the vfio_ap device
-     driver. If no domains have yet been assigned, then there must be at least
-     one APQN with the specified APID bound to the vfio_ap driver. If no such
-     APQNs are bound to the driver, the operation will terminate with an
-     error (EADDRNOTAVAIL).
-
-     No APQN that can be derived from the adapter ID and the IDs of the
-     previously assigned domains can be assigned to another mediated matrix
-     device. If an APQN is assigned to another mediated matrix device, the
-     operation will terminate with an error (EADDRINUSE).
-
-   In order to successfully assign a domain:
-
-   * The domain number specified must represent a value from 0 up to the
-     maximum domain number configured for the system. If a domain number
-     higher than the maximum is specified, the operation will terminate with
-     an error (ENODEV).
-
-   * All APQNs that can be derived from the domain ID and the IDs of
-     the previously assigned adapters must be bound to the vfio_ap device
-     driver. If no domains have yet been assigned, then there must be at least
-     one APQN with the specified APQI bound to the vfio_ap driver. If no such
-     APQNs are bound to the driver, the operation will terminate with an
-     error (EADDRNOTAVAIL).
-
-     No APQN that can be derived from the domain ID and the IDs of the
-     previously assigned adapters can be assigned to another mediated matrix
-     device. If an APQN is assigned to another mediated matrix device, the
-     operation will terminate with an error (EADDRINUSE).
-
-   In order to successfully assign a control domain, the domain number
-   specified must represent a value from 0 up to the maximum domain number
-   configured for the system. If a control domain number higher than the maximum
-   is specified, the operation will terminate with an error (ENODEV).
-
-5. Start Guest1::
+5. Start Guest1:
 
      /usr/bin/qemu-system-s390x ... -cpu host,ap=on,apqci=on,apft=on \
 	-device vfio-ap,sysfsdev=/sys/devices/vfio_ap/matrix/$uuid1 ...
@@ -851,16 +1224,74 @@ remove it if no guest will use it during the remaining lifetime of the linux
 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.
+Dynamic Changes to AP Configuration using the Support Element (SE):
+==================================================================
+The SE can be used to dynamically make the following changes to the AP
+configuration for an LPAR in which a linux host is running:
+
+   * Configure one or more adapters on
+
+     Configuring an adapter on sets its state to online, thus making it
+     available to the LPAR to which it is assigned. When an adapter is
+     configured on, it immediately becomes available to the LPAR as well as to
+     any guests using a mediated device to which the adapter is assigned.
+
+   * Configure one or more adapters off
+
+     Configuring an adapter off sets its state to standby, thus making it
+     unavailable to the LPAR to which it is assigned. When an adapter is
+     configured off, it immediately becomes unavailable to the LPAR as well as
+     to any guests using a mediated device to which the adapter is assigned.
+
+   * Add adapters or domains to the LPAR configuration
+
+     Adapters and/or domains can be assigned to an LPAR using the Change LPAR
+     Cryptographic Controls task. To make dynamic changes to the AP
+     configuration for an LPAR Running a linux guest, the online adapters
+     assigned to the LPAR must first be configured off. After performing the
+     adapter and/or domain assignments, the AP resources will automatically
+     become available to the linux host running in the LPAR as well as any
+     guests using a mediated device to which the adpaters and/or domains are
+     assigned.
+
+   * Remove adapters or domains from the LPAR configuration
+
+     Adapters and/or domains can be unassigned from an LPAR using the Change
+     LPAR Cryptographic Controls task. To make dynamic changes to the AP
+     configuration for an LPAR Running a linux guest, the online adapters
+     assigned to the LPAR must first be configured off. After performing the
+     adapter and/or domain unassignments, the AP resources will automatically
+     become unavailable to the linux host running in the LPAR as well as any
+     guests using a mediated device to which the adpaters and/or domains are
+     assigned.
+
+Dynamic Changes to the AP Configuration using the SCLP command:
+==============================================================
+The following SCLP commands may be used to dynamically configure AP adapters on
+and off:
+
+* Configure Adjunct Processor
+
+  The 'Configure Adjunct Processor' command sets an AP adapter's state to
+  online, thus making it available to the LPARs to which it is assigned. It will
+  likewise become available to any linux guest using a mediated device to which
+  the adapter is assigned.
+
+* Deconfigure Adjunct Processor
+
+  The 'Deconfigure Adjunct Processor' command sets an AP adapter's state to
+  standby, thus making it unavailable to the LPARs to which it is assigned. It
+  will likewise become unavailable to any linux guest using a mediated device to
+  which the adapter is assigned.
+
+
 
-* 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
+Live migration:
+==============
+Live guest migration is not supported for guests using AP devices. All AP
+devices in use by the guest must be unplugged prior to initiating live
+migration (see "Hot plug/unplug via mdev matrix device sysfs interfaces" section
+above). If you are using QEMU to run your guest and it supports hot plug/unplug
+of the vfio-ap device, this would be another option (consult the QEMU
+documentation for details).
 
-* Live guest migration is not supported for guests using AP devices.
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices
  2019-09-13 21:26 ` [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices Tony Krowiak
@ 2019-09-18 17:04   ` Halil Pasic
  2019-09-18 21:22     ` Tony Krowiak
  2019-09-19 10:34   ` Halil Pasic
  1 sibling, 1 reply; 20+ messages in thread
From: Halil Pasic @ 2019-09-18 17:04 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On Fri, 13 Sep 2019 17:26:52 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	unsigned long apid, apqi;
> +	unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
> +			       sizeof(unsigned long);
> +
> +	memset(matrix_mdev->crycb.apm, 0, masksz);
> +	memset(matrix_mdev->crycb.apm, 0, masksz);

I guess you wanted to zero out aqm here (and not apm again)!

> +	memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices
  2019-09-18 17:04   ` Halil Pasic
@ 2019-09-18 21:22     ` Tony Krowiak
  0 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-18 21:22 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On 9/18/19 1:04 PM, Halil Pasic wrote:
> On Fri, 13 Sep 2019 17:26:52 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +	unsigned long apid, apqi;
>> +	unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
>> +			       sizeof(unsigned long);
>> +
>> +	memset(matrix_mdev->crycb.apm, 0, masksz);
>> +	memset(matrix_mdev->crycb.apm, 0, masksz);
> 
> I guess you wanted to zero out aqm here (and not apm again)!

Cut and paste without edit. I'll fix it.

> 
>> +	memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices
  2019-09-13 21:26 ` [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices Tony Krowiak
  2019-09-18 17:04   ` Halil Pasic
@ 2019-09-19 10:34   ` Halil Pasic
  2019-09-20 14:24     ` Tony Krowiak
  1 sibling, 1 reply; 20+ messages in thread
From: Halil Pasic @ 2019-09-19 10:34 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On Fri, 13 Sep 2019 17:26:52 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	unsigned long apid, apqi;
> +	unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
> +			       sizeof(unsigned long);
> +
> +	memset(matrix_mdev->crycb.apm, 0, masksz);
> +	memset(matrix_mdev->crycb.apm, 0, masksz);
> +	memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
> +
> +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
> +			     matrix_mdev->matrix.apm_max + 1) {
> +		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
> +				     matrix_mdev->matrix.aqm_max + 1) {
> +			if (vfio_ap_find_queue(AP_MKQID(apid, apqi))) {
> +				if (!test_bit_inv(apid, matrix_mdev->crycb.apm))
> +					set_bit_inv(apid,
> +						    matrix_mdev->crycb.apm);
> +				if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm))
> +					set_bit_inv(apqi,
> +						    matrix_mdev->crycb.aqm);
> +			}
> +		}
> +	}
> +}

Even with the discussed typo fixed (zero crycb.aqm) this procedure does
not make sense to me. :(

If in doubt please consider the following example:
matrix_mdev->matrix.apm and matrix_mdev->matrix.aqm have both just bits
0 and 1 set (i.e. first byte 0xC0 the rest of the bytes 0x0). Queues
bound to the vfio_ap driver (0,0), (0,1), (1,0); not bound to vfio_ap is
however (1,1). If I read this correctly this filtering logic would grant
access to (1,1) which seems to contradict with the stated intention.

Regards,
Halil




^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices
  2019-09-19 10:34   ` Halil Pasic
@ 2019-09-20 14:24     ` Tony Krowiak
  2019-09-20 15:44       ` Tony Krowiak
  0 siblings, 1 reply; 20+ messages in thread
From: Tony Krowiak @ 2019-09-20 14:24 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On 9/19/19 6:34 AM, Halil Pasic wrote:
> On Fri, 13 Sep 2019 17:26:52 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +	unsigned long apid, apqi;
>> +	unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
>> +			       sizeof(unsigned long);
>> +
>> +	memset(matrix_mdev->crycb.apm, 0, masksz);
>> +	memset(matrix_mdev->crycb.apm, 0, masksz);
>> +	memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
>> +
>> +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>> +			     matrix_mdev->matrix.apm_max + 1) {
>> +		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>> +				     matrix_mdev->matrix.aqm_max + 1) {
>> +			if (vfio_ap_find_queue(AP_MKQID(apid, apqi))) {
>> +				if (!test_bit_inv(apid, matrix_mdev->crycb.apm))
>> +					set_bit_inv(apid,
>> +						    matrix_mdev->crycb.apm);
>> +				if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm))
>> +					set_bit_inv(apqi,
>> +						    matrix_mdev->crycb.aqm);
>> +			}
>> +		}
>> +	}
>> +}
> 
> Even with the discussed typo fixed (zero crycb.aqm) this procedure does
> not make sense to me. :(
> 
> If in doubt please consider the following example:
> matrix_mdev->matrix.apm and matrix_mdev->matrix.aqm have both just bits
> 0 and 1 set (i.e. first byte 0xC0 the rest of the bytes 0x0). Queues
> bound to the vfio_ap driver (0,0), (0,1), (1,0); not bound to vfio_ap is
> however (1,1). If I read this correctly this filtering logic would grant
> access to (1,1) which seems to contradict with the stated intention.

Yep, I see your point. I'll have to rework this code.

> 
> Regards,
> Halil
> 
> 
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices
  2019-09-20 14:24     ` Tony Krowiak
@ 2019-09-20 15:44       ` Tony Krowiak
  0 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-09-20 15:44 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On 9/20/19 10:24 AM, Tony Krowiak wrote:
> On 9/19/19 6:34 AM, Halil Pasic wrote:
>> On Fri, 13 Sep 2019 17:26:52 -0400
>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>
>>> +static void vfio_ap_mdev_get_crycb_matrix(struct ap_matrix_mdev 
>>> *matrix_mdev)
>>> +{
>>> +    unsigned long apid, apqi;
>>> +    unsigned long masksz = BITS_TO_LONGS(AP_DEVICES) *
>>> +                   sizeof(unsigned long);
>>> +
>>> +    memset(matrix_mdev->crycb.apm, 0, masksz);
>>> +    memset(matrix_mdev->crycb.apm, 0, masksz);
>>> +    memcpy(matrix_mdev->crycb.adm, matrix_mdev->matrix.adm, masksz);
>>> +
>>> +    for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>>> +                 matrix_mdev->matrix.apm_max + 1) {
>>> +        for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>>> +                     matrix_mdev->matrix.aqm_max + 1) {
>>> +            if (vfio_ap_find_queue(AP_MKQID(apid, apqi))) {
>>> +                if (!test_bit_inv(apid, matrix_mdev->crycb.apm))
>>> +                    set_bit_inv(apid,
>>> +                            matrix_mdev->crycb.apm);
>>> +                if (!test_bit_inv(apqi, matrix_mdev->crycb.aqm))
>>> +                    set_bit_inv(apqi,
>>> +                            matrix_mdev->crycb.aqm);
>>> +            }
>>> +        }
>>> +    }
>>> +}
>>
>> Even with the discussed typo fixed (zero crycb.aqm) this procedure does
>> not make sense to me. :(
>>
>> If in doubt please consider the following example:
>> matrix_mdev->matrix.apm and matrix_mdev->matrix.aqm have both just bits
>> 0 and 1 set (i.e. first byte 0xC0 the rest of the bytes 0x0). Queues
>> bound to the vfio_ap driver (0,0), (0,1), (1,0); not bound to vfio_ap is
>> however (1,1). If I read this correctly this filtering logic would grant
>> access to (1,1) which seems to contradict with the stated intention.
> 
> Yep, I see your point. I'll have to rework this code.

As I see it, we have two choices here:

1. Do not set bit 1 in the APM of the guest's CRYCB because queue
    01.0001 is not bound to the vfio_ap device driver. This would
    preclude guest access to any domain in adapter 1 - i.e., the
    guest would have access to queues 00.0000 and 00.0001.

2. Do not set bit 1 in the AQM of the guest's CRYCB because queue
    01.0001 is not bound to the vfio_ap device driver. This would
    preclude guest access to domain 1 in both adapters - i.e., the
    guest would have access to queues 00.0000 and 01.0000.

There are ramifications for either choice. For example, if only one
adapter is assigned to the mdev, then option 1 will result in the
guest not having access to any AP queues. Likewise, the guest will
not get access to any AP queues if only one domain is assigned to
the mdev. Neither choice is optimal, but option 1 seems to make sense
because it somewhat models the behavior of the host system. For example,
only AP adapters can be configured online/offline and in order to
add/remove domains, an adapter must first be configured offline.

> 
>>
>> Regards,
>> Halil
>>
>>
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support
  2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
                   ` (9 preceding siblings ...)
  2019-09-13 21:26 ` [PATCH v6 10/10] s390: vfio-ap: update documentation Tony Krowiak
@ 2019-10-08 10:48 ` Halil Pasic
  2019-10-08 12:57   ` Pierre Morel
  2019-10-15 20:27   ` Tony Krowiak
  10 siblings, 2 replies; 20+ messages in thread
From: Halil Pasic @ 2019-10-08 10:48 UTC (permalink / raw)
  To: Tony Krowiak
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On Fri, 13 Sep 2019 17:26:48 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> The current design for AP pass-through does not support making dynamic
> changes to the AP matrix of a running guest resulting in three deficiencies
> this patch series is intended to mitigate:
> 
> 1. Adapters, domains and control domains can not be added to or removed
>    from a running guest. In order to modify a guest's AP configuration,
>    the guest must be terminated; only then can AP resources be assigned
>    to or unassigned from the guest's matrix mdev. The new AP configuration
>    becomes available to the guest when it is subsequently restarted.
> 
> 2. The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask interfaces can
>    be modified by a root user without any restrictions. A change to either
>    mask can result in AP queue devices being unbound from the vfio_ap
>    device driver and bound to a zcrypt device driver even if a guest is
>    using the queues, thus giving the host access to the guest's private
>    crypto data and vice versa.
> 
> 3. The APQNs derived from the Cartesian product of the APIDs of the
>    adapters and APQIs of the domains assigned to a matrix mdev must
>    reference an AP queue device bound to the vfio_ap device driver. 
> 
> This patch series introduces the following changes to the current design
> to alleviate the shortcomings described above as well as to implement more
> of the AP architecture:
> 
> 1. A root user will be prevented from making changes to the AP bus's
>    /sys/bus/ap/apmask or /sys/bus/ap/aqmask if the ownership of an APQN
>    changes from the vfio_ap device driver to a zcrypt driver when the APQN
>    is assigned to a matrix mdev.
> 
> 2. The sysfs bind/unbind interfaces will be disabled for the vfio_ap
>    device driver.
> 

Doesn't this have the potential of breaking some userspace stuff that
might be out there?

> 3. Allow AP resources to be assigned to or removed from a matrix mdev
>    while a guest is using it and hot plug the resource into or hot unplug
>    the resource from the running guest.

This looks like a natural extension of the interface -- i.e. should not
break any userspace.

> 
> 4. Allow assignment of an AP adapter or domain to a matrix mdev even if it
>    results in assignment of an APQN that does not reference an AP queue
>    device bound to the vfio_ap device driver, as long as the APQN is owned
>    by the vfio_ap driver. Allowing over-provisioning of AP resources
>    better models the architecture which does not preclude assigning AP
>    resources that are not yet available in the system. If/when the queue
>    becomes available to the host, it will immediately also become available
>    to the guest.

Same here -- I don't think this change breaks any userspace.

> 
> 1. Rationale for changes to AP bus's apmask/aqmask interfaces:
> ----------------------------------------------------------
> Due to the extremely sensitive nature of cryptographic data, it is
> imperative that great care be taken to ensure that such data is secured.
> Allowing a root user, either inadvertently or maliciously, to configure
> these masks such that a queue is shared between the host and a guest is
> not only avoidable, it is advisable. It was suggested that this scenario
> is better handled in user space with management software, but that does
> not preclude a malicious administrator from using the sysfs interfaces
> to gain access to a guest's crypto data. It was also suggested that this
> scenario could be avoided by taking access to the adapter away from the
> guest and zeroing out the queues prior to the vfio_ap driver releasing the
> device; however, stealing an adapter in use from a guest as a by-product
> of an operation is bad and will likely cause problems for the guest
> unnecessarily. It was decided that the most effective solution with the
> least number of negative side effects is to prevent the situation at the
> source.
> 
> 2. Rationale for disabling bind/unbind interfaces for vfio_ap driver:
> -----------------------------------------------------------------
> By disabling the bind/unbind interfaces for the vfio_ap device driver, 
> the user is forced to use the AP bus's apmask/aqmask interfaces to control
> the probing and removing of AP queues. There are two primary reasons for
> disabling the bind/unbind interfaces for the vfio_ap device driver:
> 
> * The device architecture does not provide a means to prevent unbinding
>   a device from a device driver, so an AP queue device can be unbound
>   from the vfio_ap driver even when the queue is in use by a guest. By
>   disabling the unbind interface, the user is forced to use the AP bus's
>   apmask/aqmask interfaces which will prevent this.
> 

Isn't this fixed by your filtering (if implemented correctly)? BTW I believe
it solves the problem regardless whether the unbind was triggered by the
drivers unbind attribute or by a[pq]mask.

> * Binding of AP queues is controlled by the AP bus /sys/bus/ap/apmask and
>   /sys/bus/ap/aqmask interfaces. If the masks indicate that an APQN is
>   owned by zcrypt, trying to bind it to the vfio_ap device driver will
>   fail; therefore, the bind interface is somewhat redundant and certainly
>   unnecessary.        
>   

[..]

> Tony Krowiak (10):
>   s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks
>   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: filter CRYCB bits for unavailable queue devices
>   s390: vfio-ap: sysfs attribute to display the guest CRYCB
>   s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove
>     callbacks
>   s390: zcrypt: driver callback to indicate resource in use
>   s390: vfio-ap: implement in-use callback for vfio_ap driver
>   s390: vfio-ap: added versioning to vfio_ap module
>   s390: vfio-ap: update documentation

I believe it would be worthwhile to reorder the patches (fixes and
re-factoring first, the features).

Regards,
Halil

> 
>  Documentation/s390/vfio-ap.rst        | 899 +++++++++++++++++++++++++---------
>  drivers/s390/crypto/ap_bus.c          | 144 +++++-
>  drivers/s390/crypto/ap_bus.h          |   4 +
>  drivers/s390/crypto/vfio_ap_drv.c     |  27 +-
>  drivers/s390/crypto/vfio_ap_ops.c     | 610 ++++++++++++++---------
>  drivers/s390/crypto/vfio_ap_private.h |  12 +-
>  6 files changed, 1200 insertions(+), 496 deletions(-)
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support
  2019-10-08 10:48 ` [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Halil Pasic
@ 2019-10-08 12:57   ` Pierre Morel
  2019-10-15 20:33     ` Tony Krowiak
  2019-10-15 20:27   ` Tony Krowiak
  1 sibling, 1 reply; 20+ messages in thread
From: Pierre Morel @ 2019-10-08 12:57 UTC (permalink / raw)
  To: Halil Pasic, Tony Krowiak
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, alex.williamson, kwankhede, jjherne


On 10/8/19 12:48 PM, Halil Pasic wrote:
> On Fri, 13 Sep 2019 17:26:48 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> The current design for AP pass-through does not support making dynamic
>> changes to the AP matrix of a running guest resulting in three deficiencies
>> this patch series is intended to mitigate:
>>
>> 1. Adapters, domains and control domains can not be added to or removed
>>     from a running guest. In order to modify a guest's AP configuration,
>>     the guest must be terminated; only then can AP resources be assigned
>>     to or unassigned from the guest's matrix mdev. The new AP configuration
>>     becomes available to the guest when it is subsequently restarted.
>>
>> 2. The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask interfaces can
>>     be modified by a root user without any restrictions. A change to either
>>     mask can result in AP queue devices being unbound from the vfio_ap
>>     device driver and bound to a zcrypt device driver even if a guest is
>>     using the queues, thus giving the host access to the guest's private
>>     crypto data and vice versa.
>>
>> 3. The APQNs derived from the Cartesian product of the APIDs of the
>>     adapters and APQIs of the domains assigned to a matrix mdev must
>>     reference an AP queue device bound to the vfio_ap device driver.
>>
>> This patch series introduces the following changes to the current design
>> to alleviate the shortcomings described above as well as to implement more
>> of the AP architecture:
>>
>> 1. A root user will be prevented from making changes to the AP bus's
>>     /sys/bus/ap/apmask or /sys/bus/ap/aqmask if the ownership of an APQN
>>     changes from the vfio_ap device driver to a zcrypt driver when the APQN
>>     is assigned to a matrix mdev.
>>
>> 2. The sysfs bind/unbind interfaces will be disabled for the vfio_ap
>>     device driver.
>>
> Doesn't this have the potential of breaking some userspace stuff that
> might be out there?
>
>> 3. Allow AP resources to be assigned to or removed from a matrix mdev
>>     while a guest is using it and hot plug the resource into or hot unplug
>>     the resource from the running guest.
> This looks like a natural extension of the interface -- i.e. should not
> break any userspace.
>
>> 4. Allow assignment of an AP adapter or domain to a matrix mdev even if it
>>     results in assignment of an APQN that does not reference an AP queue
>>     device bound to the vfio_ap device driver, as long as the APQN is owned
>>     by the vfio_ap driver. Allowing over-provisioning of AP resources
>>     better models the architecture which does not preclude assigning AP
>>     resources that are not yet available in the system. If/when the queue
>>     becomes available to the host, it will immediately also become available
>>     to the guest.
> Same here -- I don't think this change breaks any userspace.
>
>> 1. Rationale for changes to AP bus's apmask/aqmask interfaces:
>> ----------------------------------------------------------
>> Due to the extremely sensitive nature of cryptographic data, it is
>> imperative that great care be taken to ensure that such data is secured.
>> Allowing a root user, either inadvertently or maliciously, to configure
>> these masks such that a queue is shared between the host and a guest is
>> not only avoidable, it is advisable.

Just curious: how is it possible to do such a configuration?


>>   It was suggested that this scenario
>> is better handled in user space with management software, but that does
>> not preclude a malicious administrator from using the sysfs interfaces
>> to gain access to a guest's crypto data. It was also suggested that this
>> scenario could be avoided by taking access to the adapter away from the
>> guest and zeroing out the queues prior to the vfio_ap driver releasing the
>> device; however, stealing an adapter in use from a guest as a by-product
>> of an operation is bad and will likely cause problems for the guest
>> unnecessarily. It was decided that the most effective solution with the
>> least number of negative side effects is to prevent the situation at the
>> source.


Stealing an adapter in use by a guest, insn't it what is done if we 
allow to unassign an AP/Domain using the unassign sysfs interface when 
the mediated device is in use by the guest?


>>
>> 2. Rationale for disabling bind/unbind interfaces for vfio_ap driver:
>> -----------------------------------------------------------------
>> By disabling the bind/unbind interfaces for the vfio_ap device driver,
>> the user is forced to use the AP bus's apmask/aqmask interfaces to control
>> the probing and removing of AP queues. There are two primary reasons for
>> disabling the bind/unbind interfaces for the vfio_ap device driver:
>>
>> * The device architecture does not provide a means to prevent unbinding
>>    a device from a device driver, so an AP queue device can be unbound
>>    from the vfio_ap driver even when the queue is in use by a guest. By
>>    disabling the unbind interface, the user is forced to use the AP bus's
>>    apmask/aqmask interfaces which will prevent this.
>>
> Isn't this fixed by your filtering (if implemented correctly)? BTW I believe
> it solves the problem regardless whether the unbind was triggered by the
> drivers unbind attribute or by a[pq]mask
>
>> * Binding of AP queues is controlled by the AP bus /sys/bus/ap/apmask and
>>    /sys/bus/ap/aqmask interfaces. If the masks indicate that an APQN is
>>    owned by zcrypt, trying to bind it to the vfio_ap device driver will
>>    fail; therefore, the bind interface is somewhat redundant and certainly
>>    unnecessary.
>>    
>>
>>
>> Tony Krowiak (10):
>>    s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks
>>    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: filter CRYCB bits for unavailable queue devices
>>    s390: vfio-ap: sysfs attribute to display the guest CRYCB
>>    s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove
>>      callbacks
>>    s390: zcrypt: driver callback to indicate resource in use
>>    s390: vfio-ap: implement in-use callback for vfio_ap driver
>>    s390: vfio-ap: added versioning to vfio_ap module
>>    s390: vfio-ap: update documentation
> I believe it would be worthwhile to reorder the patches (fixes and
> re-factoring first, the features).
>
> Regards,
> Halil
>
>>   Documentation/s390/vfio-ap.rst        | 899 +++++++++++++++++++++++++---------
>>   drivers/s390/crypto/ap_bus.c          | 144 +++++-
>>   drivers/s390/crypto/ap_bus.h          |   4 +
>>   drivers/s390/crypto/vfio_ap_drv.c     |  27 +-
>>   drivers/s390/crypto/vfio_ap_ops.c     | 610 ++++++++++++++---------
>>   drivers/s390/crypto/vfio_ap_private.h |  12 +-
>>   6 files changed, 1200 insertions(+), 496 deletions(-)
>>
-- 
Pierre Morel
IBM Lab Boeblingen


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support
  2019-10-08 10:48 ` [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Halil Pasic
  2019-10-08 12:57   ` Pierre Morel
@ 2019-10-15 20:27   ` Tony Krowiak
  1 sibling, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-10-15 20:27 UTC (permalink / raw)
  To: Halil Pasic
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, pmorel, alex.williamson, kwankhede, jjherne

On 10/8/19 6:48 AM, Halil Pasic wrote:
> On Fri, 13 Sep 2019 17:26:48 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> The current design for AP pass-through does not support making dynamic
>> changes to the AP matrix of a running guest resulting in three deficiencies
>> this patch series is intended to mitigate:
>>
>> 1. Adapters, domains and control domains can not be added to or removed
>>     from a running guest. In order to modify a guest's AP configuration,
>>     the guest must be terminated; only then can AP resources be assigned
>>     to or unassigned from the guest's matrix mdev. The new AP configuration
>>     becomes available to the guest when it is subsequently restarted.
>>
>> 2. The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask interfaces can
>>     be modified by a root user without any restrictions. A change to either
>>     mask can result in AP queue devices being unbound from the vfio_ap
>>     device driver and bound to a zcrypt device driver even if a guest is
>>     using the queues, thus giving the host access to the guest's private
>>     crypto data and vice versa.
>>
>> 3. The APQNs derived from the Cartesian product of the APIDs of the
>>     adapters and APQIs of the domains assigned to a matrix mdev must
>>     reference an AP queue device bound to the vfio_ap device driver.
>>
>> This patch series introduces the following changes to the current design
>> to alleviate the shortcomings described above as well as to implement more
>> of the AP architecture:
>>
>> 1. A root user will be prevented from making changes to the AP bus's
>>     /sys/bus/ap/apmask or /sys/bus/ap/aqmask if the ownership of an APQN
>>     changes from the vfio_ap device driver to a zcrypt driver when the APQN
>>     is assigned to a matrix mdev.
>>
>> 2. The sysfs bind/unbind interfaces will be disabled for the vfio_ap
>>     device driver.
>>
> 
> Doesn't this have the potential of breaking some userspace stuff that
> might be out there?

I have decided to leave these interfaces enabled.

> 
>> 3. Allow AP resources to be assigned to or removed from a matrix mdev
>>     while a guest is using it and hot plug the resource into or hot unplug
>>     the resource from the running guest.
> 
> This looks like a natural extension of the interface -- i.e. should not
> break any userspace.

We agree

> 
>>
>> 4. Allow assignment of an AP adapter or domain to a matrix mdev even if it
>>     results in assignment of an APQN that does not reference an AP queue
>>     device bound to the vfio_ap device driver, as long as the APQN is owned
>>     by the vfio_ap driver. Allowing over-provisioning of AP resources
>>     better models the architecture which does not preclude assigning AP
>>     resources that are not yet available in the system. If/when the queue
>>     becomes available to the host, it will immediately also become available
>>     to the guest.
> 
> Same here -- I don't think this change breaks any userspace.

We agree here also

> 
>>
>> 1. Rationale for changes to AP bus's apmask/aqmask interfaces:
>> ----------------------------------------------------------
>> Due to the extremely sensitive nature of cryptographic data, it is
>> imperative that great care be taken to ensure that such data is secured.
>> Allowing a root user, either inadvertently or maliciously, to configure
>> these masks such that a queue is shared between the host and a guest is
>> not only avoidable, it is advisable. It was suggested that this scenario
>> is better handled in user space with management software, but that does
>> not preclude a malicious administrator from using the sysfs interfaces
>> to gain access to a guest's crypto data. It was also suggested that this
>> scenario could be avoided by taking access to the adapter away from the
>> guest and zeroing out the queues prior to the vfio_ap driver releasing the
>> device; however, stealing an adapter in use from a guest as a by-product
>> of an operation is bad and will likely cause problems for the guest
>> unnecessarily. It was decided that the most effective solution with the
>> least number of negative side effects is to prevent the situation at the
>> source.
>>
>> 2. Rationale for disabling bind/unbind interfaces for vfio_ap driver:
>> -----------------------------------------------------------------
>> By disabling the bind/unbind interfaces for the vfio_ap device driver,
>> the user is forced to use the AP bus's apmask/aqmask interfaces to control
>> the probing and removing of AP queues. There are two primary reasons for
>> disabling the bind/unbind interfaces for the vfio_ap device driver:
>>
>> * The device architecture does not provide a means to prevent unbinding
>>    a device from a device driver, so an AP queue device can be unbound
>>    from the vfio_ap driver even when the queue is in use by a guest. By
>>    disabling the unbind interface, the user is forced to use the AP bus's
>>    apmask/aqmask interfaces which will prevent this.
>>
> 
> Isn't this fixed by your filtering (if implemented correctly)? BTW I believe
> it solves the problem regardless whether the unbind was triggered by the
> drivers unbind attribute or by a[pq]mask.

IMHO, it would be better if we didn't rely on the filtering because when
an unbind is done, the filtering will remove access to the entire
adapter. My goal was to limit the need for filtering to unbinds
triggered by AP deconfiguration via the SE or SCLP command over which we
have no control. We can control all other scenarios except for when an
adapter goes away for some other reason such as a failure.

> 
>> * Binding of AP queues is controlled by the AP bus /sys/bus/ap/apmask and
>>    /sys/bus/ap/aqmask interfaces. If the masks indicate that an APQN is
>>    owned by zcrypt, trying to bind it to the vfio_ap device driver will
>>    fail; therefore, the bind interface is somewhat redundant and certainly
>>    unnecessary.
>>    
> 
> [..]
> 
>> Tony Krowiak (10):
>>    s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks
>>    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: filter CRYCB bits for unavailable queue devices
>>    s390: vfio-ap: sysfs attribute to display the guest CRYCB
>>    s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove
>>      callbacks
>>    s390: zcrypt: driver callback to indicate resource in use
>>    s390: vfio-ap: implement in-use callback for vfio_ap driver
>>    s390: vfio-ap: added versioning to vfio_ap module
>>    s390: vfio-ap: update documentation
> 
> I believe it would be worthwhile to reorder the patches (fixes and
> re-factoring first, the features).

Suggestions?

> 
> Regards,
> Halil
> 
>>
>>   Documentation/s390/vfio-ap.rst        | 899 +++++++++++++++++++++++++---------
>>   drivers/s390/crypto/ap_bus.c          | 144 +++++-
>>   drivers/s390/crypto/ap_bus.h          |   4 +
>>   drivers/s390/crypto/vfio_ap_drv.c     |  27 +-
>>   drivers/s390/crypto/vfio_ap_ops.c     | 610 ++++++++++++++---------
>>   drivers/s390/crypto/vfio_ap_private.h |  12 +-
>>   6 files changed, 1200 insertions(+), 496 deletions(-)
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support
  2019-10-08 12:57   ` Pierre Morel
@ 2019-10-15 20:33     ` Tony Krowiak
  0 siblings, 0 replies; 20+ messages in thread
From: Tony Krowiak @ 2019-10-15 20:33 UTC (permalink / raw)
  To: Pierre Morel, Halil Pasic
  Cc: linux-s390, linux-kernel, kvm, freude, borntraeger, cohuck,
	mjrosato, alex.williamson, kwankhede, jjherne

On 10/8/19 8:57 AM, Pierre Morel wrote:
> 
> On 10/8/19 12:48 PM, Halil Pasic wrote:
>> On Fri, 13 Sep 2019 17:26:48 -0400
>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>
>>> The current design for AP pass-through does not support making dynamic
>>> changes to the AP matrix of a running guest resulting in three 
>>> deficiencies
>>> this patch series is intended to mitigate:
>>>
>>> 1. Adapters, domains and control domains can not be added to or removed
>>>     from a running guest. In order to modify a guest's AP configuration,
>>>     the guest must be terminated; only then can AP resources be assigned
>>>     to or unassigned from the guest's matrix mdev. The new AP 
>>> configuration
>>>     becomes available to the guest when it is subsequently restarted.
>>>
>>> 2. The AP bus's /sys/bus/ap/apmask and /sys/bus/ap/aqmask interfaces can
>>>     be modified by a root user without any restrictions. A change to 
>>> either
>>>     mask can result in AP queue devices being unbound from the vfio_ap
>>>     device driver and bound to a zcrypt device driver even if a guest is
>>>     using the queues, thus giving the host access to the guest's private
>>>     crypto data and vice versa.
>>>
>>> 3. The APQNs derived from the Cartesian product of the APIDs of the
>>>     adapters and APQIs of the domains assigned to a matrix mdev must
>>>     reference an AP queue device bound to the vfio_ap device driver.
>>>
>>> This patch series introduces the following changes to the current design
>>> to alleviate the shortcomings described above as well as to implement 
>>> more
>>> of the AP architecture:
>>>
>>> 1. A root user will be prevented from making changes to the AP bus's
>>>     /sys/bus/ap/apmask or /sys/bus/ap/aqmask if the ownership of an APQN
>>>     changes from the vfio_ap device driver to a zcrypt driver when 
>>> the APQN
>>>     is assigned to a matrix mdev.
>>>
>>> 2. The sysfs bind/unbind interfaces will be disabled for the vfio_ap
>>>     device driver.
>>>
>> Doesn't this have the potential of breaking some userspace stuff that
>> might be out there?
>>
>>> 3. Allow AP resources to be assigned to or removed from a matrix mdev
>>>     while a guest is using it and hot plug the resource into or hot 
>>> unplug
>>>     the resource from the running guest.
>> This looks like a natural extension of the interface -- i.e. should not
>> break any userspace.
>>
>>> 4. Allow assignment of an AP adapter or domain to a matrix mdev even 
>>> if it
>>>     results in assignment of an APQN that does not reference an AP queue
>>>     device bound to the vfio_ap device driver, as long as the APQN is 
>>> owned
>>>     by the vfio_ap driver. Allowing over-provisioning of AP resources
>>>     better models the architecture which does not preclude assigning AP
>>>     resources that are not yet available in the system. If/when the 
>>> queue
>>>     becomes available to the host, it will immediately also become 
>>> available
>>>     to the guest.
>> Same here -- I don't think this change breaks any userspace.
>>
>>> 1. Rationale for changes to AP bus's apmask/aqmask interfaces:
>>> ----------------------------------------------------------
>>> Due to the extremely sensitive nature of cryptographic data, it is
>>> imperative that great care be taken to ensure that such data is secured.
>>> Allowing a root user, either inadvertently or maliciously, to configure
>>> these masks such that a queue is shared between the host and a guest is
>>> not only avoidable, it is advisable.
> 
> Just curious: how is it possible to do such a configuration?

In the current implementation of dedicated crypto, there is nothing
stopping a sysadmin from changing the apmask/aqmask in manner that
transfers ownership of one more APQNs from the vfio_ap device driver to
zcrypt which results in unbinding the queue devices from vfio_ap and
binding them to the zcrypt drive. If a guest happens to be using the
queue at the time, both the host and guest will have access. That is
fixed by this series.

> 
> 
>>>   It was suggested that this scenario
>>> is better handled in user space with management software, but that does
>>> not preclude a malicious administrator from using the sysfs interfaces
>>> to gain access to a guest's crypto data. It was also suggested that this
>>> scenario could be avoided by taking access to the adapter away from the
>>> guest and zeroing out the queues prior to the vfio_ap driver 
>>> releasing the
>>> device; however, stealing an adapter in use from a guest as a by-product
>>> of an operation is bad and will likely cause problems for the guest
>>> unnecessarily. It was decided that the most effective solution with the
>>> least number of negative side effects is to prevent the situation at the
>>> source.
> 
> 
> Stealing an adapter in use by a guest, insn't it what is done if we 
> allow to unassign an AP/Domain using the unassign sysfs interface when 
> the mediated device is in use by the guest?

Yes, but that is a deliberate action as opposed to a side effect of
bind/unbind. It is the very definition of dynamic configuration (a.k.a.,
hot plug/unplug).

> 
> 
>>>
>>> 2. Rationale for disabling bind/unbind interfaces for vfio_ap driver:
>>> -----------------------------------------------------------------
>>> By disabling the bind/unbind interfaces for the vfio_ap device driver,
>>> the user is forced to use the AP bus's apmask/aqmask interfaces to 
>>> control
>>> the probing and removing of AP queues. There are two primary reasons for
>>> disabling the bind/unbind interfaces for the vfio_ap device driver:
>>>
>>> * The device architecture does not provide a means to prevent unbinding
>>>    a device from a device driver, so an AP queue device can be unbound
>>>    from the vfio_ap driver even when the queue is in use by a guest. By
>>>    disabling the unbind interface, the user is forced to use the AP 
>>> bus's
>>>    apmask/aqmask interfaces which will prevent this.
>>>
>> Isn't this fixed by your filtering (if implemented correctly)? BTW I 
>> believe
>> it solves the problem regardless whether the unbind was triggered by the
>> drivers unbind attribute or by a[pq]mask
>>
>>> * Binding of AP queues is controlled by the AP bus /sys/bus/ap/apmask 
>>> and
>>>    /sys/bus/ap/aqmask interfaces. If the masks indicate that an APQN is
>>>    owned by zcrypt, trying to bind it to the vfio_ap device driver will
>>>    fail; therefore, the bind interface is somewhat redundant and 
>>> certainly
>>>    unnecessary.
>>>
>>>
>>> Tony Krowiak (10):
>>>    s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks
>>>    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: filter CRYCB bits for unavailable queue devices
>>>    s390: vfio-ap: sysfs attribute to display the guest CRYCB
>>>    s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove
>>>      callbacks
>>>    s390: zcrypt: driver callback to indicate resource in use
>>>    s390: vfio-ap: implement in-use callback for vfio_ap driver
>>>    s390: vfio-ap: added versioning to vfio_ap module
>>>    s390: vfio-ap: update documentation
>> I believe it would be worthwhile to reorder the patches (fixes and
>> re-factoring first, the features).
>>
>> Regards,
>> Halil
>>
>>>   Documentation/s390/vfio-ap.rst        | 899 
>>> +++++++++++++++++++++++++---------
>>>   drivers/s390/crypto/ap_bus.c          | 144 +++++-
>>>   drivers/s390/crypto/ap_bus.h          |   4 +
>>>   drivers/s390/crypto/vfio_ap_drv.c     |  27 +-
>>>   drivers/s390/crypto/vfio_ap_ops.c     | 610 ++++++++++++++---------
>>>   drivers/s390/crypto/vfio_ap_private.h |  12 +-
>>>   6 files changed, 1200 insertions(+), 496 deletions(-)
>>>


^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2019-10-15 20:33 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-13 21:26 [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 01/10] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 02/10] s390: vfio-ap: allow assignment of unavailable AP resources to mdev device Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 03/10] s390: vfio-ap: allow hot plug/unplug of AP resources using " Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 04/10] s390: vfio-ap: filter CRYCB bits for unavailable queue devices Tony Krowiak
2019-09-18 17:04   ` Halil Pasic
2019-09-18 21:22     ` Tony Krowiak
2019-09-19 10:34   ` Halil Pasic
2019-09-20 14:24     ` Tony Krowiak
2019-09-20 15:44       ` Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 05/10] s390: vfio-ap: sysfs attribute to display the guest CRYCB Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 06/10] s390: vfio-ap: update guest CRYCB in vfio_ap probe and remove callbacks Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 07/10] s390: zcrypt: driver callback to indicate resource in use Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 08/10] s390: vfio-ap: implement in-use callback for vfio_ap driver Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 09/10] s390: vfio-ap: added versioning to vfio_ap module Tony Krowiak
2019-09-13 21:26 ` [PATCH v6 10/10] s390: vfio-ap: update documentation Tony Krowiak
2019-10-08 10:48 ` [PATCH v6 00/10] s390: vfio-ap: dynamic configuration support Halil Pasic
2019-10-08 12:57   ` Pierre Morel
2019-10-15 20:33     ` Tony Krowiak
2019-10-15 20:27   ` Tony Krowiak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).