This patch refactor's the vfio_ap device driver to use the AP bus's
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 <[email protected]>
Reported-by: kernel test robot <[email protected]>
---
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_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index f4ceb380dd61..24cdef60039a 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -53,15 +53,9 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
*/
static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
{
- struct vfio_ap_queue *q;
-
- q = kzalloc(sizeof(*q), GFP_KERNEL);
- if (!q)
- return -ENOMEM;
- dev_set_drvdata(&apdev->device, q);
- q->apqn = to_ap_queue(&apdev->device)->qid;
- q->saved_isc = VFIO_AP_ISC_INVALID;
- return 0;
+ struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+ return vfio_ap_mdev_probe_queue(queue);
}
/**
@@ -72,18 +66,9 @@ static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
*/
static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
{
- struct vfio_ap_queue *q;
- int apid, apqi;
-
- mutex_lock(&matrix_dev->lock);
- q = dev_get_drvdata(&apdev->device);
- dev_set_drvdata(&apdev->device, NULL);
- apid = AP_QID_CARD(q->apqn);
- apqi = AP_QID_QUEUE(q->apqn);
- vfio_ap_mdev_reset_queue(apid, apqi, 1);
- vfio_ap_irq_disable(q);
- kfree(q);
- mutex_unlock(&matrix_dev->lock);
+ struct ap_queue *queue = to_ap_queue(&apdev->device);
+
+ vfio_ap_mdev_remove_queue(queue);
}
static void vfio_ap_matrix_dev_release(struct device *dev)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e0bde8518745..ad3925f04f61 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -26,43 +26,26 @@
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))
+ 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;
}
@@ -144,7 +127,7 @@ static void vfio_ap_free_aqic_resources(struct vfio_ap_queue *q)
* Returns if ap_aqic function failed with invalid, deconfigured or
* checkstopped AP.
*/
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
+static struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q)
{
struct ap_qirq_ctrl aqic_gisa = {};
struct ap_queue_status status;
@@ -293,10 +276,11 @@ static int handle_pqap(struct kvm_vcpu *vcpu)
matrix_mdev = container_of(vcpu->kvm->arch.crypto.pqap_hook,
struct ap_matrix_mdev, pqap_hook);
- q = vfio_ap_get_queue(matrix_mdev, apqn);
+ q = vfio_ap_get_queue(apqn);
if (!q)
goto out_unlock;
+ q->matrix_mdev = matrix_mdev;
status = vcpu->run->s.regs.gprs[1];
/* If IR bit(16) is set we enable the interrupt */
@@ -1116,20 +1100,15 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
static void vfio_ap_irq_disable_apqn(int apqn)
{
- struct device *dev;
struct vfio_ap_queue *q;
- dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
- &apqn, match_apqn);
- if (dev) {
- q = dev_get_drvdata(dev);
+ q = vfio_ap_get_queue(apqn);
+ if (q)
vfio_ap_irq_disable(q);
- put_device(dev);
- }
}
-int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
- unsigned int retry)
+static int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
+ unsigned int retry)
{
struct ap_queue_status status;
int retry2 = 2;
@@ -1302,3 +1281,34 @@ void vfio_ap_mdev_unregister(void)
{
mdev_unregister_device(&matrix_dev->device);
}
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue)
+{
+ struct vfio_ap_queue *q;
+
+ q = kzalloc(sizeof(*q), GFP_KERNEL);
+ if (!q)
+ return -ENOMEM;
+
+ dev_set_drvdata(&queue->ap_dev.device, q);
+ q->apqn = queue->qid;
+ q->saved_isc = VFIO_AP_ISC_INVALID;
+
+ return 0;
+}
+
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue)
+{
+ struct vfio_ap_queue *q;
+ int apid, apqi;
+
+ mutex_lock(&matrix_dev->lock);
+ q = dev_get_drvdata(&queue->ap_dev.device);
+ dev_set_drvdata(&queue->ap_dev.device, NULL);
+ apid = AP_QID_CARD(q->apqn);
+ apqi = AP_QID_QUEUE(q->apqn);
+ vfio_ap_mdev_reset_queue(apid, apqi, 1);
+ vfio_ap_irq_disable(q);
+ kfree(q);
+ mutex_unlock(&matrix_dev->lock);
+}
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index f46dde56b464..a2aa05bec718 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -18,6 +18,7 @@
#include <linux/delay.h>
#include <linux/mutex.h>
#include <linux/kvm_host.h>
+#include <linux/hashtable.h>
#include "ap_bus.h"
@@ -90,8 +91,6 @@ struct ap_matrix_mdev {
extern int vfio_ap_mdev_register(void);
extern void vfio_ap_mdev_unregister(void);
-int vfio_ap_mdev_reset_queue(unsigned int apid, unsigned int apqi,
- unsigned int retry);
struct vfio_ap_queue {
struct ap_matrix_mdev *matrix_mdev;
@@ -100,5 +99,8 @@ struct vfio_ap_queue {
#define VFIO_AP_ISC_INVALID 0xff
unsigned char saved_isc;
};
-struct ap_queue_status vfio_ap_irq_disable(struct vfio_ap_queue *q);
+
+int vfio_ap_mdev_probe_queue(struct ap_queue *queue);
+void vfio_ap_mdev_remove_queue(struct ap_queue *queue);
+
#endif /* _VFIO_AP_PRIVATE_H_ */
--
2.21.1
On Fri, 21 Aug 2020 15:56:02 -0400
Tony Krowiak <[email protected]> wrote:
> This patch refactor's the vfio_ap device driver to use the AP bus's
s/refactor's/refactors/
> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> ---
> 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 @@
>
> 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.
> + 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;
> }
(...)
On 8/25/20 6:13 AM, Cornelia Huck wrote:
> On Fri, 21 Aug 2020 15:56:02 -0400
> Tony Krowiak<[email protected]> 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<[email protected]>
>> Reported-by: kernel test robot<[email protected]>
>> ---
>> 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 @@
>>
>> 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.
> + 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;
> }
> (...)
>
On Thu, 27 Aug 2020 10:24:07 -0400
Tony Krowiak <[email protected]> wrote:
> On 8/25/20 6:13 AM, Cornelia Huck wrote:
> > On Fri, 21 Aug 2020 15:56:02 -0400
> > Tony Krowiak<[email protected]> wrote:
> >> /**
> >> - * 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.
Should you add a comment that vfio_ap_get_queue() assumes that the
caller makes sure that this is only called for APQNs that are assigned
to a matrix?
>
>
> > + 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;
> > }
> > (...)
> >
>
On 8/28/20 4:13 AM, Cornelia Huck wrote:
> On Thu, 27 Aug 2020 10:24:07 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> On 8/25/20 6:13 AM, Cornelia Huck wrote:
>>> On Fri, 21 Aug 2020 15:56:02 -0400
>>> Tony Krowiak<[email protected]> wrote:
>>>> /**
>>>> - * 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.
> Should you add a comment that vfio_ap_get_queue() assumes that the
> caller makes sure that this is only called for APQNs that are assigned
> to a matrix?
I suppose it wouldn't hurt.
>
>>
>>> + 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;
>>> }
>>> (...)
>>>
On 21.08.20 21:56, Tony Krowiak wrote:
> This patch refactor's the vfio_ap device driver to use the AP bus's
> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
I think this can go. No need to mark that an earlier version of this patch had an issue.
[...]
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index f46dde56b464..a2aa05bec718 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -18,6 +18,7 @@
> #include <linux/delay.h>
> #include <linux/mutex.h>
> #include <linux/kvm_host.h>
> +#include <linux/hashtable.h>
I dont think that this header file needs it. Any user of it will now include this.
Can you move this include into the respective C file when the hash stuff is
used?
Other than that this looks good.
On 9/4/20 4:11 AM, Christian Borntraeger wrote:
>
> On 21.08.20 21:56, Tony Krowiak wrote:
>> This patch refactor's the vfio_ap device driver to use the AP bus's
>> 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 <[email protected]>
>> Reported-by: kernel test robot <[email protected]>
> I think this can go. No need to mark that an earlier version of this patch had an issue.
I was just following the instructions in the robot comments. I'll get
rid of it.
>
>
> [...]
>
>> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
>> index f46dde56b464..a2aa05bec718 100644
>> --- a/drivers/s390/crypto/vfio_ap_private.h
>> +++ b/drivers/s390/crypto/vfio_ap_private.h
>> @@ -18,6 +18,7 @@
>> #include <linux/delay.h>
>> #include <linux/mutex.h>
>> #include <linux/kvm_host.h>
>> +#include <linux/hashtable.h>
> I dont think that this header file needs it. Any user of it will now include this.
> Can you move this include into the respective C file when the hash stuff is
> used?
I can.
>
>
> Other than that this looks good.
On Thu, 27 Aug 2020 10:24:07 -0400
Tony Krowiak <[email protected]> wrote:
>
>
> On 8/25/20 6:13 AM, Cornelia Huck wrote:
> > On Fri, 21 Aug 2020 15:56:02 -0400
> > Tony Krowiak<[email protected]> 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<[email protected]>
> >> Reported-by: kernel test robot<[email protected]>
> >> ---
> >> 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 @@
> >>
> >> 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?
Another question is, can we use vfio_ap_get_mdev_queue() in
handle_pqap() (instead of vfio_ap_get_queue())?
>
>
> > + 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;
> > }
> > (...)
> >
>
On Fri, 21 Aug 2020 15:56:02 -0400
Tony Krowiak <[email protected]> wrote:
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -26,43 +26,26 @@
>
> 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))
> + 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);
Is this cast here safe? (I don't think it is.)
> + put_device(&queue->ap_dev.device);
>
> return q;
> }
On 9/24/20 10:27 PM, Halil Pasic wrote:
> On Fri, 21 Aug 2020 15:56:02 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -26,43 +26,26 @@
>>
>> 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))
>> + 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);
> Is this cast here safe? (I don't think it is.)
In the probe, we execute:
dev_set_drvdata(&queue->ap_dev.device, q);
I don't get any compile nor execution errors. Why wouldn't it be safe?
>
>> + put_device(&queue->ap_dev.device);
>>
>> return q;
>> }
On Tue, 29 Sep 2020 09:07:40 -0400
Tony Krowiak <[email protected]> wrote:
>
>
> On 9/24/20 10:27 PM, Halil Pasic wrote:
> > On Fri, 21 Aug 2020 15:56:02 -0400
> > Tony Krowiak <[email protected]> wrote:
> >
> >> --- a/drivers/s390/crypto/vfio_ap_ops.c
> >> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> >> @@ -26,43 +26,26 @@
> >>
> >> 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))
> >> + 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);
> > Is this cast here safe? (I don't think it is.)
>
> In the probe, we execute:
> dev_set_drvdata(&queue->ap_dev.device, q);
>
> I don't get any compile nor execution errors. Why wouldn't it be safe?
>
Because the queue may or may not be bound to the vfio_ap driver. AFAICT
this function can be called with an arbitrary APQN.
If it is bound to another driver then drvdata is not likely to hold a
struct vfio_ap_queue.
> >
> >> + put_device(&queue->ap_dev.device);
> >>
> >> return q;
> >> }
>
On 9/29/20 9:37 AM, Halil Pasic wrote:
> On Tue, 29 Sep 2020 09:07:40 -0400
> Tony Krowiak <[email protected]> wrote:
>
>>
>> On 9/24/20 10:27 PM, Halil Pasic wrote:
>>> On Fri, 21 Aug 2020 15:56:02 -0400
>>> Tony Krowiak <[email protected]> wrote:
>>>
>>>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>>>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>>>> @@ -26,43 +26,26 @@
>>>>
>>>> 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))
>>>> + 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);
>>> Is this cast here safe? (I don't think it is.)
>> In the probe, we execute:
>> dev_set_drvdata(&queue->ap_dev.device, q);
>>
>> I don't get any compile nor execution errors. Why wouldn't it be safe?
>>
> Because the queue may or may not be bound to the vfio_ap driver. AFAICT
> this function can be called with an arbitrary APQN.
>
> If it is bound to another driver then drvdata is not likely to hold a
> struct vfio_ap_queue.
Then the function will return NULL. All callers must check for
NULL before using it which is the case in all places where this
function is called.
>
>
>>>> + put_device(&queue->ap_dev.device);
>>>>
>>>> return q;
>>>> }
On 9/24/20 10:11 PM, Halil Pasic wrote:
> On Thu, 27 Aug 2020 10:24:07 -0400
> Tony Krowiak <[email protected]> wrote:
>
>>
>> On 8/25/20 6:13 AM, Cornelia Huck wrote:
>>> On Fri, 21 Aug 2020 15:56:02 -0400
>>> Tony Krowiak<[email protected]> 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<[email protected]>
>>>> Reported-by: kernel test robot<[email protected]>
>>>> ---
>>>> 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;
>>> }
>>> (...)
>>>