[PATCH v3 0/9] [RFC] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[PATCH v3 0/9] [RFC] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[Pierre Morel]

This patch implement PQAP/AQIC interception in KVM.

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 : to unpin the NIB on clear IRQ
 - ISC : to unregister with the GIB interface
 - MATRIX: a pointer to the matrix mediated device
 - DEV: a pointer to the AP queue device from AP bus.


1) Phase 1, probe and remove from vfio_ap_queue

The vfio_ap_queue structures are dynamically allocated when a queue
is probed by the ap_vfio_driver.
It is linked to the ap_queue device as the driver data.
We store the APQN of the ap_queue device inside the vfio_ap_queue.
The vfio_ap_queue are free during remove.


2) Phase 2, assignment of vfio_ap_queue to a mediated device

When the VFIO device is opened the queues are associated to
the mediated device, at the same time the CRYCB is setup.

When the device is closed or if a queue is removed the queue is
dissociated from the mediated device, as the same time the
CRYCB is cleared.

Two new functions are used for this: associate/disassociate.


3) Phase 3 intercepting the PQAP/AQIC instruction

On interception of the PQAP/AQIC instruction, the interception code
makes sure the pqap_hook is initialized and allowed to be called
and call it.
Otherwise it reports the usual -EOPNOTSUPP return code to let
QEMU handle the fault.
  
the pqap callback search for the queue asociated with the APQN
stored in the register 0, setting the code to "illegal APQN"
if the vfio_ap_queue can not be found.

Depending on the "i" bit of the register 1, the pqap callback
setup or clear the interruption by calling the host format PQAP/AQIC
instruction.
When seting up the interruption it uses the NIB and the guest ISC
provided by the guest and the host ISC provided by the registration
to the GIB code, pin the NIB and also stores ISC and NIB inside
the vfio_ap_queue structure.
When clearing the interrupt it retrieves the host ISC to unregister
with the GIB code and unpin the NIB.

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>


4) Phase 4 clean dissociation from the mediated device on remove

On removing of the AP device the remove callback is called.
To be sure that the guest will not access the queue anymore
we clear the APID CRYCB bit.
Cleaning the APID, over the APQI, is chosen because the architecture
specifies that only the APID can be dynamically changed outside IPL.
After this the queue is zeroes, care is taken to correctly wait until
the queue is empty.



Pierre Morel (9):
  s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem
  s390: ap: kvm: setting a hook for PQAP instructions
  s390: ap: new vfio_ap_queue structure
  s390: ap: tools to find a queue with a specific APQN
  s390: ap: tools to associate a queue to a matrix
  vfio: ap: register IOMMU VFIO notifier
  s390: ap: implement PAPQ AQIC interception in kernel
  s390: ap: Cleanup on removing the AP device
  s390: ap: kvm: add AP Queue Interruption Control facility

 arch/s390/include/asm/kvm_host.h      |   1 +
 arch/s390/kvm/priv.c                  |  50 +++++
 arch/s390/tools/gen_facilities.c      |   1 +
 drivers/s390/crypto/ap_bus.h          |   1 +
 drivers/s390/crypto/vfio_ap_drv.c     | 178 ++++++++++++++++-
 drivers/s390/crypto/vfio_ap_ops.c     | 363 +++++++++++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |  12 ++
 7 files changed, 593 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

Libudev relies on having a subsystem link for non-root devices. To
avoid libudev (and potentially other userspace tools) choking on the
matrix device let us introduce a vfio_ap bus and with that the vfio_ap
bus subsytem, and make the matrix device reside within it.

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to
the vfio_ap driver and we suppress the device_type we do not need
anymore.

Since the associated matrix driver is not the vfio_ap driver any more,
we have to change the search for the devices on the vfio_ap driver in
the function vfio_ap_verify_queue_reserved.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 3 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 31c6c84..1fd5fe6 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
 static struct ap_driver vfio_ap_drv;
 
-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;
 };
 
 struct ap_matrix_dev *matrix_dev;
@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
 	kfree(matrix_dev);
 }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
 static int vfio_ap_matrix_dev_create(void)
 {
 	int ret;
@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
 	if (IS_ERR(root_device))
 		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
 	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
 	if (!matrix_dev) {
 		ret = -ENOMEM;
@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
 	mutex_init(&matrix_dev->lock);
 	INIT_LIST_HEAD(&matrix_dev->mdev_list);
 
-	matrix_dev->device.type = &vfio_ap_dev_type;
 	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
 	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
 	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
 
 	ret = device_register(&matrix_dev->device);
 	if (ret)
 		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+
 	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
 matrix_reg_err:
 	put_device(&matrix_dev->device);
 matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
 	root_device_unregister(root_device);
-
 	return ret;
 }
 
 static void vfio_ap_matrix_dev_destroy(void)
 {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
 	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
 }
 
 static int __init vfio_ap_init(void)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 272ef42..900b9cf 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
 	qres.apqi = apqi;
 	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
 	if (ret)
 		return ret;
 
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5675492..76b7f98 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -40,6 +40,7 @@ struct ap_matrix_dev {
 	struct ap_config_info info;
 	struct list_head mdev_list;
 	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
 };
 
 extern struct ap_matrix_dev *matrix_dev;
-- 
2.7.4

[PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

This patch adds interception code for the PQAP instructions,
and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ
instructions, the driver will initialize the callback inside
the kvm_arch structure to be called when the interception of a
PQAP instruction occurs.

If the callback is not initialized, the code still returns
-EOPNOTSUPP to let userland handle the instruction as it used to.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/priv.c             | 50 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index c5f5156..49cc8b0 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
 
 struct kvm_s390_crypto {
 	struct kvm_s390_crypto_cb *crycb;
+	int (*pqap_hook)(struct kvm_vcpu *vcpu);
 	__u32 crycbd;
 	__u8 aes_kw;
 	__u8 dea_kw;
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index 8679bd7..72fdc21 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,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
 	}
 }
 
+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	uint8_t fc;
+
+	/* Verify that the hook callback is registered */
+	if (!vcpu->kvm->arch.crypto.pqap_hook)
+		return -EOPNOTSUPP;
+	/* 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;
+	/* Verify that the function code is AQIC */
+	fc = vcpu->run->s.regs.gprs[0] >> 24;
+	if (fc != 0x03)
+		return -EOPNOTSUPP;
+
+	/* PQAP instructions are 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);
+	/* AQIC instruction is allowed only if facility 65 is available */
+	if (!test_kvm_facility(vcpu->kvm, 65))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* All right, call the callback */
+	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
+}
+
 static int handle_stfl(struct kvm_vcpu *vcpu)
 {
 	int rc;
@@ -878,6 +926,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:
-- 
2.7.4

[PATCH v3 3/9] s390: ap: new vfio_ap_queue structure

[Pierre Morel]

The AP interruptions are assigned on a queue basis
and we need some information neloging to the queue
to handle interuptions.

We add a new structure vfio_ap_queue, to hold per
queue information useful to handle interruptions.

- apqn: AP queue number (defined here)
- isc : Interrupt subclass (defined later)
- nib : notification information byte (defined later)

The vfio_ap_queue structure is allocated when the vfio_ap_driver
is probed and added as driver data to the ap_queue device.
It is free on remove.

The structure is linked to the matrix_dev host device.

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

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 1fd5fe6..03153e6 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -46,14 +46,35 @@ 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;
 	return 0;
 }
 
+/**
+ * vfio_ap_queue_dev_remove:
+ *
+ * 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;
+
+	q = dev_get_drvdata(&apdev->device);
+	kfree(q);
 }
 
 static void vfio_ap_matrix_dev_release(struct device *dev)
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 76b7f98..8836f01 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -86,4 +86,7 @@ struct ap_matrix_mdev {
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
 
+struct vfio_ap_queue {
+	int	apqn;
+};
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Pierre Morel]

We need to find the queue with a specific APQN during the
handling of the interception of the PQAP/AQIC instruction.

To handle the AP associated device reference count we keep
track of it in the vfio_ap_queue until we put the device.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 2 files changed, 55 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 900b9cf..2a52c9b 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -24,6 +24,60 @@
 #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
 #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
+/**
+ * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
+ *
+ * Returns 1 if we have a match.
+ * Otherwise returns 0.
+ */
+static int vfio_ap_check_apqn(struct device *dev, void *data)
+{
+	struct vfio_ap_queue *q = dev_get_drvdata(dev);
+
+	return (q->apqn == *(int *)data);
+}
+
+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the
+ * devices associated to the vfio_ap_driver.
+ *
+ * The vfio_ap_queue has been already associated with the device
+ * during the probe.
+ * Store the associated device for reference counting
+ *
+ * Returns the pointer to the associated vfio_ap_queue
+ */
+static  __attribute__((unused))
+	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
+{
+	struct device *dev;
+	struct vfio_ap_queue *q;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
+				 vfio_ap_check_apqn);
+	if (!dev)
+		return NULL;
+	q = dev_get_drvdata(dev);
+	q->dev = dev;
+	return q;
+}
+
+/**
+ * vfio_ap_put_queue: lower device reference count for a queue
+ * @q: The queue
+ *
+ * put the associated device
+ *
+ */
+static  __attribute__((unused)) void vfio_ap_put_queue(struct vfio_ap_queue *q)
+{
+	put_device(q->dev);
+	q->dev = NULL;
+}
+
 static void vfio_ap_matrix_init(struct ap_config_info *info,
 				struct ap_matrix *matrix)
 {
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 8836f01..081f0d7 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -87,6 +87,7 @@ extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
 
 struct vfio_ap_queue {
+	struct device *dev;
 	int	apqn;
 };
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v3 5/9] s390: ap: tools to associate a queue to a matrix

[Pierre Morel]

We need tools to associate a queue and a matrix to be able
later to find the matrix associated with the queue for which
we got the APQN in the register 1 during a PQAP/AQIC instruction
interception.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 66 +++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 2 files changed, 64 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 2a52c9b..1851b24 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -50,8 +50,7 @@ static int vfio_ap_check_apqn(struct device *dev, void *data)
  *
  * Returns the pointer to the associated vfio_ap_queue
  */
-static  __attribute__((unused))
-	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
+static struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
 {
 	struct device *dev;
 	struct vfio_ap_queue *q;
@@ -72,7 +71,7 @@ static  __attribute__((unused))
  * put the associated device
  *
  */
-static  __attribute__((unused)) void vfio_ap_put_queue(struct vfio_ap_queue *q)
+static void vfio_ap_put_queue(struct vfio_ap_queue *q)
 {
 	put_device(q->dev);
 	q->dev = NULL;
@@ -867,6 +866,67 @@ static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
 	return -EBUSY;
 }
 
+/**
+ * vfio_ap_dissociate_queues: Dissociate a matrix mediated device to a queue
+ *
+ * Go through all bits in the AQM and APM and dissociate the queue
+ * from the matrix device.
+ *
+ * No return value we are throwing the mediated device anyway.
+ */
+static void vfio_ap_dissociate_queues(struct ap_matrix_mdev *matrix_mdev)
+{
+	unsigned long apid, apqi;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1) {
+			q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+			if (q) {
+				vfio_ap_mdev_reset_queue(apid, apqi, 1);
+				q->matrix = NULL;
+				vfio_ap_put_queue(q);
+			} else
+				pr_err("%s: no queue for %02lx.%04lx\n",
+				       __func__, apid, apqi);
+		}
+	}
+}
+
+/**
+ * vfio_ap_associate_queues: Associate a matrix mediated device to a queue
+ *
+ * Go through all bits in the AQM and APM and calculate the APQN, and find
+ * the matching queue to associate with the matrix device.
+ *
+ * In the case a queue could not be found return -ENODEV.
+ * Otherwise return 0.
+ */
+static __attribute__((unused))
+	int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
+{
+	unsigned long apid, apqi;
+	struct vfio_ap_queue *q;
+
+	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
+			     matrix_mdev->matrix.apm_max + 1) {
+		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
+				     matrix_mdev->matrix.aqm_max + 1) {
+			q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
+			if (!q)
+				goto error;
+			q->matrix = matrix_mdev;
+			vfio_ap_put_queue(q);
+		}
+	}
+	return 0;
+error:
+	pr_err("%s: no queue for %02lx.%04lx\n", __func__, apid, apqi);
+	vfio_ap_dissociate_queues(matrix_mdev);
+	return -ENODEV;
+}
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
 {
 	int ret;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 081f0d7..10bc8b5 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -89,5 +89,6 @@ extern void vfio_ap_mdev_unregister(void);
 struct vfio_ap_queue {
 	struct device *dev;
 	int	apqn;
+	struct ap_matrix_mdev *matrix;
 };
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Pierre Morel]

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

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 64 +++++++++++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_private.h |  2 ++
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 1851b24..6eddc2c 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -781,6 +781,36 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
 };
 
 /**
+ * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
+ *
+ * @nb: The notifier block
+ * @action: Action to be taken (VFIO_IOMMU_NOTIFY_DMA_UNMAP)
+ * @data: the specific unmap structure for vfio_iommu_type1
+ *
+ * Unpins the guest IOVA. (The NIB guest address we pinned before).
+ * Return NOTIFY_OK after unpining on a UNMAP request.
+ * otherwise, returns NOTIFY_DONE .
+ */
+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;
+}
+
+
+/**
  * vfio_ap_mdev_set_kvm
  *
  * @matrix_mdev: a mediated matrix device
@@ -904,8 +934,7 @@ static void vfio_ap_dissociate_queues(struct ap_matrix_mdev *matrix_mdev)
  * In the case a queue could not be found return -ENODEV.
  * Otherwise return 0.
  */
-static __attribute__((unused))
-	int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
+static int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
 {
 	unsigned long apid, apqi;
 	struct vfio_ap_queue *q;
@@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 
 	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				     &events, &matrix_mdev->group_notifier);
-	if (ret) {
-		module_put(THIS_MODULE);
-		return ret;
-	}
+	if (ret)
+		goto err_group;
+
+	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)
+		goto err_iommu;
+
+	ret = vfio_ap_associate_queues(matrix_mdev);
+	if (ret)
+		goto err_associate;
 
 	return 0;
+
+err_associate:
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				 &matrix_mdev->iommu_notifier);
+err_iommu:
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
+				 &matrix_mdev->group_notifier);
+err_group:
+	module_put(THIS_MODULE);
+	return ret;
 }
 
 static void vfio_ap_mdev_release(struct mdev_device *mdev)
@@ -985,6 +1034,9 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 	vfio_ap_mdev_reset_queues(mdev);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				 &matrix_mdev->group_notifier);
+	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
+				 &matrix_mdev->iommu_notifier);
+	vfio_ap_dissociate_queues(matrix_mdev);
 	matrix_mdev->kvm = NULL;
 	module_put(THIS_MODULE);
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 10bc8b5..2781720 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -80,7 +80,9 @@ 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 mdev_device *mdev;
 };
 
 extern int vfio_ap_mdev_register(void);
-- 
2.7.4

[PATCH v3 7/9] 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_ops.c     | 191 +++++++++++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |   2 +
 3 files changed, 191 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
index bfc66e4..323f2aa 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_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6eddc2c..5664cf3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -77,6 +77,28 @@ static void vfio_ap_put_queue(struct vfio_ap_queue *q)
 	q->dev = NULL;
 }
 
+/**
+ * vfio_ap_free_irq:
+ * @q: The vfio_ap_queue
+ *
+ * Unpin the guest NIB
+ * Unregister the ISC from the GIB alert
+ * Clear the vfio_ap_queue intern fields
+ */
+static void vfio_ap_free_irq(struct vfio_ap_queue *q)
+{
+	unsigned long pfn = q->nib >> PAGE_SHIFT;
+
+	if (!q)
+		return;
+	if (q->nib)
+		vfio_unpin_pages(mdev_dev(q->matrix->mdev), &pfn, 1);
+	if (q->isc)
+		kvm_s390_gisc_unregister(q->matrix->kvm, q->isc);
+	q->nib = 0;
+	q->isc = 0;
+}
+
 static void vfio_ap_matrix_init(struct ap_config_info *info,
 				struct ap_matrix *matrix)
 {
@@ -98,6 +120,7 @@ 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);
 	mutex_lock(&matrix_dev->lock);
@@ -781,6 +804,156 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
 };
 
 /**
+ * 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;
+
+	status = ap_aqic(q->apqn, aqic_gisa, NULL);
+	if (!status.response_code)
+		vfio_ap_free_irq(q);
+
+	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 g_pfn, h_nib, h_pfn;
+	int ret;
+
+	kvm = q->matrix->kvm;
+	gisa = kvm->arch.gisa_int.origin;
+
+	g_pfn = q->nib >> PAGE_SHIFT;
+	ret = vfio_pin_pages(mdev_dev(q->matrix->mdev), &g_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;
+	}
+
+	h_nib = (h_pfn << PAGE_SHIFT) | (q->nib & ~PAGE_MASK);
+	aqic_gisa.gisc = q->isc;
+	aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->isc);
+	aqic_gisa.ir = 1;
+	aqic_gisa.gisa = gisa->next_alert >> 4;
+
+	status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
+	if (status.response_code == AP_RESPONSE_INVALID_GISA) {
+		status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
+		pr_warn("vfio_ap: apqn %02x.%04x: AP_RESPONSE_INVALID_GISA\n",
+			(q->apqn >> 8) & 0xff, q->apqn & 0xff);
+	}
+
+	if (status.response_code)
+		vfio_ap_free_irq(q);
+
+	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.
+ *
+ * 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 = {};
+	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;
+	q = vfio_ap_get_queue(apqn);
+	if (!q) {
+		qstatus.response_code = AP_RESPONSE_Q_NOT_AVAIL;
+		goto out;
+	}
+
+	/* Check if the queue is associated with a guest matrix */
+	matrix_mdev = q->matrix;
+	if (!matrix_mdev || !matrix_mdev->kvm) {
+		qstatus.response_code = AP_RESPONSE_DECONFIGURED;
+		goto out;
+	}
+
+	status = vcpu->run->s.regs.gprs[1];
+
+	/* If IR bit(16) is set we enable the interrupt */
+	if ((status >> (63 - 16)) & 0x01) {
+		q->isc = status & 0x07;
+		q->nib = vcpu->run->s.regs.gprs[2];
+		qstatus = vfio_ap_setirq(q);
+		if (qstatus.response_code) {
+			q->nib = 0;
+			q->isc = 0;
+		}
+	} else
+		qstatus = vfio_ap_clrirq(q);
+
+out:
+	memcpy(&vcpu->run->s.regs.gprs[1], &qstatus, sizeof(qstatus));
+	vfio_ap_put_queue(q);
+	return 0;
+}
+
+ /*
  * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
  *
  * @nb: The notifier block
@@ -802,7 +975,9 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
 		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);
+		if (matrix_mdev->mdev)
+			vfio_unpin_pages(mdev_dev(matrix_mdev->mdev),
+					 &g_pfn, 1);
 		return NOTIFY_OK;
 	}
 
@@ -877,11 +1052,15 @@ static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
 				    unsigned int retry)
 {
 	struct ap_queue_status status;
+	struct vfio_ap_queue *q;
+
+	q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
 
 	do {
 		status = ap_zapq(AP_MKQID(apid, apqi));
 		switch (status.response_code) {
 		case AP_RESPONSE_NORMAL:
+			vfio_ap_free_irq(q);
 			return 0;
 		case AP_RESPONSE_RESET_IN_PROGRESS:
 		case AP_RESPONSE_BUSY:
@@ -999,6 +1178,11 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 	if (ret)
 		goto err_group;
 
+	if (!matrix_mdev->kvm) {
+		ret = -ENODEV;
+		goto err_iommu;
+	}
+
 	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
 	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
 
@@ -1011,6 +1195,7 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
 	if (ret)
 		goto err_associate;
 
+	matrix_mdev->kvm->arch.crypto.pqap_hook = handle_pqap;
 	return 0;
 
 err_associate:
@@ -1031,12 +1216,12 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
 	if (matrix_mdev->kvm)
 		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
 
-	vfio_ap_mdev_reset_queues(mdev);
+	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
+	vfio_ap_dissociate_queues(matrix_mdev);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
 				 &matrix_mdev->group_notifier);
 	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
 				 &matrix_mdev->iommu_notifier);
-	vfio_ap_dissociate_queues(matrix_mdev);
 	matrix_mdev->kvm = NULL;
 	module_put(THIS_MODULE);
 }
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 2781720..16c99e9 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -92,5 +92,7 @@ struct vfio_ap_queue {
 	struct device *dev;
 	int	apqn;
 	struct ap_matrix_mdev *matrix;
+	unsigned long nib;
+	unsigned char isc;
 };
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v3 8/9] s390: ap: Cleanup on removing the AP device

[Pierre Morel]

When the device is remove, we must make sure to
clear the interruption and reset the AP device.

We also need to clear the CRYCB of the guest.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 92 +++++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_ops.c     |  2 +-
 drivers/s390/crypto/vfio_ap_private.h |  2 +
 3 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 03153e6..50428be 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>
@@ -12,6 +13,8 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <asm/facility.h>
+#include <linux/bitops.h>
+#include <linux/kvm_host.h>
 #include "vfio_ap_private.h"
 
 #define VFIO_AP_ROOT_NAME "vfio_ap"
@@ -64,6 +67,88 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 	return 0;
 }
 
+/*
+ * vfio_ap_drain_queue
+ * @q: the queue to drain
+ *
+ * This function waits until the queue is empty.
+ */
+static void vfio_ap_drain_queue(struct vfio_ap_queue *q)
+{
+	struct ap_queue_status status;
+	int retry = 20;
+
+	status = ap_tapq(q->apqn, NULL);
+	while (!status.queue_empty && retry--)  {
+		msleep(200);
+		status = ap_tapq(q->apqn, NULL);
+	}
+	if (retry <= 0) {
+		pr_warn("%s: queue not empty after zapq on apqn 0x%04x\n",
+			__func__, q->apqn);
+	}
+}
+
+/*
+ * vfio_ap_zapq
+ * @q: The queue to zerro
+ *
+ * It is best effort, no return value is done, however on success
+ * we will drain the queue before getting the queue back to the
+ * AP bus.
+ */
+static void vfio_ap_zapq(struct vfio_ap_queue *q)
+{
+	struct ap_queue_status status;
+	int retry = 20;
+
+	do {
+		status = ap_zapq(q->apqn);
+		switch (status.response_code) {
+		case AP_RESPONSE_RESET_IN_PROGRESS:
+		case AP_RESPONSE_BUSY:
+			msleep(20);
+			break;
+		default:
+			pr_warn("%s: zapq error %02x on apqn 0x%04x\n",
+				__func__, status.response_code, q->apqn);
+			return;
+		case AP_RESPONSE_NORMAL:
+			vfio_ap_drain_queue(q);
+			return;
+		}
+	} while (retry--);
+	pr_warn("%s: zapq retry count exceeded code %02x on apqn 0x%04x\n",
+		__func__, status.response_code, q->apqn);
+}
+
+/**
+ * vfio_ap_update_crycb
+ * @q: A pointer to the queue being removed
+ *
+ * We clear the APID of the queue, making this queue unusable for the guest.
+ * After this function we can reset the queue without to fear a race with
+ * the guest to access the queue again.
+ * We do not fear race with the host as we still get the device.
+ */
+static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
+{
+	struct ap_matrix_mdev *matrix_mdev = q->matrix;
+
+	if (!matrix_mdev)
+		return;
+
+	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
+
+	if (!matrix_mdev->kvm)
+		return;
+
+	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
+				  matrix_mdev->matrix.apm,
+				  matrix_mdev->matrix.aqm,
+				  matrix_mdev->matrix.adm);
+}
+
 /**
  * vfio_ap_queue_dev_remove:
  *
@@ -74,6 +159,13 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 	struct vfio_ap_queue *q;
 
 	q = dev_get_drvdata(&apdev->device);
+	if (!q)
+		return;
+
+	vfio_ap_update_crycb(q);
+	vfio_ap_zapq(q);
+
+	vfio_ap_free_irq(q);
 	kfree(q);
 }
 
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 5664cf3..7ec957c 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -85,7 +85,7 @@ static void vfio_ap_put_queue(struct vfio_ap_queue *q)
  * Unregister the ISC from the GIB alert
  * Clear the vfio_ap_queue intern fields
  */
