vfio: ap: AP Queue Interrupt Control

We prepare the interception of the PQAP/AQIC instruction for the case the AQIC facility is enabled in the guest.

First of all we do not want to change existing behavior when intercepting AP instructions without the SIE allowing the guest to use AP instructions.

In this patch we only handle the AQIC interception allowed by facility 65 which will be enabled when the complete interception infrastructure will be present.

We add a callback inside the KVM arch structure for s390 for a VFIO driver to handle a specific response to the PQAP instruction with the AQIC command and only this command.

But we want to be able to return a correct answer to the guest even there is no VFIO AP driver in the kernel.
Therefor, we inject the correct exceptions from inside KVM for the case the callback is not initialized, which happens when the vfio_ap driver is not loaded.

We do consider the responsability of the driver to always initialize the PQAP callback if it defines queues by initializing the CRYCB for a guest.
If the callback has been setup we call it.
If not we setup an answer considering that no queue is available for the guest when no callback has been setup.

Signed-off-by Pierre Morel <[email protected]>

---
  arch/s390/include/asm/kvm_host.h      |  7 +++
  arch/s390/kvm/priv.c                  | 86 +++++++++++++++++++++++++++++++++++
  drivers/s390/crypto/vfio_ap_private.h |  2 +
  3 files changed, 95 insertions(+)

arch/s390/include/asm/kvm_host.h +7 -0

index 9fff9ab..af10a11 100644
@@ -18,6 +18,7 @@
  #include <linux/kvm_host.h>
  #include <linux/kvm.h>
  #include <linux/seqlock.h>
+#include <linux/module.h>
  #include <asm/debug.h>
  #include <asm/cpu.h>
  #include <asm/fpu/api.h>

@@ -722,8 +723,14 @@ struct kvm_s390_cpu_model {
  	unsigned short ibc;
  };
 
+struct kvm_s390_module_hook {
+	int (*hook)(struct kvm_vcpu *vcpu);
+	struct module *owner;
+};
+
  struct kvm_s390_crypto {
  	struct kvm_s390_crypto_cb *crycb;
+	struct kvm_s390_module_hook *pqap_hook;
  	__u32 crycbd;
  	__u8 aes_kw;
  	__u8 dea_kw;

arch/s390/kvm/priv.c +27 -0

index 8679bd7..a9be84f 100644
@@ -27,6 +27,7 @@
  #include <asm/io.h>
  #include <asm/ptrace.h>
  #include <asm/sclp.h>
+#include <asm/ap.h>
  #include "gaccess.h"
  #include "kvm-s390.h"
  #include "trace.h"

@@ -592,6 +593,89 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
  	}
  }
 
+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * We now support PQAP/AQIC instructions and we need to correctly
+ * answer the guest even if no dedicated driver's hook is available.
+ *
+ * The intercepting code calls a dedicated callback for this instruction
+ * if a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * If no callback is available, the queues are not available, return this
+ * response code to the caller and set CC to 3.
+ * Else return the response code returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	struct ap_queue_status status = {};
+	unsigned long reg0;
+	int ret;
+	uint8_t fc;
+
+	/* Verify that the AP instruction are available */
+	if (!ap_instructions_available())
+		return -EOPNOTSUPP;
+	/* Verify that the guest is allowed to use AP instructions */

arch/s390/kvm/priv.c +49 -0

