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

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

[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 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 (7):
  s390: vfio-ap: Refactor vfio_ap driver 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: 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: add logging to vfio_ap driver
  s390: vfio-ap: update documentation

 Documentation/s390/vfio-ap.rst        | 871 +++++++++++++++++++++++++---------
 drivers/s390/crypto/ap_bus.c          | 137 +++++-
 drivers/s390/crypto/ap_bus.h          |   3 +
 drivers/s390/crypto/vfio_ap_drv.c     |  87 +++-
 drivers/s390/crypto/vfio_ap_ops.c     | 533 ++++++++++++---------
 drivers/s390/crypto/vfio_ap_private.h |  26 +-
 6 files changed, 1171 insertions(+), 486 deletions(-)

-- 
2.7.4

[PATCH v5 1/7] s390: vfio-ap: Refactor vfio_ap driver probe and remove callbacks

[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     | 78 ++++++++++++++++++++++++++++-------
 drivers/s390/crypto/vfio_ap_private.h |  6 +--
 3 files changed, 71 insertions(+), 36 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..e9a6a7d8395d 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,47 @@ 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 %04x.%02x empty\n",
+				__func__, status.response_code,
+				AP_QID_CARD(qid), AP_QID_QUEUE(qid));
+			return;
+		}
+	} while (--retry);
+
+	WARN_ON_ONCE(retry <= 0);
+}
+
+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);
+			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,14 +1193,10 @@ 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 +1322,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

[PATCH v5 2/7] s390: zcrypt: driver callback to indicate resource in use

[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 | 137 +++++++++++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   3 +
 2 files changed, 134 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.c b/drivers/s390/crypto/ap_bus.c
index a76b8a8bcbbb..8d0877c821b0 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,71 @@ 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;
 
-	rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
+	memcpy(newapm, ap_perms.apm, APMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
+	if (rc)
+		return rc;
+
+	rc = apmask_commit(newapm);
+	mutex_unlock(&ap_perms_mutex);
 	if (rc)
 		return rc;
 
@@ -1229,12 +1294,72 @@ 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, APMASKSIZE);
+
+	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)];
+
+	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)
+		return rc;
+
+	rc = aqmask_commit(newaqm);
+	mutex_unlock(&ap_perms_mutex);
 
-	rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &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..0f865c5545f2 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,8 @@ 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

[PATCH v5 3/7] s390: vfio-ap: implement in-use callback for vfio_ap driver

[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 | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 9e61d4c6e6b5..d8da520ae1fa 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -158,6 +158,34 @@ static void vfio_ap_matrix_dev_destroy(void)
 	root_device_unregister(root_device);
 }
 
+static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
+{
+	bool in_use = false;
+	struct ap_matrix_mdev *matrix_mdev;
+	DECLARE_BITMAP(apm_intrsctn, AP_DEVICES);
+	DECLARE_BITMAP(aqm_intrsctn, AP_DOMAINS);
+
+	mutex_lock(&matrix_dev->lock);
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		bitmap_and(apm_intrsctn, apm, matrix_mdev->matrix.apm,
+			   AP_DEVICES);
+		if (find_first_bit_inv(apm_intrsctn, AP_DEVICES) == AP_DEVICES)
+			continue;
+
+		bitmap_and(aqm_intrsctn, aqm, matrix_mdev->matrix.aqm,
+			   AP_DOMAINS);
+		if (find_first_bit_inv(aqm_intrsctn, AP_DEVICES) == AP_DOMAINS)
+			continue;
+
+		in_use = true;
+	}
+
+	mutex_unlock(&matrix_dev->lock);
+
+	return in_use;
+}
+
 static int __init vfio_ap_init(void)
 {
 	int ret;
@@ -173,7 +201,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_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) {
-- 
2.7.4

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

[Tony Krowiak]

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

The current implementation does not allow assignment of AP adapters or
domains to an mdev device if the APQNs resulting from the assignment
reference AP queue devices that are not 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 are not reserved by the AP
BUS for use by the zcrypt device drivers and the APQNs are not assigned to
another mdev device.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e9a6a7d8395d..360a16d025dd 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
  *
@@ -529,17 +413,25 @@ vfio_ap_mdev_verify_queues_reserved_for_apid(struct ap_matrix_mdev *matrix_mdev,
  * and AP queue indexes comprising the AP matrix are not configured for another
  * mediated device. AP queue sharing is not allowed.
  *
- * @matrix_mdev: the mediated matrix device
+ * @mdev_apm: the mask identifying the adapters assigned to mdev
+ * @mdev_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 +442,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 +454,18 @@ 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 *apm, unsigned long *aqm)
+{
+	int ret;
+
+	ret = ap_apqn_in_matrix_owned_by_def_drv(apm, aqm);
+	if (ret)
+		return (ret == 1) ? -EADDRNOTAVAIL : ret;
+
+	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, apm, aqm);
+}
+
 /**
  * assign_adapter_store
  *
@@ -602,6 +504,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 +519,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 +581,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 +619,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 +634,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 +733,6 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	if (id > matrix_mdev->matrix.adm_max)
 		return -ENODEV;
 
-	/* Set the bit in the ADM (bitmask) corresponding to the AP control
-	 * domain number (id). The bits in the mask, from most significant to
-	 * least significant, correspond to IDs 0 up to the one less than the
-	 * number of control domains that can be assigned.
-	 */
 	mutex_lock(&matrix_dev->lock);
 	set_bit_inv(id, matrix_mdev->matrix.adm);
 	mutex_unlock(&matrix_dev->lock);
-- 
2.7.4

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

[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 to grant
access to the adapter, domain or control domain being assigned. If an
AP resource is unassigned while a guest is using the matrix mdev, the
guest's CRYCB will be dynamically updated to take access to the adapter,
domain or control domain away from the guest.

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

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 360a16d025dd..0e748819abb6 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -466,6 +466,16 @@ static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
 	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, apm, aqm);
 }
 
+static void vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
+{
+	if (matrix_mdev->kvm && matrix_mdev->kvm->arch.crypto.crycbd) {
+		kvm_arch_crypto_set_masks(matrix_mdev->kvm,
+					  matrix_mdev->matrix.apm,
+					  matrix_mdev->matrix.aqm,
+					  matrix_mdev->matrix.adm);
+	}
+}
+
 /**
  * assign_adapter_store
  *
@@ -476,7 +486,10 @@ 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 mediated matrix device and each
+ * new APQN formed as a result of the assignment identifies an AP queue device
+ * that is bound to the vfio_ap device driver, the guest will be granted access
+ * to the adapter with the specified APID.
  *
  * Returns the number of bytes processed if the APID is valid; otherwise,
  * returns one of the following errors:
@@ -508,10 +521,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;
@@ -529,7 +538,9 @@ static ssize_t assign_adapter_store(struct device *dev,
 		mutex_unlock(&matrix_dev->lock);
 		return ret;
 	}
+
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -545,7 +556,9 @@ static DEVICE_ATTR_WO(assign_adapter);
  * @count:	the number of bytes in @buf
  *
  * Parses the APID from @buf and clears the corresponding bit in the mediated
- * matrix device's APM.
+ * matrix device's APM. If a guest is using the mediated matrix device and has
+ * access to the AP adapter with the specified APID, access to the adapter will
+ * be taken from the guest.
  *
  * Returns the number of bytes processed if the APID is valid; otherwise,
  * returns one of the following errors:
@@ -562,10 +575,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;
@@ -575,6 +584,7 @@ static ssize_t unassign_adapter_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -591,7 +601,10 @@ static DEVICE_ATTR_WO(unassign_adapter);
  * @count:	the number of bytes in @buf
  *
  * Parses the APQI from @buf and sets the corresponding bit in the mediated
- * matrix device's AQM.
+ * matrix device's AQM. If a guest is using the mediated matrix device and each
+ * new APQN formed as a result of the assignment identifies an AP queue device
+ * that is bound to the vfio_ap device driver, the guest will be given access
+ * to the AP queue(s) with the specified APQI.
  *
  * Returns the number of bytes processed if the APQI is valid; otherwise returns
  * one of the following errors:
@@ -624,10 +637,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;
@@ -644,7 +653,9 @@ static ssize_t assign_domain_store(struct device *dev,
 		mutex_unlock(&matrix_dev->lock);
 		return ret;
 	}
+
 	set_bit_inv(apqi, matrix_mdev->matrix.aqm);
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -662,7 +673,9 @@ static DEVICE_ATTR_WO(assign_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the APQI from @buf and clears the corresponding bit in the
- * mediated matrix device's AQM.
+ * mediated matrix device's AQM. If a guest is using the mediated matrix device
+ * and has access to queue(s) with the specified domain APQI, access to
+ * the queue(s) will be taken away from the guest.
  *
  * Returns the number of bytes processed if the APQI is valid; otherwise,
  * returns one of the following errors:
@@ -678,10 +691,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;
@@ -691,6 +700,7 @@ static ssize_t unassign_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -706,7 +716,9 @@ static DEVICE_ATTR_WO(unassign_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the domain ID from @buf and sets the corresponding bit in the mediated
- * matrix device's ADM.
+ * matrix device's ADM. If a guest is using the mediated matrix device and the
+ * guest does not have access to the control domain with the specified ID, the
+ * guest will be granted access to it.
  *
  * Returns the number of bytes processed if the domain ID is valid; otherwise,
  * returns one of the following errors:
@@ -722,10 +734,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;
@@ -735,6 +743,7 @@ static ssize_t assign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	set_bit_inv(id, matrix_mdev->matrix.adm);
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
@@ -750,7 +759,9 @@ static DEVICE_ATTR_WO(assign_control_domain);
  * @count:	the number of bytes in @buf
  *
  * Parses the domain ID from @buf and clears the corresponding bit in the
- * mediated matrix device's ADM.
+ * mediated matrix device's ADM. If a guest is using the mediated matrix device
+ * and has access to control domain with the specified domain ID, access to
+ * the control domain will be taken from the guest.
  *
  * Returns the number of bytes processed if the domain ID is valid; otherwise,
  * returns one of the following errors:
@@ -767,10 +778,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;
@@ -779,6 +786,7 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 
 	mutex_lock(&matrix_dev->lock);
 	clear_bit_inv(domid, matrix_mdev->matrix.adm);
+	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
 	return count;
-- 
2.7.4

[PATCH v5 6/7] s390: vfio-ap: add logging to vfio_ap driver

[Tony Krowiak]

Added two DBF log files for logging events and errors; one for the vfio_ap
driver, and one for each matrix mediated device.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     |  34 +++++++
 drivers/s390/crypto/vfio_ap_ops.c     | 187 ++++++++++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_private.h |  20 ++++
 3 files changed, 223 insertions(+), 18 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index d8da520ae1fa..04a77246c22a 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -22,6 +22,10 @@ MODULE_AUTHOR("IBM Corporation");
 MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
 MODULE_LICENSE("GPL v2");
 
+uint dbglvl = 3;
+module_param(dbglvl, uint, 0444);
+MODULE_PARM_DESC(dbglvl, "VFIO_AP driver debug level.");
+
 static struct ap_driver vfio_ap_drv;
 
 struct ap_matrix_dev *matrix_dev;
@@ -158,6 +162,21 @@ static void vfio_ap_matrix_dev_destroy(void)
 	root_device_unregister(root_device);
 }
 
+static void vfio_ap_log_queues_in_use(struct ap_matrix_mdev *matrix_mdev,
+				  unsigned long *apm, unsigned long *aqm)
+{
+	unsigned long apid, apqi;
+
+	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
+		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
+			VFIO_AP_DBF(matrix_dev, DBF_ERR,
+				    "queue %02lx.%04lx in use by mdev %s\n",
+				    apid, apqi,
+				    dev_name(mdev_dev(matrix_mdev->mdev)));
+		}
+	}
+}
+
 static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
 {
 	bool in_use = false;
@@ -179,6 +198,8 @@ static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
 			continue;
 
 		in_use = true;
+		vfio_ap_log_queues_in_use(matrix_mdev, apm_intrsctn,
+					  aqm_intrsctn);
 	}
 
 	mutex_unlock(&matrix_dev->lock);
@@ -186,6 +207,16 @@ static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
 	return in_use;
 }
 
+static int __init vfio_ap_debug_init(void)
+{
+	matrix_dev->dbf = debug_register(VFIO_AP_DRV_NAME, 1, 1,
+					 DBF_SPRINTF_MAX_ARGS * sizeof(long));
+	debug_register_view(matrix_dev->dbf, &debug_sprintf_view);
+	debug_set_level(matrix_dev->dbf, dbglvl);
+
+	return 0;
+}
+
 static int __init vfio_ap_init(void)
 {
 	int ret;
@@ -219,6 +250,8 @@ static int __init vfio_ap_init(void)
 		return ret;
 	}
 
+	vfio_ap_debug_init();
+
 	return 0;
 }
 
@@ -227,6 +260,7 @@ static void __exit vfio_ap_exit(void)
 	vfio_ap_mdev_unregister();
 	ap_driver_unregister(&vfio_ap_drv);
 	vfio_ap_matrix_dev_destroy();
+	debug_unregister(matrix_dev->dbf);
 }
 
 module_init(vfio_ap_init);
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 0e748819abb6..1aa18eba43d0 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -167,17 +167,23 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
 		case AP_RESPONSE_INVALID_ADDRESS:
 		default:
 			/* All cases in default means AP not operational */
-			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
-				  status.response_code);
 			goto end_free;
 		}
 	} while (retries--);
 
-	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
-		  status.response_code);
 end_free:
 	vfio_ap_free_aqic_resources(q);
 	q->matrix_mdev = NULL;
+	if (status.response_code) {
+		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_WARN,
+			 "IRQ disable failed for queue %02x.%04x: status response code=%u\n",
+			 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+			 status.response_code);
+	} else {
+		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_INFO,
+				 "IRQ disabled for queue %02x.%04x\n",
+				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn));
+	}
 	return status;
 }
 
@@ -215,6 +221,10 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 	case 1:
 		break;
 	default:
+		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_ERR,
+				 "IRQ enable failed for queue %02x.%04x: vfio_pin_pages rc=%d\n",
+				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+				 ret);
 		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
 		return status;
 	}
@@ -235,16 +245,25 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		vfio_ap_free_aqic_resources(q);
 		q->saved_pfn = g_pfn;
 		q->saved_isc = isc;
+		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_INFO,
+				 "IRQ enabled for queue %02x.%04x",
+				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn));
 		break;
 	case AP_RESPONSE_OTHERWISE_CHANGED:
 		/* We could not modify IRQ setings: clear new configuration */
 		vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev), &g_pfn, 1);
 		kvm_s390_gisc_unregister(kvm, isc);
+		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_WARN,
+				 "IRQ enable failed for queue %02x.%04x: status response code=%u",
+				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+				 status.response_code);
 		break;
 	default:
-		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
-			status.response_code);
 		vfio_ap_irq_disable(q);
+		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_WARN,
+				 "IRQ enable failed for queue %02x.%04x: status response code=%u",
+				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+				 status.response_code);
 		break;
 	}
 
@@ -321,8 +340,29 @@ static void vfio_ap_matrix_init(struct ap_config_info *info,
 	matrix->adm_max = info->apxa ? info->Nd : 15;
 }
 
+static int vfio_ap_mdev_debug_init(struct ap_matrix_mdev *matrix_mdev)
+{
+	int ret;
+
+	matrix_mdev->dbf = debug_register(dev_name(mdev_dev(matrix_mdev->mdev)),
+					  1, 1,
+					  DBF_SPRINTF_MAX_ARGS * sizeof(long));
+
+	if (!matrix_mdev->dbf)
+		return -ENOMEM;
+
+	ret = debug_register_view(matrix_mdev->dbf, &debug_sprintf_view);
+	if (ret)
+		return ret;
+
+	debug_set_level(matrix_mdev->dbf, dbglvl);
+
+	return 0;
+}
+
 static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 {
+	int ret;
 	struct ap_matrix_mdev *matrix_mdev;
 
 	if ((atomic_dec_if_positive(&matrix_dev->available_instances) < 0))
@@ -335,6 +375,13 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
 	}
 
 	matrix_mdev->mdev = mdev;
+
+	ret = vfio_ap_mdev_debug_init(matrix_mdev);
+	if (ret) {
+		kfree(matrix_mdev);
+		return ret;
+	}
+
 	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
 	mdev_set_drvdata(mdev, matrix_mdev);
 	matrix_mdev->pqap_hook.hook = handle_pqap;
@@ -350,14 +397,19 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
 {
 	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	if (matrix_mdev->kvm)
+	if (matrix_mdev->kvm) {
+		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+				 "remove rejected, mdev in use by %s",
+				 matrix_mdev->kvm->debugfs_dentry->d_iname);
 		return -EBUSY;
+	}
 
 	mutex_lock(&matrix_dev->lock);
 	vfio_ap_mdev_reset_queues(mdev);
 	list_del(&matrix_mdev->node);
 	mutex_unlock(&matrix_dev->lock);
 
+	debug_unregister(matrix_mdev->dbf);
 	kfree(matrix_mdev);
 	mdev_set_drvdata(mdev, NULL);
 	atomic_inc(&matrix_dev->available_instances);
@@ -406,6 +458,22 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
 	NULL,
 };
 
+static void vfio_ap_mdev_log_sharing_error(struct ap_matrix_mdev *logdev,
+					   const char *assigned_to,
+					   unsigned long *apm,
+					   unsigned long *aqm)
+{
+	unsigned long apid, apqi;
+
+	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
+		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
+			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
+					 "queue %02lx.%04lx already assigned to %s\n",
+					 apid, apqi, assigned_to);
+		}
+	}
+}
+
 /**
  * vfio_ap_mdev_verify_no_sharing
  *
@@ -448,22 +516,39 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
 		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
 			continue;
 
+		vfio_ap_mdev_log_sharing_error(matrix_mdev,
+					       dev_name(mdev_dev(lstdev->mdev)),
+					       apm, aqm);
+
 		return -EADDRINUSE;
 	}
 
 	return 0;
 }
 
-static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
+static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *logdev,
 				       unsigned long *apm, unsigned long *aqm)
 {
-	int ret;
+	int ret = 0;
+	unsigned long apid, apqi;
+
+	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
+		for_each_set_bit_inv(apqi, aqm, AP_DEVICES) {
+			if (!ap_owned_by_def_drv(apid, apqi))
+				continue;
+
+			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
+					 "queue %02lx.%04lx owned by zcrypt\n",
+					 apid, apqi);
+
+			ret = -EADDRNOTAVAIL;
+		}
+	}
 
-	ret = ap_apqn_in_matrix_owned_by_def_drv(apm, aqm);
 	if (ret)
-		return (ret == 1) ? -EADDRNOTAVAIL : ret;
+		return ret;
 
-	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, apm, aqm);
+	return vfio_ap_mdev_verify_no_sharing(logdev, apm, aqm);
 }
 
 static void vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
@@ -536,13 +621,20 @@ static ssize_t assign_adapter_store(struct device *dev,
 					  matrix_mdev->matrix.aqm);
 	if (ret) {
 		mutex_unlock(&matrix_dev->lock);
+		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+				 "failed to assign adapter %lu(%#02lx)\n",
+				 apid, apid);
 		return ret;
 	}
 
+
 	set_bit_inv(apid, matrix_mdev->matrix.apm);
 	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+			 "assigned adapter %lu(%#02lx)\n", apid, apid);
+
 	return count;
 }
 static DEVICE_ATTR_WO(assign_adapter);
@@ -587,6 +679,9 @@ static ssize_t unassign_adapter_store(struct device *dev,
 	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+			 "unassigned adapter %lu(%#02lx)\n", apid, apid);
+
 	return count;
 }
 static DEVICE_ATTR_WO(unassign_adapter);
@@ -651,6 +746,9 @@ static ssize_t assign_domain_store(struct device *dev,
 					  aqm);
 	if (ret) {
 		mutex_unlock(&matrix_dev->lock);
+		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+				 "failed to assign domain %lu(%#04lx)\n",
+				 apqi, apqi);
 		return ret;
 	}
 
@@ -658,6 +756,9 @@ static ssize_t assign_domain_store(struct device *dev,
 	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+			 "assigned domain %lu(%#04lx)\n", apqi, apqi);
+
 	return count;
 }
 static DEVICE_ATTR_WO(assign_domain);
@@ -703,6 +804,9 @@ static ssize_t unassign_domain_store(struct device *dev,
 	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+			 "unassigned domain %lu(%#02lx)\n", apqi, apqi);
+
 	return count;
 }
 static DEVICE_ATTR_WO(unassign_domain);
@@ -746,6 +850,9 @@ static ssize_t assign_control_domain_store(struct device *dev,
 	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+			 "assigned control domain %lu(%#02lx)\n", id, id);
+
 	return count;
 }
 static DEVICE_ATTR_WO(assign_control_domain);
@@ -789,6 +896,10 @@ static ssize_t unassign_control_domain_store(struct device *dev,
 	vfio_ap_mdev_update_crycb(matrix_mdev);
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+			 "unassigned control domain %lu(%#02lx)\n",
+			 domid, domid);
+
 	return count;
 }
 static DEVICE_ATTR_WO(unassign_control_domain);
@@ -910,6 +1021,9 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	list_for_each_entry(m, &matrix_dev->mdev_list, node) {
 		if ((m != matrix_mdev) && (m->kvm == kvm)) {
 			mutex_unlock(&matrix_dev->lock);
+			VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+					 "KVM already set for mdev %s\n",
+					 dev_name(mdev_dev(m->mdev)));
 			return -EPERM;
 		}
 	}
@@ -919,6 +1033,8 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	kvm->arch.crypto.pqap_hook = &matrix_mdev->pqap_hook;
 	mutex_unlock(&matrix_dev->lock);
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO, "KVM set for mdev\n");
+
 	return 0;
 }
 
@@ -972,8 +1088,11 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 		return NOTIFY_DONE;
 
 	/* If there is no CRYCB pointer, then we can't copy the masks */
-	if (!matrix_mdev->kvm->arch.crypto.crycbd)
+	if (!matrix_mdev->kvm->arch.crypto.crycbd) {
+		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+				 "Failed to set CRYCB masks: missing CRYCBD\n");
 		return NOTIFY_DONE;
+	}
 
 	kvm_arch_crypto_set_masks(matrix_mdev->kvm, matrix_mdev->matrix.apm,
 				  matrix_mdev->matrix.aqm,
@@ -1013,9 +1132,10 @@ static void vfio_ap_mdev_wait_for_qempty(ap_qid_t qid)
 			msleep(20);
 			break;
 		default:
-			pr_warn("%s: tapq response %02x waiting for queue %04x.%02x empty\n",
-				__func__, status.response_code,
-				AP_QID_CARD(qid), AP_QID_QUEUE(qid));
+			WARN_ONCE(1,
+				  "%s: tapq response %02x waiting for queue %04x.%02x empty\n",
+				  __func__, status.response_code,
+				  AP_QID_CARD(qid), AP_QID_QUEUE(qid));
 			return;
 		}
 	} while (--retry);
@@ -1062,8 +1182,16 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
 				     matrix_mdev->matrix.aqm_max + 1) {
 			ret = vfio_ap_mdev_reset_queue(apid, apqi);
-			if (ret)
+			if (ret) {
 				rc = ret;
+				VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+						 "queue %02lx.%04lx reset failed: rc=%d\n",
+						 apid, apqi, ret);
+			} else {
+				VFIO_AP_MDEV_DBF(matrix_mdev, DBF_INFO,
+						 "queue %02lx.%04lx reset\n",
+						 apid, apqi);
+			}
 
 			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
 		}
@@ -1089,6 +1217,9 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 				     &events, &matrix_mdev->group_notifier);
 	if (ret) {
 		module_put(THIS_MODULE);
+		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+				 "failed to open mdev fd: VFIO_GROUP_NOTIFY notifier registration failed with rc=%d\n",
+				 ret);
 		return ret;
 	}
 
@@ -1096,12 +1227,19 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
 	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				     &events, &matrix_mdev->iommu_notifier);
-	if (!ret)
+	if (!ret) {
+		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_DEBUG,
+				 "opened mdev fd: guest started\n");
 		return ret;
+	}
 
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
+			 "failed to open mdev fd: VFIO_IOMMU_NOTIFY notifier registration failed with rc=%d\n",
+			 ret);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				 &matrix_mdev->group_notifier);
 	module_put(THIS_MODULE);
+
 	return ret;
 }
 
@@ -1124,6 +1262,9 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				 &matrix_mdev->group_notifier);
 	module_put(THIS_MODULE);
+
+	VFIO_AP_MDEV_DBF(matrix_mdev, DBF_DEBUG,
+			 "released mdev fd: guest terminated\n");
 }
 
 static int vfio_ap_mdev_get_device_info(unsigned long arg)
@@ -1202,6 +1343,11 @@ int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
 	q->apqn = queue->qid;
 	q->saved_isc = VFIO_AP_ISC_INVALID;
 
+	VFIO_AP_DBF(matrix_dev, DBF_DEBUG,
+		    "queue %02x.%04x bound to %s\n",
+		    AP_QID_CARD(queue->qid), AP_QID_QUEUE(queue->qid),
+		    VFIO_AP_DRV_NAME);
+
 	return 0;
 }
 
@@ -1217,4 +1363,9 @@ void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
 	vfio_ap_mdev_reset_queue(apid, apqi);
 	vfio_ap_irq_disable(q);
 	kfree(q);
+
+	VFIO_AP_DBF(matrix_dev, DBF_DEBUG,
+		    "queue %02x.%04x unbound from %s\n",
+		    AP_QID_CARD(queue->qid), AP_QID_QUEUE(queue->qid),
+		    VFIO_AP_DRV_NAME);
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5cc3c2ebf151..f717e43e10cf 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -24,6 +24,21 @@
 #define VFIO_AP_MODULE_NAME "vfio_ap"
 #define VFIO_AP_DRV_NAME "vfio_ap"
 