-static void vfio_ap_free_irq(struct vfio_ap_queue *q)
+void vfio_ap_free_irq(struct vfio_ap_queue *q)
 {
 	unsigned long pfn = q->nib >> PAGE_SHIFT;
 
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 16c99e9..d108c6d 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
  */
@@ -95,4 +96,5 @@ struct vfio_ap_queue {
 	unsigned long nib;
 	unsigned char isc;
 };
+void vfio_ap_free_irq(struct vfio_ap_queue *q);
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.7.4

[PATCH v3 9/9] s390: ap: kvm: add AP Queue Interruption Control facility

[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 fd788e0..18d317d 100644
--- a/arch/s390/tools/gen_facilities.c
+++ b/arch/s390/tools/gen_facilities.c
@@ -108,6 +108,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 v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> Libudev relies on having a subsystem link for non-root devices. To
> avoid libudev (and potentially other userspace tools) choking on the
> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
> bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that
basically does nothing...

> 
> We restrict the number of allowed devices to a single one.
> 
> Doing this we need to suppress the forced link from the matrix device to
> the vfio_ap driver and we suppress the device_type we do not need
> anymore.
> 
> Since the associated matrix driver is not the vfio_ap driver any more,
> we have to change the search for the devices on the vfio_ap driver in
> the function vfio_ap_verify_queue_reserved.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  3 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 31c6c84..1fd5fe6 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>  
>  static struct ap_driver vfio_ap_drv;
>  
> -static struct device_type vfio_ap_dev_type = {
> -	.name = VFIO_AP_DEV_TYPE_NAME,
> +struct matrix_driver {
> +	struct device_driver drv;
> +	int device_count;

This counter basically ensures that at most one device may bind with
this driver... you'd still have that device on the bus, though.

>  };
>  
>  struct ap_matrix_dev *matrix_dev;
> @@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
>  	kfree(matrix_dev);
>  }
>  
> +static int matrix_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	return 1;
> +}
> +
> +static struct bus_type matrix_bus = {
> +	.name = "vfio_ap",
> +	.match = &matrix_bus_match,
> +};
> +
> +static int matrix_probe(struct device *dev);
> +static int matrix_remove(struct device *dev);
> +static struct matrix_driver matrix_driver = {
> +	.drv = {
> +		.name = "vfio_ap",
> +		.bus = &matrix_bus,
> +		.probe = matrix_probe,
> +		.remove = matrix_remove,
> +	},
> +};
> +
> +static int matrix_probe(struct device *dev)
> +{
> +	if (matrix_driver.device_count)
> +		return -EEXIST;
> +	matrix_driver.device_count++;
> +	return 0;
> +}
> +
> +static int matrix_remove(struct device *dev)
> +{
> +	matrix_driver.device_count--;
> +	return 0;
> +}
> +
>  static int vfio_ap_matrix_dev_create(void)
>  {
>  	int ret;
> @@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
>  	if (IS_ERR(root_device))
>  		return PTR_ERR(root_device);
>  
> +	ret = bus_register(&matrix_bus);
> +	if (ret)
> +		goto bus_register_err;
> +
>  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
>  	if (!matrix_dev) {
>  		ret = -ENOMEM;
> @@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
>  	mutex_init(&matrix_dev->lock);
>  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
>  
> -	matrix_dev->device.type = &vfio_ap_dev_type;
>  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>  	matrix_dev->device.parent = root_device;
> +	matrix_dev->device.bus = &matrix_bus;
>  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead
when you need it in the function below?

>  
>  	ret = device_register(&matrix_dev->device);
>  	if (ret)
>  		goto matrix_reg_err;
>  
> +	ret = driver_register(&matrix_driver.drv);
> +	if (ret)
> +		goto matrix_drv_err;
> +

As you already have several structures that can be registered exactly
once (the root device, the bus, the driver, ...), you can already be
sure that there's only one device on the bus, can't you?

>  	return 0;
>  
> +matrix_drv_err:
> +	device_unregister(&matrix_dev->device);
>  matrix_reg_err:
>  	put_device(&matrix_dev->device);
>  matrix_alloc_err:
> +	bus_unregister(&matrix_bus);
> +bus_register_err:
>  	root_device_unregister(root_device);
> -
>  	return ret;
>  }
>  
>  static void vfio_ap_matrix_dev_destroy(void)
>  {
> +	struct device *root_device = matrix_dev->device.parent;
> +
> +	driver_unregister(&matrix_driver.drv);
>  	device_unregister(&matrix_dev->device);
> -	root_device_unregister(matrix_dev->device.parent);
> +	bus_unregister(&matrix_bus);
> +	root_device_unregister(root_device);
>  }
>  
>  static int __init vfio_ap_init(void)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 272ef42..900b9cf 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>  	qres.apqi = apqi;
>  	qres.reserved = false;
>  
> -	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
> -				     vfio_ap_has_queue);
> +	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> +				     &qres, vfio_ap_has_queue);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 5675492..76b7f98 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>  	struct ap_config_info info;
>  	struct list_head mdev_list;
>  	struct mutex lock;
> +	struct ap_driver  *vfio_ap_drv;
>  };
>  
>  extern struct ap_matrix_dev *matrix_dev;

This feels like a lot of boilerplate code, just to create a bus that
basically doesn't do anything. I'm surprised that libudev can't deal
with bus-less devices properly...

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Christian Borntraeger]

On 14.02.2019 14:51, Pierre Morel wrote:
> Libudev relies on having a subsystem link for non-root devices. To
> avoid libudev (and potentially other userspace tools) choking on the
> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
> bus subsytem, and make the matrix device reside within it.
> 
> We restrict the number of allowed devices to a single one.
> 
> Doing this we need to suppress the forced link from the matrix device to
> the vfio_ap driver and we suppress the device_type we do not need
> anymore.
> 
> Since the associated matrix driver is not the vfio_ap driver any more,
> we have to change the search for the devices on the vfio_ap driver in
> the function vfio_ap_verify_queue_reserved.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>

Didnt you want to a add a reported-by?
> ---


>  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  3 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 31c6c84..1fd5fe6 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>  
>  static struct ap_driver vfio_ap_drv;
>  
> -static struct device_type vfio_ap_dev_type = {
> -	.name = VFIO_AP_DEV_TYPE_NAME,
> +struct matrix_driver {
> +	struct device_driver drv;
> +	int device_count;
>  };
>  
>  struct ap_matrix_dev *matrix_dev;
> @@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
>  	kfree(matrix_dev);
>  }
>  
> +static int matrix_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	return 1;
> +}
> +
> +static struct bus_type matrix_bus = {
> +	.name = "vfio_ap",
> +	.match = &matrix_bus_match,
> +};
> +
> +static int matrix_probe(struct device *dev);
> +static int matrix_remove(struct device *dev);
> +static struct matrix_driver matrix_driver = {
> +	.drv = {
> +		.name = "vfio_ap",
> +		.bus = &matrix_bus,
> +		.probe = matrix_probe,
> +		.remove = matrix_remove,
> +	},
> +};
> +
> +static int matrix_probe(struct device *dev)
> +{
> +	if (matrix_driver.device_count)
> +		return -EEXIST;
> +	matrix_driver.device_count++;
> +	return 0;
> +}
> +
> +static int matrix_remove(struct device *dev)
> +{
> +	matrix_driver.device_count--;
> +	return 0;
> +}
> +
>  static int vfio_ap_matrix_dev_create(void)
>  {
>  	int ret;
> @@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
>  	if (IS_ERR(root_device))
>  		return PTR_ERR(root_device);
>  
> +	ret = bus_register(&matrix_bus);
> +	if (ret)
> +		goto bus_register_err;
> +
>  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
>  	if (!matrix_dev) {
>  		ret = -ENOMEM;
> @@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
>  	mutex_init(&matrix_dev->lock);
>  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
>  
> -	matrix_dev->device.type = &vfio_ap_dev_type;
>  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>  	matrix_dev->device.parent = root_device;
> +	matrix_dev->device.bus = &matrix_bus;
>  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>  
>  	ret = device_register(&matrix_dev->device);
>  	if (ret)
>  		goto matrix_reg_err;
>  
> +	ret = driver_register(&matrix_driver.drv);
> +	if (ret)
> +		goto matrix_drv_err;
> +
>  	return 0;
>  
> +matrix_drv_err:
> +	device_unregister(&matrix_dev->device);
>  matrix_reg_err:
>  	put_device(&matrix_dev->device);
>  matrix_alloc_err:
> +	bus_unregister(&matrix_bus);
> +bus_register_err:
>  	root_device_unregister(root_device);
> -
>  	return ret;
>  }
>  
>  static void vfio_ap_matrix_dev_destroy(void)
>  {
> +	struct device *root_device = matrix_dev->device.parent;
> +
> +	driver_unregister(&matrix_driver.drv);
>  	device_unregister(&matrix_dev->device);
> -	root_device_unregister(matrix_dev->device.parent);
> +	bus_unregister(&matrix_bus);
> +	root_device_unregister(root_device);
>  }
>  
>  static int __init vfio_ap_init(void)
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 272ef42..900b9cf 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>  	qres.apqi = apqi;
>  	qres.reserved = false;
>  
> -	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
> -				     vfio_ap_has_queue);
> +	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> +				     &qres, vfio_ap_has_queue);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 5675492..76b7f98 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>  	struct ap_config_info info;
>  	struct list_head mdev_list;
>  	struct mutex lock;
> +	struct ap_driver  *vfio_ap_drv;
>  };
>  
>  extern struct ap_matrix_dev *matrix_dev;
> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Christian Borntraeger]

On 14.02.2019 15:54, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:01 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:

Pierre,
this is independent from this series and should have been sent separately.
In the end (when we have the final solution) this will require cc stable.
> 
>> Libudev relies on having a subsystem link for non-root devices. To
>> avoid libudev (and potentially other userspace tools) choking on the
>> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
>> bus subsytem, and make the matrix device reside within it.
> 
> How does libudev choke on this? It feels wrong to introduce a bus that
> basically does nothing...

I have seen libvirt looping when a matrix device was available before the
libvirt start.
Marc Hartmayer debugged this and circumvented this in libvirt:
https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html

Still libudev expects a subsystem link in the matrix folder when doing the 
udev_enumerate_scan_devices call.

Having a bus is one way of adding a subsystem link.

> 
>>
>> We restrict the number of allowed devices to a single one.
>>
>> Doing this we need to suppress the forced link from the matrix device to
>> the vfio_ap driver and we suppress the device_type we do not need
>> anymore.
>>
>> Since the associated matrix driver is not the vfio_ap driver any more,
>> we have to change the search for the devices on the vfio_ap driver in
>> the function vfio_ap_verify_queue_reserved.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
>>  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>>  3 files changed, 60 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 31c6c84..1fd5fe6 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>>  
>>  static struct ap_driver vfio_ap_drv;
>>  
>> -static struct device_type vfio_ap_dev_type = {
>> -	.name = VFIO_AP_DEV_TYPE_NAME,
>> +struct matrix_driver {
>> +	struct device_driver drv;
>> +	int device_count;
> 
> This counter basically ensures that at most one device may bind with
> this driver... you'd still have that device on the bus, though.
> 
>>  };
>>  
>>  struct ap_matrix_dev *matrix_dev;
>> @@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
>>  	kfree(matrix_dev);
>>  }
>>  
>> +static int matrix_bus_match(struct device *dev, struct device_driver *drv)
>> +{
>> +	return 1;
>> +}
>> +
>> +static struct bus_type matrix_bus = {
>> +	.name = "vfio_ap",
>> +	.match = &matrix_bus_match,
>> +};
>> +
>> +static int matrix_probe(struct device *dev);
>> +static int matrix_remove(struct device *dev);
>> +static struct matrix_driver matrix_driver = {
>> +	.drv = {
>> +		.name = "vfio_ap",
>> +		.bus = &matrix_bus,
>> +		.probe = matrix_probe,
>> +		.remove = matrix_remove,
>> +	},
>> +};
>> +
>> +static int matrix_probe(struct device *dev)
>> +{
>> +	if (matrix_driver.device_count)
>> +		return -EEXIST;
>> +	matrix_driver.device_count++;
>> +	return 0;
>> +}
>> +
>> +static int matrix_remove(struct device *dev)
>> +{
>> +	matrix_driver.device_count--;
>> +	return 0;
>> +}
>> +
>>  static int vfio_ap_matrix_dev_create(void)
>>  {
>>  	int ret;
>> @@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
>>  	if (IS_ERR(root_device))
>>  		return PTR_ERR(root_device);
>>  
>> +	ret = bus_register(&matrix_bus);
>> +	if (ret)
>> +		goto bus_register_err;
>> +
>>  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
>>  	if (!matrix_dev) {
>>  		ret = -ENOMEM;
>> @@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
>>  	mutex_init(&matrix_dev->lock);
>>  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
>>  
>> -	matrix_dev->device.type = &vfio_ap_dev_type;
>>  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>>  	matrix_dev->device.parent = root_device;
>> +	matrix_dev->device.bus = &matrix_bus;
>>  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
>> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
>> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
> 
> Can't you get that structure through matrix_dev->device.driver instead
> when you need it in the function below?
> 
>>  
>>  	ret = device_register(&matrix_dev->device);
>>  	if (ret)
>>  		goto matrix_reg_err;
>>  
>> +	ret = driver_register(&matrix_driver.drv);
>> +	if (ret)
>> +		goto matrix_drv_err;
>> +
> 
> As you already have several structures that can be registered exactly
> once (the root device, the bus, the driver, ...), you can already be
> sure that there's only one device on the bus, can't you?
> 
>>  	return 0;
>>  
>> +matrix_drv_err:
>> +	device_unregister(&matrix_dev->device);
>>  matrix_reg_err:
>>  	put_device(&matrix_dev->device);
>>  matrix_alloc_err:
>> +	bus_unregister(&matrix_bus);
>> +bus_register_err:
>>  	root_device_unregister(root_device);
>> -
>>  	return ret;
>>  }
>>  
>>  static void vfio_ap_matrix_dev_destroy(void)
>>  {
>> +	struct device *root_device = matrix_dev->device.parent;
>> +
>> +	driver_unregister(&matrix_driver.drv);
>>  	device_unregister(&matrix_dev->device);
>> -	root_device_unregister(matrix_dev->device.parent);
>> +	bus_unregister(&matrix_bus);
>> +	root_device_unregister(root_device);
>>  }
>>  
>>  static int __init vfio_ap_init(void)
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 272ef42..900b9cf 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>>  	qres.apqi = apqi;
>>  	qres.reserved = false;
>>  
>> -	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
>> -				     vfio_ap_has_queue);
>> +	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> +				     &qres, vfio_ap_has_queue);
>>  	if (ret)
>>  		return ret;
>>  
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index 5675492..76b7f98 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>>  	struct ap_config_info info;
>>  	struct list_head mdev_list;
>>  	struct mutex lock;
>> +	struct ap_driver  *vfio_ap_drv;
>>  };
>>  
>>  extern struct ap_matrix_dev *matrix_dev;
> 
> This feels like a lot of boilerplate code, just to create a bus that
> basically doesn't do anything. I'm surprised that libudev can't deal
> with bus-less devices properly...
> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

On 14/02/2019 16:01, Christian Borntraeger wrote:
> 
> 
> On 14.02.2019 14:51, Pierre Morel wrote:
>> Libudev relies on having a subsystem link for non-root devices. To
>> avoid libudev (and potentially other userspace tools) choking on the
>> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
>> bus subsytem, and make the matrix device reside within it.
>>
>> We restrict the number of allowed devices to a single one.
>>
>> Doing this we need to suppress the forced link from the matrix device to
>> the vfio_ap driver and we suppress the device_type we do not need
>> anymore.
>>
>> Since the associated matrix driver is not the vfio_ap driver any more,
>> we have to change the search for the devices on the vfio_ap driver in
>> the function vfio_ap_verify_queue_reserved.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> 
> Didnt you want to a add a reported-by?

Yes, forgot it.

Reported-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>



>> ---
> 
> 
>>   drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
>>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>   3 files changed, 60 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>> index 31c6c84..1fd5fe6 100644
>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>>   
>>   static struct ap_driver vfio_ap_drv;
>>   
>> -static struct device_type vfio_ap_dev_type = {
>> -	.name = VFIO_AP_DEV_TYPE_NAME,
>> +struct matrix_driver {
>> +	struct device_driver drv;
>> +	int device_count;
>>   };
>>   
>>   struct ap_matrix_dev *matrix_dev;
>> @@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
>>   	kfree(matrix_dev);
>>   }
>>   
>> +static int matrix_bus_match(struct device *dev, struct device_driver *drv)
>> +{
>> +	return 1;
>> +}
>> +
>> +static struct bus_type matrix_bus = {
>> +	.name = "vfio_ap",
>> +	.match = &matrix_bus_match,
>> +};
>> +
>> +static int matrix_probe(struct device *dev);
>> +static int matrix_remove(struct device *dev);
>> +static struct matrix_driver matrix_driver = {
>> +	.drv = {
>> +		.name = "vfio_ap",
>> +		.bus = &matrix_bus,
>> +		.probe = matrix_probe,
>> +		.remove = matrix_remove,
>> +	},
>> +};
>> +
>> +static int matrix_probe(struct device *dev)
>> +{
>> +	if (matrix_driver.device_count)
>> +		return -EEXIST;
>> +	matrix_driver.device_count++;
>> +	return 0;
>> +}
>> +
>> +static int matrix_remove(struct device *dev)
>> +{
>> +	matrix_driver.device_count--;
>> +	return 0;
>> +}
>> +
>>   static int vfio_ap_matrix_dev_create(void)
>>   {
>>   	int ret;
>> @@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
>>   	if (IS_ERR(root_device))
>>   		return PTR_ERR(root_device);
>>   
>> +	ret = bus_register(&matrix_bus);
>> +	if (ret)
>> +		goto bus_register_err;
>> +
>>   	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
>>   	if (!matrix_dev) {
>>   		ret = -ENOMEM;
>> @@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
>>   	mutex_init(&matrix_dev->lock);
>>   	INIT_LIST_HEAD(&matrix_dev->mdev_list);
>>   
>> -	matrix_dev->device.type = &vfio_ap_dev_type;
>>   	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>>   	matrix_dev->device.parent = root_device;
>> +	matrix_dev->device.bus = &matrix_bus;
>>   	matrix_dev->device.release = vfio_ap_matrix_dev_release;
>> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
>> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>>   
>>   	ret = device_register(&matrix_dev->device);
>>   	if (ret)
>>   		goto matrix_reg_err;
>>   
>> +	ret = driver_register(&matrix_driver.drv);
>> +	if (ret)
>> +		goto matrix_drv_err;
>> +
>>   	return 0;
>>   
>> +matrix_drv_err:
>> +	device_unregister(&matrix_dev->device);
>>   matrix_reg_err:
>>   	put_device(&matrix_dev->device);
>>   matrix_alloc_err:
>> +	bus_unregister(&matrix_bus);
>> +bus_register_err:
>>   	root_device_unregister(root_device);
>> -
>>   	return ret;
>>   }
>>   
>>   static void vfio_ap_matrix_dev_destroy(void)
>>   {
>> +	struct device *root_device = matrix_dev->device.parent;
>> +
>> +	driver_unregister(&matrix_driver.drv);
>>   	device_unregister(&matrix_dev->device);
>> -	root_device_unregister(matrix_dev->device.parent);
>> +	bus_unregister(&matrix_bus);
>> +	root_device_unregister(root_device);
>>   }
>>   
>>   static int __init vfio_ap_init(void)
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 272ef42..900b9cf 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>>   	qres.apqi = apqi;
>>   	qres.reserved = false;
>>   
>> -	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
>> -				     vfio_ap_has_queue);
>> +	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> +				     &qres, vfio_ap_has_queue);
>>   	if (ret)
>>   		return ret;
>>   
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index 5675492..76b7f98 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>>   	struct ap_config_info info;
>>   	struct list_head mdev_list;
>>   	struct mutex lock;
>> +	struct ap_driver  *vfio_ap_drv;
>>   };
>>   
>>   extern struct ap_matrix_dev *matrix_dev;
>>


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

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Thu, 14 Feb 2019 16:05:35 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 14.02.2019 15:54, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 14:51:01 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:  
> 
> Pierre,
> this is independent from this series and should have been sent separately.
> In the end (when we have the final solution) this will require cc stable.
> >   
> >> Libudev relies on having a subsystem link for non-root devices. To
> >> avoid libudev (and potentially other userspace tools) choking on the
> >> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
> >> bus subsytem, and make the matrix device reside within it.  
> > 
> > How does libudev choke on this? It feels wrong to introduce a bus that
> > basically does nothing...  
> 
> I have seen libvirt looping when a matrix device was available before the
> libvirt start.
> Marc Hartmayer debugged this and circumvented this in libvirt:
> https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html
> 
> Still libudev expects a subsystem link in the matrix folder when doing the 
> udev_enumerate_scan_devices call.
> 
> Having a bus is one way of adding a subsystem link.

Ugh, that feels really broken. Adding a bus is probably the easiest way
to get this working, though :/

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

On 14/02/2019 15:54, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:01 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> Libudev relies on having a subsystem link for non-root devices. To
>> avoid libudev (and potentially other userspace tools) choking on the
>> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
>> bus subsytem, and make the matrix device reside within it.
> 
> How does libudev choke on this? It feels wrong to introduce a bus that
> basically does nothing...

Christian answered this in another thread, so I don't answer.

> 
>>
>> We restrict the number of allowed devices to a single one.
>>
...snip...

>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>>   
>>   static struct ap_driver vfio_ap_drv;
>>   
>> -static struct device_type vfio_ap_dev_type = {
>> -	.name = VFIO_AP_DEV_TYPE_NAME,
>> +struct matrix_driver {
>> +	struct device_driver drv;
>> +	int device_count;
> 
> This counter basically ensures that at most one device may bind with
> this driver... you'd still have that device on the bus, though.

yes, this is what is wanted: this driver can only support one device.
May be another matrix driver can support one or more other devices.

I should update comment message my be.

> 
>>   };
>>   
>>   struct ap_matrix_dev *matrix_dev;

>>   
>> -	matrix_dev->device.type = &vfio_ap_dev_type;
>>   	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>>   	matrix_dev->device.parent = root_device;
>> +	matrix_dev->device.bus = &matrix_bus;
>>   	matrix_dev->device.release = vfio_ap_matrix_dev_release;
>> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
>> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
> 
> Can't you get that structure through matrix_dev->device.driver instead
> when you need it in the function below?

Not anymore.
We have two different drivers and devices
matrix_drv <-> matrix_dev
and
vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv
what is needed here is vfio_ap_drv.

> 
>>   
>>   	ret = device_register(&matrix_dev->device);
>>   	if (ret)
>>   		goto matrix_reg_err;
>>   
>> +	ret = driver_register(&matrix_driver.drv);
>> +	if (ret)
>> +		goto matrix_drv_err;
>> +
> 
> As you already have several structures that can be registered exactly
> once (the root device, the bus, the driver, ...), you can already be
> sure that there's only one device on the bus, can't you?

hum, no I don't think so, no device can register before this module is 
loaded, but what does prevent a device to register later from another 
module?



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

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Cornelia Huck]

On Thu, 14 Feb 2019 14:51:02 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> This patch adds interception code for the PQAP instructions,
> and a callback inside the KVM arch structure for s390.
> 
> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ

s/drivers/driver/

> instructions, the driver will initialize the callback inside
> the kvm_arch structure to be called when the interception of a
> PQAP instruction occurs.
> 
> If the callback is not initialized, the code still returns
> -EOPNOTSUPP to let userland handle the instruction as it used to.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  arch/s390/include/asm/kvm_host.h |  1 +
>  arch/s390/kvm/priv.c             | 50 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c5f5156..49cc8b0 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
>  
>  struct kvm_s390_crypto {
>  	struct kvm_s390_crypto_cb *crycb;
> +	int (*pqap_hook)(struct kvm_vcpu *vcpu);
>  	__u32 crycbd;
>  	__u8 aes_kw;
>  	__u8 dea_kw;
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 8679bd7..72fdc21 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,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>  	}
>  }
>  
> +/*
> + * handle_pqap: Handling pqap interception
> + * @vcpu: the vcpu having issue the pqap instruction

s/issue/issued/

> + *
> + * This callback only handles PQAP/AQIC instruction and

Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in
the patch description?

> + * calls a dedicated callback for this instruction if
> + * a driver did register one in the CRYPTO satellite of the
> + * SIE block.
> + *
> + * Do not change the behavior if, return -EOPNOTSUPP if:
> + * - the hook is not used do not change the behavior.

The hook is not used? Or not set? (I don't think you need to repeat "do
not change the behavior".)

> + * - AP instructions are not available or not available to the guest
> + * - the instruction is not PQAP with function code indicating
> + *   AQIC do not change the previous behavior.
> + *
> + * For PQAP/AQIC instruction, verify privilege and specifications
> + *
> + * return the value returned by the callback.
> + */
> +static int handle_pqap(struct kvm_vcpu *vcpu)
> +{
> +	uint8_t fc;
> +
> +	/* Verify that the hook callback is registered */
> +	if (!vcpu->kvm->arch.crypto.pqap_hook)
> +		return -EOPNOTSUPP;
> +	/* 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;
> +	/* Verify that the function code is AQIC */
> +	fc = vcpu->run->s.regs.gprs[0] >> 24;
> +	if (fc != 0x03)
> +		return -EOPNOTSUPP;
> +
> +	/* PQAP instructions are 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);
> +	/* AQIC instruction is allowed only if facility 65 is available */
> +	if (!test_kvm_facility(vcpu->kvm, 65))
> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +	/* All right, call the callback */
> +	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);

Can that callback also return -EOPNOTSUPP to order to drop to user
space?