+	if (WARN_ON_ONCE(fc != 0x03))
+		return -EOPNOTSUPP;
+
+	/* PQAP instruction is allowed for guest kernel only */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
+
+	/* Common PQAP instruction specification exceptions */
+	/* bits 41-47 must all be zeros */
+	if (reg0 & 0x007f0000UL)
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* APFT not install and T bit set */
+	if (!test_kvm_facility(vcpu->kvm, 15) && (reg0 & 0x00800000UL))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* APXA not installed and APID greater 64 or APQI greater 16 */
+	if (!(vcpu->kvm->arch.crypto.crycbd & 0x02) && (reg0 & 0x0000c0f0UL))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+
+	/* AQIC function code specific exception */
+	/* facility 65 not present for AQIC function code */
+	if (!test_kvm_facility(vcpu->kvm, 65))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+
+	/*
+	 * Verify that the hook callback is registered, lock the owner
+	 * and call the hook.
+	 */
+	if (vcpu->kvm->arch.crypto.pqap_hook) {
+		if (!try_module_get(vcpu->kvm->arch.crypto.pqap_hook->owner))
+			return -EOPNOTSUPP;
+		ret = vcpu->kvm->arch.crypto.pqap_hook->hook(vcpu);
+		module_put(vcpu->kvm->arch.crypto.pqap_hook->owner);
+		if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000)
+			kvm_s390_set_psw_cc(vcpu, 3);
+		return ret;
+	}
+	/*
+	 * A vfio_driver must register a hook.
+	 * No hook means no driver to enable the SIE CRYCB and no queues.
+	 * We send this response to the guest.
+	 */
+	status.response_code = 0x01;
+	memcpy(&vcpu->run->s.regs.gprs[1], &status, sizeof(status));
+	kvm_s390_set_psw_cc(vcpu, 3);
+	return 0;
+}
+
  static int handle_stfl(struct kvm_vcpu *vcpu)
  {
  	int rc;

@@ -878,6 +962,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
  		return handle_sthyi(vcpu);
  	case 0x7d:
  		return handle_stsi(vcpu);
+	case 0xaf:
+		return handle_pqap(vcpu);
  	case 0xb1:
  		return handle_stfl(vcpu);
  	case 0xb2:

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

index 76b7f98..a910be1 100644
@@ -16,6 +16,7 @@
  #include <linux/mdev.h>
  #include <linux/delay.h>
  #include <linux/mutex.h>
+#include <linux/kvm_host.h>
 
  #include "ap_bus.h"
 

@@ -81,6 +82,7 @@ struct ap_matrix_mdev {
  	struct ap_matrix matrix;
  	struct notifier_block group_notifier;
  	struct kvm *kvm;
+	struct kvm_s390_module_hook pqap_hook;
  };
 
  extern int vfio_ap_mdev_register(void);

Quoted 108 lines · Pierre Morel

We prepare the interception of the PQAP/AQIC instruction for the case the AQIC facility is enabled in the guest.

First of all we do not want to change existing behavior when intercepting AP instructions without the SIE allowing the guest to use AP instructions.

In this patch we only handle the AQIC interception allowed by facility 65 which will be enabled when the complete interception infrastructure will be present.

We add a callback inside the KVM arch structure for s390 for a VFIO driver to handle a specific response to the PQAP instruction with the AQIC command and only this command.

But we want to be able to return a correct answer to the guest even there is no VFIO AP driver in the kernel.
Therefor, we inject the correct exceptions from inside KVM for the case the callback is not initialized, which happens when the vfio_ap driver is not loaded.

We do consider the responsability of the driver to always initialize the PQAP callback if it defines queues by initializing the CRYCB for a guest.
If the callback has been setup we call it.
If not we setup an answer considering that no queue is available for the guest when no callback has been setup.

Signed-off-by Pierre Morel <[email protected]>

---
  arch/s390/include/asm/kvm_host.h      |  7 +++
  arch/s390/kvm/priv.c                  | 86
+++++++++++++++++++++++++++++++++++
  drivers/s390/crypto/vfio_ap_private.h |  2 +

  3 files changed, 95 insertions(+)

arch/s390/include/asm/kvm_host.h +7 -0

index 9fff9ab..af10a11 100644
@@ -18,6 +18,7 @@
   #include <linux/kvm_host.h>
   #include <linux/kvm.h>
   #include <linux/seqlock.h>
+#include <linux/module.h>
   #include <asm/debug.h>
   #include <asm/cpu.h>
   #include <asm/fpu/api.h>

@@ -722,8 +723,14 @@ struct kvm_s390_cpu_model {
       unsigned short ibc;
   };
+struct kvm_s390_module_hook {
+    int (*hook)(struct kvm_vcpu *vcpu);
+    struct module *owner;
+};
+
   struct kvm_s390_crypto {
       struct kvm_s390_crypto_cb *crycb;
+    struct kvm_s390_module_hook *pqap_hook;
       __u32 crycbd;
       __u8 aes_kw;
       __u8 dea_kw;

arch/s390/kvm/priv.c +13 -0

index 8679bd7..a9be84f 100644
@@ -27,6 +27,7 @@
   #include <asm/io.h>
   #include <asm/ptrace.h>
   #include <asm/sclp.h>
+#include <asm/ap.h>
   #include "gaccess.h"
   #include "kvm-s390.h"
   #include "trace.h"

@@ -592,6 +593,89 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
       }
   }
+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * We now support PQAP/AQIC instructions and we need to correctly
+ * answer the guest even if no dedicated driver's hook is available.
+ *
+ * The intercepting code calls a dedicated callback for this instruction
+ * if a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * If no callback is available, the queues are not available, return 