+#define DBF_ERR		3	/* error conditions   */
+#define DBF_WARN	4	/* warning conditions */
+#define DBF_INFO	5	/* informational      */
+#define DBF_DEBUG	6	/* for debugging only */
+
+#define DBF_SPRINTF_MAX_ARGS 5
+
+#define VFIO_AP_DBF(d_matrix_dev, ...) \
+	debug_sprintf_event(d_matrix_dev->dbf, ##__VA_ARGS__)
+
+#define VFIO_AP_MDEV_DBF(d_matrix_mdev, ...) \
+	debug_sprintf_event(d_matrix_mdev->dbf, ##__VA_ARGS__)
+
+extern uint dbglvl;
+
 /**
  * ap_matrix_dev - the AP matrix device structure
  * @device:	generic device structure associated with the AP matrix device
@@ -43,6 +58,7 @@ struct ap_matrix_dev {
 	struct list_head mdev_list;
 	struct mutex lock;
 	struct ap_driver  *vfio_ap_drv;
+	debug_info_t *dbf;
 };
 
 extern struct ap_matrix_dev *matrix_dev;
@@ -77,6 +93,9 @@ struct ap_matrix {
  * @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
+ * @pqap_hook:	handler for PQAP instruction
+ * @mdev:	the matrix mediated device
+ * @dbf:	the debug info log
  */
 struct ap_matrix_mdev {
 	struct list_head node;
@@ -86,6 +105,7 @@ struct ap_matrix_mdev {
 	struct kvm *kvm;
 	struct kvm_s390_module_hook pqap_hook;
 	struct mdev_device *mdev;
+	debug_info_t *dbf;
 };
 
 extern int vfio_ap_mdev_register(void);
-- 
2.7.4

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

[Tony Krowiak]

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

Changes made to the mdev matrix assignment interfaces:

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

* Adds a section on VFIO AP subsystem debug facilities

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

diff --git a/Documentation/s390/vfio-ap.rst b/Documentation/s390/vfio-ap.rst
index b5c51f7c748d..bfe75d2f88db 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,10 +160,26 @@ If you recall from the description of an AP Queue, AP instructions include
 an APQN to identify the AP queue to which an AP command-request message is to be
 sent (NQAP and PQAP instructions), or from which a command-reply message is to
 be received (DQAP instruction). The validity of an APQN is defined by the matrix
-calculated from the APM and AQM; it is the cross product of all assigned adapter
-numbers (APM) with all assigned queue indexes (AQM). For example, if adapters 1
-and 2 and usage domains 5 and 6 are assigned to a guest, the APQNs (1,5), (1,6),
-(2,5) and (2,6) will be valid for the guest.
+calculated from the APM and AQM; it is the Cartesian product of all assigned
+adapter numbers (APM) with all assigned queue indexes (AQM). For example, if
+adapters 1 and 2 and usage domains 5 and 6 are assigned to a guest:
+
+
+		        05           06
+		   +-----------+-----------+
+		01 |  (01,05)  |  (01,06)  |
+		   +-----------|-----------+
+		02 |  (02,05)  |  (02,06)  |
+		   +-----------|-----------+
+
+		        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
@@ -248,7 +273,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'
@@ -454,45 +479,496 @@ 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
+   --------------- matrix
+   --------------- 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 of the AP queue devices 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.
+
+   * All APQNs containing the APQI corresponding to the unset bits in the aqmask
+     will be unavailable to the host, but available to its guests.
+
+   * 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 may 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
+
+   1. assign_adapter
+
+      An adapter is assigned to a mediated matrix device by echoing an adapter
+      number 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).
+
+      Note that adapters that are currently not available to the host may be
+      assigned to the mediated device. If the adapter subsequently becomes
+      available while a guest is using the mediated device, it will automatically
+      become available to 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).
+
+      Note that only domains that are currently not owned by zcrypt may be
+      assigned to the mediated device. If the domain subsequently becomes
+      available while a guest is using the mediated device, it will
+      automatically become available to the guest.
+
+The APQNs of the AP queues provisioned for the guest are derived from the
+Cartesian product of the adapter and domain numbers assigned to the mediated
+matrix device in use by a guest. To view the APQNs provisioned for the guest,
+print the 'matrix' attribute to stdout:
+
+   cat matrix
+
+Note that ownership of an APQN can not be taken from the vfio_ap device driver
+as long as it is assigned to a mediated matrix device.
+
+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 mediated matrix device $uuid is:
+
+      01.0006
+      01.0007
+      02.0006
+      02.0007
+      03.0006
+      03.0007
+
+If we execute the following:
+
+   echo +7 > aqmask
+
+This will result in the following new masks:
+
+   apmask: 0x7000000000000000000000000000000000000000000000000000000000000000
+   aqmask: 0x0d00000000000000000000000000000000000000000000000000000000000000
+
+   owned by zcrypt:
+      01.0004
+      01.0005
+      01.0007
+      02.0004
+      02.0005
+      02.0007
+      03.0004
+      03.0005
+      03.0007
+
+From this, we can see that ownership of APQNs 01.0007, 02.0007, and 03.0007
+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
+aqmask operation will fail.
+
+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).
+
+   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
+
+VFIO AP device driver logs:
+==========================
+The VFIO AP device driver (/sys/bus/ap/drivers/vfio_ap) provides S390 Debug log
+files (see Documentation/s390/s390dbf.rst) for tracing events:
+
+1. The /sys/kernel/debug/s390dbf/vfio_ap/sprintf traces the binding and
+   unbinding of AP queue devices to the VFIO AP device driver. When ownership
+   of an APQN identifying an AP queue device is transferred to the VFIO AP
+   device driver, the queue device will be bound to the driver and the bind
+   event will be recorded in the debug log file if the debug level is 3 or
+   higher. Likewise, when ownership of an APQN identifying AP queue device is
+   transferred from the VFIO AP device driver, the queue device will be unbound
+   from the driver and the unbind event will be recorded in the debug log file
+   if the debug level is 3 or higher.
+
+   Keep in mind, that modifying the AP bus's apmask or aqmask may result in
+   changing ownership of multiple APQNs. If the operation is rejected because
+   one or more of the APQNs are assigned to a matrix mediated device, a debug
+   log entry will be written to identify the mediated device to which the APQN
+   is assigned if the debug level is 3 or higher.
+
+2. A /sys/kernel/debug/s390dbf/$uuid/sprintf debug log file is created for every
+   matrix mediated device that is created. This log is used to trace the
+   following types of events:
+
+   * Successful assignment and unassignment of adapters, domains and control
+     domains if the log level is 5 or greater.
+
+   * Failure to assign or unassign an adapter or domain. Failure can occur for
+     due to the fact that one or more APQNs derived from the adapter or domain
+     number being assigned and the domain or adapter numbers already assigned
+     is owned by zcrypt or assigned to another matrix mediated device. For the
+     latter case, the mediated matrix device to which each APQNs are assigned
+     will be identified in the event log entry.
+
+Each debug log has a debug level ranging from 0 to 6. Only debug entries with a
+level that is lower or equal to the actual level are written to the log. By
+default, the log level will be set to 3. This means entries logging the most
+serious errors will have a low level value whereas informational or debug
+entries will have a higher value. The default value for all VFIO AP device
+driver logs is 3.
+
+The debug level can be set by writing a level of 0 to 6 to the sysfs attribute
+files for the VFIO AP device driver, or any of the matrix mediated device debug
+logs. For example, to set the log level for the VFIO_AP device driver debug
+log:
+
+   echo 6 > /sys/kernel/debug/s390dbf/vfio_ap/level
+
+To set the log level for matrix mediated device
+6ec3e43a-86a4-49dc-9688-8524541b2416:
+
+   cd /sys/kernel/debug/s390dbf/6ec3e43a-86a4-49dc-9688-8524541b2416
+   echo 5 > level
+
+The log level for all VFIO AP subsystem logs can be set with a module parameter
+named 'dbglvl' when the vfio_ap.ko module is loaded, or at boot time. For
+example, to set the log level to 5 using the 'modprobe' command:
+
+   modprobe vfio_ap dbglvl=5
+
+The debug level can also be set at boot time by adding the following parameter
+to the kernel command line:
+
+   vfio_ap.dbglvl=5
+
+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:
 
@@ -509,7 +985,7 @@ These are the steps:
    To build the vfio_ap module, the kernel build must be configured with the
    following Kconfig elements selected:
    * IOMMU_SUPPORT
-   * S390
+   * S390step 3 in example
    * ZCRYPT
    * S390_AP_IOMMU
    * VFIO
@@ -517,124 +993,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.
+   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.
 
-   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:
-
-	 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 05:
 
       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 +1025,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 +1055,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::
+   three guests. To create the mediated devices:
 
-     /sys/devices/vfio_ap/matrix/
-     --- [mdev_supported_types]
-     ------ [vfio_ap-passthrough] (passthrough mediated matrix device type)
-     --------- create
-     --------- [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 +1148,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 +1188,98 @@ 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.
+Hot plug/unplug via mdev matrix device sysfs interfaces:
+=======================================================
+If an mdev matrix device is in use by a running guest, AP resources can be
+plugged into or unplugged from the guest via the mdev device's sysfs
+assignment interfaces. Below are some examples.
+
+   To plug adapter 10 into a running guest:
+
+      echo 0xa > assign_adapter
+
+   To unplug domain 5 from a running guest:
+
+      echo 5 > unassign_domain
+
+To display the matrix of a guest using the mdev matrix device:
+
+   cat guest_matrix
+
+If you attempt to display the guest matrix when a guest is not using the
+mdev matrix device, an error will be displayed (ENODEV). Note that adapters and
+domains that are not yet available or not yet assigned to the LPAR can be
+assigned and will become available to the guest as soon as they become available
+to the host.
+
+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

Re: [PATCH v5 6/7] s390: vfio-ap: add logging to vfio_ap driver

[Cornelia Huck]

On Wed, 31 Jul 2019 18:41:16 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> Added two DBF log files for logging events and errors; one for the vfio_ap
> driver, and one for each matrix mediated device.

While the s390dbf is useful (especially for accessing the information
in dumps), trace points are a more standard interface. Have you
evaluated that as well? (We probably should add something to the
vfio/mdev code as well; tracing there is a good complement to tracing
in vendor drivers.)

Also, isn't this independent of the rest of the series?

> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     |  34 +++++++
>  drivers/s390/crypto/vfio_ap_ops.c     | 187 ++++++++++++++++++++++++++++++----
>  drivers/s390/crypto/vfio_ap_private.h |  20 ++++
>  3 files changed, 223 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index d8da520ae1fa..04a77246c22a 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -22,6 +22,10 @@ MODULE_AUTHOR("IBM Corporation");
>  MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
>  MODULE_LICENSE("GPL v2");
>  
> +uint dbglvl = 3;
> +module_param(dbglvl, uint, 0444);
> +MODULE_PARM_DESC(dbglvl, "VFIO_AP driver debug level.");

More the default debug level, isn't it? (IIRC, you can change the level
of the s390dbfs dynamically.)

> +
>  static struct ap_driver vfio_ap_drv;
>  
>  struct ap_matrix_dev *matrix_dev;
> @@ -158,6 +162,21 @@ static void vfio_ap_matrix_dev_destroy(void)
>  	root_device_unregister(root_device);
>  }
>  
> +static void vfio_ap_log_queues_in_use(struct ap_matrix_mdev *matrix_mdev,
> +				  unsigned long *apm, unsigned long *aqm)
> +{
> +	unsigned long apid, apqi;
> +
> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> +			VFIO_AP_DBF(matrix_dev, DBF_ERR,
> +				    "queue %02lx.%04lx in use by mdev %s\n",
> +				    apid, apqi,
> +				    dev_name(mdev_dev(matrix_mdev->mdev)));

I remember some issues wrt %s in s390dbfs (lifetime); will this dbf
potentially outlive the mdev? Or is the string copied? (Or has s390dbf
been changed to avoid that trap? If so, please disregard my comments.)

> +		}
> +	}
> +}
> +
>  static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
>  {
>  	bool in_use = false;
> @@ -179,6 +198,8 @@ static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
>  			continue;
>  
>  		in_use = true;
> +		vfio_ap_log_queues_in_use(matrix_mdev, apm_intrsctn,
> +					  aqm_intrsctn);
>  	}
>  
>  	mutex_unlock(&matrix_dev->lock);
> @@ -186,6 +207,16 @@ static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
>  	return in_use;
>  }
>  
> +static int __init vfio_ap_debug_init(void)
> +{
> +	matrix_dev->dbf = debug_register(VFIO_AP_DRV_NAME, 1, 1,
> +					 DBF_SPRINTF_MAX_ARGS * sizeof(long));

It seems that debug_register() can possibly fail? (Unlikely, but we
should check.)

> +	debug_register_view(matrix_dev->dbf, &debug_sprintf_view);
> +	debug_set_level(matrix_dev->dbf, dbglvl);
> +
> +	return 0;
> +}
> +
>  static int __init vfio_ap_init(void)
>  {
>  	int ret;
> @@ -219,6 +250,8 @@ static int __init vfio_ap_init(void)
>  		return ret;
>  	}
>  
> +	vfio_ap_debug_init();
> +
>  	return 0;
>  }
>  
> @@ -227,6 +260,7 @@ static void __exit vfio_ap_exit(void)
>  	vfio_ap_mdev_unregister();
>  	ap_driver_unregister(&vfio_ap_drv);
>  	vfio_ap_matrix_dev_destroy();
> +	debug_unregister(matrix_dev->dbf);
>  }
>  
>  module_init(vfio_ap_init);
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 0e748819abb6..1aa18eba43d0 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -167,17 +167,23 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>  		case AP_RESPONSE_INVALID_ADDRESS:
>  		default:
>  			/* All cases in default means AP not operational */
> -			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> -				  status.response_code);
>  			goto end_free;
>  		}
>  	} while (retries--);
>  
> -	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> -		  status.response_code);
>  end_free:
>  	vfio_ap_free_aqic_resources(q);
>  	q->matrix_mdev = NULL;
> +	if (status.response_code) {

If I read the code correctly, we consider AP_RESPONSE_OTHERWISE_CHANGED
a success as well, don't we? (Not sure what that means, though.)

> +		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_WARN,
> +			 "IRQ disable failed for queue %02x.%04x: status response code=%u\n",
> +			 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> +			 status.response_code);
> +	} else {
> +		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_INFO,
> +				 "IRQ disabled for queue %02x.%04x\n",
> +				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn));
> +	}
>  	return status;
>  }
>  