> +}
> +
>  static int handle_stfl(struct kvm_vcpu *vcpu)
>  {
>  	int rc;
> @@ -878,6 +926,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:

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

On 14/02/2019 16:54, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:02 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> This patch adds interception code for the PQAP instructions,
>> and a callback inside the KVM arch structure for s390.
>>
>> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ
> 
> s/drivers/driver/

thanks. OK

> 

...

>>   #include "kvm-s390.h"
>>   #include "trace.h"
>> @@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>   	}
>>   }
>>   
>> +/*
>> + * handle_pqap: Handling pqap interception
>> + * @vcpu: the vcpu having issue the pqap instruction
> 
> s/issue/issued/

OK. thanks.

> 
>> + *
>> + * This callback only handles PQAP/AQIC instruction and
> 
> Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in
> the patch description?

I can add "for now" or "in this patch" or suppress the reference to 
PAPQ/TAPQ

> 
>> + * calls a dedicated callback for this instruction if
>> + * a driver did register one in the CRYPTO satellite of the
>> + * SIE block.
>> + *
>> + * Do not change the behavior if, return -EOPNOTSUPP if:
>> + * - the hook is not used do not change the behavior.
> 
> The hook is not used? Or not set?

I think "is not set" is better.

> (I don't think you need to repeat "do
> not change the behavior".)

OK

> 
>> + * - AP instructions are not available or not available to the guest
>> + * - the instruction is not PQAP with function code indicating
>> + *   AQIC do not change the previous behavior.
>> + *
>> + * For PQAP/AQIC instruction, verify privilege and specifications
>> + *
>> + * return the value returned by the callback.
>> + */
>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>> +{
>> +	uint8_t fc;
>> +
>> +	/* Verify that the hook callback is registered */
>> +	if (!vcpu->kvm->arch.crypto.pqap_hook)
>> +		return -EOPNOTSUPP;
>> +	/* 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;
>> +	/* Verify that the function code is AQIC */
>> +	fc = vcpu->run->s.regs.gprs[0] >> 24;
>> +	if (fc != 0x03)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* PQAP instructions are 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);
>> +	/* AQIC instruction is allowed only if facility 65 is available */
>> +	if (!test_kvm_facility(vcpu->kvm, 65))
>> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> +	/* All right, call the callback */
>> +	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
> 
> Can that callback also return -EOPNOTSUPP to order to drop to user
> space?

Yes.
Why not?

> 
>> +}
>> +
>>   static int handle_stfl(struct kvm_vcpu *vcpu)
>>   {
>>   	int rc;
>> @@ -878,6 +926,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:
> 


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

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Thu, 14 Feb 2019 16:47:30 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 14/02/2019 15:54, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 14:51:01 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:

> >> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> >> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
> >>   
> >>   static struct ap_driver vfio_ap_drv;
> >>   
> >> -static struct device_type vfio_ap_dev_type = {
> >> -	.name = VFIO_AP_DEV_TYPE_NAME,
> >> +struct matrix_driver {
> >> +	struct device_driver drv;
> >> +	int device_count;  
> > 
> > This counter basically ensures that at most one device may bind with
> > this driver... you'd still have that device on the bus, though.  
> 
> yes, this is what is wanted: this driver can only support one device.
> May be another matrix driver can support one or more other devices.
> 
> I should update comment message my be.
> 
> >   
> >>   };
> >>   
> >>   struct ap_matrix_dev *matrix_dev;  
> 
> >>   
> >> -	matrix_dev->device.type = &vfio_ap_dev_type;
> >>   	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
> >>   	matrix_dev->device.parent = root_device;
> >> +	matrix_dev->device.bus = &matrix_bus;
> >>   	matrix_dev->device.release = vfio_ap_matrix_dev_release;
> >> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
> >> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;  
> > 
> > Can't you get that structure through matrix_dev->device.driver instead
> > when you need it in the function below?  
> 
> Not anymore.
> We have two different drivers and devices
> matrix_drv <-> matrix_dev
> and
> vfio_ap_drv <-> ap_devices
> 
> The driver behind the matrix_dev->dev->driver is matrix_drv
> what is needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device, which
is not on the ap bus? Maybe that's what trips libudev?

(And reading further in the current code, it seems we clear that
structure _after_ the matrix device had been setup, so how can that
even work? Where am I confused?)

> 
> >   
> >>   
> >>   	ret = device_register(&matrix_dev->device);
> >>   	if (ret)
> >>   		goto matrix_reg_err;
> >>   
> >> +	ret = driver_register(&matrix_driver.drv);
> >> +	if (ret)
> >> +		goto matrix_drv_err;
> >> +  
> > 
> > As you already have several structures that can be registered exactly
> > once (the root device, the bus, the driver, ...), you can already be
> > sure that there's only one device on the bus, can't you?  
> 
> hum, no I don't think so, no device can register before this module is 
> loaded, but what does prevent a device to register later from another 
> module?

Not unless you export the interface, I guess.

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Tony Krowiak]

On 2/14/19 10:05 AM, Christian Borntraeger wrote:
> On 14.02.2019 15:54, Cornelia Huck wrote:
>> On Thu, 14 Feb 2019 14:51:01 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> Pierre,
> this is independent from this series and should have been sent separately.
> In the end (when we have the final solution) this will require cc stable.

I agree wholeheartedly with this statement. It has nothing to do with
interrupt processing.

>>
>>> Libudev relies on having a subsystem link for non-root devices. To
>>> avoid libudev (and potentially other userspace tools) choking on the
>>> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
>>> bus subsytem, and make the matrix device reside within it.
>>
>> How does libudev choke on this? It feels wrong to introduce a bus that
>> basically does nothing...
> 
> I have seen libvirt looping when a matrix device was available before the
> libvirt start.
> Marc Hartmayer debugged this and circumvented this in libvirt:
> https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html
> 
> Still libudev expects a subsystem link in the matrix folder when doing the
> udev_enumerate_scan_devices call.
> 
> Having a bus is one way of adding a subsystem link.
> 
>>
>>>
>>> We restrict the number of allowed devices to a single one.
>>>
>>> Doing this we need to suppress the forced link from the matrix device to
>>> the vfio_ap driver and we suppress the device_type we do not need
>>> anymore.
>>>
>>> Since the associated matrix driver is not the vfio_ap driver any more,
>>> we have to change the search for the devices on the vfio_ap driver in
>>> the function vfio_ap_verify_queue_reserved.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
>>>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>>   3 files changed, 60 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 31c6c84..1fd5fe6 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>>>   
>>>   static struct ap_driver vfio_ap_drv;
>>>   
>>> -static struct device_type vfio_ap_dev_type = {
>>> -	.name = VFIO_AP_DEV_TYPE_NAME,
>>> +struct matrix_driver {
>>> +	struct device_driver drv;
>>> +	int device_count;
>>
>> This counter basically ensures that at most one device may bind with
>> this driver... you'd still have that device on the bus, though.
>>
>>>   };
>>>   
>>>   struct ap_matrix_dev *matrix_dev;
>>> @@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
>>>   	kfree(matrix_dev);
>>>   }
>>>   
>>> +static int matrix_bus_match(struct device *dev, struct device_driver *drv)
>>> +{
>>> +	return 1;
>>> +}
>>> +
>>> +static struct bus_type matrix_bus = {
>>> +	.name = "vfio_ap",
>>> +	.match = &matrix_bus_match,
>>> +};
>>> +
>>> +static int matrix_probe(struct device *dev);
>>> +static int matrix_remove(struct device *dev);
>>> +static struct matrix_driver matrix_driver = {
>>> +	.drv = {
>>> +		.name = "vfio_ap",
>>> +		.bus = &matrix_bus,
>>> +		.probe = matrix_probe,
>>> +		.remove = matrix_remove,
>>> +	},
>>> +};
>>> +
>>> +static int matrix_probe(struct device *dev)
>>> +{
>>> +	if (matrix_driver.device_count)
>>> +		return -EEXIST;
>>> +	matrix_driver.device_count++;
>>> +	return 0;
>>> +}
>>> +
>>> +static int matrix_remove(struct device *dev)
>>> +{
>>> +	matrix_driver.device_count--;
>>> +	return 0;
>>> +}
>>> +
>>>   static int vfio_ap_matrix_dev_create(void)
>>>   {
>>>   	int ret;
>>> @@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
>>>   	if (IS_ERR(root_device))
>>>   		return PTR_ERR(root_device);
>>>   
>>> +	ret = bus_register(&matrix_bus);
>>> +	if (ret)
>>> +		goto bus_register_err;
>>> +
>>>   	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
>>>   	if (!matrix_dev) {
>>>   		ret = -ENOMEM;
>>> @@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
>>>   	mutex_init(&matrix_dev->lock);
>>>   	INIT_LIST_HEAD(&matrix_dev->mdev_list);
>>>   
>>> -	matrix_dev->device.type = &vfio_ap_dev_type;
>>>   	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>>>   	matrix_dev->device.parent = root_device;
>>> +	matrix_dev->device.bus = &matrix_bus;
>>>   	matrix_dev->device.release = vfio_ap_matrix_dev_release;
>>> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
>>> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>>
>> Can't you get that structure through matrix_dev->device.driver instead
>> when you need it in the function below?
>>
>>>   
>>>   	ret = device_register(&matrix_dev->device);
>>>   	if (ret)
>>>   		goto matrix_reg_err;
>>>   
>>> +	ret = driver_register(&matrix_driver.drv);
>>> +	if (ret)
>>> +		goto matrix_drv_err;
>>> +
>>
>> As you already have several structures that can be registered exactly
>> once (the root device, the bus, the driver, ...), you can already be
>> sure that there's only one device on the bus, can't you?
>>
>>>   	return 0;
>>>   
>>> +matrix_drv_err:
>>> +	device_unregister(&matrix_dev->device);
>>>   matrix_reg_err:
>>>   	put_device(&matrix_dev->device);
>>>   matrix_alloc_err:
>>> +	bus_unregister(&matrix_bus);
>>> +bus_register_err:
>>>   	root_device_unregister(root_device);
>>> -
>>>   	return ret;
>>>   }
>>>   
>>>   static void vfio_ap_matrix_dev_destroy(void)
>>>   {
>>> +	struct device *root_device = matrix_dev->device.parent;
>>> +
>>> +	driver_unregister(&matrix_driver.drv);
>>>   	device_unregister(&matrix_dev->device);
>>> -	root_device_unregister(matrix_dev->device.parent);
>>> +	bus_unregister(&matrix_bus);
>>> +	root_device_unregister(root_device);
>>>   }
>>>   
>>>   static int __init vfio_ap_init(void)
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 272ef42..900b9cf 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>>>   	qres.apqi = apqi;
>>>   	qres.reserved = false;
>>>   
>>> -	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
>>> -				     vfio_ap_has_queue);
>>> +	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>>> +				     &qres, vfio_ap_has_queue);
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>>> index 5675492..76b7f98 100644
>>> --- a/drivers/s390/crypto/vfio_ap_private.h
>>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>>> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>>>   	struct ap_config_info info;
>>>   	struct list_head mdev_list;
>>>   	struct mutex lock;
>>> +	struct ap_driver  *vfio_ap_drv;
>>>   };
>>>   
>>>   extern struct ap_matrix_dev *matrix_dev;
>>
>> This feels like a lot of boilerplate code, just to create a bus that
>> basically doesn't do anything. I'm surprised that libudev can't deal
>> with bus-less devices properly...
>>
> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

On 14/02/2019 16:05, Christian Borntraeger wrote:
> On 14.02.2019 15:54, Cornelia Huck wrote:
>> On Thu, 14 Feb 2019 14:51:01 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
> Pierre,
> this is independent from this series and should have been sent separately.
> In the end (when we have the final solution) this will require cc stable.

Yes, I will wait until tomorrow for more feed back and I will send a 
separate v2 for this patch.

Regards,
Pierre

>>
>>> Libudev relies on having a subsystem link for non-root devices. To
>>> avoid libudev (and potentially other userspace tools) choking on the
>>> matrix device let us introduce a vfio_ap bus and with that the vfio_ap
>>> bus subsytem, and make the matrix device reside within it.
>>
>> How does libudev choke on this? It feels wrong to introduce a bus that
>> basically does nothing...
> 
> I have seen libvirt looping when a matrix device was available before the
> libvirt start.
> Marc Hartmayer debugged this and circumvented this in libvirt:
> https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html
> 
> Still libudev expects a subsystem link in the matrix folder when doing the
> udev_enumerate_scan_devices call.
> 
> Having a bus is one way of adding a subsystem link.
> 
>>
>>>
>>> We restrict the number of allowed devices to a single one.
>>>
>>> Doing this we need to suppress the forced link from the matrix device to
>>> the vfio_ap driver and we suppress the device_type we do not need
>>> anymore.
>>>
>>> Since the associated matrix driver is not the vfio_ap driver any more,
>>> we have to change the search for the devices on the vfio_ap driver in
>>> the function vfio_ap_verify_queue_reserved.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>   drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
>>>   drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
>>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>>   3 files changed, 60 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
>>> index 31c6c84..1fd5fe6 100644
>>> --- a/drivers/s390/crypto/vfio_ap_drv.c
>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c
>>> @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
>>>   
>>>   static struct ap_driver vfio_ap_drv;
>>>   
>>> -static struct device_type vfio_ap_dev_type = {
>>> -	.name = VFIO_AP_DEV_TYPE_NAME,
>>> +struct matrix_driver {
>>> +	struct device_driver drv;
>>> +	int device_count;
>>
>> This counter basically ensures that at most one device may bind with
>> this driver... you'd still have that device on the bus, though.
>>
>>>   };
>>>   
>>>   struct ap_matrix_dev *matrix_dev;
>>> @@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
>>>   	kfree(matrix_dev);
>>>   }
>>>   
>>> +static int matrix_bus_match(struct device *dev, struct device_driver *drv)
>>> +{
>>> +	return 1;
>>> +}
>>> +
>>> +static struct bus_type matrix_bus = {
>>> +	.name = "vfio_ap",
>>> +	.match = &matrix_bus_match,
>>> +};
>>> +
>>> +static int matrix_probe(struct device *dev);
>>> +static int matrix_remove(struct device *dev);
>>> +static struct matrix_driver matrix_driver = {
>>> +	.drv = {
>>> +		.name = "vfio_ap",
>>> +		.bus = &matrix_bus,
>>> +		.probe = matrix_probe,
>>> +		.remove = matrix_remove,
>>> +	},
>>> +};
>>> +
>>> +static int matrix_probe(struct device *dev)
>>> +{
>>> +	if (matrix_driver.device_count)
>>> +		return -EEXIST;
>>> +	matrix_driver.device_count++;
>>> +	return 0;
>>> +}
>>> +
>>> +static int matrix_remove(struct device *dev)
>>> +{
>>> +	matrix_driver.device_count--;
>>> +	return 0;
>>> +}
>>> +
>>>   static int vfio_ap_matrix_dev_create(void)
>>>   {
>>>   	int ret;
>>> @@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
>>>   	if (IS_ERR(root_device))
>>>   		return PTR_ERR(root_device);
>>>   
>>> +	ret = bus_register(&matrix_bus);
>>> +	if (ret)
>>> +		goto bus_register_err;
>>> +
>>>   	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
>>>   	if (!matrix_dev) {
>>>   		ret = -ENOMEM;
>>> @@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
>>>   	mutex_init(&matrix_dev->lock);
>>>   	INIT_LIST_HEAD(&matrix_dev->mdev_list);
>>>   
>>> -	matrix_dev->device.type = &vfio_ap_dev_type;
>>>   	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>>>   	matrix_dev->device.parent = root_device;
>>> +	matrix_dev->device.bus = &matrix_bus;
>>>   	matrix_dev->device.release = vfio_ap_matrix_dev_release;
>>> -	matrix_dev->device.driver = &vfio_ap_drv.driver;
>>> +	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>>
>> Can't you get that structure through matrix_dev->device.driver instead
>> when you need it in the function below?
>>
>>>   
>>>   	ret = device_register(&matrix_dev->device);
>>>   	if (ret)
>>>   		goto matrix_reg_err;
>>>   
>>> +	ret = driver_register(&matrix_driver.drv);
>>> +	if (ret)
>>> +		goto matrix_drv_err;
>>> +
>>
>> As you already have several structures that can be registered exactly
>> once (the root device, the bus, the driver, ...), you can already be
>> sure that there's only one device on the bus, can't you?
>>
>>>   	return 0;
>>>   
>>> +matrix_drv_err:
>>> +	device_unregister(&matrix_dev->device);
>>>   matrix_reg_err:
>>>   	put_device(&matrix_dev->device);
>>>   matrix_alloc_err:
>>> +	bus_unregister(&matrix_bus);
>>> +bus_register_err:
>>>   	root_device_unregister(root_device);
>>> -
>>>   	return ret;
>>>   }
>>>   
>>>   static void vfio_ap_matrix_dev_destroy(void)
>>>   {
>>> +	struct device *root_device = matrix_dev->device.parent;
>>> +
>>> +	driver_unregister(&matrix_driver.drv);
>>>   	device_unregister(&matrix_dev->device);
>>> -	root_device_unregister(matrix_dev->device.parent);
>>> +	bus_unregister(&matrix_bus);
>>> +	root_device_unregister(root_device);
>>>   }
>>>   
>>>   static int __init vfio_ap_init(void)
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 272ef42..900b9cf 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>> @@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
>>>   	qres.apqi = apqi;
>>>   	qres.reserved = false;
>>>   
>>> -	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
>>> -				     vfio_ap_has_queue);
>>> +	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>>> +				     &qres, vfio_ap_has_queue);
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>>> index 5675492..76b7f98 100644
>>> --- a/drivers/s390/crypto/vfio_ap_private.h
>>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>>> @@ -40,6 +40,7 @@ struct ap_matrix_dev {
>>>   	struct ap_config_info info;
>>>   	struct list_head mdev_list;
>>>   	struct mutex lock;
>>> +	struct ap_driver  *vfio_ap_drv;
>>>   };
>>>   
>>>   extern struct ap_matrix_dev *matrix_dev;
>>
>> This feels like a lot of boilerplate code, just to create a bus that
>> basically doesn't do anything. I'm surprised that libudev can't deal
>> with bus-less devices properly...
>>
> 


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

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Pierre Morel]

On 14/02/2019 17:57, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
> <pmorel@linux.ibm.com> wrote:
> 
>> On 14/02/2019 15:54, Cornelia Huck wrote:
>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
>>> <pmorel@linux.ibm.com> wrote:
> 
>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -24,8 +24,9 @@
>>>> MODULE_LICENSE("GPL v2");
>>>> 
>>>> static struct ap_driver vfio_ap_drv;
>>>> 
>>>> -static struct device_type vfio_ap_dev_type = { -	.name =
>>>> VFIO_AP_DEV_TYPE_NAME, +struct matrix_driver { +	struct
>>>> device_driver drv; +	int device_count;
>>> 
>>> This counter basically ensures that at most one device may bind
>>> with this driver... you'd still have that device on the bus,
>>> though.
>> 
>> yes, this is what is wanted: this driver can only support one
>> device. May be another matrix driver can support one or more other
>> devices.
>> 
>> I should update comment message my be.
>> 
>>> 
>>>> };
>>>> 
>>>> struct ap_matrix_dev *matrix_dev;
>> 
>>>> 
>>>> -	matrix_dev->device.type = &vfio_ap_dev_type; 
>>>> dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); 
>>>> matrix_dev->device.parent = root_device; +
>>>> matrix_dev->device.bus = &matrix_bus; 
>>>> matrix_dev->device.release = vfio_ap_matrix_dev_release; -
>>>> matrix_dev->device.driver = &vfio_ap_drv.driver; +
>>>> matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>>> 
>>> Can't you get that structure through matrix_dev->device.driver
>>> instead when you need it in the function below?
>> 
>> Not anymore. We have two different drivers and devices matrix_drv
>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
>> 
>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
>> needed here is vfio_ap_drv.
> 
> Wait, we had tacked a driver for ap devices unto a matrix device,
> which is not on the ap bus?

...yes -(

> Maybe that's what trips libudev? >
> (And reading further in the current code, it seems we clear that 
> structure _after_ the matrix device had been setup, so how can that 
> even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a 
driver and this field was nor tested nor overwritten.

> 
>> 
>>> 
>>>> 
>>>> ret = device_register(&matrix_dev->device); if (ret) goto
>>>> matrix_reg_err;
>>>> 
>>>> +	ret = driver_register(&matrix_driver.drv); +	if (ret) +		goto
>>>> matrix_drv_err; +
>>> 
>>> As you already have several structures that can be registered
>>> exactly once (the root device, the bus, the driver, ...), you can
>>> already be sure that there's only one device on the bus, can't
>>> you?
>> 
>> hum, no I don't think so, no device can register before this module
>> is loaded, but what does prevent a device to register later from
>> another module?
> 
> Not unless you export the interface, I guess.
> 

:) definitively right
thanks, this will simplify the code in the next version.
I will take the patch away from this series to get the way to stable as 
Christian requested.

Regards,
Pierre

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

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Tony Krowiak]

On 2/14/19 12:36 PM, Pierre Morel wrote:
> On 14/02/2019 17:57, Cornelia Huck wrote:
>> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
>> <pmorel@linux.ibm.com> wrote:
>>
>>> On 14/02/2019 15:54, Cornelia Huck wrote:
>>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
>>>> <pmorel@linux.ibm.com> wrote:
>>
>>>>> +++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -24,8 +24,9 @@
>>>>> MODULE_LICENSE("GPL v2");
>>>>>
>>>>> static struct ap_driver vfio_ap_drv;
>>>>>
>>>>> -static struct device_type vfio_ap_dev_type = { -    .name =
>>>>> VFIO_AP_DEV_TYPE_NAME, +struct matrix_driver { +    struct
>>>>> device_driver drv; +    int device_count;
>>>>
>>>> This counter basically ensures that at most one device may bind
>>>> with this driver... you'd still have that device on the bus,
>>>> though.
>>>
>>> yes, this is what is wanted: this driver can only support one
>>> device. May be another matrix driver can support one or more other
>>> devices.
>>>
>>> I should update comment message my be.
>>>
>>>>
>>>>> };
>>>>>
>>>>> struct ap_matrix_dev *matrix_dev;
>>>
>>>>>
>>>>> -    matrix_dev->device.type = &vfio_ap_dev_type; 
>>>>> dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); 
>>>>> matrix_dev->device.parent = root_device; +
>>>>> matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = 
>>>>> vfio_ap_matrix_dev_release; -
>>>>> matrix_dev->device.driver = &vfio_ap_drv.driver; +
>>>>> matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>>>>
>>>> Can't you get that structure through matrix_dev->device.driver
>>>> instead when you need it in the function below?
>>>
>>> Not anymore. We have two different drivers and devices matrix_drv
>>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
>>>
>>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
>>> needed here is vfio_ap_drv.
>>
>> Wait, we had tacked a driver for ap devices unto a matrix device,
>> which is not on the ap bus?

It's really a bit more complicated than that. Without going into a
lengthy description of the history of AP passthrough support, suffice it
to say that we needed a device to serve as the parent of each mediated
device used to configure a matrix of AP adapter IDs and domain indexes
identifying the devices to which a guest would be granted access. The
AP devices themselves are attached to the AP bus, but the matrix device
is an artificial (virtual?) device whose sole purpose in life is to
serve as an anchor for the mediated devices whose sysfs interfaces are
created and managed by the vfio_ap device driver. The matrix device
itself is created by the vfio_ap device driver - when it is initialized 
- for that purpose. In hindsight, maybe there was a better way to
implement this, but neither this patch nor this discussion belongs in
this series. It distracts from discussion of interrupt support which is
the sole purpose of the patch series.

> 
> ...yes -(
> 
>> Maybe that's what trips libudev? >
>> (And reading further in the current code, it seems we clear that 
>> structure _after_ the matrix device had been setup, so how can that 
>> even work? Where am I confused?)
> 
> On device_register there were no bus, so the core just do not look for a 
> driver and this field was nor tested nor overwritten.
> 
>>
>>>
>>>>
>>>>>
>>>>> ret = device_register(&matrix_dev->device); if (ret) goto
>>>>> matrix_reg_err;
>>>>>
>>>>> +    ret = driver_register(&matrix_driver.drv); +    if (ret) 
>>>>> +        goto
>>>>> matrix_drv_err; +
>>>>
>>>> As you already have several structures that can be registered
>>>> exactly once (the root device, the bus, the driver, ...), you can
>>>> already be sure that there's only one device on the bus, can't
>>>> you?
>>>
>>> hum, no I don't think so, no device can register before this module
>>> is loaded, but what does prevent a device to register later from
>>> another module?
>>
>> Not unless you export the interface, I guess.
>>
> 
> :) definitively right
> thanks, this will simplify the code in the next version.
> I will take the patch away from this series to get the way to stable as 
> Christian requested.
> 
> Regards,
> Pierre
> 

Re: [PATCH v3 0/9] [RFC] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:

The subject leads one to believe this patch series introduces
ioctl definitions for AQIC, but there is not a single ioctl
definition in the series. Something like this might be more
appropriate:

