2022-08-23 17:50:01

by Anthony Krowiak

Subject: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev

The vfio_ap_mdev_unlink_adapter and vfio_ap_mdev_unlink_domain functions
add the associated vfio_ap_queue objects to the hashtable that links them
to the matrix mdev to which their APQN is assigned. In order to unlink
them, they must be deleted from the hashtable; if not, they will continue
to be reset whenever userspace closes the mdev fd or removes the mdev.
This patch fixes that issue.

Cc: [email protected]
Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
Reported-by: Tony Krowiak <[email protected]>
Signed-off-by: Tony Krowiak <[email protected]>
---
drivers/s390/crypto/vfio_ap_ops.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index ee82207b4e60..2493926b5dfb 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1049,8 +1049,7 @@ static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
if (q && qtable) {
if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
- hash_add(qtable->queues, &q->mdev_qnode,
- q->apqn);
+ vfio_ap_unlink_queue_fr_mdev(q);
}
}
}
@@ -1236,8 +1235,7 @@ static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
if (q && qtable) {
if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
- hash_add(qtable->queues, &q->mdev_qnode,
- q->apqn);
+ vfio_ap_unlink_queue_fr_mdev(q);
}
}
}
--
2.31.1

2022-09-13 15:25:32

by Halil Pasic

Subject: Re: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev

On Tue, 23 Aug 2022 11:06:43 -0400
Tony Krowiak <[email protected]> wrote:

> The vfio_ap_mdev_unlink_adapter and vfio_ap_mdev_unlink_domain functions
> add the associated vfio_ap_queue objects to the hashtable that links them
> to the matrix mdev to which their APQN is assigned. In order to unlink
> them, they must be deleted from the hashtable; if not, they will continue
> to be reset whenever userspace closes the mdev fd or removes the mdev.
> This patch fixes that issue.

I'm not so sure about that!

>
> Cc: [email protected]
> Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
> Reported-by: Tony Krowiak <[email protected]>
> Signed-off-by: Tony Krowiak <[email protected]>
> ---
> drivers/s390/crypto/vfio_ap_ops.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index ee82207b4e60..2493926b5dfb 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1049,8 +1049,7 @@ static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
> if (q && qtable) {
> if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
> test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
> - hash_add(qtable->queues, &q->mdev_qnode,
> - q->apqn);

Careful qtable->queues is not supposed to be the same as
matrix_mdev->qtable.queues it is rather a function local
qtable that you use to know which queues were unlinked and
need resetting.

Have a look at vfio_ap_mdev_hot_unplug_adapter()

> + vfio_ap_unlink_queue_fr_mdev(q);

IMHO this change is completely bogous. BTW
vfio_ap_unlink_apqn_fr_mdev() a couple of lines above in the source
(not seen in diff context) calls vfio_ap_unlink_queue_fr_mdev().

> }
> }
> }
> @@ -1236,8 +1235,7 @@ static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
> if (q && qtable) {
> if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
> test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
> - hash_add(qtable->queues, &q->mdev_qnode,
> - q->apqn);
> + vfio_ap_unlink_queue_fr_mdev(q);

Same as above...

Regards,
Halil

> }
> }
> }

2022-09-13 17:20:28

by Anthony Krowiak

Subject: Re: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev

On 9/13/22 10:07 AM, Halil Pasic wrote:
> On Tue, 23 Aug 2022 11:06:43 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> The vfio_ap_mdev_unlink_adapter and vfio_ap_mdev_unlink_domain functions
>> add the associated vfio_ap_queue objects to the hashtable that links them
>> to the matrix mdev to which their APQN is assigned. In order to unlink
>> them, they must be deleted from the hashtable; if not, they will continue
>> to be reset whenever userspace closes the mdev fd or removes the mdev.
>> This patch fixes that issue.
> I'm not so sure about that!
>
>> Cc: [email protected]
>> Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
>> Reported-by: Tony Krowiak <[email protected]>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index ee82207b4e60..2493926b5dfb 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -1049,8 +1049,7 @@ static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
>> if (q && qtable) {
>> if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>> test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>> - hash_add(qtable->queues, &q->mdev_qnode,
>> - q->apqn);
> Careful qtable->queues is not supposed to be the same as
> matrix_mdev->qtable.queues it is rather a function local
> qtable that you use to know which queues were unlinked and
> need resetting.


You are correct. This patch is unnecessary.


>
> Have a look at vfio_ap_mdev_hot_unplug_adapter()
>
>> + vfio_ap_unlink_queue_fr_mdev(q);
> IMHO this change is completely bogous. BTW
> vfio_ap_unlink_apqn_fr_mdev() a couple of lines above in the source
> (not seen in diff context) calls vfio_ap_unlink_queue_fr_mdev().


After further review, this patch is not only bogus, it is not necessary.


>
>> }
>> }
>> }
>> @@ -1236,8 +1235,7 @@ static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
>> if (q && qtable) {
>> if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>> test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>> - hash_add(qtable->queues, &q->mdev_qnode,
>> - q->apqn);
>> + vfio_ap_unlink_queue_fr_mdev(q);
> Same as above...
>
> Regards,
> Halil
>
>> }
>> }
>> }

2022-09-13 17:27:21

by Anthony Krowiak

Subject: Re: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev

On 9/13/22 10:07 AM, Halil Pasic wrote:
> On Tue, 23 Aug 2022 11:06:43 -0400
> Tony Krowiak <[email protected]> wrote:
>
>> The vfio_ap_mdev_unlink_adapter and vfio_ap_mdev_unlink_domain functions
>> add the associated vfio_ap_queue objects to the hashtable that links them
>> to the matrix mdev to which their APQN is assigned. In order to unlink
>> them, they must be deleted from the hashtable; if not, they will continue
>> to be reset whenever userspace closes the mdev fd or removes the mdev.
>> This patch fixes that issue.
> I'm not so sure about that!
>
>> Cc: [email protected]
>> Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
>> Reported-by: Tony Krowiak <[email protected]>
>> Signed-off-by: Tony Krowiak <[email protected]>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index ee82207b4e60..2493926b5dfb 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -1049,8 +1049,7 @@ static void vfio_ap_mdev_unlink_adapter(struct ap_matrix_mdev *matrix_mdev,
>> if (q && qtable) {
>> if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>> test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>> - hash_add(qtable->queues, &q->mdev_qnode,
>> - q->apqn);
> Careful qtable->queues is not supposed to be the same as
> matrix_mdev->qtable.queues it is rather a function local
> qtable that you use to know which queues were unlinked and
> need resetting.
>
> Have a look at vfio_ap_mdev_hot_unplug_adapter()
>
>> + vfio_ap_unlink_queue_fr_mdev(q);
> IMHO this change is completely bogous. BTW
> vfio_ap_unlink_apqn_fr_mdev() a couple of lines above in the source
> (not seen in diff context) calls vfio_ap_unlink_queue_fr_mdev().


Wow! After looking at this in context I agree, it is bogus. I've got to
figure out what happened to this function between commit f8de623330c6
("s390/vfio-ap: manage link between queue struct and matrix mdev") and
this patch. Somewhere along the line it got changed. I'll get to the
bottom of it and fix this issue then resubmit this patch.


>
>> }
>> }
>> }
>> @@ -1236,8 +1235,7 @@ static void vfio_ap_mdev_unlink_domain(struct ap_matrix_mdev *matrix_mdev,
>> if (q && qtable) {
>> if (test_bit_inv(apid, matrix_mdev->shadow_apcb.apm) &&
>> test_bit_inv(apqi, matrix_mdev->shadow_apcb.aqm))
>> - hash_add(qtable->queues, &q->mdev_qnode,
>> - q->apqn);
>> + vfio_ap_unlink_queue_fr_mdev(q);
> Same as above...
>
> Regards,
> Halil
>
>> }
>> }
>> }