(...)

> @@ -321,8 +340,29 @@ static void vfio_ap_matrix_init(struct ap_config_info *info,
>  	matrix->adm_max = info->apxa ? info->Nd : 15;
>  }
>  
> +static int vfio_ap_mdev_debug_init(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	int ret;
> +
> +	matrix_mdev->dbf = debug_register(dev_name(mdev_dev(matrix_mdev->mdev)),
> +					  1, 1,
> +					  DBF_SPRINTF_MAX_ARGS * sizeof(long));
> +
> +	if (!matrix_mdev->dbf)
> +		return -ENOMEM;

Ok, here we do check for the result of debug_register().

> +
> +	ret = debug_register_view(matrix_mdev->dbf, &debug_sprintf_view);
> +	if (ret)
> +		return ret;

Don't we need to clean up ->dbf in the failure case?

Also, we probably need to check this as well for the other dbf.

> +
> +	debug_set_level(matrix_mdev->dbf, dbglvl);
> +
> +	return 0;
> +}
> +
>  static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>  {
> +	int ret;
>  	struct ap_matrix_mdev *matrix_mdev;
>  
>  	if ((atomic_dec_if_positive(&matrix_dev->available_instances) < 0))
> @@ -335,6 +375,13 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>  	}
>  
>  	matrix_mdev->mdev = mdev;
> +
> +	ret = vfio_ap_mdev_debug_init(matrix_mdev);
> +	if (ret) {
> +		kfree(matrix_mdev);
> +		return ret;

You also should bump available_instances again in the failure case.

> +	}
> +
>  	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
>  	mdev_set_drvdata(mdev, matrix_mdev);
>  	matrix_mdev->pqap_hook.hook = handle_pqap;
> @@ -350,14 +397,19 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
>  {
>  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>  
> -	if (matrix_mdev->kvm)
> +	if (matrix_mdev->kvm) {
> +		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
> +				 "remove rejected, mdev in use by %s",
> +				 matrix_mdev->kvm->debugfs_dentry->d_iname);

Can this be a problem when the kvm goes away (and the d_iname is gone)?

Regardless of s390dbf implementation details, is d_iname even valid in
all cases (no debugfs)?

>  		return -EBUSY;
> +	}
>  
>  	mutex_lock(&matrix_dev->lock);
>  	vfio_ap_mdev_reset_queues(mdev);
>  	list_del(&matrix_mdev->node);
>  	mutex_unlock(&matrix_dev->lock);
>  
> +	debug_unregister(matrix_mdev->dbf);
>  	kfree(matrix_mdev);
>  	mdev_set_drvdata(mdev, NULL);
>  	atomic_inc(&matrix_dev->available_instances);
> @@ -406,6 +458,22 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
>  	NULL,
>  };
>  
> +static void vfio_ap_mdev_log_sharing_error(struct ap_matrix_mdev *logdev,
> +					   const char *assigned_to,
> +					   unsigned long *apm,
> +					   unsigned long *aqm)
> +{
> +	unsigned long apid, apqi;
> +
> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> +			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
> +					 "queue %02lx.%04lx already assigned to %s\n",

I'm also not 100% sure about string lifetimes here.

> +					 apid, apqi, assigned_to);
> +		}
> +	}
> +}
> +
>  /**
>   * vfio_ap_mdev_verify_no_sharing
>   *
> @@ -448,22 +516,39 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
>  		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
>  			continue;
>  
> +		vfio_ap_mdev_log_sharing_error(matrix_mdev,
> +					       dev_name(mdev_dev(lstdev->mdev)),
> +					       apm, aqm);
> +
>  		return -EADDRINUSE;
>  	}
>  
>  	return 0;
>  }
>  
> -static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
> +static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *logdev,
>  				       unsigned long *apm, unsigned long *aqm)
>  {
> -	int ret;
> +	int ret = 0;
> +	unsigned long apid, apqi;
> +
> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> +		for_each_set_bit_inv(apqi, aqm, AP_DEVICES) {
> +			if (!ap_owned_by_def_drv(apid, apqi))
> +				continue;
> +
> +			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
> +					 "queue %02lx.%04lx owned by zcrypt\n",

s/zcrypt/default driver/ ?

> +					 apid, apqi);
> +
> +			ret = -EADDRNOTAVAIL;
> +		}
> +	}
>  
> -	ret = ap_apqn_in_matrix_owned_by_def_drv(apm, aqm);
>  	if (ret)
> -		return (ret == 1) ? -EADDRNOTAVAIL : ret;
> +		return ret;
>  
> -	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, apm, aqm);
> +	return vfio_ap_mdev_verify_no_sharing(logdev, apm, aqm);
>  }
>  
>  static void vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)

(...)

> @@ -1013,9 +1132,10 @@ static void vfio_ap_mdev_wait_for_qempty(ap_qid_t qid)
>  			msleep(20);
>  			break;
>  		default:
> -			pr_warn("%s: tapq response %02x waiting for queue %04x.%02x empty\n",
> -				__func__, status.response_code,
> -				AP_QID_CARD(qid), AP_QID_QUEUE(qid));
> +			WARN_ONCE(1,
> +				  "%s: tapq response %02x waiting for queue %04x.%02x empty\n",
> +				  __func__, status.response_code,
> +				  AP_QID_CARD(qid), AP_QID_QUEUE(qid));

Why this change?

>  			return;
>  		}
>  	} while (--retry);

(...)

> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 5cc3c2ebf151..f717e43e10cf 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -24,6 +24,21 @@
>  #define VFIO_AP_MODULE_NAME "vfio_ap"
>  #define VFIO_AP_DRV_NAME "vfio_ap"
>  
> +#define DBF_ERR		3	/* error conditions   */
> +#define DBF_WARN	4	/* warning conditions */
> +#define DBF_INFO	5	/* informational      */
> +#define DBF_DEBUG	6	/* for debugging only */

Can you reuse the LOGLEVEL_* constants instead of rolling your own?