vfio: ap: support for AP Queue Interrupt Control

Regards
T. Krow

> This patch implement PQAP/AQIC interception in KVM.
> 
> 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 : to unpin the NIB on clear IRQ
>   - ISC : to unregister with the GIB interface
>   - MATRIX: a pointer to the matrix mediated device
>   - DEV: a pointer to the AP queue device from AP bus.
> 
> 
> 1) Phase 1, probe and remove from vfio_ap_queue
> 
> The vfio_ap_queue structures are dynamically allocated when a queue
> is probed by the ap_vfio_driver.
> It is linked to the ap_queue device as the driver data.
> We store the APQN of the ap_queue device inside the vfio_ap_queue.
> The vfio_ap_queue are free during remove.
> 
> 
> 2) Phase 2, assignment of vfio_ap_queue to a mediated device
> 
> When the VFIO device is opened the queues are associated to
> the mediated device, at the same time the CRYCB is setup.
> 
> When the device is closed or if a queue is removed the queue is
> dissociated from the mediated device, as the same time the
> CRYCB is cleared.
> 
> Two new functions are used for this: associate/disassociate.
> 
> 
> 3) Phase 3 intercepting the PQAP/AQIC instruction
> 
> On interception of the PQAP/AQIC instruction, the interception code
> makes sure the pqap_hook is initialized and allowed to be called
> and call it.
> Otherwise it reports the usual -EOPNOTSUPP return code to let
> QEMU handle the fault.
>    
> the pqap callback search for the queue asociated with the APQN
> stored in the register 0, setting the code to "illegal APQN"
> if the vfio_ap_queue can not be found.
> 
> Depending on the "i" bit of the register 1, the pqap callback
> setup or clear the interruption by calling the host format PQAP/AQIC
> instruction.
> When seting up the interruption it uses the NIB and the guest ISC
> provided by the guest and the host ISC provided by the registration
> to the GIB code, pin the NIB and also stores ISC and NIB inside
> the vfio_ap_queue structure.
> When clearing the interrupt it retrieves the host ISC to unregister
> with the GIB code and unpin the NIB.
> 
> 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>
> 
> 
> 4) Phase 4 clean dissociation from the mediated device on remove
> 
> On removing of the AP device the remove callback is called.
> To be sure that the guest will not access the queue anymore
> we clear the APID CRYCB bit.
> Cleaning the APID, over the APQI, is chosen because the architecture
> specifies that only the APID can be dynamically changed outside IPL.
> After this the queue is zeroes, care is taken to correctly wait until
> the queue is empty.
> 
> 
> 
> Pierre Morel (9):
>    s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem
>    s390: ap: kvm: setting a hook for PQAP instructions
>    s390: ap: new vfio_ap_queue structure
>    s390: ap: tools to find a queue with a specific APQN
>    s390: ap: tools to associate a queue to a matrix
>    vfio: ap: register IOMMU VFIO notifier
>    s390: ap: implement PAPQ AQIC interception in kernel
>    s390: ap: Cleanup on removing the AP device
>    s390: ap: kvm: add AP Queue Interruption Control facility
> 
>   arch/s390/include/asm/kvm_host.h      |   1 +
>   arch/s390/kvm/priv.c                  |  50 +++++
>   arch/s390/tools/gen_facilities.c      |   1 +
>   drivers/s390/crypto/ap_bus.h          |   1 +
>   drivers/s390/crypto/vfio_ap_drv.c     | 178 ++++++++++++++++-
>   drivers/s390/crypto/vfio_ap_ops.c     | 363 +++++++++++++++++++++++++++++++++-
>   drivers/s390/crypto/vfio_ap_private.h |  12 ++
>   7 files changed, 593 insertions(+), 13 deletions(-)
> 

Re: [PATCH v3 0/9] [RFC] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[Pierre Morel]

On 14/02/2019 21:33, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
> 
> The subject leads one to believe this patch series introduces
> ioctl definitions for AQIC, but there is not a single ioctl
> definition in the series. Something like this might be more
> appropriate:

This is right Tony, no ioctl here.
I kept the original subject and increment the version number to let 
people know that this is a new version of the implementation of 
PQAP/AQIC interception for which we already had 2 iterations.

May be you are right and I should have change the subject.

Thanks for making this clear.

Regards,
Pierre

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

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Thu, 14 Feb 2019 13:30:59 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 2/14/19 12:36 PM, Pierre Morel wrote:
> > On 14/02/2019 17:57, Cornelia Huck wrote:  
> >> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
> >> <pmorel@linux.ibm.com> wrote:
> >>  
> >>> On 14/02/2019 15:54, Cornelia Huck wrote:  
> >>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
> >>>> <pmorel@linux.ibm.com> wrote:  

> >>>>> -    matrix_dev->device.type = &vfio_ap_dev_type; 
> >>>>> dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); 
> >>>>> matrix_dev->device.parent = root_device; +
> >>>>> matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = 
> >>>>> vfio_ap_matrix_dev_release; -
> >>>>> matrix_dev->device.driver = &vfio_ap_drv.driver; +
> >>>>> matrix_dev->vfio_ap_drv = &vfio_ap_drv;  
> >>>>
> >>>> Can't you get that structure through matrix_dev->device.driver
> >>>> instead when you need it in the function below?  
> >>>
> >>> Not anymore. We have two different drivers and devices matrix_drv
> >>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
> >>>
> >>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
> >>> needed here is vfio_ap_drv.  
> >>
> >> Wait, we had tacked a driver for ap devices unto a matrix device,
> >> which is not on the ap bus?  
> 
> It's really a bit more complicated than that. Without going into a
> lengthy description of the history of AP passthrough support, suffice it
> to say that we needed a device to serve as the parent of each mediated
> device used to configure a matrix of AP adapter IDs and domain indexes
> identifying the devices to which a guest would be granted access. The
> AP devices themselves are attached to the AP bus, but the matrix device
> is an artificial (virtual?) device whose sole purpose in life is to
> serve as an anchor for the mediated devices whose sysfs interfaces are
> created and managed by the vfio_ap device driver. The matrix device
> itself is created by the vfio_ap device driver - when it is initialized 
> - for that purpose. In hindsight, maybe there was a better way to
> implement this, but neither this patch nor this discussion belongs in
> this series. It distracts from discussion of interrupt support which is
> the sole purpose of the patch series.

The we-need-a-parent part is fine; but whatever we're doing with that
driver just looks wrong, so that even the new bus that basically does
nothing looks better...

> 
> > 
> > ...yes -(
> >   
> >> Maybe that's what trips libudev? >
> >> (And reading further in the current code, it seems we clear that 
> >> structure _after_ the matrix device had been setup, so how can that 
> >> even work? Where am I confused?)  
> > 
> > On device_register there were no bus, so the core just do not look for a 
> > driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in
vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
patch fixes more than just libudev issues...

> >   
> >>  
> >>>  
> >>>>  
> >>>>>
> >>>>> ret = device_register(&matrix_dev->device); if (ret) goto
> >>>>> matrix_reg_err;
> >>>>>
> >>>>> +    ret = driver_register(&matrix_driver.drv); +    if (ret) 
> >>>>> +        goto
> >>>>> matrix_drv_err; +  
> >>>>
> >>>> As you already have several structures that can be registered
> >>>> exactly once (the root device, the bus, the driver, ...), you can
> >>>> already be sure that there's only one device on the bus, can't
> >>>> you?  
> >>>
> >>> hum, no I don't think so, no device can register before this module
> >>> is loaded, but what does prevent a device to register later from
> >>> another module?  
> >>
> >> Not unless you export the interface, I guess.
> >>  
> > 
> > :) definitively right
> > thanks, this will simplify the code in the next version.
> > I will take the patch away from this series to get the way to stable as 
> > Christian requested.

Yeah, makes sense.

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Cornelia Huck]

On Thu, 14 Feb 2019 17:45:06 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 14/02/2019 16:54, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 14:51:02 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> This patch adds interception code for the PQAP instructions,
> >> and a callback inside the KVM arch structure for s390.
> >>
> >> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ  
> > 
> > s/drivers/driver/  
> 
> thanks. OK
> 

> >> + *
> >> + * This callback only handles PQAP/AQIC instruction and  
> > 
> > Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in
> > the patch description?  
> 
> I can add "for now" or "in this patch" or suppress the reference to 
> PAPQ/TAPQ

I'd just add a note to the patch description that this patch only
handles PQAP/AQCI and that handling PQAP/TAPQ is something for a
follow-on patch.

> 
> >   
> >> + * calls a dedicated callback for this instruction if
> >> + * a driver did register one in the CRYPTO satellite of the
> >> + * SIE block.
> >> + *
> >> + * Do not change the behavior if, return -EOPNOTSUPP if:
> >> + * - the hook is not used do not change the behavior.  
> > 
> > The hook is not used? Or not set?  
> 
> I think "is not set" is better.

Ok.

> 
> > (I don't think you need to repeat "do
> > not change the behavior".)  
> 
> OK
> 
> >   
> >> + * - AP instructions are not available or not available to the guest
> >> + * - the instruction is not PQAP with function code indicating
> >> + *   AQIC do not change the previous behavior.
> >> + *
> >> + * For PQAP/AQIC instruction, verify privilege and specifications
> >> + *
> >> + * return the value returned by the callback.
> >> + */
> >> +static int handle_pqap(struct kvm_vcpu *vcpu)
> >> +{
> >> +	uint8_t fc;
> >> +
> >> +	/* Verify that the hook callback is registered */
> >> +	if (!vcpu->kvm->arch.crypto.pqap_hook)
> >> +		return -EOPNOTSUPP;
> >> +	/* 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;
> >> +	/* Verify that the function code is AQIC */
> >> +	fc = vcpu->run->s.regs.gprs[0] >> 24;
> >> +	if (fc != 0x03)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	/* PQAP instructions are 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);
> >> +	/* AQIC instruction is allowed only if facility 65 is available */
> >> +	if (!test_kvm_facility(vcpu->kvm, 65))
> >> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> >> +	/* All right, call the callback */
> >> +	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);  
> > 
> > Can that callback also return -EOPNOTSUPP to order to drop to user
> > space?  
> 
> Yes.
> Why not?

Maybe also mention that in the function description?

Re: [PATCH v3 3/9] s390: ap: new vfio_ap_queue structure

[Cornelia Huck]

On Thu, 14 Feb 2019 14:51:03 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> The AP interruptions are assigned on a queue basis
> and we need some information neloging to the queue

s/neloging/belonging/

> to handle interuptions.

s/interuptions/interruptions/

> 
> We add a new structure vfio_ap_queue, to hold per
> queue information useful to handle interruptions.
> 
> - apqn: AP queue number (defined here)
> - isc : Interrupt subclass (defined later)
> - nib : notification information byte (defined later)

So, you basically simply want quick access to these? Makes sense.

> 
> The vfio_ap_queue structure is allocated when the vfio_ap_driver
> is probed and added as driver data to the ap_queue device.
> It is free on remove.

s/free/freed/

> 
> The structure is linked to the matrix_dev host device.

Hm, I don't quite follow?

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

Patch looks reasonable; might make sense to squash it with later
patches (still need to read).

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Cornelia Huck]

On Thu, 14 Feb 2019 14:51:04 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We need to find the queue with a specific APQN during the
> handling of the interception of the PQAP/AQIC instruction.
> 
> To handle the AP associated device reference count we keep
> track of it in the vfio_ap_queue until we put the device.

So, the relationship is
(struct ap_device)--(driver_data)-->(struct vfio_ap_queue)--(pointer)-->(struct ap_device)
? IOW, a backlink?

If so, can't you already set that up during probe?

Or am I confused by the various similar devices again? Maybe a diagram
would help...

> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
>  drivers/s390/crypto/vfio_ap_private.h |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 900b9cf..2a52c9b 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -24,6 +24,60 @@
>  #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
>  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>  
> +/**
> + * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
> + *
> + * Returns 1 if we have a match.
> + * Otherwise returns 0.
> + */
> +static int vfio_ap_check_apqn(struct device *dev, void *data)
> +{
> +	struct vfio_ap_queue *q = dev_get_drvdata(dev);
> +
> +	return (q->apqn == *(int *)data);
> +}
> +
> +/**
> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
> + * @apqn: The queue APQN
> + *
> + * Retrieve a queue with a specific APQN from the list of the
> + * devices associated to the vfio_ap_driver.
> + *
> + * The vfio_ap_queue has been already associated with the device
> + * during the probe.
> + * Store the associated device for reference counting
> + *
> + * Returns the pointer to the associated vfio_ap_queue
> + */
> +static  __attribute__((unused))

Eww. Can you get rid of that by reordering or squashing patches?

> +	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
> +{
> +	struct device *dev;
> +	struct vfio_ap_queue *q;
> +
> +	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
> +				 vfio_ap_check_apqn);
> +	if (!dev)
> +		return NULL;
> +	q = dev_get_drvdata(dev);
> +	q->dev = dev;
> +	return q;
> +}
> +
> +/**
> + * vfio_ap_put_queue: lower device reference count for a queue
> + * @q: The queue
> + *
> + * put the associated device
> + *
> + */
> +static  __attribute__((unused)) void vfio_ap_put_queue(struct vfio_ap_queue *q)
> +{
> +	put_device(q->dev);
> +	q->dev = NULL;
> +}
> +
>  static void vfio_ap_matrix_init(struct ap_config_info *info,
>  				struct ap_matrix *matrix)
>  {
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 8836f01..081f0d7 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -87,6 +87,7 @@ extern int vfio_ap_mdev_register(void);
>  extern void vfio_ap_mdev_unregister(void);
>  
>  struct vfio_ap_queue {
> +	struct device *dev;
>  	int	apqn;
>  };
>  #endif /* _VFIO_AP_PRIVATE_H_ */

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

On 15/02/2019 10:26, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 17:45:06 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 14/02/2019 16:54, Cornelia Huck wrote:
>>> On Thu, 14 Feb 2019 14:51:02 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>    
>>>> This patch adds interception code for the PQAP instructions,
>>>> and a callback inside the KVM arch structure for s390.
>>>>
>>>> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ
>>>
>>> s/drivers/driver/
>>
>> thanks. OK
>>
> 
>>>> + *
>>>> + * This callback only handles PQAP/AQIC instruction and
>>>
>>> Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in
>>> the patch description?
>>
>> I can add "for now" or "in this patch" or suppress the reference to
>> PAPQ/TAPQ
> 
> I'd just add a note to the patch description that this patch only
> handles PQAP/AQCI and that handling PQAP/TAPQ is something for a
> follow-on patch.

OK, I will clear this.
Thanks


> 
>>
>>>   

...snip...

>>>> +	/* PQAP instructions are 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);
>>>> +	/* AQIC instruction is allowed only if facility 65 is available */
>>>> +	if (!test_kvm_facility(vcpu->kvm, 65))
>>>> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>>>> +	/* All right, call the callback */
>>>> +	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
>>>
>>> Can that callback also return -EOPNOTSUPP to order to drop to user
>>> space?
>>
>> Yes.
>> Why not?
> 
> Maybe also mention that in the function description?
> 

Will do thanks.

Regards,
Pierre

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

Re: [PATCH v3 3/9] s390: ap: new vfio_ap_queue structure

[Pierre Morel]

On 15/02/2019 10:37, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:03 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> The AP interruptions are assigned on a queue basis
>> and we need some information neloging to the queue
> 
> s/neloging/belonging/
> 
>> to handle interuptions.
> 
> s/interuptions/interruptions/
> 
>>
>> We add a new structure vfio_ap_queue, to hold per
>> queue information useful to handle interruptions.
>>
>> - apqn: AP queue number (defined here)
>> - isc : Interrupt subclass (defined later)
>> - nib : notification information byte (defined later)
> 
> So, you basically simply want quick access to these? Makes sense.
> 
>>
>> The vfio_ap_queue structure is allocated when the vfio_ap_driver
>> is probed and added as driver data to the ap_queue device.
>> It is free on remove.
> 
> s/free/freed/
> 
>>
>> The structure is linked to the matrix_dev host device.
> 
> Hm, I don't quite follow?

Sorry, do not try to, this has been left from a previous internal version.
I will suppress this

> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 23 ++++++++++++++++++++++-
>>   drivers/s390/crypto/vfio_ap_private.h |  3 +++
>>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> Patch looks reasonable; might make sense to squash it with later
> patches (still need to read).
> 


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

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Pierre Morel]

On 15/02/2019 10:49, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 14:51:04 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We need to find the queue with a specific APQN during the
>> handling of the interception of the PQAP/AQIC instruction.
>>
>> To handle the AP associated device reference count we keep
>> track of it in the vfio_ap_queue until we put the device.
> 
> So, the relationship is
> (struct ap_device)--(driver_data)-->(struct vfio_ap_queue)--(pointer)-->(struct ap_device)
> ? IOW, a backlink?
> 
> If so, can't you already set that up during probe?

Will do.

> 
> Or am I confused by the various similar devices again? Maybe a diagram
> would help...

No you are right.


> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>   2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 900b9cf..2a52c9b 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -24,6 +24,60 @@
>>   #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
>>   #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>>   
>> +/**
>> + * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
>> + *
>> + * Returns 1 if we have a match.
>> + * Otherwise returns 0.
>> + */
>> +static int vfio_ap_check_apqn(struct device *dev, void *data)
>> +{
>> +	struct vfio_ap_queue *q = dev_get_drvdata(dev);
>> +
>> +	return (q->apqn == *(int *)data);
>> +}
>> +
>> +/**
>> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
>> + * @apqn: The queue APQN
>> + *
>> + * Retrieve a queue with a specific APQN from the list of the
>> + * devices associated to the vfio_ap_driver.
>> + *
>> + * The vfio_ap_queue has been already associated with the device
>> + * during the probe.
>> + * Store the associated device for reference counting
>> + *
>> + * Returns the pointer to the associated vfio_ap_queue
>> + */
>> +static  __attribute__((unused))
> 
> Eww. Can you get rid of that by reordering or squashing patches?

I did this to avoid posting a very big patch.
I will of course squash 4 and 5 with patch 6, when the two patches 4 and 
5 are reviewed.

If you think it brings more clarity to squash all for the next iteration 
I will do.

Regards,
Pierre


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

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Cornelia Huck]

On Fri, 15 Feb 2019 11:10:43 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 15/02/2019 10:49, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 14:51:04 +0100
> > Pierre Morel <pmorel@linux.ibm.com> wrote:
> >   
> >> We need to find the queue with a specific APQN during the
> >> handling of the interception of the PQAP/AQIC instruction.
> >>
> >> To handle the AP associated device reference count we keep
> >> track of it in the vfio_ap_queue until we put the device.  
> > 
> > So, the relationship is
> > (struct ap_device)--(driver_data)-->(struct vfio_ap_queue)--(pointer)-->(struct ap_device)
> > ? IOW, a backlink?
> > 
> > If so, can't you already set that up during probe?  
> 
> Will do.
> 
> > 
> > Or am I confused by the various similar devices again? Maybe a diagram
> > would help...  
> 
> No you are right.

Good, I was fearing that I was more confused than normal for Fridays ;)

> 
> 
> >   
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> >> ---
> >>   drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
> >>   drivers/s390/crypto/vfio_ap_private.h |  1 +
> >>   2 files changed, 55 insertions(+)
> >>
> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> >> index 900b9cf..2a52c9b 100644
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -24,6 +24,60 @@
> >>   #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
> >>   #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
> >>   
> >> +/**
> >> + * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
> >> + *
> >> + * Returns 1 if we have a match.
> >> + * Otherwise returns 0.
> >> + */
> >> +static int vfio_ap_check_apqn(struct device *dev, void *data)
> >> +{
> >> +	struct vfio_ap_queue *q = dev_get_drvdata(dev);
> >> +
> >> +	return (q->apqn == *(int *)data);
> >> +}
> >> +
> >> +/**
> >> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
> >> + * @apqn: The queue APQN
> >> + *
> >> + * Retrieve a queue with a specific APQN from the list of the
> >> + * devices associated to the vfio_ap_driver.
> >> + *
> >> + * The vfio_ap_queue has been already associated with the device
> >> + * during the probe.
> >> + * Store the associated device for reference counting
> >> + *
> >> + * Returns the pointer to the associated vfio_ap_queue
> >> + */
> >> +static  __attribute__((unused))  
> > 
> > Eww. Can you get rid of that by reordering or squashing patches?  
> 
> I did this to avoid posting a very big patch.
> I will of course squash 4 and 5 with patch 6, when the two patches 4 and 
> 5 are reviewed.
> 
> If you think it brings more clarity to squash all for the next iteration 
> I will do.

Let's just see what the patches look like in the end. If a squashed
patch is not too unwieldy, I'd prefer that over those unused
annotations, though.

Hoping for review from others as well ;)

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Tony Krowiak]

On 2/15/19 4:11 AM, Cornelia Huck wrote:
> On Thu, 14 Feb 2019 13:30:59 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/14/19 12:36 PM, Pierre Morel wrote:
>>> On 14/02/2019 17:57, Cornelia Huck wrote:
>>>> On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel
>>>> <pmorel@linux.ibm.com> wrote:
>>>>   
>>>>> On 14/02/2019 15:54, Cornelia Huck wrote:
>>>>>> On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel
>>>>>> <pmorel@linux.ibm.com> wrote:
> 
>>>>>>> -    matrix_dev->device.type = &vfio_ap_dev_type;
>>>>>>> dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
>>>>>>> matrix_dev->device.parent = root_device; +
>>>>>>> matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release =
>>>>>>> vfio_ap_matrix_dev_release; -
>>>>>>> matrix_dev->device.driver = &vfio_ap_drv.driver; +
>>>>>>> matrix_dev->vfio_ap_drv = &vfio_ap_drv;
>>>>>>
>>>>>> Can't you get that structure through matrix_dev->device.driver
>>>>>> instead when you need it in the function below?
>>>>>
>>>>> Not anymore. We have two different drivers and devices matrix_drv
>>>>> <-> matrix_dev and vfio_ap_drv <-> ap_devices
>>>>>
>>>>> The driver behind the matrix_dev->dev->driver is matrix_drv what is
>>>>> needed here is vfio_ap_drv.
>>>>
>>>> Wait, we had tacked a driver for ap devices unto a matrix device,
>>>> which is not on the ap bus?
>>
>> It's really a bit more complicated than that. Without going into a
>> lengthy description of the history of AP passthrough support, suffice it
>> to say that we needed a device to serve as the parent of each mediated
>> device used to configure a matrix of AP adapter IDs and domain indexes
>> identifying the devices to which a guest would be granted access. The
>> AP devices themselves are attached to the AP bus, but the matrix device
>> is an artificial (virtual?) device whose sole purpose in life is to
>> serve as an anchor for the mediated devices whose sysfs interfaces are
>> created and managed by the vfio_ap device driver. The matrix device
>> itself is created by the vfio_ap device driver - when it is initialized
>> - for that purpose. In hindsight, maybe there was a better way to
>> implement this, but neither this patch nor this discussion belongs in
>> this series. It distracts from discussion of interrupt support which is
>> the sole purpose of the patch series.
> 
> The we-need-a-parent part is fine; but whatever we're doing with that
> driver just looks wrong, so that even the new bus that basically does
> nothing looks better...

I believe there might be a much better way to handle this which is why
I objected to this patch being delivered with this series, which
provides AP interrupt support. Quite simply, this patch has no
relationship to interrupt support and should be considered as an item
unto itself. To conduct a review within the context of interrupt
support distracts focus from the pertinent subject matter.

