On 9/24/20 10:11 PM, Halil Pasic wrote:
On Thu, 27 Aug 2020 10:24:07 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
On 8/25/20 6:13 AM, Cornelia Huck wrote:
On Fri, 21 Aug 2020 15:56:02 -0400
Tony Krowiak<akrowiak@xxxxxxxxxxxxx> wrote:
This patch refactor's the vfio_ap device driver to use the AP bus's
s/refactor's/refactors/
Of course, what was I thinking?:)
ap_get_qdev() function to retrieve the vfio_ap_queue struct containing
information about a queue that is bound to the vfio_ap device driver.
The bus's ap_get_qdev() function retrieves the queue device from a
hashtable keyed by APQN. This is much more efficient than looping over
the list of devices attached to the AP bus by several orders of
magnitude.
Signed-off-by: Tony Krowiak<akrowiak@xxxxxxxxxxxxx>
Reported-by: kernel test robot<lkp@xxxxxxxxx>
---
drivers/s390/crypto/vfio_ap_drv.c | 27 ++-------
drivers/s390/crypto/vfio_ap_ops.c | 86 +++++++++++++++------------
drivers/s390/crypto/vfio_ap_private.h | 8 ++-
3 files changed, 59 insertions(+), 62 deletions(-)
(...)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e0bde8518745..ad3925f04f61 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -26,43 +26,26 @@vfio_ap_get_queue()
static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
-static int match_apqn(struct device *dev, const void *data)
-{
- struct vfio_ap_queue *q = dev_get_drvdata(dev);
-
- return (q->apqn == *(int *)(data)) ? 1 : 0;
-}
-
/**
- * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
- * @matrix_mdev: the associated mediated matrix
+ * 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 of the vfio_ap_drv.
- * Verify that the APID and the APQI are set in the matrix.
+ * Retrieve a queue with a specific APQN from the AP queue devices attached to
+ * the AP bus.
*
- * Returns the pointer to the associated vfio_ap_queue
+ * Returns the pointer to the vfio_ap_queue with the specified APQN, or NULL.
*/
-static struct vfio_ap_queue *vfio_ap_get_queue(
- struct ap_matrix_mdev *matrix_mdev,
- int apqn)
+static struct vfio_ap_queue *vfio_ap_get_queue(unsigned long apqn)
{
+ struct ap_queue *queue;
struct vfio_ap_queue *q;
- struct device *dev;
- if (!test_bit_inv(AP_QID_CARD(apqn), matrix_mdev->matrix.apm))
- return NULL;
- if (!test_bit_inv(AP_QID_QUEUE(apqn), matrix_mdev->matrix.aqm))
I think you should add some explanation to the patch description why
testing the matrix bitmasks is not needed anymore.
As a result of this comment, I took a closer look at the code to
determine the reason for eliminating the matrix_mdev
parameter. The reason is because the code below (i.e., find the device
and get the driver data) was also repeated in the vfio_ap_irq_disable_apqn()
function, so I replaced it with a call to the function above; however, the
vfio_ap_irq_disable_apqn() function does not have a reference to the
matrix_mdev, so I eliminated the matrix_mdev parameter. Note that the
vfio_ap_irq_disable_apqn() is called for each APQN assigned to a matrix
mdev, so there is no need to test the bitmasks there.
The other place from which the function above is called is
the handle_pqap() function which does have a reference to the
matrix_mdev. In order to ensure the integrity of the instruction
being intercepted - i.e., PQAP(AQIC) enable/disable IRQ for aN
AP queue - the testing of the matrix bitmasks probably ought to
be performed, so it will be done there instead of in the
vfio_ap_get_queue() function above.
I'm a little confused. I do agree that in handle_pqap() we do want to
make sure that we only operate on queues that belong to the given guest
that issued the PQAP instruction.
AFAICT with this patch set applied, this is not the case any more. Does
that 'will be done there instead' refer to v11?
I understand your confusion, so here is what I'm going to do
to clear things up. I will leave the signature of the vfio_ap_get_queue()
function the same and leave in the bitmap checking. As per your
comment below, in patch 3 I will replace the call to
vfio_ap_get_queue() with a call to vfio_ap_get_mdev_queue().
Since the vfio_ap_get_mdev_queue() function is mdev-specific,
I can then remove the mdev parameter from the
vfio_ap_get_queue() function since it will no longer be needed.
Another question is, can we use vfio_ap_get_mdev_queue() in
handle_pqap() (instead of vfio_ap_get_queue())?
Yes, we can and should do that as it will eliminate both the need to
test the matrix bitmasks and several lines of code; however, that
function is not available until patch 3/16, so that change will be
made there.
+ queue = ap_get_qdev(apqn);
+ if (!queue)
return NULL;
- dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
- &apqn, match_apqn);
- if (!dev)
- return NULL;
- q = dev_get_drvdata(dev);
- q->matrix_mdev = matrix_mdev;
- put_device(dev);
+ q = dev_get_drvdata(&queue->ap_dev.device);
+ put_device(&queue->ap_dev.device);
return q;
}
(...)