this
+ * response code to the caller and set CC to 3.
+ * Else return the response code returned by the callback.
+ */

arch/s390/kvm/priv.c +15 -0

+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    struct ap_queue_status status = {};
+    unsigned long reg0;
+    int ret;
+    uint8_t fc;
+
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;
+    /*
+     * The only possibly intercepted functions when AP instructions are

Quoted 6 lines

+     * available for the guest are AQIC and TAPQ with the t bit set +     * since we do not set IC.3 (FIII) we currently will only intercept +     * the AQIC function code.
+     */
+    reg0 = vcpu->run->s.regs.gprs[0];
+    fc = reg0 >> 24;

Should you also mask off bits 0-32 in case they might not be zeroes?

Other than this, r-b

We register a AP PQAP instruction hook during the open of the mediated device. And unregister it on release.

During the probe of the AP device, we allocate a vfio_ap_queue structure to keep track of the information we need for the PQAP/AQIC instruction interception.

In the AP PQAP instruction hook, if we receive a demand to enable IRQs,
- we retrieve the vfio_ap_queue based on the APQN we receive in REG1,
- we retrieve the page of the guest address, (NIB), from register REG2
- we retrieve the mediated device to use the VFIO pinning infrastructure to pin the page of the guest address,
- we retrieve the pointer to KVM to register the guest ISC and retrieve the host ISC
- finaly we activate GISA

If we receive a demand to disable IRQs,
- we deactivate GISA
- unregister from the GIB
- unping the NIB

When removing the AP device from the driver the device is reseted and this process unregisters the GISA from the GIB, and unpins the NIB address then we free the vfio_ap_queue structure.

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/ap_bus.h          |   1 +
  drivers/s390/crypto/vfio_ap_drv.c     |  36 ++++-
  drivers/s390/crypto/vfio_ap_ops.c     | 283 +++++++++++++++++++++++++++++++++-
  drivers/s390/crypto/vfio_ap_private.h |  11 ++
  4 files changed, 324 insertions(+), 7 deletions(-)

drivers/s390/crypto/ap_bus.h +1 -0

index 15a98a6..60e70f3 100644
@@ -43,6 +43,7 @@ static inline int ap_test_bit(unsigned int *ptr, unsigned int nr)
  #define AP_RESPONSE_BUSY		0x05
  #define AP_RESPONSE_INVALID_ADDRESS	0x06
  #define AP_RESPONSE_OTHERWISE_CHANGED	0x07
+#define AP_RESPONSE_INVALID_GISA	0x08
  #define AP_RESPONSE_Q_FULL		0x10
  #define AP_RESPONSE_NO_PENDING_REPLY	0x10
  #define AP_RESPONSE_INDEX_TOO_BIG	0x11

drivers/s390/crypto/vfio_ap_drv.c +21 -1

index e9824c3..d7d8341 100644
@@ -5,6 +5,7 @@
   * Copyright IBM Corp. 2018
   *
   * Author(s): Tony Krowiak <[email protected]>
+ *	      Pierre Morel <[email protected]>
   */
 
  #include <linux/module.h>

@@ -40,14 +41,47 @@ static struct ap_device_id ap_queue_ids[] = {
 
  MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
 
+/**
+ * vfio_ap_queue_dev_probe:
+ *
+ * Allocate a vfio_ap_queue structure and associate it
+ * with the device as driver_data.
+ */
  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;
  }
 
+/**
+ * vfio_ap_queue_dev_remove:
+ *
+ * Takes the matrix lock to avoid actions on this device while removing
+ * Free the associated vfio_ap_queue structure
+ */
  static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
  {
-	/* Nothing to do yet */

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

+		vfio_ap_irq_disable(q);
+		kfree(q);
+	}
+	mutex_unlock(&matrix_dev->lock);
  }
 
  static void vfio_ap_matrix_dev_release(struct device *dev)

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

index e8e87bf..448c40c 100644
@@ -24,6 +24,242 @@
  #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"

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

+
+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
+ * @matrix_mdev: the associated mediated matrix
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the
+ * devices of the vfio_ap_drv.
+ * Verify that the APID and the APQI are set in the matrix.
+ *
+ * Returns the pointer to the associated vfio_ap_queue
+ */
+struct vfio_ap_queue *vfio_ap_get_queue(struct ap_matrix_mdev *matrix_mdev,
+					int apqn)
+{

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

+	if (!dev)
+		return NULL;
+	q = dev_get_drvdata(dev);
+	q->matrix_mdev = matrix_mdev;
+	put_device(dev);
+
+	return q;
+}
+
+/**
+ * vfio_ap_irq_disable
+ * @q: The vfio_ap_queue
+ *
+ * Unpin the guest NIB
+ * Unregister the ISC from the GIB alert
+ * Clear the vfio_ap_queue intern fields
+ *
+ * Important notice:
+ * Before calling this function, interrupt must have been
+ * unregistered by use of ap_zapq/ap_reset or ap_aqic.
+ */

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

+		}
+		break;
+	case AP_RESPONSE_BUSY:
+		msleep(20);
+		if (retry_aqic-- < 0)
+			return status;
+		goto again;
+	default:

drivers/s390/crypto/vfio_ap_ops.c +184 -6

+
+	q->saved_pfn = 0;
+	q->saved_isc = VFIO_AP_ISC_INVALID;
+	q->matrix_mdev = NULL;
+	return (struct ap_queue_status) {};
+}
+
+/**
+ * vfio_ap_setirq: Enable Interruption for a APQN
+ *
+ * @dev: the device associated with the ap_queue
+ * @q:   the vfio_ap_queue holding AQIC parameters
+ *
+ * Pin the NIB saved in *q
+ * Register the guest ISC to GIB interface and retrieve the
+ * host ISC to issue the host side PQAP/AQIC
+ *
+ * Response.status may be set to following Response Code in case of error:
+ * - AP_RESPONSE_INVALID_ADDRESS: vfio_pin_pages failed
+ * - AP_RESPONSE_OTHERWISE_CHANGED: Hypervizor GISA internal error
+ *
+ * Otherwise return the ap_queue_status returned by the ap_aqic()
+ */
+static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
+						 int isc,
+						 unsigned long nib)
+{
+	struct ap_qirq_ctrl aqic_gisa = {};
+	struct ap_queue_status status = {};
+	struct kvm_s390_gisa *gisa;
+	struct kvm *kvm;
+	unsigned long h_nib, g_pfn, h_pfn;
+	int ret;
+
+	g_pfn = nib >> PAGE_SHIFT;
+	ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &g_pfn, 1,
+			     IOMMU_READ | IOMMU_WRITE, &h_pfn);
+	switch (ret) {
+	case 1:
+		break;
+	default:
+		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
+		return status;
+	}
+
+	kvm = q->matrix_mdev->kvm;
+	gisa = kvm->arch.gisa_int.origin;
+
+	h_nib = (h_pfn << PAGE_SHIFT) | (nib & ~PAGE_MASK);
+	aqic_gisa.gisc = isc;
+	aqic_gisa.isc = kvm_s390_gisc_register(kvm, isc);
+	aqic_gisa.ir = 1;
+	aqic_gisa.gisa = (uint64_t)gisa >> 4;
+
+	status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
+	switch (status.response_code) {
+	case AP_RESPONSE_NORMAL:
+		/* See if we did clear older IRQ configuration */
+		if (q->saved_pfn)
+			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
+					 &q->saved_pfn, 1);
+		if (q->saved_isc != VFIO_AP_ISC_INVALID)
+			kvm_s390_gisc_unregister(kvm, q->saved_isc);
+		q->saved_pfn = g_pfn;
+		q->saved_isc = isc;
+		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);
+		break;
+	default:
+		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
+			status.response_code);
+		vfio_ap_irq_disable(q);
+		break;
+	}
+
+	return status;
+}
+
+/**
+ * handle_pqap: PQAP instruction callback
+ *
+ * @vcpu: The vcpu on which we received the PQAP instruction
+ *
+ * Get the general register contents to initialize internal variables.
+ * REG[0]: APQN
+ * REG[1]: IR and ISC
+ * REG[2]: NIB
+ *
+ * Response.status may be set to following Response Code:
+ * - AP_RESPONSE_Q_NOT_AVAIL: if the queue is not available
+ * - AP_RESPONSE_DECONFIGURED: if the queue is not configured
+ * - AP_RESPONSE_NORMAL (0) : in case of successs
+ *   Check vfio_ap_setirq() and vfio_ap_clrirq() for other possible RC.
+ * We take the matrix_dev lock to ensure serialization on queues and
+ * mediated device access.
+ *
+ * Return 0 if we could handle the request inside KVM.
+ * otherwise, returns -EOPNOTSUPP to let QEMU handle the fault.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	uint64_t status;
+	uint16_t apqn;
+	struct vfio_ap_queue *q;
+	struct ap_queue_status qstatus = {
+			       .response_code = AP_RESPONSE_Q_NOT_AVAIL, };
+	struct ap_matrix_mdev *matrix_mdev;
+
+	/* If we do not use the AIV facility just go to userland */
+	if (!(vcpu->arch.sie_block->eca & ECA_AIV))
+		return -EOPNOTSUPP;
+
+	apqn = vcpu->run->s.regs.gprs[0] & 0xffff;
+	mutex_lock(&matrix_dev->lock);
+
+	if (!vcpu->kvm->arch.crypto.pqap_hook)
+		goto out_unlock;
+	matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
+				   struct ap_matrix_mdev, pqap_hook);
+
+	q = vfio_ap_get_queue(matrix_mdev, apqn);
+	if (!q)
+		goto out_unlock;
+
+	status = vcpu->run->s.regs.gprs[1];
+
+	/* If IR bit(16) is set we enable the interrupt */
+	if ((status >> (63 - 16)) & 0x01)
+		qstatus = vfio_ap_irq_enable(q, status & 0x07,
+					     vcpu->run->s.regs.gprs[2]);
+	else
+		qstatus = vfio_ap_irq_disable(q);
+
+out_unlock:
+	memcpy(&vcpu->run->s.regs.gprs[1], &qstatus, sizeof(qstatus));
+	vcpu->run->s.regs.gprs[1] >>= 32;
+	mutex_unlock(&matrix_dev->lock);
+	return 0;
+}
+
  static void vfio_ap_matrix_init(struct ap_config_info *info,
  				struct ap_matrix *matrix)
  {

@@ -45,8 +281,11 @@ static int vfio_ap_mdev_create(struct kobject *kobj, struct mdev_device *mdev)
  		return -ENOMEM;
  	}
 
+	matrix_mdev->mdev = mdev;
  	vfio_ap_matrix_init(&matrix_dev->info, &matrix_mdev->matrix);
  	mdev_set_drvdata(mdev, matrix_mdev);
+	matrix_mdev->pqap_hook.hook = handle_pqap;
+	matrix_mdev->pqap_hook.owner = THIS_MODULE;
  	mutex_lock(&matrix_dev->lock);
  	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
  	mutex_unlock(&matrix_dev->lock);

@@ -62,6 +301,7 @@ static int vfio_ap_mdev_remove(struct mdev_device *mdev)
  		return -EBUSY;
 
  	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_reset_queues(mdev);
  	list_del(&matrix_mdev->node);
  	mutex_unlock(&matrix_dev->lock);
 

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

@@ -819,15 +1061,36 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
  	return NOTIFY_OK;
  }
 
-static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
-				    unsigned int retry)
+static void vfio_ap_irq_disable_apqn(int apqn)
+{
+	struct device *dev;
+	struct vfio_ap_queue *q;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				 &apqn, match_apqn);
+	if (dev) {
+		q = dev_get_drvdata(dev);
+		vfio_ap_irq_disable(q);
+		put_device(dev);
+	}
+}
+
+int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
+			     unsigned int retry)
  {
  	struct ap_queue_status status;
+	int retry2 = 2;
+	int apqn = AP_MKQID(apid, apqi);
 
  	do {
-		status = ap_zapq(AP_MKQID(apid, apqi));
+		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);
  			return 0;
  		case AP_RESPONSE_RESET_IN_PROGRESS:
  		case AP_RESPONSE_BUSY:

@@ -861,6 +1124,7 @@ static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
  			 */
  			if (ret)
  				rc = ret;
+			vfio_ap_irq_disable_apqn(AP_MKQID(apid, apqi));
  		}
  	}
 

@@ -904,15 +1168,20 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
  {
  	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
 
-	if (matrix_mdev->kvm)
+	mutex_lock(&matrix_dev->lock);
+	if (matrix_mdev->kvm) {
  		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
+		matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+		vfio_ap_mdev_reset_queues(mdev);
+		kvm_put_kvm(matrix_mdev->kvm);
+		matrix_mdev->kvm = NULL;
+	}
+	mutex_unlock(&matrix_dev->lock);
 
-	vfio_ap_mdev_reset_queues(mdev);
  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
  				 &matrix_mdev->iommu_notifier);
  	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
  				 &matrix_mdev->group_notifier);