> 
>>
>>>
>>> ...yes -(
>>>    
>>>> Maybe that's what trips libudev? >
>>>> (And reading further in the current code, it seems we clear that
>>>> structure _after_ the matrix device had been setup, so how can that
>>>> even work? Where am I confused?)
>>>
>>> On device_register there were no bus, so the core just do not look for a
>>> driver and this field was nor tested nor overwritten.
> 
> Hm... so has the callback in driver_for_each_device() in
> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> patch fixes more than just libudev issues...

It is this patch that rendered the driver_for_each_device() in
vfio_ap_verify_queue_reserved() erroneous. That function gets called
every time an adapter or domain is assigned to the mdev. This patch
introduced the problem with driver_for_each_device().

> 
>>>    
>>>>   
>>>>>   
>>>>>>   
>>>>>>>
>>>>>>> ret = device_register(&matrix_dev->device); if (ret) goto
>>>>>>> matrix_reg_err;
>>>>>>>
>>>>>>> +    ret = driver_register(&matrix_driver.drv); +    if (ret)
>>>>>>> +        goto
>>>>>>> matrix_drv_err; +
>>>>>>
>>>>>> As you already have several structures that can be registered
>>>>>> exactly once (the root device, the bus, the driver, ...), you can
>>>>>> already be sure that there's only one device on the bus, can't
>>>>>> you?
>>>>>
>>>>> hum, no I don't think so, no device can register before this module
>>>>> is loaded, but what does prevent a device to register later from
>>>>> another module?
>>>>
>>>> Not unless you export the interface, I guess.
>>>>   
>>>
>>> :) definitively right
>>> thanks, this will simplify the code in the next version.
>>> I will take the patch away from this series to get the way to stable as
>>> Christian requested.
> 
> Yeah, makes sense.
> 

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> This patch adds interception code for the PQAP instructions,
> and a callback inside the KVM arch structure for s390.
> 
> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ
> instructions, the driver will initialize the callback inside
> the kvm_arch structure to be called when the interception of a
> PQAP instruction occurs.
> 
> If the callback is not initialized, the code still returns
> -EOPNOTSUPP to let userland handle the instruction as it used to.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>   arch/s390/include/asm/kvm_host.h |  1 +
>   arch/s390/kvm/priv.c             | 50 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 51 insertions(+)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index c5f5156..49cc8b0 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
>   
>   struct kvm_s390_crypto {
>   	struct kvm_s390_crypto_cb *crycb;
> +	int (*pqap_hook)(struct kvm_vcpu *vcpu);
>   	__u32 crycbd;
>   	__u8 aes_kw;
>   	__u8 dea_kw;
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index 8679bd7..72fdc21 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,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>   	}
>   }
>   
> +/*
> + * handle_pqap: Handling pqap interception
> + * @vcpu: the vcpu having issue the pqap instruction
> + *
> + * This callback only handles PQAP/AQIC instruction and
> + * calls a dedicated callback for this instruction if
> + * a driver did register one in the CRYPTO satellite of the
> + * SIE block.
> + *
> + * Do not change the behavior if, return -EOPNOTSUPP if:
> + * - the hook is not used do not change the behavior.
> + * - AP instructions are not available or not available to the guest
> + * - the instruction is not PQAP with function code indicating
> + *   AQIC do not change the previous behavior.
> + *
> + * For PQAP/AQIC instruction, verify privilege and specifications
> + *
> + * return the value returned by the callback.
> + */
> +static int handle_pqap(struct kvm_vcpu *vcpu)
> +{
> +	uint8_t fc;
> +
> +	/* Verify that the hook callback is registered */
> +	if (!vcpu->kvm->arch.crypto.pqap_hook)
> +		return -EOPNOTSUPP;
> +	/* 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;
> +	/* Verify that the function code is AQIC */
> +	fc = vcpu->run->s.regs.gprs[0] >> 24;
> +	if (fc != 0x03)
> +		return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes
that can be sent with the PQAP instruction. This belongs in the PQAP
hook code.

> +
> +	/* PQAP instructions are 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);
> +	/* AQIC instruction is allowed only if facility 65 is available */
> +	if (!test_kvm_facility(vcpu->kvm, 65))
> +		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> +	/* All right, call the callback */
> +	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
> +}
> +
>   static int handle_stfl(struct kvm_vcpu *vcpu)
>   {
>   	int rc;
> @@ -878,6 +926,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:
> 

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> We need to find the queue with a specific APQN during the
> handling of the interception of the PQAP/AQIC instruction.
> 
> To handle the AP associated device reference count we keep
> track of it in the vfio_ap_queue until we put the device.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>   2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 900b9cf..2a52c9b 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -24,6 +24,60 @@
>   #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
>   #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
>   
> +/**
> + * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
> + *
> + * Returns 1 if we have a match.
> + * Otherwise returns 0.
> + */
> +static int vfio_ap_check_apqn(struct device *dev, void *data)
> +{
> +	struct vfio_ap_queue *q = dev_get_drvdata(dev);
> +
> +	return (q->apqn == *(int *)data);
> +}
> +
> +/**
> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
> + * @apqn: The queue APQN
> + *
> + * Retrieve a queue with a specific APQN from the list of the
> + * devices associated to the vfio_ap_driver.
> + *
> + * The vfio_ap_queue has been already associated with the device
> + * during the probe.
> + * Store the associated device for reference counting
> + *
> + * Returns the pointer to the associated vfio_ap_queue
> + */
> +static  __attribute__((unused))
> +	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)

I think you should change this signature for the reasons I stated
below:

struct device *vfio_ap_get_queue_dev(int apqn)

> +{
> +	struct device *dev;
> +	struct vfio_ap_queue *q;
> +
> +	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
> +				 vfio_ap_check_apqn);
> +	if (!dev)
> +		return NULL;
> +	q = dev_get_drvdata(dev);
> +	q->dev = dev;

Why store the device with the vfio_ap_queue object? Why not just return
the device. The caller can get the vfio_ap_queue from the device's
driver data. It seems the only reason for the 'dev' field is to
temporarily hold a ref to the device so it can be put later. Why not
just put the device.

> +	return q;
> +}
> +
> +/**
> + * vfio_ap_put_queue: lower device reference count for a queue
> + * @q: The queue
> + *
> + * put the associated device
> + *
> + */
> +static  __attribute__((unused)) void vfio_ap_put_queue(struct vfio_ap_queue *q)
> +{
> +	put_device(q->dev);
> +	q->dev = NULL;
> +}

I would get rid of this function. If you take my suggestion above, you
can just call the put_device() directly. There will be no need for
this superfluous 'dev' field.

> +
>   static void vfio_ap_matrix_init(struct ap_config_info *info,
>   				struct ap_matrix *matrix)
>   {
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 8836f01..081f0d7 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -87,6 +87,7 @@ extern int vfio_ap_mdev_register(void);
>   extern void vfio_ap_mdev_unregister(void);
>   
>   struct vfio_ap_queue {
> +	struct device *dev;

No need for this as per my comments above.

>   	int	apqn;
>   };
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 

Re: [PATCH v3 5/9] s390: ap: tools to associate a queue to a matrix

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> We need tools to associate a queue and a matrix to be able
> later to find the matrix associated with the queue for which
> we got the APQN in the register 1 during a PQAP/AQIC instruction
> interception.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c     | 66 +++++++++++++++++++++++++++++++++--
>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>   2 files changed, 64 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 2a52c9b..1851b24 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -50,8 +50,7 @@ static int vfio_ap_check_apqn(struct device *dev, void *data)
>    *
>    * Returns the pointer to the associated vfio_ap_queue
>    */
> -static  __attribute__((unused))
> -	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)

I don't like this, it indicates to me that this and the previous
patch should have been squashed together. I realize your intent
was to make the patches smaller, but I don't think this makes it
any easier to review them. In fact, it makes it harder IMHO, because
you have to flip back and forth between patches to fully comprehend
the usage.

> +static struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
>   {
>   	struct device *dev;
>   	struct vfio_ap_queue *q;
> @@ -72,7 +71,7 @@ static  __attribute__((unused))
>    * put the associated device
>    *
>    */
> -static  __attribute__((unused)) void vfio_ap_put_queue(struct vfio_ap_queue *q)

Ditto for this.

> +static void vfio_ap_put_queue(struct vfio_ap_queue *q)
>   {
>   	put_device(q->dev);
>   	q->dev = NULL;
> @@ -867,6 +866,67 @@ static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>   	return -EBUSY;
>   }
>   
> +/**
> + * vfio_ap_dissociate_queues: Dissociate a matrix mediated device to a queue
> + *
> + * Go through all bits in the AQM and APM and dissociate the queue
> + * from the matrix device.
> + *
> + * No return value we are throwing the mediated device anyway.
> + */
> +static void vfio_ap_dissociate_queues(struct ap_matrix_mdev *matrix_mdev)
> +{
> +	unsigned long apid, apqi;

The changes I recommend here are based on the comments in my
response to patch 4.

> +	struct vfio_ap_queue *q;

Add this:

	struct device *qdev;

> +
> +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
> +			     matrix_mdev->matrix.apm_max + 1) {
> +		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
> +				     matrix_mdev->matrix.aqm_max + 1) {
> +			q = vfio_ap_get_queue(AP_MKQID(apid, apqi));

Replace with:
	qdev = vfio_ap_get_queue_dev(AP_MKQID(apid, apqi);

> +			if (q) {

Replace with:
	if (qdev) {

Add this:
	q = dev_get_drvdata(qdev);

> +				vfio_ap_mdev_reset_queue(apid, apqi, 1);
> +				q->matrix = NULL;
> +				vfio_ap_put_queue(q);

Replace with:
	put_device(qdev)

> +			} else
> +				pr_err("%s: no queue for %02lx.%04lx\n",
> +				       __func__, apid, apqi);
> +		}
> +	}
> +}
> +
> +/**
> + * vfio_ap_associate_queues: Associate a matrix mediated device to a queue
> + *
> + * Go through all bits in the AQM and APM and calculate the APQN, and find
> + * the matching queue to associate with the matrix device.
> + *
> + * In the case a queue could not be found return -ENODEV.
> + * Otherwise return 0.
> + */
> +static __attribute__((unused))

This indicates this patch should be squashed with the patch that
introduces this function. Why have a patch that introduces a function
that is not used and put and artificially specify an attribute just to
keep the compiler from issuing a warning?

> +	int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
> +{

The same as above, the changes I recommend here are based on the
comments in my response to patch 4.

> +	unsigned long apid, apqi;
> +	struct vfio_ap_queue *q;

Add:
	struct device *qdev;

> +
> +	for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
> +			     matrix_mdev->matrix.apm_max + 1) {
> +		for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
> +				     matrix_mdev->matrix.aqm_max + 1) {
> +			q = vfio_ap_get_queue(AP_MKQID(apid, apqi));

Replace with:
	qdev = vfio_ap_get_queue_dev(AP_MKQID(apid, apqi));
> +			if (!q)

Replace with:
	if (!qdev)

> +				goto error;

Add:
	q = dev_get_drvdata(qdev);

> +			q->matrix = matrix_mdev;
> +			vfio_ap_put_queue(q);


Replace with:
	put_device(qdev);



> +		}
> +	}
> +	return 0;
> +error:
> +	pr_err("%s: no queue for %02lx.%04lx\n", __func__, apid, apqi);
> +	vfio_ap_dissociate_queues(matrix_mdev);
> +	return -ENODEV;
> +}
>   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>   {
>   	int ret;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 081f0d7..10bc8b5 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -89,5 +89,6 @@ extern void vfio_ap_mdev_unregister(void);
>   struct vfio_ap_queue {
>   	struct device *dev;
>   	int	apqn;
> +	struct ap_matrix_mdev *matrix;

Can you rename this variable matrix_mdev?
* That is how struct ap_matrix_mdev vars are named everywhere else
* It prevents confusing it with naming of vars for struct ap_matrix.

>   };
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 

Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> To be able to use the VFIO interface to facilitate the
> mediated device memory pining/unpining we need to register
> a notifier for IOMMU.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c     | 64 +++++++++++++++++++++++++++++++----
>   drivers/s390/crypto/vfio_ap_private.h |  2 ++
>   2 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 1851b24..6eddc2c 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -781,6 +781,36 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
>   };
>   
>   /**
> + * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
> + *
> + * @nb: The notifier block
> + * @action: Action to be taken (VFIO_IOMMU_NOTIFY_DMA_UNMAP)
> + * @data: the specific unmap structure for vfio_iommu_type1
> + *
> + * Unpins the guest IOVA. (The NIB guest address we pinned before).
> + * Return NOTIFY_OK after unpining on a UNMAP request.
> + * otherwise, returns NOTIFY_DONE .
> + */
> +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;
> +}
> +
> +
> +/**
>    * vfio_ap_mdev_set_kvm
>    *
>    * @matrix_mdev: a mediated matrix device
> @@ -904,8 +934,7 @@ static void vfio_ap_dissociate_queues(struct ap_matrix_mdev *matrix_mdev)
>    * In the case a queue could not be found return -ENODEV.
>    * Otherwise return 0.
>    */
> -static __attribute__((unused))
> -	int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
> +static int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)

Maybe this function should be introduced in this patch instead?

>   {
>   	unsigned long apid, apqi;
>   	struct vfio_ap_queue *q;
> @@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>   
>   	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>   				     &events, &matrix_mdev->group_notifier);
> -	if (ret) {
> -		module_put(THIS_MODULE);
> -		return ret;
> -	}
> +	if (ret)
> +		goto err_group;
> +
> +	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)
> +		goto err_iommu;
> +
> +	ret = vfio_ap_associate_queues(matrix_mdev);
> +	if (ret)
> +		goto err_associate;

I think the matrix_mdev should be associated with queues when an 
assignment of an adapter or domain is made to the mdev device via its
sysfs interfaces. I say this because assigning an adapter or domain to
an mdev device effectively grants ownership of any additional AP queues 
added to the mdev device's AP matrix as a result of the assignment. It
only makes sense to assign ownership to the vfio_ap_queue objects
representing the queues at that time. If an adapter or domain is
dynamically assigned while a guest is using the affected queues, then
the associations will have to be made at that time and this code will
likely go bye bye.

>   
>   	return 0;
> +
> +err_associate:
> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> +				 &matrix_mdev->iommu_notifier);
> +err_iommu:
> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> +				 &matrix_mdev->group_notifier);
> +err_group:
> +	module_put(THIS_MODULE);
> +	return ret;
>   }
>   
>   static void vfio_ap_mdev_release(struct mdev_device *mdev)
> @@ -985,6 +1034,9 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>   	vfio_ap_mdev_reset_queues(mdev);
>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>   				 &matrix_mdev->group_notifier);
> +	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
> +				 &matrix_mdev->iommu_notifier);
> +	vfio_ap_dissociate_queues(matrix_mdev);

I think the matrix_mdev should be dissociated from queues when an
adapter or domain is unassigned from the mdev device via its
sysfs interfaces. I say this because unassigning an adapter or domain 
from an mdev device effectively takes away ownership of any AP queues
removed from the mdev device's AP matrix as a result of the
unassignment. It only makes sense to remove ownership from the
vfio_ap_queue objects representing the queues at that time. This will
become necessary for the forthcoming dynamic configuration patches.
If an adapter or domain is dynamically unassigned while a guest is
using the affected queues, then the dissociation will have to be made
at that time and this code will likely go bye bye.

>   	matrix_mdev->kvm = NULL;
>   	module_put(THIS_MODULE);
>   }
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 10bc8b5..2781720 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -80,7 +80,9 @@ 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 mdev_device *mdev;
>   };
>   
>   extern int vfio_ap_mdev_register(void);
> 

Re: [PATCH v3 7/9] s390: ap: implement PAPQ AQIC interception in kernel

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> 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_ops.c     | 191 +++++++++++++++++++++++++++++++++-
>   drivers/s390/crypto/vfio_ap_private.h |   2 +
>   3 files changed, 191 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
> index bfc66e4..323f2aa 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_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6eddc2c..5664cf3 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -77,6 +77,28 @@ static void vfio_ap_put_queue(struct vfio_ap_queue *q)
>   	q->dev = NULL;
>   }
>   
> +/**
> + * vfio_ap_free_irq:
> + * @q: The vfio_ap_queue
> + *
> + * Unpin the guest NIB
> + * Unregister the ISC from the GIB alert
> + * Clear the vfio_ap_queue intern fields
> + */
> +static void vfio_ap_free_irq(struct vfio_ap_queue *q)
> +{
> +	unsigned long pfn = q->nib >> PAGE_SHIFT;
> +
> +	if (!q)
> +		return;
> +	if (q->nib)
> +		vfio_unpin_pages(mdev_dev(q->matrix->mdev), &pfn, 1);
> +	if (q->isc)
> +		kvm_s390_gisc_unregister(q->matrix->kvm, q->isc);
> +	q->nib = 0;
> +	q->isc = 0;
> +}
> +
>   static void vfio_ap_matrix_init(struct ap_config_info *info,
>   				struct ap_matrix *matrix)
>   {
> @@ -98,6 +120,7 @@ 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);
>   	mutex_lock(&matrix_dev->lock);
> @@ -781,6 +804,156 @@ static const struct attribute_group *vfio_ap_mdev_attr_groups[] = {
>   };
>   
>   /**
> + * 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;
> +
> +	status = ap_aqic(q->apqn, aqic_gisa, NULL);
> +	if (!status.response_code)
> +		vfio_ap_free_irq(q);
> +
> +	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 g_pfn, h_nib, h_pfn;
> +	int ret;
> +
> +	kvm = q->matrix->kvm;
> +	gisa = kvm->arch.gisa_int.origin;
> +
> +	g_pfn = q->nib >> PAGE_SHIFT;
> +	ret = vfio_pin_pages(mdev_dev(q->matrix->mdev), &g_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;
> +	}
> +
> +	h_nib = (h_pfn << PAGE_SHIFT) | (q->nib & ~PAGE_MASK);
> +	aqic_gisa.gisc = q->isc;
> +	aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->isc);
> +	aqic_gisa.ir = 1;
> +	aqic_gisa.gisa = gisa->next_alert >> 4;
> +
> +	status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
> +	if (status.response_code == AP_RESPONSE_INVALID_GISA) {
> +		status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
> +		pr_warn("vfio_ap: apqn %02x.%04x: AP_RESPONSE_INVALID_GISA\n",
> +			(q->apqn >> 8) & 0xff, q->apqn & 0xff);
> +	}
> +
> +	if (status.response_code)
> +		vfio_ap_free_irq(q);
> +
> +	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.
> + *
> + * Return 0 if we could handle the request inside KVM.
> + * otherwise, returns -EOPNOTSUPP to let QEMU handle the fault.
> + */

This function be nothing more than a switch statement for the
function code sent with the PQAP instruction. Each case should
be a call to a appropriate PQAP function handler. This will make
it much easier to add additional handlers for the 6 other
PQAP functions if necessary at some time down the road.

> +static int handle_pqap(struct kvm_vcpu *vcpu)
> +{	int ret.
	uint8_t fc;

	fc = vcpu->run->s.regs.gprs[0] >> 24;
	switch(fc) {
	case 0x03:
		ret = handle_pqap_aqic(vcpu);
	default:
		ret = -EOPNOTSUPP;
	}

	return ret;
}

