* [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources
2022-08-23 15:06 [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Tony Krowiak
@ 2022-08-23 15:06 ` Tony Krowiak
2022-09-15 3:00 ` Halil Pasic
2022-08-23 15:06 ` [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev Tony Krowiak
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Tony Krowiak @ 2022-08-23 15:06 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, cohuck, mjrosato, pasic, alex.williamson, stable
It is not necessary to go through the process of validation, linking of
queues to mdev and vice versa and filtering the APQNs assigned to the
matrix mdev to build an AP configuration for a guest if an adapter or
domain being assigned is already assigned to the matrix mdev. Likewise, it
is not necessary to proceed through the process the unassignment of an
adapter, domain or control domain if it is not assigned to the matrix mdev.
Since it is not necessary to process assignment of a resource resource
already assigned or process unassignment of a resource that is been assigned,
this patch will bypass all assignment/unassignment operations for an adapter,
domain or control domain under these circumstances.
Not only is assignment of a duplicate adapter or domain unnecessary, it
will also cause a hang situation when removing the matrix mdev to which it is
assigned. The reason is because the same vfio_ap_queue objects with an
APQN containing the APID of the adapter or APQI of the domain being
assigned will get added multiple times to the hashtable that holds them.
This results in the pprev and next pointers of the hlist_node (mdev_qnode
field in the vfio_ap_queue object) pointing to the queue object itself
resulting in an interminable loop when the mdev is removed and the queue
table is iterated to reset the queues.
Cc: stable@vger.kernel.org
Fixes: 11cb2419fafe ("s390/vfio-ap: manage link between queue struct and matrix mdev")
Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
drivers/s390/crypto/vfio_ap_ops.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6c8c41fac4e1..ee82207b4e60 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -984,6 +984,11 @@ static ssize_t assign_adapter_store(struct device *dev,
goto done;
}
+ if (test_bit_inv(apid, matrix_mdev->matrix.apm)) {
+ ret = count;
+ goto done;
+ }
+
set_bit_inv(apid, matrix_mdev->matrix.apm);
ret = vfio_ap_mdev_validate_masks(matrix_mdev);
@@ -1109,6 +1114,11 @@ static ssize_t unassign_adapter_store(struct device *dev,
goto done;
}
+ if (!test_bit_inv(apid, matrix_mdev->matrix.apm)) {
+ ret = count;
+ goto done;
+ }
+
clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
vfio_ap_mdev_hot_unplug_adapter(matrix_mdev, apid);
ret = count;
@@ -1183,6 +1193,11 @@ static ssize_t assign_domain_store(struct device *dev,
goto done;
}
+ if (test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
+ ret = count;
+ goto done;
+ }
+
set_bit_inv(apqi, matrix_mdev->matrix.aqm);
ret = vfio_ap_mdev_validate_masks(matrix_mdev);
@@ -1286,6 +1301,11 @@ static ssize_t unassign_domain_store(struct device *dev,
goto done;
}
+ if (!test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
+ ret = count;
+ goto done;
+ }
+
clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
vfio_ap_mdev_hot_unplug_domain(matrix_mdev, apqi);
ret = count;
@@ -1329,6 +1349,11 @@ static ssize_t assign_control_domain_store(struct device *dev,
goto done;
}
+ if (test_bit_inv(id, matrix_mdev->matrix.adm)) {
+ ret = count;
+ goto done;
+ }
+
/* Set the bit in the ADM (bitmask) corresponding to the AP control
* domain number (id). The bits in the mask, from most significant to
* least significant, correspond to IDs 0 up to the one less than the
@@ -1378,6 +1403,11 @@ static ssize_t unassign_control_domain_store(struct device *dev,
goto done;
}
+ if (!test_bit_inv(domid, matrix_mdev->matrix.adm)) {
+ ret = count;
+ goto done;
+ }
+
clear_bit_inv(domid, matrix_mdev->matrix.adm);
if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
--
2.31.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources
2022-08-23 15:06 ` [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources Tony Krowiak
@ 2022-09-15 3:00 ` Halil Pasic
2022-09-15 14:53 ` Christian Borntraeger
0 siblings, 1 reply; 14+ messages in thread
From: Halil Pasic @ 2022-09-15 3:00 UTC (permalink / raw)
To: Tony Krowiak
Cc: linux-s390, linux-kernel, kvm, jjherne, borntraeger, cohuck,
mjrosato, alex.williamson, stable, Halil Pasic
On Tue, 23 Aug 2022 11:06:42 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:
> It is not necessary to go through the process of validation, linking of
> queues to mdev and vice versa and filtering the APQNs assigned to the
> matrix mdev to build an AP configuration for a guest if an adapter or
> domain being assigned is already assigned to the matrix mdev. Likewise, it
> is not necessary to proceed through the process the unassignment of an
> adapter, domain or control domain if it is not assigned to the matrix mdev.
>
> Since it is not necessary to process assignment of a resource resource
> already assigned or process unassignment of a resource that is been assigned,
> this patch will bypass all assignment/unassignment operations for an adapter,
> domain or control domain under these circumstances.
>
> Not only is assignment of a duplicate adapter or domain unnecessary, it
> will also cause a hang situation when removing the matrix mdev to which it is
> assigned. The reason is because the same vfio_ap_queue objects with an
> APQN containing the APID of the adapter or APQI of the domain being
> assigned will get added multiple times to the hashtable that holds them.
> This results in the pprev and next pointers of the hlist_node (mdev_qnode
> field in the vfio_ap_queue object) pointing to the queue object itself
> resulting in an interminable loop when the mdev is removed and the queue
> table is iterated to reset the queues.
>
> Cc: stable@vger.kernel.org
> Fixes: 11cb2419fafe ("s390/vfio-ap: manage link between queue struct and matrix mdev")
> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> ---
> drivers/s390/crypto/vfio_ap_ops.c | 30 ++++++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6c8c41fac4e1..ee82207b4e60 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -984,6 +984,11 @@ static ssize_t assign_adapter_store(struct device *dev,
> goto done;
> }
>
> + if (test_bit_inv(apid, matrix_mdev->matrix.apm)) {
> + ret = count;
> + goto done;
> + }
> +
> set_bit_inv(apid, matrix_mdev->matrix.apm);
>
> ret = vfio_ap_mdev_validate_masks(matrix_mdev);
> @@ -1109,6 +1114,11 @@ static ssize_t unassign_adapter_store(struct device *dev,
> goto done;
> }
>
> + if (!test_bit_inv(apid, matrix_mdev->matrix.apm)) {
> + ret = count;
> + goto done;
> + }
> +
> clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
> vfio_ap_mdev_hot_unplug_adapter(matrix_mdev, apid);
> ret = count;
> @@ -1183,6 +1193,11 @@ static ssize_t assign_domain_store(struct device *dev,
> goto done;
> }
>
> + if (test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
> + ret = count;
> + goto done;
> + }
> +
> set_bit_inv(apqi, matrix_mdev->matrix.aqm);
>
> ret = vfio_ap_mdev_validate_masks(matrix_mdev);
> @@ -1286,6 +1301,11 @@ static ssize_t unassign_domain_store(struct device *dev,
> goto done;
> }
>
> + if (!test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
> + ret = count;
> + goto done;
> + }
> +
> clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
> vfio_ap_mdev_hot_unplug_domain(matrix_mdev, apqi);
> ret = count;
> @@ -1329,6 +1349,11 @@ static ssize_t assign_control_domain_store(struct device *dev,
> goto done;
> }
>
> + if (test_bit_inv(id, matrix_mdev->matrix.adm)) {
> + ret = count;
> + goto done;
> + }
> +
> /* Set the bit in the ADM (bitmask) corresponding to the AP control
> * domain number (id). The bits in the mask, from most significant to
> * least significant, correspond to IDs 0 up to the one less than the
> @@ -1378,6 +1403,11 @@ static ssize_t unassign_control_domain_store(struct device *dev,
> goto done;
> }
>
> + if (!test_bit_inv(domid, matrix_mdev->matrix.adm)) {
> + ret = count;
> + goto done;
> + }
> +
> clear_bit_inv(domid, matrix_mdev->matrix.adm);
>
> if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources
2022-09-15 3:00 ` Halil Pasic
@ 2022-09-15 14:53 ` Christian Borntraeger
2022-09-15 15:03 ` Halil Pasic
0 siblings, 1 reply; 14+ messages in thread
From: Christian Borntraeger @ 2022-09-15 14:53 UTC (permalink / raw)
To: Halil Pasic, Tony Krowiak
Cc: linux-s390, linux-kernel, kvm, jjherne, cohuck, mjrosato,
alex.williamson, stable, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev
Am 15.09.22 um 05:00 schrieb Halil Pasic:
> On Tue, 23 Aug 2022 11:06:42 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> wrote:
>
>> It is not necessary to go through the process of validation, linking of
>> queues to mdev and vice versa and filtering the APQNs assigned to the
>> matrix mdev to build an AP configuration for a guest if an adapter or
>> domain being assigned is already assigned to the matrix mdev. Likewise, it
>> is not necessary to proceed through the process the unassignment of an
>> adapter, domain or control domain if it is not assigned to the matrix mdev.
>>
>> Since it is not necessary to process assignment of a resource resource
>> already assigned or process unassignment of a resource that is been assigned,
>> this patch will bypass all assignment/unassignment operations for an adapter,
>> domain or control domain under these circumstances.
>>
>> Not only is assignment of a duplicate adapter or domain unnecessary, it
>> will also cause a hang situation when removing the matrix mdev to which it is
>> assigned. The reason is because the same vfio_ap_queue objects with an
>> APQN containing the APID of the adapter or APQI of the domain being
>> assigned will get added multiple times to the hashtable that holds them.
>> This results in the pprev and next pointers of the hlist_node (mdev_qnode
>> field in the vfio_ap_queue object) pointing to the queue object itself
>> resulting in an interminable loop when the mdev is removed and the queue
>> table is iterated to reset the queues.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 11cb2419fafe ("s390/vfio-ap: manage link between queue struct and matrix mdev")
>> Reported-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Shall the patch go via the s390 tree (still into 6.0 I guess)?
>
>> ---
>> drivers/s390/crypto/vfio_ap_ops.c | 30 ++++++++++++++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 6c8c41fac4e1..ee82207b4e60 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -984,6 +984,11 @@ static ssize_t assign_adapter_store(struct device *dev,
>> goto done;
>> }
>>
>> + if (test_bit_inv(apid, matrix_mdev->matrix.apm)) {
>> + ret = count;
>> + goto done;
>> + }
>> +
>> set_bit_inv(apid, matrix_mdev->matrix.apm);
>>
>> ret = vfio_ap_mdev_validate_masks(matrix_mdev);
>> @@ -1109,6 +1114,11 @@ static ssize_t unassign_adapter_store(struct device *dev,
>> goto done;
>> }
>>
>> + if (!test_bit_inv(apid, matrix_mdev->matrix.apm)) {
>> + ret = count;
>> + goto done;
>> + }
>> +
>> clear_bit_inv((unsigned long)apid, matrix_mdev->matrix.apm);
>> vfio_ap_mdev_hot_unplug_adapter(matrix_mdev, apid);
>> ret = count;
>> @@ -1183,6 +1193,11 @@ static ssize_t assign_domain_store(struct device *dev,
>> goto done;
>> }
>>
>> + if (test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
>> + ret = count;
>> + goto done;
>> + }
>> +
>> set_bit_inv(apqi, matrix_mdev->matrix.aqm);
>>
>> ret = vfio_ap_mdev_validate_masks(matrix_mdev);
>> @@ -1286,6 +1301,11 @@ static ssize_t unassign_domain_store(struct device *dev,
>> goto done;
>> }
>>
>> + if (!test_bit_inv(apqi, matrix_mdev->matrix.aqm)) {
>> + ret = count;
>> + goto done;
>> + }
>> +
>> clear_bit_inv((unsigned long)apqi, matrix_mdev->matrix.aqm);
>> vfio_ap_mdev_hot_unplug_domain(matrix_mdev, apqi);
>> ret = count;
>> @@ -1329,6 +1349,11 @@ static ssize_t assign_control_domain_store(struct device *dev,
>> goto done;
>> }
>>
>> + if (test_bit_inv(id, matrix_mdev->matrix.adm)) {
>> + ret = count;
>> + goto done;
>> + }
>> +
>> /* Set the bit in the ADM (bitmask) corresponding to the AP control
>> * domain number (id). The bits in the mask, from most significant to
>> * least significant, correspond to IDs 0 up to the one less than the
>> @@ -1378,6 +1403,11 @@ static ssize_t unassign_control_domain_store(struct device *dev,
>> goto done;
>> }
>>
>> + if (!test_bit_inv(domid, matrix_mdev->matrix.adm)) {
>> + ret = count;
>> + goto done;
>> + }
>> +
>> clear_bit_inv(domid, matrix_mdev->matrix.adm);
>>
>> if (test_bit_inv(domid, matrix_mdev->shadow_apcb.adm)) {
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources
2022-09-15 14:53 ` Christian Borntraeger
@ 2022-09-15 15:03 ` Halil Pasic
0 siblings, 0 replies; 14+ messages in thread
From: Halil Pasic @ 2022-09-15 15:03 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Tony Krowiak, linux-s390, linux-kernel, kvm, jjherne, cohuck,
mjrosato, alex.williamson, stable, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Halil Pasic
On Thu, 15 Sep 2022 16:53:51 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>
> Shall the patch go via the s390 tree (still into 6.0 I guess)?
Yes please!
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev
2022-08-23 15:06 [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Tony Krowiak
2022-08-23 15:06 ` [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources Tony Krowiak
@ 2022-08-23 15:06 ` Tony Krowiak
2022-09-13 14:07 ` Halil Pasic
2022-08-29 13:28 ` [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Anthony Krowiak
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Tony Krowiak @ 2022-08-23 15:06 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, cohuck, mjrosato, pasic, alex.williamson, stable
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: stable@vger.kernel.org
Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
Reported-by: Tony Krowiak <akrowiak@linux.ibm.com>
Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev
2022-08-23 15:06 ` [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev Tony Krowiak
@ 2022-09-13 14:07 ` Halil Pasic
2022-09-13 14:40 ` Anthony Krowiak
2022-09-13 14:53 ` Anthony Krowiak
0 siblings, 2 replies; 14+ messages in thread
From: Halil Pasic @ 2022-09-13 14:07 UTC (permalink / raw)
To: Tony Krowiak
Cc: linux-s390, linux-kernel, kvm, jjherne, borntraeger, cohuck,
mjrosato, alex.williamson, stable, Halil Pasic
On Tue, 23 Aug 2022 11:06:43 -0400
Tony Krowiak <akrowiak@linux.ibm.com> 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: stable@vger.kernel.org
> Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
> Reported-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
> 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
> }
> }
> }
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev
2022-09-13 14:07 ` Halil Pasic
@ 2022-09-13 14:40 ` Anthony Krowiak
2022-09-13 14:53 ` Anthony Krowiak
1 sibling, 0 replies; 14+ messages in thread
From: Anthony Krowiak @ 2022-09-13 14:40 UTC (permalink / raw)
To: Halil Pasic
Cc: linux-s390, linux-kernel, kvm, jjherne, borntraeger, cohuck,
mjrosato, alex.williamson, stable
On 9/13/22 10:07 AM, Halil Pasic wrote:
> On Tue, 23 Aug 2022 11:06:43 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> 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: stable@vger.kernel.org
>> Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
>> Reported-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>> 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
>
>> }
>> }
>> }
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev
2022-09-13 14:07 ` Halil Pasic
2022-09-13 14:40 ` Anthony Krowiak
@ 2022-09-13 14:53 ` Anthony Krowiak
1 sibling, 0 replies; 14+ messages in thread
From: Anthony Krowiak @ 2022-09-13 14:53 UTC (permalink / raw)
To: Halil Pasic
Cc: linux-s390, linux-kernel, kvm, jjherne, borntraeger, cohuck,
mjrosato, alex.williamson, stable
On 9/13/22 10:07 AM, Halil Pasic wrote:
> On Tue, 23 Aug 2022 11:06:43 -0400
> Tony Krowiak <akrowiak@linux.ibm.com> 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: stable@vger.kernel.org
>> Fixes: 70aeefe574cb ("s390/vfio-ap: reset queues after adapter/domain unassignment")
>> Reported-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> ---
>> 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
>
>> }
>> }
>> }
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver
2022-08-23 15:06 [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Tony Krowiak
2022-08-23 15:06 ` [PATCH v3 1/2] s390/vfio-ap: bypass unnecessary processing of AP resources Tony Krowiak
2022-08-23 15:06 ` [PATCH v3 2/2] s390/vfio-ap: fix unlinking of queues from the mdev Tony Krowiak
@ 2022-08-29 13:28 ` Anthony Krowiak
2022-09-06 18:22 ` Anthony Krowiak
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Anthony Krowiak @ 2022-08-29 13:28 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, cohuck, mjrosato, pasic, alex.williamson
These two patches fix a couple of bugs in the vfio_ap driver code, it
would be nice to get some r-b action so we can merge these fixes ASAP.
Thanks.
On 8/23/22 11:06 AM, Tony Krowiak wrote:
> Two problems have been discovered with the vfio_ap device driver since the
> hot plug support was recently introduced:
>
> 1. Attempting to remove a matrix mdev after assigning a duplicate adapter
> or duplicate domain results in a hang.
>
> 2. The queues associated with an adapter or domain being unassigned from
> the matrix mdev do not get unlinked from it.
>
> Two patches are provided to resolve these problems.
>
> Change log v2 => v3:
> --------------------
> * Replaced the wrong commit IDs in the 'Fixes' tags in both patches.
> (Halil and Alexander)
>
> * Changed the subject line and description of patch 01/02 to better reflect the
> code changes in the patch. (Halil)
>
> Tony Krowiak (2):
> s390/vfio-ap: bypass unnecessary processing of AP resources
> s390/vfio-ap: fix unlinking of queues from the mdev
>
> drivers/s390/crypto/vfio_ap_ops.c | 36 +++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver
2022-08-23 15:06 [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Tony Krowiak
` (2 preceding siblings ...)
2022-08-29 13:28 ` [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Anthony Krowiak
@ 2022-09-06 18:22 ` Anthony Krowiak
2022-09-06 23:17 ` Anthony Krowiak
2022-09-13 13:22 ` Anthony Krowiak
5 siblings, 0 replies; 14+ messages in thread
From: Anthony Krowiak @ 2022-09-06 18:22 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, cohuck, mjrosato, pasic, alex.williamson
PING!
On 8/23/22 11:06 AM, Tony Krowiak wrote:
> Two problems have been discovered with the vfio_ap device driver since the
> hot plug support was recently introduced:
>
> 1. Attempting to remove a matrix mdev after assigning a duplicate adapter
> or duplicate domain results in a hang.
>
> 2. The queues associated with an adapter or domain being unassigned from
> the matrix mdev do not get unlinked from it.
>
> Two patches are provided to resolve these problems.
>
> Change log v2 => v3:
> --------------------
> * Replaced the wrong commit IDs in the 'Fixes' tags in both patches.
> (Halil and Alexander)
>
> * Changed the subject line and description of patch 01/02 to better reflect the
> code changes in the patch. (Halil)
>
> Tony Krowiak (2):
> s390/vfio-ap: bypass unnecessary processing of AP resources
> s390/vfio-ap: fix unlinking of queues from the mdev
>
> drivers/s390/crypto/vfio_ap_ops.c | 36 +++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver
2022-08-23 15:06 [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Tony Krowiak
` (3 preceding siblings ...)
2022-09-06 18:22 ` Anthony Krowiak
@ 2022-09-06 23:17 ` Anthony Krowiak
2022-09-07 17:19 ` Halil Pasic
2022-09-13 13:22 ` Anthony Krowiak
5 siblings, 1 reply; 14+ messages in thread
From: Anthony Krowiak @ 2022-09-06 23:17 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, cohuck, mjrosato, pasic, alex.williamson
PING?
On 8/23/22 11:06 AM, Tony Krowiak wrote:
> Two problems have been discovered with the vfio_ap device driver since the
> hot plug support was recently introduced:
>
> 1. Attempting to remove a matrix mdev after assigning a duplicate adapter
> or duplicate domain results in a hang.
>
> 2. The queues associated with an adapter or domain being unassigned from
> the matrix mdev do not get unlinked from it.
>
> Two patches are provided to resolve these problems.
>
> Change log v2 => v3:
> --------------------
> * Replaced the wrong commit IDs in the 'Fixes' tags in both patches.
> (Halil and Alexander)
>
> * Changed the subject line and description of patch 01/02 to better reflect the
> code changes in the patch. (Halil)
>
> Tony Krowiak (2):
> s390/vfio-ap: bypass unnecessary processing of AP resources
> s390/vfio-ap: fix unlinking of queues from the mdev
>
> drivers/s390/crypto/vfio_ap_ops.c | 36 +++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver
2022-09-06 23:17 ` Anthony Krowiak
@ 2022-09-07 17:19 ` Halil Pasic
0 siblings, 0 replies; 14+ messages in thread
From: Halil Pasic @ 2022-09-07 17:19 UTC (permalink / raw)
To: Anthony Krowiak
Cc: linux-s390, linux-kernel, kvm, jjherne, borntraeger, cohuck,
mjrosato, alex.williamson, Halil Pasic
On Tue, 6 Sep 2022 19:17:50 -0400
Anthony Krowiak <akrowiak@linux.ibm.com> wrote:
> PING?
I'm looking at the series. Expect results soon :D
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver
2022-08-23 15:06 [PATCH v3 0/2] s390/vfio-ap: fix two problems discovered in the vfio_ap driver Tony Krowiak
` (4 preceding siblings ...)
2022-09-06 23:17 ` Anthony Krowiak
@ 2022-09-13 13:22 ` Anthony Krowiak
5 siblings, 0 replies; 14+ messages in thread
From: Anthony Krowiak @ 2022-09-13 13:22 UTC (permalink / raw)
To: linux-s390, linux-kernel, kvm
Cc: jjherne, borntraeger, cohuck, mjrosato, pasic, alex.williamson
These are pretty simple, straight forward patches that fix a couple of
issues recently introduced in the vfio_ap device driver. I would greatly
appreciate it if someone could review these so we can get the fixes into
the kernel. Thank you in advance for your attention to this.
On 8/23/22 11:06 AM, Tony Krowiak wrote:
> Two problems have been discovered with the vfio_ap device driver since the
> hot plug support was recently introduced:
>
> 1. Attempting to remove a matrix mdev after assigning a duplicate adapter
> or duplicate domain results in a hang.
>
> 2. The queues associated with an adapter or domain being unassigned from
> the matrix mdev do not get unlinked from it.
>
> Two patches are provided to resolve these problems.
>
> Change log v2 => v3:
> --------------------
> * Replaced the wrong commit IDs in the 'Fixes' tags in both patches.
> (Halil and Alexander)
>
> * Changed the subject line and description of patch 01/02 to better reflect the
> code changes in the patch. (Halil)
>
> Tony Krowiak (2):
> s390/vfio-ap: bypass unnecessary processing of AP resources
> s390/vfio-ap: fix unlinking of queues from the mdev
>
> drivers/s390/crypto/vfio_ap_ops.c | 36 +++++++++++++++++++++++++++----
> 1 file changed, 32 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread