[PATCH v7 0/4] vfio: ap: AP Queue Interrupt Control

[PATCH v7 0/4] vfio: ap: AP Queue Interrupt Control

[Pierre Morel]

This patch series implements PQAP/AQIC interception in KVM.

1) Data to handle GISA interrupt for AQIC

To implement this we need to add a new structure, vfio_ap_queue,
to be able to retrieve the mediated device associated with a queue
and specific values needed to register/unregister the interrupt
structures:
  - APQN: to be able to issue the commands and search for queue
    structures
  - NIB and old NIB : to unpin the NIB on clear IRQ
  - ISC and old ISC : to unregister with the GIB interface
  - matrix_mdev: to retrieve the associate matrix and mediated device

Specific handling bei keeping old values when re-registering is
needed because the guest could unregister interrupt in a invisble
manner bei issuing an un-interceptible RESET command.

Reset commands issued directly by the guest and indirectly when
removing the guest unpin the memory and deregister the ISC.

The vfio_ap_queue is associated to the ap_device during the probe
of the device and dissociated during the remove of the ap_device.

The vfio_ap_queue is associated to the matrix mediated device during
each interception of the AQIC command, so it do not need to be
dissociated until the guest is terminated.

The life of the vfio_ap_queue will be protected by the matrix_dev lock
to guaranty that no change can occur to the CRYCB or that devices can
not be removed when a vfio_ap_queue is in use.

2) KVM destroy race conditions

To make sure that KVM do not vanish and GISA is still available
when the VFIO_AP driver is in used we take a reference to KVM
during the opening of the mediated device and release it on
releasing the mediated device.

3) Interception of PQAP

The driver registers a hook structure to KVM providing:
- a pointer to a function implementing PQAP(AQIC) handling
- the reference to the module owner of the hook

On interception by KVM we do not change the behavior, returning
 -EOPNOTSUPP to the user in the case AP instructions are not
supported by the host or by the guest.
Otherwise we verify the exceptions cases before trying to call 
the vfio_ap hook.

In the case we do not find a hook we assume that the CRYCB has not
been setup for the guest and is empty.


4) Removing the AP device

Removing the AP device without having unassign it is clearly
discourage by the documentation.
The patch series does not check if the queue is used by a
guest and simply free the vfio_ap_queue.

5) Associated QEMU patch

There is a QEMU patch which is needed to enable the PQAP/AQIC
facility in the guest.

Posted in qemu-devel@nongnu.org as:
Message-Id: <1550146494-21085-1-git-send-email-pmorel@linux.ibm.com>


Pierre Morel (4):
  s390: ap: kvm: add PQAP interception for AQIC
  vfio: ap: register IOMMU VFIO notifier
  s390: ap: implement PAPQ AQIC interception in kernel
  s390: ap: kvm: Enable PQAP/AQIC facility for the guest

 arch/s390/include/asm/kvm_host.h      |   7 +
 arch/s390/kvm/priv.c                  |  86 +++++++++
 arch/s390/tools/gen_facilities.c      |   1 +
 drivers/s390/crypto/ap_bus.h          |   1 +
 drivers/s390/crypto/vfio_ap_drv.c     |  30 ++-
 drivers/s390/crypto/vfio_ap_ops.c     | 336 +++++++++++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |  15 ++
 7 files changed, 468 insertions(+), 8 deletions(-)

-- 
2.7.4

Changelog:

Changelog from v6:
- Not taking care if the AP queue is associated with a guest
  admin is warn in the odcumentation
 (Tony, Halil)
- Using WARN_ON_ONCE, direct call to report specification errors
  (Christian)
- Wait until the IRQ bit is cleared when clearing interrupts
 (Tony, Halil)
- Some minor changes and add some comments before
  vfio_ap_free_irq_data
  (Pierre)
- initializing the pointer to matrix_mdev in vfio_ap_queue 
  during the interception and suppress the association during
  assignment and the usage of lists.
  (Tony, Halil)
- Merging patches for creation of vfio_ap_queue, initialization
  and use of vfio_ap_queue during interception of PQAP/AQIC
  (Conny)

Changelog from v5:
- Refactoring of the PQAP interception after all discussions
  (Conny, Halil (offline))
- take a big lock around open to avoid parallel changes through
  assignment
- verify that at least one queue has a APID or APQI when
  first assignment is done to not accept unavailable APID/APQI
  (myself)
- Adding comment for locks on free_list
  (Conny)
- Modified comment for 
  "s390: ap: setup relation betwen KVM and mediated device"
  (Halil)

Changelog from v4:
- Add forgotten locking for vfio_get_queue() in pqap callback
  (Conny / Halil)
- Add KVM reference counting to make sure GISA is free after IRQ
  (Christian / Halil)
- Take care that ISC = 0 is a valid ISC
  (Halil)
- Integrate the PQAP call back in a structure with module owner
  reference counting to make sure the callback does not disappear.
- Restrict functionality to always open KVM before opening the
  VFIO device.
- Search all devices in the vfio_ap driver list when associating
  a queue to a mediated device
  (Halil / Tony)
- Get vfio_ap_free_irq() out of vfio_ap_mdev_reset_queue() to call
  it always, whatever the result of the reset.
  (Tony)

Changelog from v3:
- Associating the vfio_queues during APID/APQI assign
  (Tony)
- Dissociating the vfio_queues during APID/APQI unassign
  (Tony)
- Taking care that the guest can directly disable the interrupt
  by using a RESET
  (Halil)
- Remove the patch creating the matrix bus to accelerate its
  integration in Linux stable
  (Christian)

Changelog from v1:
- Refactoring to handle interception in kernel instead of in
  QEMU
  (Halil)

[PATCH v7 1/4] s390: ap: kvm: add PQAP interception for AQIC

[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 <pmorel@linux.ibm.com>
---
 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(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 9fff9ab..af10a11 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -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;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 8679bd7..a9be84f 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -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 */
+	if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+		return -EOPNOTSUPP;
+	/*
+	 * The only possibly intercepted functions when AP instructions are
+	 * 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;
+	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:
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 76b7f98..a910be1 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -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);
-- 
2.7.4

[PATCH v7 2/4] vfio: ap: register IOMMU VFIO notifier

[Pierre Morel]

To be able to use the VFIO interface to facilitate the
mediated device memory pinning/unpinning we need to register
a notifier for IOMMU.

While we will start to pin one guest page for the interrupt indicator
byte, this is still ok with ballooning as this page will never be
used by the guest virtio-balloon driver.
So the pinned page will never be freed. And even a broken guest does
so, that would not impact the host as the original page is still
in control by vfio.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 43 ++++++++++++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |  2 ++
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 900b9cf..e8e87bf 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -759,6 +759,35 @@ static int vfio_ap_mdev_set_kvm(struct ap_matrix_mdev *matrix_mdev,
 	return 0;
 }
 
+/*
+ * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
+ *
+ * @nb: The notifier block
+ * @action: Action to be taken
+ * @data: data associated with the request
+ *
+ * For an UNMAP request, unpin the guest IOVA (the NIB guest address we
+ * pinned before). Other requests are ignored.
+ *
+ */
+static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
+				       unsigned long action, void *data)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	matrix_mdev = container_of(nb, struct ap_matrix_mdev, iommu_notifier);
+
+	if (action == VFIO_IOMMU_NOTIFY_DMA_UNMAP) {
+		struct vfio_iommu_type1_dma_unmap *unmap = data;
+		unsigned long g_pfn = unmap->iova >> PAGE_SHIFT;
+
+		vfio_unpin_pages(mdev_dev(matrix_mdev->mdev), &g_pfn, 1);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 				       unsigned long action, void *data)
 {
@@ -858,7 +887,17 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 		return ret;
 	}
 
-	return 0;
+	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
+	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
+	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				     &events, &matrix_mdev->iommu_notifier);
+	if (!ret)
+		return ret;
+
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
+				 &matrix_mdev->group_notifier);
+	module_put(THIS_MODULE);
+	return ret;
 }
 
 static void vfio_ap_mdev_release(struct mdev_device *mdev)
@@ -869,6 +908,8 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
 
 	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;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index a910be1..18dcc4d 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -81,8 +81,10 @@ struct ap_matrix_mdev {
 	struct list_head node;
 	struct ap_matrix matrix;
 	struct notifier_block group_notifier;
+	struct notifier_block iommu_notifier;
 	struct kvm *kvm;
 	struct kvm_s390_module_hook pqap_hook;
+	struct mdev_device *mdev;
 };
 
 extern int vfio_ap_mdev_register(void);
-- 
2.7.4

[PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

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

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 the mediated device to use the VFIO pinning infratrsucture
  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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/ap_bus.h          |   1 +
 drivers/s390/crypto/vfio_ap_drv.c     |  30 +++-
 drivers/s390/crypto/vfio_ap_ops.c     | 293 +++++++++++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |  11 ++
 4 files changed, 328 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index 15a98a6..60e70f3 100644
--- a/drivers/s390/crypto/ap_bus.h
+++ b/drivers/s390/crypto/ap_bus.h
@@ -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
diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index e9824c3..ecaa4ab 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -5,6 +5,7 @@
  * Copyright IBM Corp. 2018
  *
  * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
+ *	      Pierre Morel <pmorel@linux.ibm.com>
  */
 
 #include <linux/module.h>
@@ -40,14 +41,41 @@ 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->a_isc = VFIO_AP_ISC_INVALID;
+	q->p_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 */
+	struct vfio_ap_queue *q;
+
+	mutex_lock(&matrix_dev->lock);
+	q = dev_get_drvdata(&apdev->device);
+	kfree(q);
+	dev_set_drvdata(&apdev->device, NULL);
+	mutex_unlock(&matrix_dev->lock);
 }
 
 static void vfio_ap_matrix_dev_release(struct device *dev)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e8e87bf..c22b996 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -24,6 +24,252 @@
 #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
 #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