static int handle_pqap_aqic(struct kvm_vcpu *vcpu) {
> +	uint64_t status;
> +	uint16_t apqn;
	struct device *qdev;
> +	struct vfio_ap_queue *q;
> +	struct ap_queue_status qstatus = {};
> +	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;
> +	q = vfio_ap_get_queue(apqn);

Replace with:
	qdev = vfio_ap_get_queue_dev(apqn);

> +	if (!q) {

Replace with:
	if (!qdev)

> +		qstatus.response_code = AP_RESPONSE_Q_NOT_AVAIL;
> +		goto out;
> +	}
> +
> +	/* Check if the queue is associated with a guest matrix */

Add this:
	q = dev_get_drvdata(qdev);

> +	matrix_mdev = q->matrix;
> +	if (!matrix_mdev || !matrix_mdev->kvm) {
> +		qstatus.response_code = AP_RESPONSE_DECONFIGURED

> +		goto out;
> +	}
> +
> +	status = vcpu->run->s.regs.gprs[1];
> +
> +	/* If IR bit(16) is set we enable the interrupt */
> +	if ((status >> (63 - 16)) & 0x01) {
> +		q->isc = status & 0x07;
> +		q->nib = vcpu->run->s.regs.gprs[2];
> +		qstatus = vfio_ap_setirq(q);
> +		if (qstatus.response_code) {
> +			q->nib = 0;
> +			q->isc = 0;
> +		}
> +	} else
> +		qstatus = vfio_ap_clrirq(q);
> +
> +out:
> +	memcpy(&vcpu->run->s.regs.gprs[1], &qstatus, sizeof(qstatus));
> +	vfio_ap_put_queue(q);

Replace with:
	put_device(qdev);

Can we just do a put_device(qdev) right after we get it since we only
need it to get the drvdata, or do we need to wait until the other work
is done?

> +	return 0;
> +}
> +
> + /*
>    * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
>    *
>    * @nb: The notifier block
> @@ -802,7 +975,9 @@ static int vfio_ap_mdev_iommu_notifier(struct notifier_block *nb,
>   		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);
> +		if (matrix_mdev->mdev)
> +			vfio_unpin_pages(mdev_dev(matrix_mdev->mdev),
> +					 &g_pfn, 1);
>   		return NOTIFY_OK;
>   	}
>   
> @@ -877,11 +1052,15 @@ static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>   				    unsigned int retry)
>   {
>   	struct ap_queue_status status;
> +	struct vfio_ap_queue *q;
> +
> +	q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
>   
>   	do {
>   		status = ap_zapq(AP_MKQID(apid, apqi));
>   		switch (status.response_code) {
>   		case AP_RESPONSE_NORMAL:
> +			vfio_ap_free_irq(q);
>   			return 0;
>   		case AP_RESPONSE_RESET_IN_PROGRESS:
>   		case AP_RESPONSE_BUSY:
> @@ -999,6 +1178,11 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>   	if (ret)
>   		goto err_group;
>   
> +	if (!matrix_mdev->kvm) {
> +		ret = -ENODEV;
> +		goto err_iommu;
> +	}
> +
>   	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
>   	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>   
> @@ -1011,6 +1195,7 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>   	if (ret)
>   		goto err_associate;
>   
> +	matrix_mdev->kvm->arch.crypto.pqap_hook = handle_pqap;
>   	return 0;
>   
>   err_associate:
> @@ -1031,12 +1216,12 @@ static void vfio_ap_mdev_release(struct mdev_device *mdev)
>   	if (matrix_mdev->kvm)
>   		kvm_arch_crypto_clear_masks(matrix_mdev->kvm);
>   
> -	vfio_ap_mdev_reset_queues(mdev);
> +	matrix_mdev->kvm->arch.crypto.pqap_hook = NULL;
> +	vfio_ap_dissociate_queues(matrix_mdev);
>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>   				 &matrix_mdev->group_notifier);
>   	vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>   				 &matrix_mdev->iommu_notifier);
> -	vfio_ap_dissociate_queues(matrix_mdev);
>   	matrix_mdev->kvm = NULL;
>   	module_put(THIS_MODULE);
>   }
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 2781720..16c99e9 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -92,5 +92,7 @@ struct vfio_ap_queue {
>   	struct device *dev;
>   	int	apqn;
>   	struct ap_matrix_mdev *matrix;
> +	unsigned long nib;
> +	unsigned char isc;

>   };
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 

Re: [PATCH v3 8/9] s390: ap: Cleanup on removing the AP device

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> When the device is remove, we must make sure to
> clear the interruption and reset the AP device.
> 
> We also need to clear the CRYCB of the guest.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 92 +++++++++++++++++++++++++++++++++++
>   drivers/s390/crypto/vfio_ap_ops.c     |  2 +-
>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>   3 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 03153e6..50428be 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>
> @@ -12,6 +13,8 @@
>   #include <linux/slab.h>
>   #include <linux/string.h>
>   #include <asm/facility.h>
> +#include <linux/bitops.h>
> +#include <linux/kvm_host.h>
>   #include "vfio_ap_private.h"
>   
>   #define VFIO_AP_ROOT_NAME "vfio_ap"
> @@ -64,6 +67,88 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   	return 0;
>   }
>   
> +/*
> + * vfio_ap_drain_queue
> + * @q: the queue to drain
> + *
> + * This function waits until the queue is empty.
> + */
> +static void vfio_ap_drain_queue(struct vfio_ap_queue *q)
> +{
> +	struct ap_queue_status status;
> +	int retry = 20;
> +
> +	status = ap_tapq(q->apqn, NULL);
> +	while (!status.queue_empty && retry--)  {
> +		msleep(200);
> +		status = ap_tapq(q->apqn, NULL);
> +	}
> +	if (retry <= 0) {
> +		pr_warn("%s: queue not empty after zapq on apqn 0x%04x\n",
> +			__func__, q->apqn);
> +	}
> +}
> +
> +/*
> + * vfio_ap_zapq
> + * @q: The queue to zerro
> + *
> + * It is best effort, no return value is done, however on success
> + * we will drain the queue before getting the queue back to the
> + * AP bus.
> + */
> +static void vfio_ap_zapq(struct vfio_ap_queue *q)
> +{
> +	struct ap_queue_status status;
> +	int retry = 20;
> +
> +	do {
> +		status = ap_zapq(q->apqn);
> +		switch (status.response_code) {
> +		case AP_RESPONSE_RESET_IN_PROGRESS:
> +		case AP_RESPONSE_BUSY:
> +			msleep(20);
> +			break;
> +		default:
> +			pr_warn("%s: zapq error %02x on apqn 0x%04x\n",
> +				__func__, status.response_code, q->apqn);
> +			return;
> +		case AP_RESPONSE_NORMAL:
> +			vfio_ap_drain_queue(q);

I don't think this is necessary. The zeroize is performed on
each AP-queue entry in an AP queue. My understanding is that when a
reset or zeroize is pending, any AP instructions subsequently issued
are rejected with condition-code 3 indicating an AP queue reset is in
progress. It is also my understanding that once the AP commands 
currently executing in a given AP queue entry complete, the queue
entry will be zeroized. So it seems to me that there is no need to
"drain" the queue, it will have already been done by the zeroize.

If you agree we don't need to "drain" the queue, then I'd rather just
make the zapq function in the vfio_ap_ops.c non-static and make it
available to the driver. There is no sense in duplicating this code. In
fact, even if you keep the draining function, you still don't need to
duplicate a zaapq instruction here, you can just call the vfio_ap_ops.c
version and then drain the queue on AP_RESPONSE_NORMAL.


> +			return;
> +		}
> +	} while (retry--);
> +	pr_warn("%s: zapq retry count exceeded code %02x on apqn 0x%04x\n",
> +		__func__, status.response_code, q->apqn);
> +}
> +
> +/**
> + * vfio_ap_update_crycb
> + * @q: A pointer to the queue being removed
> + *
> + * We clear the APID of the queue, making this queue unusable for the guest.
> + * After this function we can reset the queue without to fear a race with
> + * the guest to access the queue again.
> + * We do not fear race with the host as we still get the device.
> + */
> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = q->matrix;
> +
> +	if (!matrix_mdev)
> +		return;
> +
> +	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
> +
> +	if (!matrix_mdev->kvm)
> +		return;
> +
> +	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
> +				  matrix_mdev->matrix.apm,
> +				  matrix_mdev->matrix.aqm,
> +				  matrix_mdev->matrix.adm);
> +}
> +
>   /**
>    * vfio_ap_queue_dev_remove:
>    *
> @@ -74,6 +159,13 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>   	struct vfio_ap_queue *q;
>   
>   	q = dev_get_drvdata(&apdev->device);
> +	if (!q)
> +		return;
> +
> +	vfio_ap_update_crycb(q);
> +	vfio_ap_zapq(q);
> +
> +	vfio_ap_free_irq(q);

If you make the zapq function in vfio_ap_ops.c available to the driver
rather than duplicating it in this file, you won't need this call
to vfio_ap_free_irq because it is done as part of the zapq in
vfio_ap_ops.c.

>   	kfree(q);
>   }
>   
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 5664cf3..7ec957c 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -85,7 +85,7 @@ static void vfio_ap_put_queue(struct vfio_ap_queue *q)
>    * Unregister the ISC from the GIB alert
>    * Clear the vfio_ap_queue intern fields
>    */
> -static void vfio_ap_free_irq(struct vfio_ap_queue *q)
> +void vfio_ap_free_irq(struct vfio_ap_queue *q)
>   {
>   	unsigned long pfn = q->nib >> PAGE_SHIFT;
>   
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 16c99e9..d108c6d 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
>    */
> @@ -95,4 +96,5 @@ struct vfio_ap_queue {
>   	unsigned long nib;
>   	unsigned char isc;
>   };
> +void vfio_ap_free_irq(struct vfio_ap_queue *q);
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 

Re: [PATCH v3 8/9] s390: ap: Cleanup on removing the AP device

[Tony Krowiak]

On 2/14/19 8:51 AM, Pierre Morel wrote:
> When the device is remove, we must make sure to
> clear the interruption and reset the AP device.
> 
> We also need to clear the CRYCB of the guest.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 92 +++++++++++++++++++++++++++++++++++
>   drivers/s390/crypto/vfio_ap_ops.c     |  2 +-
>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>   3 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 03153e6..50428be 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>
> @@ -12,6 +13,8 @@
>   #include <linux/slab.h>
>   #include <linux/string.h>
>   #include <asm/facility.h>
> +#include <linux/bitops.h>
> +#include <linux/kvm_host.h>
>   #include "vfio_ap_private.h"
>   
>   #define VFIO_AP_ROOT_NAME "vfio_ap"
> @@ -64,6 +67,88 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
>   	return 0;
>   }
>   
> +/*
> + * vfio_ap_drain_queue
> + * @q: the queue to drain
> + *
> + * This function waits until the queue is empty.
> + */
> +static void vfio_ap_drain_queue(struct vfio_ap_queue *q)
> +{
> +	struct ap_queue_status status;
> +	int retry = 20;
> +
> +	status = ap_tapq(q->apqn, NULL);
> +	while (!status.queue_empty && retry--)  {
> +		msleep(200);
> +		status = ap_tapq(q->apqn, NULL);
> +	}
> +	if (retry <= 0) {
> +		pr_warn("%s: queue not empty after zapq on apqn 0x%04x\n",
> +			__func__, q->apqn);
> +	}
> +}
> +
> +/*
> + * vfio_ap_zapq
> + * @q: The queue to zerro
> + *
> + * It is best effort, no return value is done, however on success
> + * we will drain the queue before getting the queue back to the
> + * AP bus.
> + */
> +static void vfio_ap_zapq(struct vfio_ap_queue *q)
> +{
> +	struct ap_queue_status status;
> +	int retry = 20;
> +
> +	do {
> +		status = ap_zapq(q->apqn);
> +		switch (status.response_code) {
> +		case AP_RESPONSE_RESET_IN_PROGRESS:
> +		case AP_RESPONSE_BUSY:
> +			msleep(20);
> +			break;
> +		default:
> +			pr_warn("%s: zapq error %02x on apqn 0x%04x\n",
> +				__func__, status.response_code, q->apqn);
> +			return;
> +		case AP_RESPONSE_NORMAL:
> +			vfio_ap_drain_queue(q);
> +			return;
> +		}
> +	} while (retry--);
> +	pr_warn("%s: zapq retry count exceeded code %02x on apqn 0x%04x\n",
> +		__func__, status.response_code, q->apqn);
> +}
> +
> +/**
> + * vfio_ap_update_crycb
> + * @q: A pointer to the queue being removed
> + *
> + * We clear the APID of the queue, making this queue unusable for the guest.
> + * After this function we can reset the queue without to fear a race with
> + * the guest to access the queue again.
> + * We do not fear race with the host as we still get the device.
> + */
> +static void vfio_ap_update_crycb(struct vfio_ap_queue *q)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = q->matrix;
> +
> +	if (!matrix_mdev)
> +		return;
> +
> +	clear_bit_inv(AP_QID_CARD(q->apqn), matrix_mdev->matrix.apm);
> +
> +	if (!matrix_mdev->kvm)
> +		return;
> +
> +	kvm_arch_crypto_set_masks(matrix_mdev->kvm,
> +				  matrix_mdev->matrix.apm,
> +				  matrix_mdev->matrix.aqm,
> +				  matrix_mdev->matrix.adm);
> +}
> +
>   /**
>    * vfio_ap_queue_dev_remove:
>    *
> @@ -74,6 +159,13 @@ static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
>   	struct vfio_ap_queue *q;
>   
>   	q = dev_get_drvdata(&apdev->device);
> +	if (!q)
> +		return;
> +
> +	vfio_ap_update_crycb(q);

The root user is warned in the Limitations section of the vfio-ap.txt
doc delivered with the AP pass-through support warns that the
administrator (i.e., root user) should ensure that AP devices are not
removed without taking proper care to ensure they are not in use by a
guest. I am currently working on a patch set to handle this, so this
may simply get ripped out when those patches are integrated. That may
very well be simultaneously with this patch series as I plan on posting
those soon.

If this call is to remain, then you ought to update the vfio-ap.txt
document to let users know that when queues are unbound, the guests
will lose access to them unbeknown to the admin of the guest.

> +	vfio_ap_zapq(q);

One last thing. I've explained before that prior to the AP bus
invoking this remove callback, it flushes and zeroizes the
queue. Why do you insist it needs to be done again in the remove
callback?

> +
> +	vfio_ap_free_irq(q);
>   	kfree(q);
>   }
>   
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 5664cf3..7ec957c 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -85,7 +85,7 @@ static void vfio_ap_put_queue(struct vfio_ap_queue *q)
>    * Unregister the ISC from the GIB alert
>    * Clear the vfio_ap_queue intern fields
>    */
> -static void vfio_ap_free_irq(struct vfio_ap_queue *q)
> +void vfio_ap_free_irq(struct vfio_ap_queue *q)
>   {
>   	unsigned long pfn = q->nib >> PAGE_SHIFT;
>   
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 16c99e9..d108c6d 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
>    */
> @@ -95,4 +96,5 @@ struct vfio_ap_queue {
>   	unsigned long nib;
>   	unsigned char isc;
>   };
> +void vfio_ap_free_irq(struct vfio_ap_queue *q);
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Fri, 15 Feb 2019 16:59:33 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 2/15/19 4:11 AM, Cornelia Huck wrote:
> > On Thu, 14 Feb 2019 13:30:59 -0500
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >   
> >> On 2/14/19 12:36 PM, Pierre Morel wrote:  
> >>> On 14/02/2019 17:57, Cornelia Huck wrote:  

> >>>> (And reading further in the current code, it seems we clear that
> >>>> structure _after_ the matrix device had been setup, so how can that
> >>>> even work? Where am I confused?)  
> >>>
> >>> On device_register there were no bus, so the core just do not look for a
> >>> driver and this field was nor tested nor overwritten.  
> > 
> > Hm... so has the callback in driver_for_each_device() in
> > vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> > patch fixes more than just libudev issues...  
> 
> It is this patch that rendered the driver_for_each_device() in
> vfio_ap_verify_queue_reserved() erroneous. That function gets called
> every time an adapter or domain is assigned to the mdev. This patch
> introduced the problem with driver_for_each_device().

So, does this function need to be removed or called from another place,
then? (It looks like it was dead code before.)

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Cornelia Huck]

On Fri, 15 Feb 2019 17:13:21 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 2/14/19 8:51 AM, Pierre Morel wrote:
> > We need to find the queue with a specific APQN during the
> > handling of the interception of the PQAP/AQIC instruction.
> > 
> > To handle the AP associated device reference count we keep
> > track of it in the vfio_ap_queue until we put the device.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
> >   drivers/s390/crypto/vfio_ap_private.h |  1 +
> >   2 files changed, 55 insertions(+)

> > +/**
> > + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
> > + * @apqn: The queue APQN
> > + *
> > + * Retrieve a queue with a specific APQN from the list of the
> > + * devices associated to the vfio_ap_driver.
> > + *
> > + * The vfio_ap_queue has been already associated with the device
> > + * during the probe.
> > + * Store the associated device for reference counting
> > + *
> > + * Returns the pointer to the associated vfio_ap_queue
> > + */
> > +static  __attribute__((unused))
> > +	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)  
> 
> I think you should change this signature for the reasons I stated
> below:
> 
> struct device *vfio_ap_get_queue_dev(int apqn)
> 
> > +{
> > +	struct device *dev;
> > +	struct vfio_ap_queue *q;
> > +
> > +	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
> > +				 vfio_ap_check_apqn);
> > +	if (!dev)
> > +		return NULL;
> > +	q = dev_get_drvdata(dev);
> > +	q->dev = dev;  
> 
> Why store the device with the vfio_ap_queue object? Why not just return
> the device. The caller can get the vfio_ap_queue from the device's
> driver data. It seems the only reason for the 'dev' field is to
> temporarily hold a ref to the device so it can be put later. Why not
> just put the device.

Having looked at the remainder of the patches, I tend to agree that we
don't really need the backlink; we walk the driver's list of devices in
any case IIUC.

We *might* want a mechanism to grab the queue quickly (i.e. without
walking the list) if there's anything performance sensitive in there;
but from the patch descriptions, I don't think anything is done in a
hot path, so it should be fine.

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Tony Krowiak]

On 2/18/19 7:01 AM, Cornelia Huck wrote:
> On Fri, 15 Feb 2019 16:59:33 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/15/19 4:11 AM, Cornelia Huck wrote:
>>> On Thu, 14 Feb 2019 13:30:59 -0500
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>    
>>>> On 2/14/19 12:36 PM, Pierre Morel wrote:
>>>>> On 14/02/2019 17:57, Cornelia Huck wrote:
> 
>>>>>> (And reading further in the current code, it seems we clear that
>>>>>> structure _after_ the matrix device had been setup, so how can that
>>>>>> even work? Where am I confused?)
>>>>>
>>>>> On device_register there were no bus, so the core just do not look for a
>>>>> driver and this field was nor tested nor overwritten.
>>>
>>> Hm... so has the callback in driver_for_each_device() in
>>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
>>> patch fixes more than just libudev issues...
>>
>> It is this patch that rendered the driver_for_each_device() in
>> vfio_ap_verify_queue_reserved() erroneous. That function gets called
>> every time an adapter or domain is assigned to the mdev. This patch
>> introduced the problem with driver_for_each_device().
> 
> So, does this function need to be removed or called from another place,
> then? (It looks like it was dead code before.)

I don't see why you think it's dead code:

assign_adapter_store
==> vfio_ap_mdev_verify_queues_reserved_for_apid
     ==> vfio_ap_verify_queue_reserved
         ==> driver_for_each_device

The only way that the vfio_ap_verify_queue_reserved - the function that
calls driver_for_each_device - does not get called is if no bits have
yet been set in matrix_mdev->matrix.aqm.

> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Mon, 18 Feb 2019 11:35:45 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 2/18/19 7:01 AM, Cornelia Huck wrote:
> > On Fri, 15 Feb 2019 16:59:33 -0500
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >   
> >> On 2/15/19 4:11 AM, Cornelia Huck wrote:  
> >>> On Thu, 14 Feb 2019 13:30:59 -0500
> >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >>>      
> >>>> On 2/14/19 12:36 PM, Pierre Morel wrote:  
> >>>>> On 14/02/2019 17:57, Cornelia Huck wrote:  
> >   
> >>>>>> (And reading further in the current code, it seems we clear that
> >>>>>> structure _after_ the matrix device had been setup, so how can that
> >>>>>> even work? Where am I confused?)  
> >>>>>
> >>>>> On device_register there were no bus, so the core just do not look for a
> >>>>> driver and this field was nor tested nor overwritten.  
> >>>
> >>> Hm... so has the callback in driver_for_each_device() in
> >>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> >>> patch fixes more than just libudev issues...  
> >>
> >> It is this patch that rendered the driver_for_each_device() in
> >> vfio_ap_verify_queue_reserved() erroneous. That function gets called
> >> every time an adapter or domain is assigned to the mdev. This patch
> >> introduced the problem with driver_for_each_device().  
> > 
> > So, does this function need to be removed or called from another place,
> > then? (It looks like it was dead code before.)  
> 
> I don't see why you think it's dead code:
> 
> assign_adapter_store
> ==> vfio_ap_mdev_verify_queues_reserved_for_apid
>      ==> vfio_ap_verify_queue_reserved
>          ==> driver_for_each_device  
> 
> The only way that the vfio_ap_verify_queue_reserved - the function that
> calls driver_for_each_device - does not get called is if no bits have
> yet been set in matrix_mdev->matrix.aqm.

What I don't see is how this can be called if no device has been, in
fact, bound to the driver in the driver core...

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

On 15/02/2019 23:02, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> This patch adds interception code for the PQAP instructions,
>> and a callback inside the KVM arch structure for s390.
>>
>> If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ
>> instructions, the driver will initialize the callback inside
>> the kvm_arch structure to be called when the interception of a
>> PQAP instruction occurs.
>>
>> If the callback is not initialized, the code still returns
>> -EOPNOTSUPP to let userland handle the instruction as it used to.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h |  1 +
>>   arch/s390/kvm/priv.c             | 50 
>> ++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h 
>> b/arch/s390/include/asm/kvm_host.h
>> index c5f5156..49cc8b0 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
>>   struct kvm_s390_crypto {
>>       struct kvm_s390_crypto_cb *crycb;
>> +    int (*pqap_hook)(struct kvm_vcpu *vcpu);
>>       __u32 crycbd;
>>       __u8 aes_kw;
>>       __u8 dea_kw;
>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
>> index 8679bd7..72fdc21 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,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
>>       }
>>   }
>> +/*
>> + * handle_pqap: Handling pqap interception
>> + * @vcpu: the vcpu having issue the pqap instruction
>> + *
>> + * This callback only handles PQAP/AQIC instruction and
>> + * calls a dedicated callback for this instruction if
>> + * a driver did register one in the CRYPTO satellite of the
>> + * SIE block.
>> + *
>> + * Do not change the behavior if, return -EOPNOTSUPP if:
>> + * - the hook is not used do not change the behavior.
>> + * - AP instructions are not available or not available to the guest
>> + * - the instruction is not PQAP with function code indicating
>> + *   AQIC do not change the previous behavior.
>> + *
>> + * For PQAP/AQIC instruction, verify privilege and specifications
>> + *
>> + * return the value returned by the callback.
>> + */
>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>> +{
>> +    uint8_t fc;
>> +
>> +    /* Verify that the hook callback is registered */
>> +    if (!vcpu->kvm->arch.crypto.pqap_hook)
>> +        return -EOPNOTSUPP;
>> +    /* 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;
>> +    /* Verify that the function code is AQIC */
>> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
>> +    if (fc != 0x03)
>> +        return -EOPNOTSUPP;
> 
> This does not belong here. Function code 3 is one of 7 function codes
> that can be sent with the PQAP instruction. This belongs in the PQAP
> hook code.

On one hand, effectively I would prefer to put the code in the VFIO 
driver code.
On the other hand, doing this would lead to export the code for 
test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

> 
>> +
>> +    /* PQAP instructions are allowed for guest kernel only */
>> +    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
>> +        return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
>> +    /* AQIC instruction is allowed only if facility 65 is available */
>> +    if (!test_kvm_facility(vcpu->kvm, 65))
>> +        return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
>> +    /* All right, call the callback */
>> +    return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
>> +}
>> +
>>   static int handle_stfl(struct kvm_vcpu *vcpu)
>>   {
>>       int rc;
>> @@ -878,6 +926,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:
>>
> 


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

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Pierre Morel]

On 18/02/2019 13:21, Cornelia Huck wrote:
> On Fri, 15 Feb 2019 17:13:21 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>> We need to find the queue with a specific APQN during the
>>> handling of the interception of the PQAP/AQIC instruction.
>>>
>>> To handle the AP associated device reference count we keep
>>> track of it in the vfio_ap_queue until we put the device.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
>>>    drivers/s390/crypto/vfio_ap_private.h |  1 +
>>>    2 files changed, 55 insertions(+)
> 
>>> +/**
>>> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
>>> + * @apqn: The queue APQN
>>> + *
>>> + * Retrieve a queue with a specific APQN from the list of the
>>> + * devices associated to the vfio_ap_driver.
>>> + *
>>> + * The vfio_ap_queue has been already associated with the device
>>> + * during the probe.
>>> + * Store the associated device for reference counting
>>> + *
>>> + * Returns the pointer to the associated vfio_ap_queue
>>> + */
>>> +static  __attribute__((unused))
>>> +	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
>>
>> I think you should change this signature for the reasons I stated
>> below:
>>
>> struct device *vfio_ap_get_queue_dev(int apqn)
>>
>>> +{
>>> +	struct device *dev;
>>> +	struct vfio_ap_queue *q;
>>> +
>>> +	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
>>> +				 vfio_ap_check_apqn);
>>> +	if (!dev)
>>> +		return NULL;
>>> +	q = dev_get_drvdata(dev);
>>> +	q->dev = dev;
>>
>> Why store the device with the vfio_ap_queue object? Why not just return
>> the device. The caller can get the vfio_ap_queue from the device's
>> driver data. It seems the only reason for the 'dev' field is to
>> temporarily hold a ref to the device so it can be put later. Why not
>> just put the device.
> 
> Having looked at the remainder of the patches, I tend to agree that we
> don't really need the backlink; we walk the driver's list of devices in
> any case IIUC.
> 
> We *might* want a mechanism to grab the queue quickly (i.e. without
> walking the list) if there's anything performance sensitive in there;
> but from the patch descriptions, I don't think anything is done in a
> hot path, so it should be fine.
> 

OK you are right, I ll drop it


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

Re: [PATCH v3 5/9] s390: ap: tools to associate a queue to a matrix

[Pierre Morel]

On 15/02/2019 23:30, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> We need tools to associate a queue and a matrix to be able
>> later to find the matrix associated with the queue for which
>> we got the APQN in the register 1 during a PQAP/AQIC instruction
>> interception.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c     | 66 
>> +++++++++++++++++++++++++++++++++--
>>   drivers/s390/crypto/vfio_ap_private.h |  1 +
>>   2 files changed, 64 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 2a52c9b..1851b24 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -50,8 +50,7 @@ static int vfio_ap_check_apqn(struct device *dev, 
>> void *data)
>>    *
>>    * Returns the pointer to the associated vfio_ap_queue
>>    */
>> -static  __attribute__((unused))
>> -    struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
> 
> I don't like this, it indicates to me that this and the previous
> patch should have been squashed together. I realize your intent
> was to make the patches smaller, but I don't think this makes it
> any easier to review them. In fact, it makes it harder IMHO, because
> you have to flip back and forth between patches to fully comprehend
> the usage.
> 
>> +static struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
>>   {
>>       struct device *dev;
>>       struct vfio_ap_queue *q;
>> @@ -72,7 +71,7 @@ static  __attribute__((unused))
>>    * put the associated device
>>    *
>>    */
>> -static  __attribute__((unused)) void vfio_ap_put_queue(struct 
>> vfio_ap_queue *q)
> 
> Ditto for this.
> 
>> +static void vfio_ap_put_queue(struct vfio_ap_queue *q)
>>   {
>>       put_device(q->dev);
>>       q->dev = NULL;
>> @@ -867,6 +866,67 @@ static int vfio_ap_mdev_reset_queue(unsigned int 
>> apid, unsigned int apqi,
>>       return -EBUSY;
>>   }
>> +/**
>> + * vfio_ap_dissociate_queues: Dissociate a matrix mediated device to 
>> a queue
>> + *
>> + * Go through all bits in the AQM and APM and dissociate the queue
>> + * from the matrix device.
>> + *
>> + * No return value we are throwing the mediated device anyway.
>> + */
>> +static void vfio_ap_dissociate_queues(struct ap_matrix_mdev 
>> *matrix_mdev)
>> +{
>> +    unsigned long apid, apqi;
> 
> The changes I recommend here are based on the comments in my
> response to patch 4.
> 
>> +    struct vfio_ap_queue *q;
> 
> Add this:
> 
>      struct device *qdev;
> 
>> +
>> +    for_each_set_bit_inv(apid, matrix_mdev->matrix.apm,
>> +                 matrix_mdev->matrix.apm_max + 1) {
>> +        for_each_set_bit_inv(apqi, matrix_mdev->matrix.aqm,
>> +                     matrix_mdev->matrix.aqm_max + 1) {
>> +            q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
> 
> Replace with:
>      qdev = vfio_ap_get_queue_dev(AP_MKQID(apid, apqi);
> 
>> +            if (q) {
> 
> Replace with:
>      if (qdev) {
> 
> Add this:
>      q = dev_get_drvdata(qdev);
> 
>> +                vfio_ap_mdev_reset_queue(apid, apqi, 1);
>> +                q->matrix = NULL;
>> +                vfio_ap_put_queue(q);
> 
> Replace with:
>      put_device(qdev)
> 
>> +            } else
>> +                pr_err("%s: no queue for %02lx.%04lx\n",
>> +                       __func__, apid, apqi);
>> +        }
>> +    }
>> +}
>> +
>> +/**
>> + * vfio_ap_associate_queues: Associate a matrix mediated device to a 
>> queue
>> + *
>> + * Go through all bits in the AQM and APM and calculate the APQN, and 
>> find
>> + * the matching queue to associate with the matrix device.
>> + *
>> + * In the case a queue could not be found return -ENODEV.
>> + * Otherwise return 0.
>> + */
>> +static __attribute__((unused))
> 
> This indicates this patch should be squashed with the patch that
> introduces this function. Why have a patch that introduces a function
> that is not used and put and artificially specify an attribute just to
> keep the compiler from issuing a warning?

Seems to be a majority for this so... will do.

> 
>> +    int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
>> +{
> 
> The same as above, the changes I recommend here are based on the
> comments in my response to patch 4.
> 
... will do

>> +    pr_err("%s: no queue for %02lx.%04lx\n", __func__, apid, apqi);
>> +    vfio_ap_dissociate_queues(matrix_mdev);
>> +    return -ENODEV;
>> +}
>>   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev)
>>   {
>>       int ret;
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h 
>> b/drivers/s390/crypto/vfio_ap_private.h
>> index 081f0d7..10bc8b5 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -89,5 +89,6 @@ extern void vfio_ap_mdev_unregister(void);
>>   struct vfio_ap_queue {
>>       struct device *dev;
>>       int    apqn;
>> +    struct ap_matrix_mdev *matrix;
> 
> Can you rename this variable matrix_mdev?
> * That is how struct ap_matrix_mdev vars are named everywhere else
> * It prevents confusing it with naming of vars for struct ap_matrix.

Sorry, I did this but it seems to have been forgotten in a rebase.
Will do.

> 
>>   };
>>   #endif /* _VFIO_AP_PRIVATE_H_ */
>>
> 


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

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Cornelia Huck]

On Mon, 18 Feb 2019 19:29:10 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 15/02/2019 23:02, Tony Krowiak wrote:
> > On 2/14/19 8:51 AM, Pierre Morel wrote:  

> >> +/*
> >> + * handle_pqap: Handling pqap interception
> >> + * @vcpu: the vcpu having issue the pqap instruction
> >> + *
> >> + * This callback only handles PQAP/AQIC instruction and
> >> + * calls a dedicated callback for this instruction if
> >> + * a driver did register one in the CRYPTO satellite of the
> >> + * SIE block.
> >> + *
> >> + * Do not change the behavior if, return -EOPNOTSUPP if:
> >> + * - the hook is not used do not change the behavior.
> >> + * - AP instructions are not available or not available to the guest
> >> + * - the instruction is not PQAP with function code indicating
> >> + *   AQIC do not change the previous behavior.
> >> + *
> >> + * For PQAP/AQIC instruction, verify privilege and specifications
> >> + *
> >> + * return the value returned by the callback.
> >> + */
> >> +static int handle_pqap(struct kvm_vcpu *vcpu)
> >> +{
> >> +    uint8_t fc;
> >> +
> >> +    /* Verify that the hook callback is registered */
> >> +    if (!vcpu->kvm->arch.crypto.pqap_hook)
> >> +        return -EOPNOTSUPP;
> >> +    /* 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;
> >> +    /* Verify that the function code is AQIC */
> >> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
> >> +    if (fc != 0x03)
> >> +        return -EOPNOTSUPP;  
> > 
> > This does not belong here. Function code 3 is one of 7 function codes
> > that can be sent with the PQAP instruction. This belongs in the PQAP
> > hook code.  
> 
> On one hand, effectively I would prefer to put the code in the VFIO 
> driver code.
> On the other hand, doing this would lead to export the code for 
> test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h
> 
> I choose not to export these functions from the KVM code.
> 
> Would like opinion from KVM maintainers?

Looking at this (and without access to the specification...), I think
the check for problem state makes sense in here (if this applies to all
PQAP functions equally, which seems likely). The check for the facility
makes more sense in the handler. You can probably still inject the
specification exception here if you use a clever return code.

Another option: Provide a way to register a callback per function code;
this allows you to still do the check here and extend it later for
other function codes (which will probably be indicated by another
facility).

> 
> >   
> >> +
> >> +    /* PQAP instructions are allowed for guest kernel only */
> >> +    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
> >> +        return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
> >> +    /* AQIC instruction is allowed only if facility 65 is available */
> >> +    if (!test_kvm_facility(vcpu->kvm, 65))
> >> +        return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> >> +    /* All right, call the callback */
> >> +    return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
> >> +}
> >> +
> >>   static int handle_stfl(struct kvm_vcpu *vcpu)
> >>   {
> >>       int rc;
> >> @@ -878,6 +926,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:
> >>  
> >   
> 
> 

Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Halil Pasic]

On Fri, 15 Feb 2019 17:55:35 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 2/14/19 8:51 AM, Pierre Morel wrote:
> > To be able to use the VFIO interface to facilitate the
> > mediated device memory pining/unpining we need to register
> > a notifier for IOMMU.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   drivers/s390/crypto/vfio_ap_ops.c     | 64 +++++++++++++++++++++++++++++++----
> >   drivers/s390/crypto/vfio_ap_private.h |  2 ++
> >   2 files changed, 60 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> > index 1851b24..6eddc2c 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c

[..]
  	struct vfio_ap_queue *q;
> > @@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
> >   
> >   	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
> >   				     &events, &matrix_mdev->group_notifier);
> > -	if (ret) {
> > -		module_put(THIS_MODULE);
> > -		return ret;
> > -	}
> > +	if (ret)
> > +		goto err_group;
> > +
> > +	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)
> > +		goto err_iommu;
> > +
> > +	ret = vfio_ap_associate_queues(matrix_mdev);
> > +	if (ret)
> > +		goto err_associate;
> 
> I think the matrix_mdev should be associated with queues when an 
> assignment of an adapter or domain is made to the mdev device via its
> sysfs interfaces. I say this because assigning an adapter or domain to
> an mdev device effectively grants ownership of any additional AP queues 
> added to the mdev device's AP matrix as a result of the assignment. It
> only makes sense to assign ownership to the vfio_ap_queue objects
> representing the queues at that time. If an adapter or domain is
> dynamically assigned while a guest is using the affected queues, then
> the associations will have to be made at that time and this code will
> likely go bye bye.
>


Actually, we need the stuff in vfio_ap_queues only if the guest is
offered to  decides to use AP adaper interrupts. I would prefer to have
it around only when needed: as long as we have the nib pinned for any
given queue. Is there a reason why not to do so? 

Regards,
Halil
 
[..]

Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Pierre Morel]