> +
> +#define DBF_SPRINTF_MAX_ARGS 5
> +
> +#define VFIO_AP_DBF(d_matrix_dev, ...) \
> +	debug_sprintf_event(d_matrix_dev->dbf, ##__VA_ARGS__)
> +
> +#define VFIO_AP_MDEV_DBF(d_matrix_mdev, ...) \
> +	debug_sprintf_event(d_matrix_mdev->dbf, ##__VA_ARGS__)
> +
> +extern uint dbglvl;
> +
>  /**
>   * ap_matrix_dev - the AP matrix device structure
>   * @device:	generic device structure associated with the AP matrix device
> @@ -43,6 +58,7 @@ struct ap_matrix_dev {
>  	struct list_head mdev_list;
>  	struct mutex lock;
>  	struct ap_driver  *vfio_ap_drv;
> +	debug_info_t *dbf;
>  };
>  
>  extern struct ap_matrix_dev *matrix_dev;
> @@ -77,6 +93,9 @@ struct ap_matrix {
>   * @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
> + * @pqap_hook:	handler for PQAP instruction
> + * @mdev:	the matrix mediated device

Should updating the description for these two go into a trivial
separate patch?

> + * @dbf:	the debug info log
>   */
>  struct ap_matrix_mdev {
>  	struct list_head node;
> @@ -86,6 +105,7 @@ struct ap_matrix_mdev {
>  	struct kvm *kvm;
>  	struct kvm_s390_module_hook pqap_hook;
>  	struct mdev_device *mdev;
> +	debug_info_t *dbf;
>  };
>  
>  extern int vfio_ap_mdev_register(void);

Re: [PATCH v5 6/7] s390: vfio-ap: add logging to vfio_ap driver

[Tony Krowiak]

On 8/12/19 6:35 AM, Cornelia Huck wrote:
> On Wed, 31 Jul 2019 18:41:16 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> Added two DBF log files for logging events and errors; one for the vfio_ap
>> driver, and one for each matrix mediated device.
> 
> While the s390dbf is useful (especially for accessing the information
> in dumps), trace points are a more standard interface. Have you
> evaluated that as well? (We probably should add something to the
> vfio/mdev code as well; tracing there is a good complement to tracing
> in vendor drivers.)

I assume you are talking about the TRACE() macro here? I have not
evaluated that. I chose s390dbf for the sole reason that the
AP bus (drivers/s390/crypto/ap_bus.c) uses s390dbf. I can look into
using trace points. The genesis of this patch was in response to
comments you made in the previous series (v4). Recall that assignment
of an adapter or domain to a matrix mdev will fail if the APQN(s)
resulting from the assignment are either owned by zcrypt or another
matrix mdev. I said I'd provide a means for the admin to determine
why the assignment failed.

I will look into using trace points, but before I expend the effort
to make such a change, what would be the advantage of trace points
over s390dbf?

> 
> Also, isn't this independent of the rest of the series?

I guess that depends upon your definition of independent. Yes, this
patch could be posted as an entity unto itself, but then the rest of
the series would have to pre-req it given much of the logging is
done in code that has been modified by the series. Is there a
good reason to make this independent?

> 
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     |  34 +++++++
>>   drivers/s390/crypto/vfio_ap_ops.c     | 187 ++++++++++++++++++++++++++++++----
>>   drivers/s390/crypto/vfio_ap_private.h |  20 ++++
>>   3 files changed, 223 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index d8da520ae1fa..04a77246c22a 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -22,6 +22,10 @@ MODULE_AUTHOR("IBM Corporation");
>>   MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
>>   MODULE_LICENSE("GPL v2");
>>   
>> +uint dbglvl = 3;
>> +module_param(dbglvl, uint, 0444);
>> +MODULE_PARM_DESC(dbglvl, "VFIO_AP driver debug level.");
> 
> More the default debug level, isn't it? (IIRC, you can change the level
> of the s390dbfs dynamically.)

The default debug level is 3. This allows the admin to change the debug
level at boot time so as not to miss trace events that might occur prior
to using the sysfs 'level' file to change the debug level.

For the record, I had a suggestion from Harald to change the name to
vfio_dbglvl or something of that nature to avoid namespace collisions.

> 
>> +
>>   static struct ap_driver vfio_ap_drv;
>>   
>>   struct ap_matrix_dev *matrix_dev;
>> @@ -158,6 +162,21 @@ static void vfio_ap_matrix_dev_destroy(void)
>>   	root_device_unregister(root_device);
>>   }
>>   
>> +static void vfio_ap_log_queues_in_use(struct ap_matrix_mdev *matrix_mdev,
>> +				  unsigned long *apm, unsigned long *aqm)
>> +{
>> +	unsigned long apid, apqi;
>> +
>> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
>> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
>> +			VFIO_AP_DBF(matrix_dev, DBF_ERR,
>> +				    "queue %02lx.%04lx in use by mdev %s\n",
>> +				    apid, apqi,
>> +				    dev_name(mdev_dev(matrix_mdev->mdev)));
> 
> I remember some issues wrt %s in s390dbfs (lifetime); will this dbf
> potentially outlive the mdev? Or is the string copied? (Or has s390dbf
> been changed to avoid that trap? If so, please disregard my comments.)

If I understand your question, then this should not be a problem. The
lifespan of the mdev dbf files coincides with the lifespan of the mdev.
The dbf for the matrix mdev is registered when the mdev is created
and unregistered when the mdev is removed. Likewise, the vfio_ap dbf
is created when the module is initialized and unregistered when the
module is exited.

> 
>> +		}
>> +	}
>> +}
>> +
>>   static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
>>   {
>>   	bool in_use = false;
>> @@ -179,6 +198,8 @@ static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
>>   			continue;
>>   
>>   		in_use = true;
>> +		vfio_ap_log_queues_in_use(matrix_mdev, apm_intrsctn,
>> +					  aqm_intrsctn);
>>   	}
>>   
>>   	mutex_unlock(&matrix_dev->lock);
>> @@ -186,6 +207,16 @@ static bool vfio_ap_resource_in_use(unsigned long *apm, unsigned long *aqm)
>>   	return in_use;
>>   }
>>   
>> +static int __init vfio_ap_debug_init(void)
>> +{
>> +	matrix_dev->dbf = debug_register(VFIO_AP_DRV_NAME, 1, 1,
>> +					 DBF_SPRINTF_MAX_ARGS * sizeof(long));
> 
> It seems that debug_register() can possibly fail? (Unlikely, but we
> should check.)

You are right! There should be a check for matrix_dev->dbf not NULL
like we do for the matrix mdev dbf in vfio_ap_mdev_debug_init();

> 
>> +	debug_register_view(matrix_dev->dbf, &debug_sprintf_view);
>> +	debug_set_level(matrix_dev->dbf, dbglvl);
>> +
>> +	return 0;
>> +}
>> +
>>   static int __init vfio_ap_init(void)
>>   {
>>   	int ret;
>> @@ -219,6 +250,8 @@ static int __init vfio_ap_init(void)
>>   		return ret;
>>   	}
>>   
>> +	vfio_ap_debug_init();
>> +
>>   	return 0;
>>   }
>>   
>> @@ -227,6 +260,7 @@ static void __exit vfio_ap_exit(void)
>>   	vfio_ap_mdev_unregister();
>>   	ap_driver_unregister(&vfio_ap_drv);
>>   	vfio_ap_matrix_dev_destroy();
>> +	debug_unregister(matrix_dev->dbf);
>>   }
>>   
>>   module_init(vfio_ap_init);
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 0e748819abb6..1aa18eba43d0 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -167,17 +167,23 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
>>   		case AP_RESPONSE_INVALID_ADDRESS:
>>   		default:
>>   			/* All cases in default means AP not operational */
>> -			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>> -				  status.response_code);
>>   			goto end_free;
>>   		}
>>   	} while (retries--);
>>   
>> -	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
>> -		  status.response_code);
>>   end_free:
>>   	vfio_ap_free_aqic_resources(q);
>>   	q->matrix_mdev = NULL;
>> +	if (status.response_code) {
> 
> If I read the code correctly, we consider AP_RESPONSE_OTHERWISE_CHANGED
> a success as well, don't we? (Not sure what that means, though.)

It indicates that IRQ enable/disable has already been set as requested,
or a the async portion of a previous PQAP(AQIC) has not yet completed.
This just a warning, not an error.

> 
>> +		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_WARN,
>> +			 "IRQ disable failed for queue %02x.%04x: status response code=%u\n",
>> +			 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
>> +			 status.response_code);
>> +	} else {
>> +		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_INFO,
>> +				 "IRQ disabled for queue %02x.%04x\n",
>> +				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn));
>> +	}
>>   	return status;
>>   }
>>   
> 
> (...)
> 
>> @@ -321,8 +340,29 @@ static void vfio_ap_matrix_init(struct ap_config_info *info,
>>   	matrix->adm_max = info->apxa ? info->Nd : 15;
>>   }
>>   
>> +static int vfio_ap_mdev_debug_init(struct ap_matrix_mdev *matrix_mdev)
>> +{
>> +	int ret;
>> +
>> +	matrix_mdev->dbf = debug_register(dev_name(mdev_dev(matrix_mdev->mdev)),
>> +					  1, 1,
>> +					  DBF_SPRINTF_MAX_ARGS * sizeof(long));
>> +
>> +	if (!matrix_mdev->dbf)
>> +		return -ENOMEM;
> 
> Ok, here we do check for the result of debug_register().

Of course:)

> 
>> +
>> +	ret = debug_register_view(matrix_mdev->dbf, &debug_sprintf_view);
>> +	if (ret)
>> +		return ret;
> 
> Don't we need to clean up ->dbf in the failure case?

What's to clean up if it failed?

> 
> Also, we probably need to check this as well for the other dbf.

Yes.

> 
>> +
>> +	debug_set_level(matrix_mdev->dbf, dbglvl);
>> +
>> +	return 0;
>> +}
>> +
>>   static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>>   {
>> +	int ret;
>>   	struct ap_matrix_mdev *matrix_mdev;
>>   
>>   	if ((atomic_dec_if_positive(&matrix_dev->available_instances) < 0))
>> @@ -335,6 +375,13 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
>>   	}
>>   
>>   	matrix_mdev->mdev = mdev;
>> +
>> +	ret = vfio_ap_mdev_debug_init(matrix_mdev);
>> +	if (ret) {
>> +		kfree(matrix_mdev);
>> +		return ret;
> 
> You also should bump available_instances again in the failure case.

I agree and will fix this.

> 
>> +	}
>> +
>>   	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
>>   	mdev_set_drvdata(mdev, matrix_mdev);
>>   	matrix_mdev->pqap_hook.hook = handle_pqap;
>> @@ -350,14 +397,19 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
>>   {
>>   	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>>   
>> -	if (matrix_mdev->kvm)
>> +	if (matrix_mdev->kvm) {
>> +		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
>> +				 "remove rejected, mdev in use by %s",
>> +				 matrix_mdev->kvm->debugfs_dentry->d_iname);
> 
> Can this be a problem when the kvm goes away (and the d_iname is gone)?
> 
> Regardless of s390dbf implementation details, is d_iname even valid in
> all cases (no debugfs)?

I don't know the answer to that. Can you point me to a way to get the
name of the guest?

> 
>>   		return -EBUSY;
>> +	}
>>   
>>   	mutex_lock(&matrix_dev->lock);
>>   	vfio_ap_mdev_reset_queues(mdev);
>>   	list_del(&matrix_mdev->node);
>>   	mutex_unlock(&matrix_dev->lock);
>>   
>> +	debug_unregister(matrix_mdev->dbf);
>>   	kfree(matrix_mdev);
>>   	mdev_set_drvdata(mdev, NULL);
>>   	atomic_inc(&matrix_dev->available_instances);
>> @@ -406,6 +458,22 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
>>   	NULL,
>>   };
>>   
>> +static void vfio_ap_mdev_log_sharing_error(struct ap_matrix_mdev *logdev,
>> +					   const char *assigned_to,
>> +					   unsigned long *apm,
>> +					   unsigned long *aqm)
>> +{
>> +	unsigned long apid, apqi;
>> +
>> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
>> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
>> +			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
>> +					 "queue %02lx.%04lx already assigned to %s\n",
> 
> I'm also not 100% sure about string lifetimes here.

I don't understand your concern here, can you elaborate?

> 
>> +					 apid, apqi, assigned_to);
>> +		}
>> +	}
>> +}
>> +
>>   /**
>>    * vfio_ap_mdev_verify_no_sharing
>>    *
>> @@ -448,22 +516,39 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
>>   		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
>>   			continue;
>>   
>> +		vfio_ap_mdev_log_sharing_error(matrix_mdev,
>> +					       dev_name(mdev_dev(lstdev->mdev)),
>> +					       apm, aqm);
>> +
>>   		return -EADDRINUSE;
>>   	}
>>   
>>   	return 0;
>>   }
>>   
>> -static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
>> +static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *logdev,
>>   				       unsigned long *apm, unsigned long *aqm)
>>   {
>> -	int ret;
>> +	int ret = 0;
>> +	unsigned long apid, apqi;
>> +
>> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
>> +		for_each_set_bit_inv(apqi, aqm, AP_DEVICES) {
>> +			if (!ap_owned_by_def_drv(apid, apqi))
>> +				continue;
>> +
>> +			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
>> +					 "queue %02lx.%04lx owned by zcrypt\n",
> 
> s/zcrypt/default driver/ ?

I don't like default driver because IMHO default driver implies that if
no driver passes the bus match - which matches based on device type -
then it is bound to some default driver. How about:

s/zcrypt/default zcrypt driver/?

> 
>> +					 apid, apqi);
>> +
>> +			ret = -EADDRNOTAVAIL;
>> +		}
>> +	}
>>   
>> -	ret = ap_apqn_in_matrix_owned_by_def_drv(apm, aqm);
>>   	if (ret)
>> -		return (ret == 1) ? -EADDRNOTAVAIL : ret;
>> +		return ret;
>>   
>> -	return vfio_ap_mdev_verify_no_sharing(matrix_mdev, apm, aqm);
>> +	return vfio_ap_mdev_verify_no_sharing(logdev, apm, aqm);
>>   }
>>   
>>   static void vfio_ap_mdev_update_crycb(struct ap_matrix_mdev *matrix_mdev)
> 
> (...)
> 
>> @@ -1013,9 +1132,10 @@ static void vfio_ap_mdev_wait_for_qempty(ap_qid_t qid)
>>   			msleep(20);
>>   			break;
>>   		default:
>> -			pr_warn("%s: tapq response %02x waiting for queue %04x.%02x empty\n",
>> -				__func__, status.response_code,
>> -				AP_QID_CARD(qid), AP_QID_QUEUE(qid));
>> +			WARN_ONCE(1,
>> +				  "%s: tapq response %02x waiting for queue %04x.%02x empty\n",
>> +				  __func__, status.response_code,
>> +				  AP_QID_CARD(qid), AP_QID_QUEUE(qid));
> 
> Why this change?

Given the return following this, it is probably unnecessary. I'll
restore it.

> 
>>   			return;
>>   		}
>>   	} while (--retry);
> 
> (...)
> 
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index 5cc3c2ebf151..f717e43e10cf 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -24,6 +24,21 @@
>>   #define VFIO_AP_MODULE_NAME "vfio_ap"
>>   #define VFIO_AP_DRV_NAME "vfio_ap"
>>   
>> +#define DBF_ERR		3	/* error conditions   */
>> +#define DBF_WARN	4	/* warning conditions */
>> +#define DBF_INFO	5	/* informational      */
>> +#define DBF_DEBUG	6	/* for debugging only */
> 
> Can you reuse the LOGLEVEL_* constants instead of rolling your own?