+static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
+
+static int match_apqn(struct device *dev, void *data)
+{
+	struct vfio_ap_queue *q = dev_get_drvdata(dev);
+
+	return (q->apqn == *(int *)(data)) ? 1 : 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)
+{
+	struct vfio_ap_queue *q;
+	struct device *dev;
+
+	if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
+		return NULL;
+	if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
+		return NULL;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				 &apqn, match_apqn);
+	if (!dev)
+		return NULL;
+	q = dev_get_drvdata(dev);
+	q->matrix_mdev = matrix_mdev;
+	put_device(dev);
+
+	return q;
+}
+
+/**
+ * vfio_ap_free_irq_data:
+ * @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.
+ */
+static void vfio_ap_free_irq_data(struct vfio_ap_queue *q)
+{
+	if (!q)
+		return;
+
+	if (q->a_isc != VFIO_AP_ISC_INVALID && q->matrix_mdev)
+		kvm_s390_gisc_unregister(q->matrix_mdev->kvm, q->a_isc);
+	if (q->a_pfn && q->matrix_mdev)
+		vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1);
+
+	q->a_pfn = 0;
+	q->p_pfn = 0;
+	q->a_isc = VFIO_AP_ISC_INVALID;
+	q->p_isc = VFIO_AP_ISC_INVALID;
+	q->matrix_mdev = NULL;
+}
+
+/**
+ * vfio_ap_clrirq: Disable Interruption for a APQN
+ *
+ * @dev: the device associated with the ap_queue
+ * @q:   the vfio_ap_queue holding AQIC parameters
+ *
+ * Issue the host side PQAP/AQIC
+ * On success: unpin the NIB saved in *q and unregister from GIB
+ * interface
+ *
+ * Return the ap_queue_status returned by the ap_aqic()
+ */
+static struct ap_queue_status vfio_ap_clrirq(struct vfio_ap_queue *q)
+{
+	struct ap_qirq_ctrl aqic_gisa = {};
+	struct ap_queue_status status;
+	int checks = 10;
+
+	status = ap_aqic(q->apqn, aqic_gisa, NULL);
+	if (!status.response_code) {
+		while (status.irq_enabled && checks--) {
+			msleep(20);
+			status = ap_tapq(q->apqn, NULL);
+		}
+		if (checks >= 0)
+			vfio_ap_free_irq_data(q);
+		else
+			WARN_ONCE("%s: failed disabling IRQ", __func__);
+	}
+
+	return 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_setirq(struct vfio_ap_queue *q)
+{
+	struct ap_qirq_ctrl aqic_gisa = {};
+	struct ap_queue_status status = {};
+	struct kvm_s390_gisa *gisa;
+	struct kvm *kvm;
+	unsigned long h_nib, h_pfn;
+	int ret;
+
+	q->a_pfn = q->a_nib >> PAGE_SHIFT;
+	ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1,
+			     IOMMU_READ | IOMMU_WRITE, &h_pfn);
+	switch (ret) {
+	case 1:
+		break;
+	case -EINVAL:
+	case -E2BIG:
+		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
+		/* Fallthrough */
+	default:
+		return status;
+	}
+
+	kvm = q->matrix_mdev->kvm;
+	gisa = kvm->arch.gisa_int.origin;
+
+	h_nib = (h_pfn << PAGE_SHIFT) | (q->a_nib & ~PAGE_MASK);
+	aqic_gisa.gisc = q->a_isc;
+	aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->a_isc);
+	aqic_gisa.ir = 1;
+	aqic_gisa.gisa = gisa->next_alert >> 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->p_pfn)
+			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
+					 &q->p_pfn, 1);
+		if (q->p_isc != VFIO_AP_ISC_INVALID)
+			kvm_s390_gisc_unregister(kvm, q->p_isc);
+		q->p_pfn = q->a_pfn;
+		q->p_isc = q->a_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), &q->a_pfn, 1);
+		kvm_s390_gisc_unregister(kvm, q->a_isc);
+		break;
+	default:	/* Fall Through */
+		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
+			status.response_code);
+		vfio_ap_free_irq_data(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) {
+		q->a_isc = status & 0x07;
+		q->a_nib = vcpu->run->s.regs.gprs[2];
+		qstatus = vfio_ap_setirq(q);
+		if (qstatus.response_code) {
+			q->a_nib = 0;
+			q->a_isc = VFIO_AP_ISC_INVALID;
+		}
+	} else
+		qstatus = vfio_ap_clrirq(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 +291,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 +311,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 +1004,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 +1071,37 @@ 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)
+void vfio_ap_free_irq_data_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_free_irq_data(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);
+			vfio_ap_free_irq_data_apqn(apqn);
 			return 0;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
@@ -904,15 +1178,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 +1220,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 +1232,7 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
 		ret = -EOPNOTSUPP;
 		break;
 	}
+	mutex_unlock(&matrix_dev->lock);
 
 	return ret;
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 18dcc4d..7cc02ff 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -4,6 +4,7 @@
  *
  * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
  *	      Halil Pasic <pasic@linux.ibm.com>
+ *	      Pierre Morel <pmorel@linux.ibm.com>
  *
  * Copyright IBM Corp. 2018
  */
@@ -90,4 +91,14 @@ struct ap_matrix_mdev {
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
 
+struct vfio_ap_queue {
+	struct ap_matrix_mdev *matrix_mdev;
+	unsigned long a_nib;
+	unsigned long a_pfn;
+	unsigned long p_pfn;
+	int	apqn;
+#define VFIO_AP_ISC_INVALID 0xff
+	unsigned char a_isc;
+	unsigned char p_isc;
+};
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v7 4/4] s390: ap: kvm: Enable PQAP/AQIC facility for the guest

[Pierre Morel]

AP Queue Interruption Control (AQIC) facility gives
the guest the possibility to control interruption for
the Cryptographic Adjunct Processor queues.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/tools/gen_facilities.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
index 61ce5b5..aed14fc 100644
--- a/arch/s390/tools/gen_facilities.c
+++ b/arch/s390/tools/gen_facilities.c
@@ -114,6 +114,7 @@ static struct facility_def facility_defs[] = {
 		.bits = (int[]){
 			12, /* AP Query Configuration Information */
 			15, /* AP Facilities Test */
+			65, /* AP Queue Interruption Control */
 			156, /* etoken facility */
 			-1  /* END */
 		}
-- 
2.7.4

Re: [PATCH v7 2/4] vfio: ap: register IOMMU VFIO notifier

[Halil Pasic]

On Fri, 26 Apr 2019 15:01:26 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> @@ -858,7 +887,17 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>  		return ret;
>  	}
>  
> -	return 0;
> +	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
> +	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> +	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> +				     &events, &matrix_mdev->iommu_notifier);
> +	if (!ret)
> +		return ret;
> +
> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> +				 &matrix_mdev->group_notifier);
> +	module_put(THIS_MODULE);

Can you please explain this module_put() here? I don't see anything in
the cover letter.

Regards,
Halil

> +	return ret;
>  }

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Halil Pasic]

On Fri, 26 Apr 2019 15:01:27 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> +static struct ap_queue_status vfio_ap_setirq(struct vfio_ap_queue *q)
> +{
> +	struct ap_qirq_ctrl aqic_gisa = {};
> +	struct ap_queue_status status = {};
> +	struct kvm_s390_gisa *gisa;
> +	struct kvm *kvm;
> +	unsigned long h_nib, h_pfn;
> +	int ret;
> +
> +	q->a_pfn = q->a_nib >> PAGE_SHIFT;
> +	ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1,
> +			     IOMMU_READ | IOMMU_WRITE, &h_pfn);
> +	switch (ret) {
> +	case 1:
> +		break;
> +	case -EINVAL:
> +	case -E2BIG:
> +		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
> +		/* Fallthrough */
> +	default:
> +		return status;

Can we actually hit the default label? AFICT you would return an
all-zero status, i.e. status.response_code == 0 'Normal completion'.

> +	}
> +
> +	kvm = q->matrix_mdev->kvm;
> +	gisa = kvm->arch.gisa_int.origin;
> +
> +	h_nib = (h_pfn << PAGE_SHIFT) | (q->a_nib & ~PAGE_MASK);
> +	aqic_gisa.gisc = q->a_isc;
> +	aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->a_isc);
> +	aqic_gisa.ir = 1;
> +	aqic_gisa.gisa = gisa->next_alert >> 4;

Why gisa->next_alert? Isn't this supposed to get set to gisa origin
(without some bits on the left)?

> +
> +	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->p_pfn)
> +			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
> +					 &q->p_pfn, 1);
> +		if (q->p_isc != VFIO_AP_ISC_INVALID)
> +			kvm_s390_gisc_unregister(kvm, q->p_isc);
> +		q->p_pfn = q->a_pfn;
> +		q->p_isc = q->a_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), &q->a_pfn, 1);
> +		kvm_s390_gisc_unregister(kvm, q->a_isc);

Hm, see below. Wouldn't you want to set a_isc to VFIO_AP_ISC_INVALID?

> +		break;
> +	default:	/* Fall Through */

Is it 'break' or is it 'Fall Through'?

> +		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
> +			status.response_code);
> +		vfio_ap_free_irq_data(q);
> +		break;
> +	}
> +
> +	return status;
> +}

Re: [PATCH v7 2/4] vfio: ap: register IOMMU VFIO notifier

[Pierre Morel]

On 29/04/2019 18:07, Halil Pasic wrote:
> On Fri, 26 Apr 2019 15:01:26 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> @@ -858,7 +887,17 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>>   		return ret;
>>   	}
>>   
>> -	return 0;
>> +	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
>> +	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>> +	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>> +				     &events, &matrix_mdev->iommu_notifier);
>> +	if (!ret)
>> +		return ret;
>> +
>> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> +				 &matrix_mdev->group_notifier);
>> +	module_put(THIS_MODULE);
> 
> Can you please explain this module_put() here? I don't see anything in
> the cover letter.

May be you should have a look at the sources or the original patch 
series of Tony, there is a try_module_get() at the beginning of open to 
make sure that the module is not taken away while in use by the guest.

In the case we failed to open the mediated device we let fall the reference.

Regards,
Pierre


> 
> Regards,
> Halil
> 
>> +	return ret;
>>   }
> 


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

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 29/04/2019 18:50, Halil Pasic wrote:
> On Fri, 26 Apr 2019 15:01:27 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> +static struct ap_queue_status vfio_ap_setirq(struct vfio_ap_queue *q)
>> +{
>> +	struct ap_qirq_ctrl aqic_gisa = {};
>> +	struct ap_queue_status status = {};
>> +	struct kvm_s390_gisa *gisa;
>> +	struct kvm *kvm;
>> +	unsigned long h_nib, h_pfn;
>> +	int ret;
>> +
>> +	q->a_pfn = q->a_nib >> PAGE_SHIFT;
>> +	ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1,
>> +			     IOMMU_READ | IOMMU_WRITE, &h_pfn);
>> +	switch (ret) {
>> +	case 1:
>> +		break;
>> +	case -EINVAL:
>> +	case -E2BIG:
>> +		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
>> +		/* Fallthrough */
>> +	default:
>> +		return status;
> 
> Can we actually hit the default label? AFICT you would return an
> all-zero status, i.e. status.response_code == 0 'Normal completion'.

hum right, the setting of AP_INVALID_ADDRESS should be in the default 
and there is no need for the two error cases, they will match the default.


> 
>> +	}
>> +
>> +	kvm = q->matrix_mdev->kvm;
>> +	gisa = kvm->arch.gisa_int.origin;
>> +
>> +	h_nib = (h_pfn << PAGE_SHIFT) | (q->a_nib & ~PAGE_MASK);
>> +	aqic_gisa.gisc = q->a_isc;
>> +	aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->a_isc);
>> +	aqic_gisa.ir = 1;
>> +	aqic_gisa.gisa = gisa->next_alert >> 4;
> 
> Why gisa->next_alert? Isn't this supposed to get set to gisa origin
> (without some bits on the left)?

Someone already asked this question.
The answer is: look at the ap_qirq_ctrl structure, you will see that the 
gisa field is 27 bits wide.

> 
>> +
>> +	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->p_pfn)
>> +			vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
>> +					 &q->p_pfn, 1);
>> +		if (q->p_isc != VFIO_AP_ISC_INVALID)
>> +			kvm_s390_gisc_unregister(kvm, q->p_isc);
>> +		q->p_pfn = q->a_pfn;
>> +		q->p_isc = q->a_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), &q->a_pfn, 1);
>> +		kvm_s390_gisc_unregister(kvm, q->a_isc);
> 
> Hm, see below. Wouldn't you want to set a_isc to VFIO_AP_ISC_INVALID?

grrr!!! when did I insert these 3 lines, it was OK in previous series!
all 3 lines, vfio_unpin() / gisc_unregister / break must go away.

> 
>> +		break;
>> +	default:	/* Fall Through */
> 
> Is it 'break' or is it 'Fall Through'?

it is a fall through