On 15/02/2019 23:55, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> To be able to use the VFIO interface to facilitate the
>> mediated device memory pining/unpining we need to register
>> a notifier for IOMMU.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c     | 64 
>> +++++++++++++++++++++++++++++++----
>>   drivers/s390/crypto/vfio_ap_private.h |  2 ++
>>   2 files changed, 60 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 1851b24..6eddc2c 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -781,6 +781,36 @@ static const struct attribute_group 
>> *vfio_ap_mdev_attr_groups[] = {
>>   };
>>   /**
>> + * vfio_ap_mdev_iommu_notifier: IOMMU notifier callback
>> + *
>> + * @nb: The notifier block
>> + * @action: Action to be taken (VFIO_IOMMU_NOTIFY_DMA_UNMAP)
>> + * @data: the specific unmap structure for vfio_iommu_type1
>> + *
>> + * Unpins the guest IOVA. (The NIB guest address we pinned before).
>> + * Return NOTIFY_OK after unpining on a UNMAP request.
>> + * otherwise, returns NOTIFY_DONE .
>> + */
>> +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;
>> +}
>> +
>> +
>> +/**
>>    * vfio_ap_mdev_set_kvm
>>    *
>>    * @matrix_mdev: a mediated matrix device
>> @@ -904,8 +934,7 @@ static void vfio_ap_dissociate_queues(struct 
>> ap_matrix_mdev *matrix_mdev)
>>    * In the case a queue could not be found return -ENODEV.
>>    * Otherwise return 0.
>>    */
>> -static __attribute__((unused))
>> -    int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
>> +static int vfio_ap_associate_queues(struct ap_matrix_mdev *matrix_mdev)
> 
> Maybe this function should be introduced in this patch instead?
> 
>>   {
>>       unsigned long apid, apqi;
>>       struct vfio_ap_queue *q;
>> @@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device 
>> *mdev)
>>       ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>                        &events, &matrix_mdev->group_notifier);
>> -    if (ret) {
>> -        module_put(THIS_MODULE);
>> -        return ret;
>> -    }
>> +    if (ret)
>> +        goto err_group;
>> +
>> +    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)
>> +        goto err_iommu;
>> +
>> +    ret = vfio_ap_associate_queues(matrix_mdev);
>> +    if (ret)
>> +        goto err_associate;
> 
> I think the matrix_mdev should be associated with queues when an 
> assignment of an adapter or domain is made to the mdev device via its
> sysfs interfaces. I say this because assigning an adapter or domain to
> an mdev device effectively grants ownership of any additional AP queues 
> added to the mdev device's AP matrix as a result of the assignment. It
> only makes sense to assign ownership to the vfio_ap_queue objects
> representing the queues at that time. If an adapter or domain is
> dynamically assigned while a guest is using the affected queues, then
> the associations will have to be made at that time and this code will
> likely go bye bye.
> 
>>       return 0;
>> +
>> +err_associate:
>> +    vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>> +                 &matrix_mdev->iommu_notifier);
>> +err_iommu:
>> +    vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>> +                 &matrix_mdev->group_notifier);
>> +err_group:
>> +    module_put(THIS_MODULE);
>> +    return ret;
>>   }
>>   static void vfio_ap_mdev_release(struct mdev_device *mdev)
>> @@ -985,6 +1034,9 @@ static void vfio_ap_mdev_release(struct 
>> mdev_device *mdev)
>>       vfio_ap_mdev_reset_queues(mdev);
>>       vfio_unregister_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>                    &matrix_mdev->group_notifier);
>> +    vfio_unregister_notifier(mdev_dev(mdev), VFIO_IOMMU_NOTIFY,
>> +                 &matrix_mdev->iommu_notifier);
>> +    vfio_ap_dissociate_queues(matrix_mdev);
> 
> I think the matrix_mdev should be dissociated from queues when an
> adapter or domain is unassigned from the mdev device via its
> sysfs interfaces. I say this because unassigning an adapter or domain 
> from an mdev device effectively takes away ownership of any AP queues
> removed from the mdev device's AP matrix as a result of the
> unassignment. It only makes sense to remove ownership from the
> vfio_ap_queue objects representing the queues at that time. This will
> become necessary for the forthcoming dynamic configuration patches.
> If an adapter or domain is dynamically unassigned while a guest is
> using the affected queues, then the dissociation will have to be made
> at that time and this code will likely go bye bye.

Effectively,having a link from the vfio_ap_queue to the mediated device 
will simplify the sanity checking during assignment.

I will go this way.

Regards,
Pierre

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

Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Pierre Morel]

On 19/02/2019 10:59, Halil Pasic wrote:
> On Fri, 15 Feb 2019 17:55:35 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>> To be able to use the VFIO interface to facilitate the
>>> mediated device memory pining/unpining we need to register
>>> a notifier for IOMMU.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 64 +++++++++++++++++++++++++++++++----
>>>    drivers/s390/crypto/vfio_ap_private.h |  2 ++
>>>    2 files changed, 60 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 1851b24..6eddc2c 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> 
> [..]
>    	struct vfio_ap_queue *q;
>>> @@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>>>    
>>>    	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>>    				     &events, &matrix_mdev->group_notifier);
>>> -	if (ret) {
>>> -		module_put(THIS_MODULE);
>>> -		return ret;
>>> -	}
>>> +	if (ret)
>>> +		goto err_group;
>>> +
>>> +	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)
>>> +		goto err_iommu;
>>> +
>>> +	ret = vfio_ap_associate_queues(matrix_mdev);
>>> +	if (ret)
>>> +		goto err_associate;
>>
>> I think the matrix_mdev should be associated with queues when an
>> assignment of an adapter or domain is made to the mdev device via its
>> sysfs interfaces. I say this because assigning an adapter or domain to
>> an mdev device effectively grants ownership of any additional AP queues
>> added to the mdev device's AP matrix as a result of the assignment. It
>> only makes sense to assign ownership to the vfio_ap_queue objects
>> representing the queues at that time. If an adapter or domain is
>> dynamically assigned while a guest is using the affected queues, then
>> the associations will have to be made at that time and this code will
>> likely go bye bye.
>>
> 
> 
> Actually, we need the stuff in vfio_ap_queues only if the guest is
> offered to  decides to use AP adaper interrupts. I would prefer to have
> it around only when needed: as long as we have the nib pinned for any
> given queue. Is there a reason why not to do so?
> 
> Regards,
> Halil
>   
> [..]
> 

Since I will rework the vfio_ap_queue life cycle, I will study this.
But I find the creation / deletion in probe/remove and the link/unlimk 
to the matrix device in assign/un_assign as suggested by Tony more logical.

Regards,
Pierre

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

Re: [PATCH v3 7/9] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 16/02/2019 00:11, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> 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_ops.c     | 191 
>> +++++++++++++++++++++++++++++++++-
>>   drivers/s390/crypto/vfio_ap_private.h |   2 +
>>   3 files changed, 191 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/ap_bus.h b/drivers/s390/crypto/ap_bus.h
>> index bfc66e4..323f2aa 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_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 6eddc2c..5664cf3 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -77,6 +77,28 @@ static void vfio_ap_put_queue(struct vfio_ap_queue *q)
>>       q->dev = NULL;
>>   }
>> +/**
>> + * vfio_ap_free_irq:
>> + * @q: The vfio_ap_queue
>> + *
>> + * Unpin the guest NIB
>> + * Unregister the ISC from the GIB alert
>> + * Clear the vfio_ap_queue intern fields
>> + */
>> +static void vfio_ap_free_irq(struct vfio_ap_queue *q)
>> +{
>> +    unsigned long pfn = q->nib >> PAGE_SHIFT;
>> +
>> +    if (!q)
>> +        return;
>> +    if (q->nib)
>> +        vfio_unpin_pages(mdev_dev(q->matrix->mdev), &pfn, 1);
>> +    if (q->isc)
>> +        kvm_s390_gisc_unregister(q->matrix->kvm, q->isc);
>> +    q->nib = 0;
>> +    q->isc = 0;
>> +}
>> +
>>   static void vfio_ap_matrix_init(struct ap_config_info *info,
>>                   struct ap_matrix *matrix)
>>   {
>> @@ -98,6 +120,7 @@ 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);
>>       mutex_lock(&matrix_dev->lock);
>> @@ -781,6 +804,156 @@ static const struct attribute_group 
>> *vfio_ap_mdev_attr_groups[] = {
>>   };
>>   /**
>> + * 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;
>> +
>> +    status = ap_aqic(q->apqn, aqic_gisa, NULL);
>> +    if (!status.response_code)
>> +        vfio_ap_free_irq(q);
>> +
>> +    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 g_pfn, h_nib, h_pfn;
>> +    int ret;
>> +
>> +    kvm = q->matrix->kvm;
>> +    gisa = kvm->arch.gisa_int.origin;
>> +
>> +    g_pfn = q->nib >> PAGE_SHIFT;
>> +    ret = vfio_pin_pages(mdev_dev(q->matrix->mdev), &g_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;
>> +    }
>> +
>> +    h_nib = (h_pfn << PAGE_SHIFT) | (q->nib & ~PAGE_MASK);
>> +    aqic_gisa.gisc = q->isc;
>> +    aqic_gisa.isc = kvm_s390_gisc_register(kvm, q->isc);
>> +    aqic_gisa.ir = 1;
>> +    aqic_gisa.gisa = gisa->next_alert >> 4;
>> +
>> +    status = ap_aqic(q->apqn, aqic_gisa, (void *)h_nib);
>> +    if (status.response_code == AP_RESPONSE_INVALID_GISA) {
>> +        status.response_code = AP_RESPONSE_OTHERWISE_CHANGED;
>> +        pr_warn("vfio_ap: apqn %02x.%04x: AP_RESPONSE_INVALID_GISA\n",
>> +            (q->apqn >> 8) & 0xff, q->apqn & 0xff);
>> +    }
>> +
>> +    if (status.response_code)
>> +        vfio_ap_free_irq(q);
>> +
>> +    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.
>> + *
>> + * Return 0 if we could handle the request inside KVM.
>> + * otherwise, returns -EOPNOTSUPP to let QEMU handle the fault.
>> + */
> 
> This function be nothing more than a switch statement for the
> function code sent with the PQAP instruction. Each case should
> be a call to a appropriate PQAP function handler. This will make
> it much easier to add additional handlers for the 6 other
> PQAP functions if necessary at some time down the road.

AFAIK there are only two PQAP functions we can intercept.

I will give attention to this function.
I will wait to this on the answers from KVM maintainers to know which 
KVM functions I can use here.


> 
>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>> +{    int ret.
>      uint8_t fc;
> 
>      fc = vcpu->run->s.regs.gprs[0] >> 24;
>      switch(fc) {
>      case 0x03:
>          ret = handle_pqap_aqic(vcpu);
>      default:
>          ret = -EOPNOTSUPP;
>      }
> 
>      return ret;
> }
> 
> static int handle_pqap_aqic(struct kvm_vcpu *vcpu) {
>> +    uint64_t status;
>> +    uint16_t apqn;
>      struct device *qdev;
>> +    struct vfio_ap_queue *q;
>> +    struct ap_queue_status qstatus = {};
>> +    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;
>> +    q = vfio_ap_get_queue(apqn);
> 
> Replace with:
>      qdev = vfio_ap_get_queue_dev(apqn);

You asked to revisit the life cycle of the vfio_ap_queues in another thread.
I will do it.

Thanks
Pierre

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

Re: [PATCH v3 8/9] s390: ap: Cleanup on removing the AP device

[Pierre Morel]

On 16/02/2019 00:29, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> When the device is remove, we must make sure to
>> clear the interruption and reset the AP device.
>>
>> We also need to clear the CRYCB of the guest.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_drv.c     | 92 
>> +++++++++++++++++++++++++++++++++++
>>   drivers/s390/crypto/vfio_ap_ops.c     |  2 +-
>>   drivers/s390/crypto/vfio_ap_private.h |  2 +
>>   3 files changed, 95 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_drv.c 
>> b/drivers/s390/crypto/vfio_ap_drv.c
>> index 03153e6..50428be 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>
>> @@ -12,6 +13,8 @@
>>   #include <linux/slab.h>
>>   #include <linux/string.h>
>>   #include <asm/facility.h>
>> +#include <linux/bitops.h>
>> +#include <linux/kvm_host.h>
>>   #include "vfio_ap_private.h"
>>   #define VFIO_AP_ROOT_NAME "vfio_ap"
>> @@ -64,6 +67,88 @@ static int vfio_ap_queue_dev_probe(struct ap_device 
>> *apdev)
>>       return 0;
>>   }
>> +/*
>> + * vfio_ap_drain_queue
>> + * @q: the queue to drain
>> + *
>> + * This function waits until the queue is empty.
>> + */
>> +static void vfio_ap_drain_queue(struct vfio_ap_queue *q)
>> +{
>> +    struct ap_queue_status status;
>> +    int retry = 20;
>> +
>> +    status = ap_tapq(q->apqn, NULL);
>> +    while (!status.queue_empty && retry--)  {
>> +        msleep(200);
>> +        status = ap_tapq(q->apqn, NULL);
>> +    }
>> +    if (retry <= 0) {
>> +        pr_warn("%s: queue not empty after zapq on apqn 0x%04x\n",
>> +            __func__, q->apqn);
>> +    }
>> +}
>> +
>> +/*
>> + * vfio_ap_zapq
>> + * @q: The queue to zerro
>> + *
>> + * It is best effort, no return value is done, however on success
>> + * we will drain the queue before getting the queue back to the
>> + * AP bus.
>> + */
>> +static void vfio_ap_zapq(struct vfio_ap_queue *q)
>> +{
>> +    struct ap_queue_status status;
>> +    int retry = 20;
>> +
>> +    do {
>> +        status = ap_zapq(q->apqn);
>> +        switch (status.response_code) {
>> +        case AP_RESPONSE_RESET_IN_PROGRESS:
>> +        case AP_RESPONSE_BUSY:
>> +            msleep(20);
>> +            break;
>> +        default:
>> +            pr_warn("%s: zapq error %02x on apqn 0x%04x\n",
>> +                __func__, status.response_code, q->apqn);
>> +            return;
>> +        case AP_RESPONSE_NORMAL:
>> +            vfio_ap_drain_queue(q);
> 
> I don't think this is necessary. The zeroize is performed on
> each AP-queue entry in an AP queue. My understanding is that when a
> reset or zeroize is pending, any AP instructions subsequently issued
> are rejected with condition-code 3 indicating an AP queue reset is in
> progress. It is also my understanding that once the AP commands 
> currently executing in a given AP queue entry complete, the queue
> entry will be zeroized. So it seems to me that there is no need to
> "drain" the queue, it will have already been done by the zeroize.

My understanding from the specifications is that after a RAPQ or a ZAPQ
we should wait for the queue to be really empty.

> 
> If you agree we don't need to "drain" the queue, then I'd rather just
> make the zapq function in the vfio_ap_ops.c non-static and make it
> available to the driver. There is no sense in duplicating this code. In
> fact, even if you keep the draining function, you still don't need to
> duplicate a zaapq instruction here, you can just call the vfio_ap_ops.c
> version and then drain the queue on AP_RESPONSE_NORMAL.

OK I will see how to optimize this.



>> +        return;
>> +
>> +    vfio_ap_update_crycb(q);
>> +    vfio_ap_zapq(q);
>> +
>> +    vfio_ap_free_irq(q);
> 
> If you make the zapq function in vfio_ap_ops.c available to the driver
> rather than duplicating it in this file, you won't need this call
> to vfio_ap_free_irq because it is done as part of the zapq in
> vfio_ap_ops.c.

Another solution.
OK I will look at this.

Thanks,
Pierre


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

Re: [PATCH v3 8/9] s390: ap: Cleanup on removing the AP device

[Pierre Morel]

On 16/02/2019 00:36, Tony Krowiak wrote:
> On 2/14/19 8:51 AM, Pierre Morel wrote:
>> When the device is remove, we must make sure to
>> clear the interruption and reset the AP device.
>>
...snip...

>> @@ -74,6 +159,13 @@ static void vfio_ap_queue_dev_remove(struct 
>> ap_device *apdev)
>>       struct vfio_ap_queue *q;
>>       q = dev_get_drvdata(&apdev->device);
>> +    if (!q)
>> +        return;
>> +
>> +    vfio_ap_update_crycb(q);
> 
> The root user is warned in the Limitations section of the vfio-ap.txt
> doc delivered with the AP pass-through support warns that the
> administrator (i.e., root user) should ensure that AP devices are not
> removed without taking proper care to ensure they are not in use by a
> guest. I am currently working on a patch set to handle this, so this
> may simply get ripped out when those patches are integrated. That may
> very well be simultaneously with this patch series as I plan on posting
> those soon.
> 
> If this call is to remain, then you ought to update the vfio-ap.txt
> document to let users know that when queues are unbound, the guests
> will lose access to them unbeknown to the admin of the guest.

I do not see where is the problem, the admin should still take care the 
APQN are not in use by the guest when he does an unbind.

This just makes sure it is not used anymore by the guest when the admin 
rebound it to the host or another guest.

> 
>> +    vfio_ap_zapq(q);
> 
> One last thing. I've explained before that prior to the AP bus
> invoking this remove callback, it flushes and zeroizes the
> queue. Why do you insist it needs to be done again in the remove
> callback?

The problem is that the AP_BUS is not aware from the CRYCB and let the 
guest play with the queue.

The sequence an be like:

-> AP_BUS remove RESET the queue and zeroes with ZAPQ
-> AP_BUS call remove from driver
	- the APQN still belong to the guest !
		-> the guest issue a NQAP
===> We need to take the queue away from the guest
===> and we need to RESET the queue with ZAPQ and wait
      until no more message is in the queue
-> driver remove ends


Regards,
Pierre


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

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

On 18/02/2019 23:42, Cornelia Huck wrote:
> On Mon, 18 Feb 2019 19:29:10 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 15/02/2019 23:02, Tony Krowiak wrote:
>>> On 2/14/19 8:51 AM, Pierre Morel wrote:
> 
>>>> +/*
>>>> + * handle_pqap: Handling pqap interception
>>>> + * @vcpu: the vcpu having issue the pqap instruction
>>>> + *
>>>> + * This callback only handles PQAP/AQIC instruction and
>>>> + * calls a dedicated callback for this instruction if
>>>> + * a driver did register one in the CRYPTO satellite of the
>>>> + * SIE block.
>>>> + *
>>>> + * Do not change the behavior if, return -EOPNOTSUPP if:
>>>> + * - the hook is not used do not change the behavior.
>>>> + * - AP instructions are not available or not available to the guest
>>>> + * - the instruction is not PQAP with function code indicating
>>>> + *   AQIC do not change the previous behavior.
>>>> + *
>>>> + * For PQAP/AQIC instruction, verify privilege and specifications
>>>> + *
>>>> + * return the value returned by the callback.
>>>> + */
>>>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>>>> +{
>>>> +    uint8_t fc;
>>>> +
>>>> +    /* Verify that the hook callback is registered */
>>>> +    if (!vcpu->kvm->arch.crypto.pqap_hook)
>>>> +        return -EOPNOTSUPP;
>>>> +    /* 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;
>>>> +    /* Verify that the function code is AQIC */
>>>> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
>>>> +    if (fc != 0x03)
>>>> +        return -EOPNOTSUPP;
>>>
>>> This does not belong here. Function code 3 is one of 7 function codes
>>> that can be sent with the PQAP instruction. This belongs in the PQAP
>>> hook code.
>>
>> On one hand, effectively I would prefer to put the code in the VFIO
>> driver code.
>> On the other hand, doing this would lead to export the code for
>> test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h
>>
>> I choose not to export these functions from the KVM code.
>>
>> Would like opinion from KVM maintainers?
> 
> Looking at this (and without access to the specification...), I think
> the check for problem state makes sense in here (if this applies to all
> PQAP functions equally, which seems likely). The check for the facility
> makes more sense in the handler. You can probably still inject the
> specification exception here if you use a clever return code.
> 

If there is no objection on exporting the KVM functions... I can do this.

> Another option: Provide a way to register a callback per function code;
> this allows you to still do the check here and extend it later for
> other function codes (which will probably be indicated by another
> facility).

I like this option even better.

Regards,
Pierre


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

Re: [PATCH v3 6/9] vfio: ap: register IOMMU VFIO notifier

[Tony Krowiak]

On 2/19/19 4:59 AM, Halil Pasic wrote:
> On Fri, 15 Feb 2019 17:55:35 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>> To be able to use the VFIO interface to facilitate the
>>> mediated device memory pining/unpining we need to register
>>> a notifier for IOMMU.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 64 +++++++++++++++++++++++++++++++----
>>>    drivers/s390/crypto/vfio_ap_private.h |  2 ++
>>>    2 files changed, 60 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>>> index 1851b24..6eddc2c 100644
>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> 
> [..]
>    	struct vfio_ap_queue *q;
>>> @@ -967,12 +996,32 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>>>    
>>>    	ret = vfio_register_notifier(mdev_dev(mdev), VFIO_GROUP_NOTIFY,
>>>    				     &events, &matrix_mdev->group_notifier);
>>> -	if (ret) {
>>> -		module_put(THIS_MODULE);
>>> -		return ret;
>>> -	}
>>> +	if (ret)
>>> +		goto err_group;
>>> +
>>> +	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)
>>> +		goto err_iommu;
>>> +
>>> +	ret = vfio_ap_associate_queues(matrix_mdev);
>>> +	if (ret)
>>> +		goto err_associate;
>>
>> I think the matrix_mdev should be associated with queues when an
>> assignment of an adapter or domain is made to the mdev device via its
>> sysfs interfaces. I say this because assigning an adapter or domain to
>> an mdev device effectively grants ownership of any additional AP queues
>> added to the mdev device's AP matrix as a result of the assignment. It
>> only makes sense to assign ownership to the vfio_ap_queue objects
>> representing the queues at that time. If an adapter or domain is
>> dynamically assigned while a guest is using the affected queues, then
>> the associations will have to be made at that time and this code will
>> likely go bye bye.
>>
> 
> 
> Actually, we need the stuff in vfio_ap_queues only if the guest is
> offered to  decides to use AP adaper interrupts. I would prefer to have
> it around only when needed: as long as we have the nib pinned for any
> given queue. Is there a reason why not to do so?

Even if we keep the vfio_ap_queue around for the life of a guest,
association/dissociation will have to be done when an adapter or domain
is dynamically assigned.

> 
> Regards,
> Halil
>   
> [..]
> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Tony Krowiak]

On 2/18/19 11:57 AM, Cornelia Huck wrote:
> On Mon, 18 Feb 2019 11:35:45 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/18/19 7:01 AM, Cornelia Huck wrote:
>>> On Fri, 15 Feb 2019 16:59:33 -0500
>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>    
>>>> On 2/15/19 4:11 AM, Cornelia Huck wrote:
>>>>> On Thu, 14 Feb 2019 13:30:59 -0500
>>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>>>>>       
>>>>>> On 2/14/19 12:36 PM, Pierre Morel wrote:
>>>>>>> On 14/02/2019 17:57, Cornelia Huck wrote:
>>>    
>>>>>>>> (And reading further in the current code, it seems we clear that
>>>>>>>> structure _after_ the matrix device had been setup, so how can that
>>>>>>>> even work? Where am I confused?)
>>>>>>>
>>>>>>> On device_register there were no bus, so the core just do not look for a
>>>>>>> driver and this field was nor tested nor overwritten.
>>>>>
>>>>> Hm... so has the callback in driver_for_each_device() in
>>>>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
>>>>> patch fixes more than just libudev issues...
>>>>
>>>> It is this patch that rendered the driver_for_each_device() in
>>>> vfio_ap_verify_queue_reserved() erroneous. That function gets called
>>>> every time an adapter or domain is assigned to the mdev. This patch
>>>> introduced the problem with driver_for_each_device().
>>>
>>> So, does this function need to be removed or called from another place,
>>> then? (It looks like it was dead code before.)
>>
>> I don't see why you think it's dead code:
>>
>> assign_adapter_store
>> ==> vfio_ap_mdev_verify_queues_reserved_for_apid
>>       ==> vfio_ap_verify_queue_reserved
>>           ==> driver_for_each_device
>>
>> The only way that the vfio_ap_verify_queue_reserved - the function that
>> calls driver_for_each_device - does not get called is if no bits have
>> yet been set in matrix_mdev->matrix.aqm.
> 
> What I don't see is how this can be called if no device has been, in
> fact, bound to the driver in the driver core...

Let's start with the fact that one can create an mdev device regardless
of whether a queue has been bound to the vfio_ap driver. Once an mdev
device is created, one can start assigning adapters, domains and control
domains to it. Let's say the admin now attempts to assign an adapter, in
which case the assign_adapter_store() function is invoked. After
verifying that the APID passed in is a valid adapter number, the
vfio_ap_mdev_verify_queues_reserved_for_apid() function is called.
This function first checks if any domains have been assigned and if not,
calls vfio_ap_verify_queue_reserved(&apid, NULL). It is in this function
that the driver_for_each_device() function is called. Since there are
no devices bound to the vfio_ap device driver, the callback passed in to
the driver_for_each_device() function will never get called, so the
vfio_ap_mdev_verify_queues_reserved_for_apid() function will return
-EADDRNOTAVAIL. A similar flow will occur if the first assignment is for
a domain. The bottom line is, the driver_for_each_device() function is
called every time an adapter or domain is assigned.

> 

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Tony Krowiak]

On 2/19/19 2:50 PM, Pierre Morel wrote:
> On 18/02/2019 23:42, Cornelia Huck wrote:
>> On Mon, 18 Feb 2019 19:29:10 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 15/02/2019 23:02, Tony Krowiak wrote:
>>>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>
>>>>> +/*
>>>>> + * handle_pqap: Handling pqap interception
>>>>> + * @vcpu: the vcpu having issue the pqap instruction
>>>>> + *
>>>>> + * This callback only handles PQAP/AQIC instruction and
>>>>> + * calls a dedicated callback for this instruction if
>>>>> + * a driver did register one in the CRYPTO satellite of the
>>>>> + * SIE block.
>>>>> + *
>>>>> + * Do not change the behavior if, return -EOPNOTSUPP if:
>>>>> + * - the hook is not used do not change the behavior.
>>>>> + * - AP instructions are not available or not available to the guest
>>>>> + * - the instruction is not PQAP with function code indicating
>>>>> + *   AQIC do not change the previous behavior.
>>>>> + *
>>>>> + * For PQAP/AQIC instruction, verify privilege and specifications
>>>>> + *
>>>>> + * return the value returned by the callback.
>>>>> + */
>>>>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +    uint8_t fc;
>>>>> +
>>>>> +    /* Verify that the hook callback is registered */
>>>>> +    if (!vcpu->kvm->arch.crypto.pqap_hook)
>>>>> +        return -EOPNOTSUPP;
>>>>> +    /* 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;
>>>>> +    /* Verify that the function code is AQIC */
>>>>> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
>>>>> +    if (fc != 0x03)
>>>>> +        return -EOPNOTSUPP;
>>>>
>>>> This does not belong here. Function code 3 is one of 7 function codes
>>>> that can be sent with the PQAP instruction. This belongs in the PQAP
>>>> hook code.
>>>
>>> On one hand, effectively I would prefer to put the code in the VFIO
>>> driver code.
>>> On the other hand, doing this would lead to export the code for
>>> test_kvm_facility() and kvm_s390_inject_program_int() from the 
>>> kvm-s390.h
>>>
>>> I choose not to export these functions from the KVM code.
>>>
>>> Would like opinion from KVM maintainers?
>>
>> Looking at this (and without access to the specification...), I think
>> the check for problem state makes sense in here (if this applies to all
>> PQAP functions equally, which seems likely). The check for the facility
>> makes more sense in the handler. You can probably still inject the
>> specification exception here if you use a clever return code.
>>
> 
> If there is no objection on exporting the KVM functions... I can do this.

I do not understand why you would have to export KVM functions to place
the check for FC 0x03 in the pqap hook? What am I missing here? Maybe
you misunderstood my comment? The following is the only code I suggested
you move:

+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

What does that have to do with test_kvm_facility() and
kvm_s390_inject_program_int()? Please explain.

> 
>> Another option: Provide a way to register a callback per function code;
>> this allows you to still do the check here and extend it later for
>> other function codes (which will probably be indicated by another
>> facility).
> 
> I like this option even better.
> 
> Regards,
> Pierre
> 
> 

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Tony Krowiak]