-	matrix_mdev->kvm = NULL;
  	module_put(THIS_MODULE);
  }
 

@@ -941,6 +1210,7 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
  {
  	int ret;
 
+	mutex_lock(&matrix_dev->lock);
  	switch (cmd) {
  	case VFIO_DEVICE_GET_INFO:
  		ret = vfio_ap_mdev_get_device_info(arg);

@@ -952,6 +1222,7 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
  		ret = -EOPNOTSUPP;
  		break;
  	}
+	mutex_unlock(&matrix_dev->lock);
 
  	return ret;
  }

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

index 18dcc4d..f46dde5 100644
@@ -4,6 +4,7 @@
   *
   * Author(s): Tony Krowiak <[email protected]>
   *	      Halil Pasic <[email protected]>
+ *	      Pierre Morel <[email protected]>
   *
   * Copyright IBM Corp. 2018
   */

@@ -89,5 +90,15 @@ 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;
+	unsigned long saved_pfn;
+	int	apqn;
+#define VFIO_AP_ISC_INVALID 0xff
+	unsigned char saved_isc;
+};
+struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
  #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 19 lines · Pierre Morel

We register a AP PQAP instruction hook during the open of the mediated device. And unregister it on release.

During the probe of the AP device, we allocate a vfio_ap_queue structure to keep track of the information we need for the PQAP/AQIC instruction interception.

In the AP PQAP instruction hook, if we receive a demand to enable IRQs,
- we retrieve the vfio_ap_queue based on the APQN we receive in REG1,
- we retrieve the page of the guest address, (NIB), from register REG2
- we retrieve the mediated device to use the VFIO pinning infrastructure to pin the page of the guest address,
- we retrieve the pointer to KVM to register the guest ISC and retrieve the host ISC
- finaly we activate GISA

Quoted 4 lines

If we receive a demand to disable IRQs,
- we deactivate GISA
- unregister from the GIB
- unping the NIB

s/unping/unpin

Quoted 5 lines

  static void vfio_ap_queue_dev_remove(struct ap_device *apdev) {
-    /* Nothing to do yet */
+    struct vfio_ap_queue *q;
+    int apid, apqi;

Quoted 8 lines

+
+    mutex_lock(&matrix_dev->lock);
+    q = dev_get_drvdata(&apdev->device);
+    dev_set_drvdata(&apdev->device, NULL);
+    if (q) {
+        apid = AP_QID_CARD(q->apqn);
+        apqi = AP_QID_QUEUE(q->apqn);
+        vfio_ap_mdev_reset_queue(apid, apqi, 1);

As you know, there is another patch series (s390: vfio-ap: dynamic configuration support) posted concurrently with this series. That series handles reset on remove of an AP queue device. It looks like your choices here will greatly conflict with the reset processing in the other patch series and create a nasty merge conflict. My suggestion is that you build this patch series on top of the other series and do the following:

There are two new functions introduced in vfio_ap_private.h:
void vfio_ap_mdev_remove_queue(struct ap_queue *queue); void vfio_ap_mdev_probe_queue(struct ap_queue *queue);

These are called from the probe and remove callbacks in vfio_ap_drv.c. If you embed your changes to the probe and remove functions above into those new functions, that will make merging the two much easier and the code cleaner IMHO.

Quoted 5 lines

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

+struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
+{
+    struct ap_qirq_ctrl aqic_gisa;
+    struct ap_queue_status status = {
+            .response_code = AP_RESPONSE_Q_NOT_AVAIL,

Quoted 5 lines

+            };
+    int retry_tapq = 5;
+    int retry_aqic = 5;
+
+    if (!q)

When will q ever be NULL? I checked all places where this is called and it looks to me like this will never happen.

Review

Quoted 3 lines

+        return status;

(quoted diff) +2 -0

+
+again:

I'm not crazy about using a label, why not a do/while loop or something of that nature?

Quoted 9 lines

+    status = ap_aqic(q->apqn, aqic_gisa, NULL); +    switch (status.response_code) {
+    case AP_RESPONSE_OTHERWISE_CHANGED:
+    case AP_RESPONSE_RESET_IN_PROGRESS:
+    case AP_RESPONSE_NORMAL: /* Fall through */ +        while (status.irq_enabled && retry_tapq--) { +            msleep(20);
+            status = ap_tapq(q->apqn, NULL);

Shouldn't you be checking response codes from the TAPQ function? Maybe there should be a function call here to with for IRQ disabled?