> 
>> +		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>> +			status.response_code);
>> +		vfio_ap_free_irq_data(q);
>> +		break;
>> +	}
>> +
>> +	return status;
>> +}


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

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 30/04/2019 10:18, Pierre Morel wrote:
> On 29/04/2019 18:50, Halil Pasic wrote:
>> On Fri, 26 Apr 2019 15:01:27 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> +static struct ap_queue_status vfio_ap_setirq(struct vfio_ap_queue *q)
>>> +{
>>> +    struct ap_qirq_ctrl aqic_gisa = {};
>>> +    struct ap_queue_status status = {};
>>> +    struct kvm_s390_gisa *gisa;
>>> +    struct kvm *kvm;
>>> +    unsigned long h_nib, h_pfn;
>>> +    int ret;
>>> +
>>> +    q->a_pfn = q->a_nib >> PAGE_SHIFT;
>>> +    ret = vfio_pin_pages(mdev_dev(q->matrix_mdev->mdev), &q->a_pfn, 1,
>>> +                 IOMMU_READ | IOMMU_WRITE, &h_pfn);
>>> +    switch (ret) {
>>> +    case 1:
>>> +        break;
>>> +    case -EINVAL:
>>> +    case -E2BIG:
>>> +        status.response_code = AP_RESPONSE_INVALID_ADDRESS;
>>> +        /* Fallthrough */
>>> +    default:
>>> +        return status;
>>
>> Can we actually hit the default label? AFICT you would return an
>> all-zero status, i.e. status.response_code == 0 'Normal completion'.
> 
> hum right, the setting of AP_INVALID_ADDRESS should be in the default 
> and there is no need for the two error cases, they will match the default.
> 
> 
>>
>>> +    }
>>> +
>>> +    kvm = q->matrix_mdev->kvm;
>>> +    gisa = kvm->arch.gisa_int.origin;
>>> +
>>> +    h_nib = (h_pfn << PAGE_SHIFT) | (q->a_nib & ~PAGE_MASK);
>>> +    aqic_gisa.gisc = q->a_isc;
>>> +    aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->a_isc);
>>> +    aqic_gisa.ir = 1;
>>> +    aqic_gisa.gisa = gisa->next_alert >> 4;
>>
>> Why gisa->next_alert? Isn't this supposed to get set to gisa origin
>> (without some bits on the left)?
> 
> Someone already asked this question.
> The answer is: look at the ap_qirq_ctrl structure, you will see that the 
> gisa field is 27 bits wide.
> 
>>
>>> +
>>> +    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->p_pfn)
>>> +            vfio_unpin_pages(mdev_dev(q->matrix_mdev->mdev),
>>> +                     &q->p_pfn, 1);
>>> +        if (q->p_isc != VFIO_AP_ISC_INVALID)
>>> +            kvm_s390_gisc_unregister(kvm, q->p_isc);
>>> +        q->p_pfn = q->a_pfn;
>>> +        q->p_isc = q->a_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), &q->a_pfn, 1);
>>> +        kvm_s390_gisc_unregister(kvm, q->a_isc);
>>
>> Hm, see below. Wouldn't you want to set a_isc to VFIO_AP_ISC_INVALID?
> 
> grrr!!! when did I insert these 3 lines, it was OK in previous series!
> all 3 lines, vfio_unpin() / gisc_unregister / break must go away.

No it wasn't, I will correct this.

> 
>>
>>> +        break;
>>> +    default:    /* Fall Through */
>>
>> Is it 'break' or is it 'Fall Through'?
> 
> it is a fall through
> 
>>
>>> +        pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
>>> +            status.response_code);
>>> +        vfio_ap_free_irq_data(q);
>>> +        break;
>>> +    }
>>> +
>>> +    return status;
>>> +}
> 
> 


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

Re: [PATCH v7 2/4] vfio: ap: register IOMMU VFIO notifier

[Halil Pasic]

On Tue, 30 Apr 2019 09:59:51 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 29/04/2019 18:07, Halil Pasic wrote:
> > On Fri, 26 Apr 2019 15:01:26 +0200
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> > 
> >> @@ -858,7 +887,17 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
> >>   		return ret;
> >>   	}
> >>   
> >> -	return 0;
> >> +	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
> >> +	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
> >> +	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> >> +				     &events, &matrix_mdev->iommu_notifier);
> >> +	if (!ret)
> >> +		return ret;
> >> +
> >> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> >> +				 &matrix_mdev->group_notifier);
> >> +	module_put(THIS_MODULE);
> > 
> > Can you please explain this module_put() here? I don't see anything in
> > the cover letter.
> 
> May be you should have a look at the sources or the original patch 
> series of Tony, there is a try_module_get() at the beginning of open to 
> make sure that the module is not taken away while in use by the guest.
> 
> In the case we failed to open the mediated device we let fall the reference.
> 

Right, my bad. I did not notice we were on the error recovery path.

Regards,
Halil

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Halil Pasic]

On Tue, 30 Apr 2019 10:32:52 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> >>> +    aqic_gisa.gisa = gisa->next_alert >> 4;  
> >>
> >> Why gisa->next_alert? Isn't this supposed to get set to gisa origin
> >> (without some bits on the left)?  

s/left/right/

> > 
> > Someone already asked this question.

It must have been in some previous iteration... Can you give me a
pointer?

> > The answer is: look at the ap_qirq_ctrl structure, you will see that the 
> > gisa field is 27 bits wide.

My question was not about the width, but about gisa->next_alert being
used.

Regards,
Halil

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 30/04/2019 11:37, Halil Pasic wrote:
> On Tue, 30 Apr 2019 10:32:52 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>>>>> +    aqic_gisa.gisa = gisa->next_alert >> 4;
>>>>
>>>> Why gisa->next_alert? Isn't this supposed to get set to gisa origin
>>>> (without some bits on the left)?
> 
> s/left/right/
> 
>>>
>>> Someone already asked this question.
> 
> It must have been in some previous iteration... Can you give me a
> pointer?
> 
>>> The answer is: look at the ap_qirq_ctrl structure, you will see that the
>>> gisa field is 27 bits wide.
> 
> My question was not about the width, but about gisa->next_alert being
> used.
> 
> Regards,
> Halil
> 

Ah, OK, I understand.
it is inherited from the time I allocated the GISA myself, before the 
Mimu GISA/GIB patches.

So now indeed I must use the GISA origin for the case next_alert is used 
by GIB alert queue.


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

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Halil Pasic]