On 2/19/19 2:50 PM, Pierre Morel wrote:
> On 18/02/2019 23:42, Cornelia Huck wrote:
>> On Mon, 18 Feb 2019 19:29:10 +0100
>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>
>>> On 15/02/2019 23:02, Tony Krowiak wrote:
>>>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>
>>>>> +/*
>>>>> + * handle_pqap: Handling pqap interception
>>>>> + * @vcpu: the vcpu having issue the pqap instruction
>>>>> + *
>>>>> + * This callback only handles PQAP/AQIC instruction and
>>>>> + * calls a dedicated callback for this instruction if
>>>>> + * a driver did register one in the CRYPTO satellite of the
>>>>> + * SIE block.
>>>>> + *
>>>>> + * Do not change the behavior if, return -EOPNOTSUPP if:
>>>>> + * - the hook is not used do not change the behavior.
>>>>> + * - AP instructions are not available or not available to the guest
>>>>> + * - the instruction is not PQAP with function code indicating
>>>>> + *   AQIC do not change the previous behavior.
>>>>> + *
>>>>> + * For PQAP/AQIC instruction, verify privilege and specifications
>>>>> + *
>>>>> + * return the value returned by the callback.
>>>>> + */
>>>>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> +    uint8_t fc;
>>>>> +
>>>>> +    /* Verify that the hook callback is registered */
>>>>> +    if (!vcpu->kvm->arch.crypto.pqap_hook)
>>>>> +        return -EOPNOTSUPP;
>>>>> +    /* 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;
>>>>> +    /* Verify that the function code is AQIC */
>>>>> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
>>>>> +    if (fc != 0x03)
>>>>> +        return -EOPNOTSUPP;
>>>>
>>>> This does not belong here. Function code 3 is one of 7 function codes
>>>> that can be sent with the PQAP instruction. This belongs in the PQAP
>>>> hook code.
>>>
>>> On one hand, effectively I would prefer to put the code in the VFIO
>>> driver code.
>>> On the other hand, doing this would lead to export the code for
>>> test_kvm_facility() and kvm_s390_inject_program_int() from the 
>>> kvm-s390.h
>>>
>>> I choose not to export these functions from the KVM code.
>>>
>>> Would like opinion from KVM maintainers?
>>
>> Looking at this (and without access to the specification...), I think
>> the check for problem state makes sense in here (if this applies to all
>> PQAP functions equally, which seems likely). The check for the facility
>> makes more sense in the handler. You can probably still inject the
>> specification exception here if you use a clever return code.
>>
> 
> If there is no objection on exporting the KVM functions... I can do this.

I think I understand where you are coming from. In looking back at the
original patch, I see there are checks using the test_kvm_facility and
kvm_s390_inject_program_int functions placed after your check for
fc != 0x03. You clearly misunderstood what I was asking you to do.
I was suggesting that ONLY the check for 'fc != 0x03' be done in the
hook. I was NOT suggesting the instructions following the check for
fc != 0x03 be done in the hook, so there is no need to export any KVM
functions.


> 
>> Another option: Provide a way to register a callback per function code;
>> this allows you to still do the check here and extend it later for
>> other function codes (which will probably be indicated by another
>> facility).
> 
> I like this option even better.
> 
> Regards,
> Pierre
> 
> 

Re: [PATCH v3 1/9] s390: vfio_ap: link the vfio_ap devices to the vfio_ap bus subsystem

[Cornelia Huck]

On Tue, 19 Feb 2019 17:27:05 -0500
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> On 2/18/19 11:57 AM, Cornelia Huck wrote:
> > On Mon, 18 Feb 2019 11:35:45 -0500
> > Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >   
> >> On 2/18/19 7:01 AM, Cornelia Huck wrote:  
> >>> On Fri, 15 Feb 2019 16:59:33 -0500
> >>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >>>      
> >>>> On 2/15/19 4:11 AM, Cornelia Huck wrote:  
> >>>>> On Thu, 14 Feb 2019 13:30:59 -0500
> >>>>> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> >>>>>         
> >>>>>> On 2/14/19 12:36 PM, Pierre Morel wrote:  
> >>>>>>> On 14/02/2019 17:57, Cornelia Huck wrote:  
> >>>      
> >>>>>>>> (And reading further in the current code, it seems we clear that
> >>>>>>>> structure _after_ the matrix device had been setup, so how can that
> >>>>>>>> even work? Where am I confused?)  
> >>>>>>>
> >>>>>>> On device_register there were no bus, so the core just do not look for a
> >>>>>>> driver and this field was nor tested nor overwritten.  
> >>>>>
> >>>>> Hm... so has the callback in driver_for_each_device() in
> >>>>> vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this
> >>>>> patch fixes more than just libudev issues...  
> >>>>
> >>>> It is this patch that rendered the driver_for_each_device() in
> >>>> vfio_ap_verify_queue_reserved() erroneous. That function gets called
> >>>> every time an adapter or domain is assigned to the mdev. This patch
> >>>> introduced the problem with driver_for_each_device().  
> >>>
> >>> So, does this function need to be removed or called from another place,
> >>> then? (It looks like it was dead code before.)  
> >>
> >> I don't see why you think it's dead code:
> >>
> >> assign_adapter_store  
> >> ==> vfio_ap_mdev_verify_queues_reserved_for_apid
> >>       ==> vfio_ap_verify_queue_reserved
> >>           ==> driver_for_each_device  
> >>
> >> The only way that the vfio_ap_verify_queue_reserved - the function that
> >> calls driver_for_each_device - does not get called is if no bits have
> >> yet been set in matrix_mdev->matrix.aqm.  
> > 
> > What I don't see is how this can be called if no device has been, in
> > fact, bound to the driver in the driver core...  
> 
> Let's start with the fact that one can create an mdev device regardless
> of whether a queue has been bound to the vfio_ap driver. Once an mdev
> device is created, one can start assigning adapters, domains and control
> domains to it. Let's say the admin now attempts to assign an adapter, in
> which case the assign_adapter_store() function is invoked. After
> verifying that the APID passed in is a valid adapter number, the
> vfio_ap_mdev_verify_queues_reserved_for_apid() function is called.
> This function first checks if any domains have been assigned and if not,
> calls vfio_ap_verify_queue_reserved(&apid, NULL). It is in this function
> that the driver_for_each_device() function is called. Since there are
> no devices bound to the vfio_ap device driver, the callback passed in to
> the driver_for_each_device() function will never get called, so the
> vfio_ap_mdev_verify_queues_reserved_for_apid() function will return
> -EADDRNOTAVAIL. A similar flow will occur if the first assignment is for
> a domain. The bottom line is, the driver_for_each_device() function is
> called every time an adapter or domain is assigned.

Indeed. I just got lost with the various drivers and devices in play
here :(

Re: [PATCH v3 7/9] s390: ap: implement PAPQ AQIC interception in kernel

[Halil Pasic]

On Thu, 14 Feb 2019 14:51:07 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

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

[..]

> +static int handle_pqap(struct kvm_vcpu *vcpu)
> +{
> +	uint64_t status;
> +	uint16_t apqn;
> +	struct vfio_ap_queue *q;
> +	struct ap_queue_status qstatus = {};
> +	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;
> +	q = vfio_ap_get_queue(apqn);
> +	if (!q) {
> +		qstatus.response_code = AP_RESPONSE_Q_NOT_AVAIL;
> +		goto out;
> +	}
> +
> +	/* Check if the queue is associated with a guest matrix */
> +	matrix_mdev = q->matrix;
> +	if (!matrix_mdev || !matrix_mdev->kvm) {
> +		qstatus.response_code = AP_RESPONSE_DECONFIGURED;
> +		goto out;
> +	}
> +
> +	status = vcpu->run->s.regs.gprs[1];
> +
> +	/* If IR bit(16) is set we enable the interrupt */
> +	if ((status >> (63 - 16)) & 0x01) {
> +		q->isc = status & 0x07;
> +		q->nib = vcpu->run->s.regs.gprs[2];

Careful! You may already have q->nib set (may or may not be a 0x07
scenario). You should unpin the old nib if set interruption controls
works out, if we get 0x07 response then you unpin the new one.


> +		qstatus = vfio_ap_setirq(q);
> +		if (qstatus.response_code) {
> +			q->nib = 0;
> +			q->isc = 0;
> +		}
> +	} else
> +		qstatus = vfio_ap_clrirq(q);
> +
> +out:
> +	memcpy(&vcpu->run->s.regs.gprs[1], &qstatus, sizeof(qstatus));
> +	vfio_ap_put_queue(q);
> +	return 0;
> +}

[..]

>  
> @@ -877,11 +1052,15 @@ static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
>  				    unsigned int retry)
>  {
>  	struct ap_queue_status status;
> +	struct vfio_ap_queue *q;
> +
> +	q = vfio_ap_get_queue(AP_MKQID(apid, apqi));
>  
>  	do {
>  		status = ap_zapq(AP_MKQID(apid, apqi));
>  		switch (status.response_code) {
>  		case AP_RESPONSE_NORMAL:

You could do the get queue here

> +			vfio_ap_free_irq(q);

And the put here.


Regards,
Halil

>  			return 0;
>  		case AP_RESPONSE_RESET_IN_PROGRESS:
>  		case AP_RESPONSE_BUSY:
> @@ -999,6 +1178,11 @@ static int vfio_ap_mdev_open(struct mdev_device *mdev)
>  	if (ret)
>  		goto err_group;
>  
> +	if (!matrix_mdev->kvm) {
> +		ret = -ENODEV;
> +		goto err_iommu;
> +	}
> +
>  	matrix_mdev->iommu_notifier.notifier_call = vfio_ap_mdev_iommu_notifier;
>  	events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
>  

[..]

Re: [PATCH v3 2/9] s390: ap: kvm: setting a hook for PQAP instructions

[Pierre Morel]

On 19/02/2019 23:36, Tony Krowiak wrote:
> On 2/19/19 2:50 PM, Pierre Morel wrote:
>> On 18/02/2019 23:42, Cornelia Huck wrote:
>>> On Mon, 18 Feb 2019 19:29:10 +0100
>>> Pierre Morel <pmorel@linux.ibm.com> wrote:
>>>
>>>> On 15/02/2019 23:02, Tony Krowiak wrote:
>>>>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>>
>>>>>> +/*
>>>>>> + * handle_pqap: Handling pqap interception
>>>>>> + * @vcpu: the vcpu having issue the pqap instruction
>>>>>> + *
>>>>>> + * This callback only handles PQAP/AQIC instruction and
>>>>>> + * calls a dedicated callback for this instruction if
>>>>>> + * a driver did register one in the CRYPTO satellite of the
>>>>>> + * SIE block.
>>>>>> + *
>>>>>> + * Do not change the behavior if, return -EOPNOTSUPP if:
>>>>>> + * - the hook is not used do not change the behavior.
>>>>>> + * - AP instructions are not available or not available to the guest
>>>>>> + * - the instruction is not PQAP with function code indicating
>>>>>> + *   AQIC do not change the previous behavior.
>>>>>> + *
>>>>>> + * For PQAP/AQIC instruction, verify privilege and specifications
>>>>>> + *
>>>>>> + * return the value returned by the callback.
>>>>>> + */
>>>>>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>>>>>> +{
>>>>>> +    uint8_t fc;
>>>>>> +
>>>>>> +    /* Verify that the hook callback is registered */
>>>>>> +    if (!vcpu->kvm->arch.crypto.pqap_hook)
>>>>>> +        return -EOPNOTSUPP;
>>>>>> +    /* 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;
>>>>>> +    /* Verify that the function code is AQIC */
>>>>>> +    fc = vcpu->run->s.regs.gprs[0] >> 24;
>>>>>> +    if (fc != 0x03)
>>>>>> +        return -EOPNOTSUPP;
>>>>>
>>>>> This does not belong here. Function code 3 is one of 7 function codes
>>>>> that can be sent with the PQAP instruction. This belongs in the PQAP
>>>>> hook code.
>>>>
>>>> On one hand, effectively I would prefer to put the code in the VFIO
>>>> driver code.
>>>> On the other hand, doing this would lead to export the code for
>>>> test_kvm_facility() and kvm_s390_inject_program_int() from the 
>>>> kvm-s390.h
>>>>
>>>> I choose not to export these functions from the KVM code.
>>>>
>>>> Would like opinion from KVM maintainers?
>>>
>>> Looking at this (and without access to the specification...), I think
>>> the check for problem state makes sense in here (if this applies to all
>>> PQAP functions equally, which seems likely). The check for the facility
>>> makes more sense in the handler. You can probably still inject the
>>> specification exception here if you use a clever return code.
>>>
>>
>> If there is no objection on exporting the KVM functions... I can do this.
> 
> I do not understand why you would have to export KVM functions to place
> the check for FC 0x03 in the pqap hook? What am I missing here? Maybe
> you misunderstood my comment?

No I did not but in between I discovered an error in the handling of the 
interception of PQAP/AQIC.

QEMU and KVM can both accept PQAP/AQIC even if the vfio_ap driver is not 
loaded.
However now that the guest officially get the PQAP/AQIC instruction we 
need to handle the specification and operation exceptions inside KVM
_before_ testing and even calling the driver hook.

I will make the changes in the next iteration.

Regards,
Pierre



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

Re: [PATCH v3 7/9] s390: ap: implement PAPQ AQIC interception in kernel

[Pierre Morel]

On 20/02/2019 12:54, Halil Pasic wrote:
> On Thu, 14 Feb 2019 14:51:07 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> 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>
>> ---
> 
> [..]
> 
>> +static int handle_pqap(struct kvm_vcpu *vcpu)
>> +{
>> +	uint64_t status;
>> +	uint16_t apqn;
>> +	struct vfio_ap_queue *q;
>> +	struct ap_queue_status qstatus = {};
>> +	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;
>> +	q = vfio_ap_get_queue(apqn);
>> +	if (!q) {
>> +		qstatus.response_code = AP_RESPONSE_Q_NOT_AVAIL;
>> +		goto out;
>> +	}
>> +
>> +	/* Check if the queue is associated with a guest matrix */
>> +	matrix_mdev = q->matrix;
>> +	if (!matrix_mdev || !matrix_mdev->kvm) {
>> +		qstatus.response_code = AP_RESPONSE_DECONFIGURED;
>> +		goto out;
>> +	}
>> +
>> +	status = vcpu->run->s.regs.gprs[1];
>> +
>> +	/* If IR bit(16) is set we enable the interrupt */
>> +	if ((status >> (63 - 16)) & 0x01) {
>> +		q->isc = status & 0x07;
>> +		q->nib = vcpu->run->s.regs.gprs[2];
> 
> Careful! You may already have q->nib set (may or may not be a 0x07
> scenario). You should unpin the old nib if set interruption controls
> works out, if we get 0x07 response then you unpin the new one.

You are right. We already had this discussion and the corrected patch.
It seems I misunderstood what we wanted to do by sending this patch 
series out.

I will make this right.

regards,
Pierre


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

Re: [PATCH v3 4/9] s390: ap: tools to find a queue with a specific APQN

[Tony Krowiak]

On 2/18/19 7:21 AM, Cornelia Huck wrote:
> On Fri, 15 Feb 2019 17:13:21 -0500
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> 
>> On 2/14/19 8:51 AM, Pierre Morel wrote:
>>> We need to find the queue with a specific APQN during the
>>> handling of the interception of the PQAP/AQIC instruction.
>>>
>>> To handle the AP associated device reference count we keep
>>> track of it in the vfio_ap_queue until we put the device.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
>>>    drivers/s390/crypto/vfio_ap_private.h |  1 +
>>>    2 files changed, 55 insertions(+)
> 
>>> +/**
>>> + * vfio_ap_get_queue: Retrieve a queue with a specific APQN
>>> + * @apqn: The queue APQN
>>> + *
>>> + * Retrieve a queue with a specific APQN from the list of the
>>> + * devices associated to the vfio_ap_driver.
>>> + *
>>> + * The vfio_ap_queue has been already associated with the device
>>> + * during the probe.
>>> + * Store the associated device for reference counting
>>> + *
>>> + * Returns the pointer to the associated vfio_ap_queue
>>> + */
>>> +static  __attribute__((unused))
>>> +	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
>>
>> I think you should change this signature for the reasons I stated
>> below:
>>
>> struct device *vfio_ap_get_queue_dev(int apqn)
>>
>>> +{
>>> +	struct device *dev;
>>> +	struct vfio_ap_queue *q;
>>> +
>>> +	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
>>> +				 vfio_ap_check_apqn);
>>> +	if (!dev)
>>> +		return NULL;
>>> +	q = dev_get_drvdata(dev);
>>> +	q->dev = dev;
>>
>> Why store the device with the vfio_ap_queue object? Why not just return
>> the device. The caller can get the vfio_ap_queue from the device's
>> driver data. It seems the only reason for the 'dev' field is to
>> temporarily hold a ref to the device so it can be put later. Why not
>> just put the device.

After thinking about this further, I question whether we even need
this function if it is going to return 'struct device *'. In that case,
why not just call driver_find_device() when the device is needed?
If you want to keep the function, then the function needs only one 
statement:

	return driver_find_device(...).

> 
> Having looked at the remainder of the patches, I tend to agree that we
> don't really need the backlink; we walk the driver's list of devices in
> any case IIUC.
> 
> We *might* want a mechanism to grab the queue quickly (i.e. without
> walking the list) if there's anything performance sensitive in there;
> but from the patch descriptions, I don't think anything is done in a
> hot path, so it should be fine.
>