I assume you are talking about the log levels in linux/kern_levels.h?
Those levels range from -2 to 7. The dbf log levels range from 0 to 6.
It looks like most other drivers that use dbf hard code the levels.
I can do that if you prefer.

> 
>> +
>> +#define DBF_SPRINTF_MAX_ARGS 5
>> +
>> +#define VFIO_AP_DBF(d_matrix_dev, ...) \
>> +	debug_sprintf_event(d_matrix_dev->dbf, ##__VA_ARGS__)
>> +
>> +#define VFIO_AP_MDEV_DBF(d_matrix_mdev, ...) \
>> +	debug_sprintf_event(d_matrix_mdev->dbf, ##__VA_ARGS__)
>> +
>> +extern uint dbglvl;
>> +
>>   /**
>>    * ap_matrix_dev - the AP matrix device structure
>>    * @device:	generic device structure associated with the AP matrix device
>> @@ -43,6 +58,7 @@ struct ap_matrix_dev {
>>   	struct list_head mdev_list;
>>   	struct mutex lock;
>>   	struct ap_driver  *vfio_ap_drv;
>> +	debug_info_t *dbf;
>>   };
>>   
>>   extern struct ap_matrix_dev *matrix_dev;
>> @@ -77,6 +93,9 @@ struct ap_matrix {
>>    * @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
>> + * @pqap_hook:	handler for PQAP instruction
>> + * @mdev:	the matrix mediated device
> 
> Should updating the description for these two go into a trivial
> separate patch?

I will if you insist, but what is gained by that?

> 
>> + * @dbf:	the debug info log
>>    */
>>   struct ap_matrix_mdev {
>>   	struct list_head node;
>> @@ -86,6 +105,7 @@ struct ap_matrix_mdev {
>>   	struct kvm *kvm;
>>   	struct kvm_s390_module_hook pqap_hook;
>>   	struct mdev_device *mdev;
>> +	debug_info_t *dbf;
>>   };
>>   
>>   extern int vfio_ap_mdev_register(void);
> 

Re: [PATCH v5 6/7] s390: vfio-ap: add logging to vfio_ap driver

[Cornelia Huck]

On Mon, 12 Aug 2019 16:34:10 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 8/12/19 6:35 AM, Cornelia Huck wrote:
> > On Wed, 31 Jul 2019 18:41:16 -0400
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >   
> >> Added two DBF log files for logging events and errors; one for the vfio_ap
> >> driver, and one for each matrix mediated device.  
> > 
> > While the s390dbf is useful (especially for accessing the information
> > in dumps), trace points are a more standard interface. Have you
> > evaluated that as well? (We probably should add something to the
> > vfio/mdev code as well; tracing there is a good complement to tracing
> > in vendor drivers.)  
> 
> I assume you are talking about the TRACE() macro here? I have not

TRACE_EVENT() and friends (Documentation/trace/tracepoints.rst).

> evaluated that. I chose s390dbf for the sole reason that the
> AP bus (drivers/s390/crypto/ap_bus.c) uses s390dbf. I can look into
> using trace points. The genesis of this patch was in response to
> comments you made in the previous series (v4). Recall that assignment
> of an adapter or domain to a matrix mdev will fail if the APQN(s)
> resulting from the assignment are either owned by zcrypt or another
> matrix mdev. I said I'd provide a means for the admin to determine
> why the assignment failed.

Yes, providing a way to find out what happened is definitely a good
idea.

> 
> I will look into using trace points, but before I expend the effort
> to make such a change, what would be the advantage of trace points
> over s390dbf?

The question of what system to use is not a new one :)

I think it boils down to who will use it, and what other drivers you
want to correlate with. If it's zcrypt, s390dbf is the more obvious
choice; if vfio/mdev (which currently does not have real tracing), it
would be trace points. Doing both might be overkill... your call, as you
probably know better who the consumers are.

> 
> > 
> > Also, isn't this independent of the rest of the series?  
> 
> I guess that depends upon your definition of independent. Yes, this
> patch could be posted as an entity unto itself, but then the rest of
> the series would have to pre-req it given much of the logging is
> done in code that has been modified by the series. Is there a
> good reason to make this independent?

Tracing would be nice regardless of this series; but reworking it to
separate out this patch does not really make sense.

> 
> >   
> >>
> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> >> ---
> >>   drivers/s390/crypto/vfio_ap_drv.c     |  34 +++++++
> >>   drivers/s390/crypto/vfio_ap_ops.c     | 187 ++++++++++++++++++++++++++++++----
> >>   drivers/s390/crypto/vfio_ap_private.h |  20 ++++
> >>   3 files changed, 223 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> >> index d8da520ae1fa..04a77246c22a 100644
> >> --- a/drivers/s390/crypto/vfio_ap_drv.c
> >> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> >> @@ -22,6 +22,10 @@ MODULE_AUTHOR("IBM Corporation");
> >>   MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
> >>   MODULE_LICENSE("GPL v2");
> >>   
> >> +uint dbglvl = 3;
> >> +module_param(dbglvl, uint, 0444);
> >> +MODULE_PARM_DESC(dbglvl, "VFIO_AP driver debug level.");  
> > 
> > More the default debug level, isn't it? (IIRC, you can change the level
> > of the s390dbfs dynamically.)  
> 
> The default debug level is 3. This allows the admin to change the debug
> level at boot time so as not to miss trace events that might occur prior
> to using the sysfs 'level' file to change the debug level.

What I meant is that the description should probably use the term
"default debug level".

> 
> For the record, I had a suggestion from Harald to change the name to
> vfio_dbglvl or something of that nature to avoid namespace collisions.
> 
> >   
> >> +
> >>   static struct ap_driver vfio_ap_drv;
> >>   
> >>   struct ap_matrix_dev *matrix_dev;
> >> @@ -158,6 +162,21 @@ static void vfio_ap_matrix_dev_destroy(void)
> >>   	root_device_unregister(root_device);
> >>   }
> >>   
> >> +static void vfio_ap_log_queues_in_use(struct ap_matrix_mdev *matrix_mdev,
> >> +				  unsigned long *apm, unsigned long *aqm)
> >> +{
> >> +	unsigned long apid, apqi;
> >> +
> >> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> >> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> >> +			VFIO_AP_DBF(matrix_dev, DBF_ERR,
> >> +				    "queue %02lx.%04lx in use by mdev %s\n",
> >> +				    apid, apqi,
> >> +				    dev_name(mdev_dev(matrix_mdev->mdev)));  
> > 
> > I remember some issues wrt %s in s390dbfs (lifetime); will this dbf
> > potentially outlive the mdev? Or is the string copied? (Or has s390dbf
> > been changed to avoid that trap? If so, please disregard my comments.)  
> 
> If I understand your question, then this should not be a problem. The
> lifespan of the mdev dbf files coincides with the lifespan of the mdev.
> The dbf for the matrix mdev is registered when the mdev is created
> and unregistered when the mdev is removed. Likewise, the vfio_ap dbf
> is created when the module is initialized and unregistered when the
> module is exited.

FTR, I was referring to the following from s390dbf.rst:

"IMPORTANT:                                                                      
  Using "%s" in sprintf event functions is dangerous. You can only              
  use "%s" in the sprintf event functions, if the memory for the passed string  
  is available as long as the debug feature exists. The reason behind this is   
  that due to performance considerations only a pointer to the string is stored 
  in  the debug feature. If you log a string that is freed afterwards, you will 
  get an OOPS when inspecting the debug feature, because then the debug feature 
  will access the already freed memory."

But it isn't a problem in this case.

(...)

> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> >> index 0e748819abb6..1aa18eba43d0 100644
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -167,17 +167,23 @@ static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
> >>   		case AP_RESPONSE_INVALID_ADDRESS:
> >>   		default:
> >>   			/* All cases in default means AP not operational */
> >> -			WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> >> -				  status.response_code);
> >>   			goto end_free;
> >>   		}
> >>   	} while (retries--);
> >>   
> >> -	WARN_ONCE(1, "%s: ap_aqic status %d\n", __func__,
> >> -		  status.response_code);
> >>   end_free:
> >>   	vfio_ap_free_aqic_resources(q);
> >>   	q->matrix_mdev = NULL;
> >> +	if (status.response_code) {  
> > 
> > If I read the code correctly, we consider AP_RESPONSE_OTHERWISE_CHANGED
> > a success as well, don't we? (Not sure what that means, though.)  
> 
> It indicates that IRQ enable/disable has already been set as requested,
> or a the async portion of a previous PQAP(AQIC) has not yet completed.
> This just a warning, not an error.

Ok.

> 
> >   
> >> +		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_WARN,
> >> +			 "IRQ disable failed for queue %02x.%04x: status response code=%u\n",
> >> +			 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
> >> +			 status.response_code);
> >> +	} else {
> >> +		VFIO_AP_MDEV_DBF(q->matrix_mdev, DBF_INFO,
> >> +				 "IRQ disabled for queue %02x.%04x\n",
> >> +				 AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn));
> >> +	}
> >>   	return status;
> >>   }
> >>     
> > 
> > (...)
> >   
> >> @@ -321,8 +340,29 @@ static void vfio_ap_matrix_init(struct ap_config_info *info,
> >>   	matrix->adm_max = info->apxa ? info->Nd : 15;
> >>   }
> >>   
> >> +static int vfio_ap_mdev_debug_init(struct ap_matrix_mdev *matrix_mdev)
> >> +{
> >> +	int ret;
> >> +
> >> +	matrix_mdev->dbf = debug_register(dev_name(mdev_dev(matrix_mdev->mdev)),
> >> +					  1, 1,
> >> +					  DBF_SPRINTF_MAX_ARGS * sizeof(long));
> >> +
> >> +	if (!matrix_mdev->dbf)
> >> +		return -ENOMEM;  
> > 
> > Ok, here we do check for the result of debug_register().  
> 
> Of course:)
> 
> >   
> >> +
> >> +	ret = debug_register_view(matrix_mdev->dbf, &debug_sprintf_view);
> >> +	if (ret)
> >> +		return ret;  
> > 
> > Don't we need to clean up ->dbf in the failure case?  
> 
> What's to clean up if it failed?

