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: [email protected]
Fixes: 11cb2419fafe ("s390/vfio-ap: manage link between queue struct and matrix mdev")
Reported-by: Matthew Rosato <[email protected]>
Signed-off-by: Tony Krowiak <[email protected]>
---
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
On Tue, 23 Aug 2022 11:06:42 -0400
Tony Krowiak <[email protected]> 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: [email protected]
> Fixes: 11cb2419fafe ("s390/vfio-ap: manage link between queue struct and matrix mdev")
> Reported-by: Matthew Rosato <[email protected]>
> Signed-off-by: Tony Krowiak <[email protected]>
Reviewed-by: Halil Pasic <[email protected]>
> ---
> 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)) {
Am 15.09.22 um 05:00 schrieb Halil Pasic:
> On Tue, 23 Aug 2022 11:06:42 -0400
> Tony Krowiak <[email protected]> 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: [email protected]
>> Fixes: 11cb2419fafe ("s390/vfio-ap: manage link between queue struct and matrix mdev")
>> Reported-by: Matthew Rosato <[email protected]>
>> Signed-off-by: Tony Krowiak <[email protected]>
>
> Reviewed-by: Halil Pasic <[email protected]>
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)) {
>
On Thu, 15 Sep 2022 16:53:51 +0200
Christian Borntraeger <[email protected]> wrote:
> > Reviewed-by: Halil Pasic <[email protected]>
>
> Shall the patch go via the s390 tree (still into 6.0 I guess)?
Yes please!