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

From: Pierre Morel
Date: Mon Feb 18 2019 - 13:36:35 EST


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