debug_register() has allocated some memory, don't we need to free it?

(...)

> >> @@ -350,14 +397,19 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
> >>   {
> >>   	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> >>   
> >> -	if (matrix_mdev->kvm)
> >> +	if (matrix_mdev->kvm) {
> >> +		VFIO_AP_MDEV_DBF(matrix_mdev, DBF_ERR,
> >> +				 "remove rejected, mdev in use by %s",
> >> +				 matrix_mdev->kvm->debugfs_dentry->d_iname);  
> > 
> > Can this be a problem when the kvm goes away (and the d_iname is gone)?
> > 
> > Regardless of s390dbf implementation details, is d_iname even valid in
> > all cases (no debugfs)?  
> 
> I don't know the answer to that. Can you point me to a way to get the
> name of the guest?

I just checked, and this will break if debugfs is not configured.

Unfortunately, I don't have a good idea for an alternative way to
identify the owning machine :/

> 
> >   
> >>   		return -EBUSY;
> >> +	}
> >>   
> >>   	mutex_lock(&matrix_dev->lock);
> >>   	vfio_ap_mdev_reset_queues(mdev);
> >>   	list_del(&matrix_mdev->node);
> >>   	mutex_unlock(&matrix_dev->lock);
> >>   
> >> +	debug_unregister(matrix_mdev->dbf);
> >>   	kfree(matrix_mdev);
> >>   	mdev_set_drvdata(mdev, NULL);
> >>   	atomic_inc(&matrix_dev->available_instances);
> >> @@ -406,6 +458,22 @@ static struct attribute_group *vfio_ap_mdev_type_groups[] = {
> >>   	NULL,
> >>   };
> >>   
> >> +static void vfio_ap_mdev_log_sharing_error(struct ap_matrix_mdev *logdev,
> >> +					   const char *assigned_to,
> >> +					   unsigned long *apm,
> >> +					   unsigned long *aqm)
> >> +{
> >> +	unsigned long apid, apqi;
> >> +
> >> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> >> +		for_each_set_bit_inv(apqi, aqm, AP_DOMAINS) {
> >> +			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
> >> +					 "queue %02lx.%04lx already assigned to %s\n",  
> > 
> > I'm also not 100% sure about string lifetimes here.  
> 
> I don't understand your concern here, can you elaborate?

See the excerpt from the documentation for s390dbf I cited above. Are
we sure that assigned_to will stay around at least as long as the debug
feature does?

> 
> >   
> >> +					 apid, apqi, assigned_to);
> >> +		}
> >> +	}
> >> +}
> >> +
> >>   /**
> >>    * vfio_ap_mdev_verify_no_sharing
> >>    *
> >> @@ -448,22 +516,39 @@ static int vfio_ap_mdev_verify_no_sharing(struct ap_matrix_mdev *matrix_mdev,
> >>   		if (!bitmap_and(aqm, mdev_aqm, lstdev->matrix.aqm, AP_DOMAINS))
> >>   			continue;
> >>   
> >> +		vfio_ap_mdev_log_sharing_error(matrix_mdev,
> >> +					       dev_name(mdev_dev(lstdev->mdev)),
> >> +					       apm, aqm);
> >> +
> >>   		return -EADDRINUSE;
> >>   	}
> >>   
> >>   	return 0;
> >>   }
> >>   
> >> -static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *matrix_mdev,
> >> +static int vfio_ap_mdev_validate_masks(struct ap_matrix_mdev *logdev,
> >>   				       unsigned long *apm, unsigned long *aqm)
> >>   {
> >> -	int ret;
> >> +	int ret = 0;
> >> +	unsigned long apid, apqi;
> >> +
> >> +	for_each_set_bit_inv(apid, apm, AP_DEVICES) {
> >> +		for_each_set_bit_inv(apqi, aqm, AP_DEVICES) {
> >> +			if (!ap_owned_by_def_drv(apid, apqi))
> >> +				continue;
> >> +
> >> +			VFIO_AP_MDEV_DBF(logdev, DBF_ERR,
> >> +					 "queue %02lx.%04lx owned by zcrypt\n",  
> > 
> > s/zcrypt/default driver/ ?  
> 
> I don't like default driver because IMHO default driver implies that if
> no driver passes the bus match - which matches based on device type -
> then it is bound to some default driver. How about:
> 
> s/zcrypt/default zcrypt driver/?

Yes, makes sense to me.

(...)

> >> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> >> index 5cc3c2ebf151..f717e43e10cf 100644
> >> --- a/drivers/s390/crypto/vfio_ap_private.h
> >> +++ b/drivers/s390/crypto/vfio_ap_private.h
> >> @@ -24,6 +24,21 @@
> >>   #define VFIO_AP_MODULE_NAME "vfio_ap"
> >>   #define VFIO_AP_DRV_NAME "vfio_ap"
> >>   
> >> +#define DBF_ERR		3	/* error conditions   */
> >> +#define DBF_WARN	4	/* warning conditions */
> >> +#define DBF_INFO	5	/* informational      */
> >> +#define DBF_DEBUG	6	/* for debugging only */  
> > 
> > Can you reuse the LOGLEVEL_* constants instead of rolling your own?  
> 
> I assume you are talking about the log levels in linux/kern_levels.h?
> Those levels range from -2 to 7. The dbf log levels range from 0 to 6.
> It looks like most other drivers that use dbf hard code the levels.
> I can do that if you prefer.

If the other users of the s390dbf use hard-coded values, it's probably
ok to do so here as well. (But that sounds like s390dbf should provide
some definitions for levels... in a separate series.)

> 
> >   
> >> +
> >> +#define DBF_SPRINTF_MAX_ARGS 5
> >> +
> >> +#define VFIO_AP_DBF(d_matrix_dev, ...) \
> >> +	debug_sprintf_event(d_matrix_dev->dbf, ##__VA_ARGS__)
> >> +
> >> +#define VFIO_AP_MDEV_DBF(d_matrix_mdev, ...) \
> >> +	debug_sprintf_event(d_matrix_mdev->dbf, ##__VA_ARGS__)
> >> +
> >> +extern uint dbglvl;
> >> +
> >>   /**
> >>    * ap_matrix_dev - the AP matrix device structure
> >>    * @device:	generic device structure associated with the AP matrix device
> >> @@ -43,6 +58,7 @@ struct ap_matrix_dev {
> >>   	struct list_head mdev_list;
> >>   	struct mutex lock;
> >>   	struct ap_driver  *vfio_ap_drv;
> >> +	debug_info_t *dbf;
> >>   };
> >>   
> >>   extern struct ap_matrix_dev *matrix_dev;
> >> @@ -77,6 +93,9 @@ struct ap_matrix {
> >>    * @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
> >> + * @pqap_hook:	handler for PQAP instruction
> >> + * @mdev:	the matrix mediated device  
> > 
> > Should updating the description for these two go into a trivial
> > separate patch?  
> 
> I will if you insist, but what is gained by that?

I had to look twice to see that those two fields have not been
introduced by this patch :)

If this series will make the next merge window anyway, no need to spend
effort to split this out, though.

> 
> >   
> >> + * @dbf:	the debug info log
> >>    */
> >>   struct ap_matrix_mdev {
> >>   	struct list_head node;
> >> @@ -86,6 +105,7 @@ struct ap_matrix_mdev {
> >>   	struct kvm *kvm;
> >>   	struct kvm_s390_module_hook pqap_hook;
> >>   	struct mdev_device *mdev;
> >> +	debug_info_t *dbf;
> >>   };
> >>   
> >>   extern int vfio_ap_mdev_register(void);  
> >   
>