On Fri, 26 Apr 2019 15:01:27 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> +/**
> + * vfio_ap_clrirq: Disable Interruption for a APQN
> + *
> + * @dev: the device associated with the ap_queue
> + * @q:   the vfio_ap_queue holding AQIC parameters
> + *
> + * Issue the host side PQAP/AQIC
> + * On success: unpin the NIB saved in *q and unregister from GIB
> + * interface
> + *
> + * Return the ap_queue_status returned by the ap_aqic()
> + */
> +static struct ap_queue_status vfio_ap_clrirq(struct vfio_ap_queue *q)
> +{
> +	struct ap_qirq_ctrl aqic_gisa = {};
> +	struct ap_queue_status status;
> +	int checks = 10;
> +
> +	status = ap_aqic(q->apqn, aqic_gisa, NULL);
> +	if (!status.response_code) {
> +		while (status.irq_enabled && checks--) {
> +			msleep(20);

Hm, that seems like a lot of time to me. And I suppose we are holding the
kvm lock: e.g. no other instruction can be interpreted by kvm in the
meantime.

> +			status = ap_tapq(q->apqn, NULL);
> +		}
> +		if (checks >= 0)
> +			vfio_ap_free_irq_data(q);

Actually we don't have to wait for the async part to do it's magic
(indicated by the status.irq_enabled --> !status.irq_enabled transition)
in the instruction handler. We have to wait so we can unpin the NIB but
that could be done async (e.g. workqueue).

BTW do you have any measurements here? How many msleep(20) do we
experience for one clear on average?

If linux is not using clear (you told so offline, and I also remember
something similar), we can probably get away with something like this,
and do it properly (from performance standpoint) later.

Regards,
Halil

> +		else
> +			WARN_ONCE("%s: failed disabling IRQ", __func__);
> +	}
> +
> +	return status;
> +}

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Halil Pasic]

On Fri, 26 Apr 2019 15:01:27 +0200
Pierre Morel <pmorel@linux.ibm.com> wrote:

> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 18dcc4d..7cc02ff 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -4,6 +4,7 @@
>   *
>   * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
>   *	      Halil Pasic <pasic@linux.ibm.com>
> + *	      Pierre Morel <pmorel@linux.ibm.com>
>   *
>   * Copyright IBM Corp. 2018
>   */
> @@ -90,4 +91,14 @@ struct ap_matrix_mdev {
>  extern int vfio_ap_mdev_register(void);
>  extern void vfio_ap_mdev_unregister(void);
>  
> +struct vfio_ap_queue {
> +	struct ap_matrix_mdev *matrix_mdev;
> +	unsigned long a_nib;
> +	unsigned long a_pfn;
> +	unsigned long p_pfn;
> +	int	apqn;
> +#define VFIO_AP_ISC_INVALID 0xff

How about -1?

> +	unsigned char a_isc;
> +	unsigned char p_isc;
> +};
>  #endif /* _VFIO_AP_PRIVATE_H_ */

I assume a_ and p_ are for argument and private, or? Anyway it would be
nice to have nicer names for these.

If the a_ members are really just arguments, we could probably live
without the. I'm fine either way.

Regards,
Halil

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 30/04/2019 16:00, Halil Pasic wrote:
> On Fri, 26 Apr 2019 15:01:27 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index 18dcc4d..7cc02ff 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -4,6 +4,7 @@
>>    *
>>    * Author(s): Tony Krowiak <akrowiak@linux.ibm.com>
>>    *	      Halil Pasic <pasic@linux.ibm.com>
>> + *	      Pierre Morel <pmorel@linux.ibm.com>
>>    *
>>    * Copyright IBM Corp. 2018
>>    */
>> @@ -90,4 +91,14 @@ struct ap_matrix_mdev {
>>   extern int vfio_ap_mdev_register(void);
>>   extern void vfio_ap_mdev_unregister(void);
>>   
>> +struct vfio_ap_queue {
>> +	struct ap_matrix_mdev *matrix_mdev;
>> +	unsigned long a_nib;
>> +	unsigned long a_pfn;
>> +	unsigned long p_pfn;
>> +	int	apqn;
>> +#define VFIO_AP_ISC_INVALID 0xff
> 
> How about -1?
> 
>> +	unsigned char a_isc;
>> +	unsigned char p_isc;
>> +};
>>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 
> I assume a_ and p_ are for argument and private, or? Anyway it would be
> nice to have nicer names for these.
> 
> If the a_ members are really just arguments, we could probably live
> without the. I'm fine either way.
> 
> Regards,
> Halil
> 

Since there will be another iteration to modify other part of this patch
I will simplify this handling and make the names clearer.

Thanks
Pierre

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

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 30/04/2019 15:26, Halil Pasic wrote:
> On Fri, 26 Apr 2019 15:01:27 +0200
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> +/**
>> + * vfio_ap_clrirq: Disable Interruption for a APQN
>> + *
>> + * @dev: the device associated with the ap_queue
>> + * @q:   the vfio_ap_queue holding AQIC parameters
>> + *
>> + * Issue the host side PQAP/AQIC
>> + * On success: unpin the NIB saved in *q and unregister from GIB
>> + * interface
>> + *
>> + * Return the ap_queue_status returned by the ap_aqic()
>> + */
>> +static struct ap_queue_status vfio_ap_clrirq(struct vfio_ap_queue *q)
>> +{
>> +	struct ap_qirq_ctrl aqic_gisa = {};
>> +	struct ap_queue_status status;
>> +	int checks = 10;
>> +
>> +	status = ap_aqic(q->apqn, aqic_gisa, NULL);
>> +	if (!status.response_code) {
>> +		while (status.irq_enabled && checks--) {
>> +			msleep(20);
> 
> Hm, that seems like a lot of time to me. And I suppose we are holding the
> kvm lock: e.g. no other instruction can be interpreted by kvm in the
> meantime.
> 
>> +			status = ap_tapq(q->apqn, NULL);
>> +		}
>> +		if (checks >= 0)
>> +			vfio_ap_free_irq_data(q);
> 
> Actually we don't have to wait for the async part to do it's magic
> (indicated by the status.irq_enabled --> !status.irq_enabled transition)
> in the instruction handler. We have to wait so we can unpin the NIB but
> that could be done async (e.g. workqueue).
> 
> BTW do you have any measurements here? How many msleep(20) do we
> experience for one clear on average?

No idea but it is probably linked to the queue state and usage history.
I can use a lower sleep time and increment the retry count.

> 
> If linux is not using clear (you told so offline, and I also remember
> something similar), we can probably get away with something like this,
> and do it properly (from performance standpoint) later.

In the Linux AP code it is only used once, in the explicit
ap_queue_enable_interruption() function.

Yes, thanks, I will keep it as is, may be just play with msleep()time 
and retry count.

Regards,
Pierre

> 
> Regards,
> Halil
> 
>> +		else
>> +			WARN_ONCE("%s: failed disabling IRQ", __func__);
>> +	}
>> +
>> +	return status;
>> +}
> 


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

Re: [PATCH v7 3/4] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 30/04/2019 16:09, Pierre Morel wrote:
> On 30/04/2019 15:26, Halil Pasic wrote:
>> On Fri, 26 Apr 2019 15:01:27 +0200
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> +/**
>>> + * vfio_ap_clrirq: Disable Interruption for a APQN
>>> + *
>>> + * @dev: the device associated with the ap_queue
>>> + * @q:   the vfio_ap_queue holding AQIC parameters
>>> + *
>>> + * Issue the host side PQAP/AQIC
>>> + * On success: unpin the NIB saved in *q and unregister from GIB
>>> + * interface
>>> + *
>>> + * Return the ap_queue_status returned by the ap_aqic()
>>> + */
>>> +static struct ap_queue_status vfio_ap_clrirq(struct vfio_ap_queue *q)
>>> +{
>>> +    struct ap_qirq_ctrl aqic_gisa = {};
>>> +    struct ap_queue_status status;
>>> +    int checks = 10;
>>> +
>>> +    status = ap_aqic(q->apqn, aqic_gisa, NULL);
>>> +    if (!status.response_code) {
>>> +        while (status.irq_enabled && checks--) {
>>> +            msleep(20);
>>
>> Hm, that seems like a lot of time to me. And I suppose we are holding the
>> kvm lock: e.g. no other instruction can be interpreted by kvm in the
>> meantime.
>>
>>> +            status = ap_tapq(q->apqn, NULL);
>>> +        }
>>> +        if (checks >= 0)
>>> +            vfio_ap_free_irq_data(q);
>>
>> Actually we don't have to wait for the async part to do it's magic
>> (indicated by the status.irq_enabled --> !status.irq_enabled transition)
>> in the instruction handler. We have to wait so we can unpin the NIB but
>> that could be done async (e.g. workqueue).
>>
>> BTW do you have any measurements here? How many msleep(20) do we
>> experience for one clear on average?
> 
> No idea but it is probably linked to the queue state and usage history.
> I can use a lower sleep time and increment the retry count.
> 
>>
>> If linux is not using clear (you told so offline, and I also remember
>> something similar), we can probably get away with something like this,
>> and do it properly (from performance standpoint) later.
> 
> In the Linux AP code it is only used once, in the explicit
> ap_queue_enable_interruption() function.

My answer is not clear: ap_aqic() is used only once, during the bus 
probe, in the all code to enable interrupt and is never used to disable 
interrupt.

Interrupt disabling is only done by using ap_zapq() or ap_rapq() which 
can not be intercepted.


> 
> Yes, thanks, I will keep it as is, may be just play with msleep()time 
> and retry count.
> 
> Regards,
> Pierre
> 
>>
>> Regards,
>> Halil
>>
>>> +        else
>>> +            WARN_ONCE("%s: failed disabling IRQ", __func__);
>>> +    }
>>> +
>>> +    return status;
>>> +}
>>
> 
> 


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