[PATCH v2 0/6] improve AP queue reset processing

[PATCH v2 0/6] improve AP queue reset processing

[Tony Krowiak]

This series introduces several improvements to the function that performs
AP queue resets:

* Breaks up reset processing into multiple smaller, more concise functions.

* Use TAPQ to verify completion of a reset in progress rather than mulitple
  invocations of ZAPQ.

* Check TAPQ response codes when verifying successful completion of ZAPQ.

* Fix erroneous handling of some error response codes.

* Increase the maximum amount of time to wait for successful completion of
  ZAPQ.
 
Change log v1 => v2:
-------------------
Remove patch 7/7 to restore original behavior since we don't know whether
interrupts are disabled when an unexpected response code is returned from 
ZAPQ. (Halil)

Tony Krowiak (6):
  s390/vfio-ap: verify reset complete in separate function
  s390/vfio_ap: check TAPQ response code when waiting for queue reset
  s390/vfio_ap: use TAPQ to verify reset in progress completes
  s390/vfio_ap: verify ZAPQ completion after return of response code
    zero
  s390/vfio_ap: fix handling of error response codes
  s390/vfio_ap: increase max wait time for reset verification

 drivers/s390/crypto/vfio_ap_ops.c | 104 +++++++++++++++++++++---------
 1 file changed, 72 insertions(+), 32 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] s390/vfio-ap: verify reset complete in separate function

[Tony Krowiak]

The vfio_ap_mdev_reset_queue() function contains a loop to verify that the
reset successfully completes within 40ms. This patch moves that loop into
a separate function.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 9c01957e56b3..7523496bfbae 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1598,12 +1598,30 @@ static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
 	return q;
 }
 
+static int apq_reset_check(struct vfio_ap_queue *q)
+{
+	int iters = 2;
+	struct ap_queue_status status;
+
+	while (iters--) {
+		msleep(20);
+		status = ap_tapq(q->apqn, NULL);
+		if (status.queue_empty && !status.irq_enabled)
+			return 0;
+	}
+	WARN_ONCE(iters <= 0,
+		  "timeout verifying reset of queue %02x.%04x (%u, %u, %u)",
+		  AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
+		  status.queue_empty, status.irq_enabled, status.response_code);
+
+	return -EBUSY;
+}
+
 static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
 				    unsigned int retry)
 {
 	struct ap_queue_status status;
 	int ret;
-	int retry2 = 2;
 
 	if (!q)
 		return 0;
@@ -1640,14 +1658,8 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
 	}
 
 	/* wait for the reset to take effect */
-	while (retry2--) {
-		if (status.queue_empty && !status.irq_enabled)
-			break;
-		msleep(20);
-		status = ap_tapq(q->apqn, NULL);
-	}
-	WARN_ONCE(retry2 <= 0, "unable to verify reset of queue %02x.%04x",
-		  AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn));
+	if (!(status.queue_empty && !status.irq_enabled))
+		ret = apq_reset_check(q);
 
 free_resources:
 	vfio_ap_free_aqic_resources(q);
-- 
2.31.1

[PATCH v2 2/6] s390/vfio_ap: check TAPQ response code when waiting for queue reset

[Tony Krowiak]

The vfio_ap_mdev_reset_queue() function does not check the status
response code returned form the PQAP(TAPQ) function when verifying the
queue's status; consequently, there is no way of knowing whether
verification failed because the wait time was exceeded, or because the
PQAP(TAPQ) failed.

This patch adds a function to check the status response code from the
PQAP(TAPQ) instruction and logs an appropriate message if it fails.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 36 ++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 7523496bfbae..b27daaaa79b4 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1598,23 +1598,49 @@ static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
 	return q;
 }
 
+static int apq_status_check(int apqn, struct ap_queue_status *status)
+{
+	switch (status->response_code) {
+	case AP_RESPONSE_NORMAL:
+	case AP_RESPONSE_RESET_IN_PROGRESS:
+		if (status->queue_empty && !status->irq_enabled)
+			return 0;
+		return -EBUSY;
+	case AP_RESPONSE_DECONFIGURED:
+		/*
+		 * If the AP queue is deconfigured, any subsequent AP command
+		 * targeting the queue will fail with the same response code. On the
+		 * other hand, when an AP adapter is deconfigured, the associated
+		 * queues are reset, so let's return a value indicating the reset
+		 * for which we're waiting completed successfully.
+		 */
+		return 0;
+	default:
+		WARN(true,
+		     "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n",
+		     AP_QID_CARD(apqn), AP_QID_QUEUE(apqn),
+		     status->response_code);
+		return -EIO;
+	}
+}
+
 static int apq_reset_check(struct vfio_ap_queue *q)
 {
-	int iters = 2;
+	int iters = 2, ret;
 	struct ap_queue_status status;
 
 	while (iters--) {
 		msleep(20);
 		status = ap_tapq(q->apqn, NULL);
-		if (status.queue_empty && !status.irq_enabled)
-			return 0;
+		ret = apq_status_check(q->apqn, &status);
+		if (ret != -EBUSY)
+			return ret;
 	}
 	WARN_ONCE(iters <= 0,
 		  "timeout verifying reset of queue %02x.%04x (%u, %u, %u)",
 		  AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
 		  status.queue_empty, status.irq_enabled, status.response_code);
-
-	return -EBUSY;
+	return ret;
 }
 
 static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
-- 
2.31.1

[PATCH v2 3/6] s390/vfio_ap: use TAPQ to verify reset in progress completes

[Tony Krowiak]

To eliminate the repeated calls to the PQAP(ZAPQ) function to verify that
a reset in progress completed successfully and ensure that error response
codes get appropriately logged, let's call the apq_reset_check() function
when the ZAPQ response code indicates that a reset that is already in
progress.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index b27daaaa79b4..a443ee5f7789 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -33,7 +33,7 @@
 static int vfio_ap_mdev_reset_queues(struct ap_queue_table *qtable);
 static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 static const struct vfio_device_ops vfio_ap_matrix_dev_ops;
-static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q, unsigned int retry);
+static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q);
 
 /**
  * get_update_locks_for_kvm: Acquire the locks required to dynamically update a
@@ -1643,8 +1643,7 @@ static int apq_reset_check(struct vfio_ap_queue *q)
 	return ret;
 }
 
-static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
-				    unsigned int retry)
+static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 {
 	struct ap_queue_status status;
 	int ret;
@@ -1659,12 +1658,15 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q,
 		ret = 0;
 		break;
 	case AP_RESPONSE_RESET_IN_PROGRESS:
-		if (retry--) {
-			msleep(20);
-			goto retry_zapq;
-		}
-		ret = -EBUSY;
-		break;
+		/*
+		 * There is a reset issued by another process in progress. Let's wait
+		 * for that to complete. Since we have no idea whether it was a RAPQ or
+		 * ZAPQ, then if it completes successfully, let's issue the ZAPQ.
+		 */
+		ret = apq_reset_check(q);
+		if (ret)
+			break;
+		goto retry_zapq;
 	case AP_RESPONSE_Q_NOT_AVAIL:
 	case AP_RESPONSE_DECONFIGURED:
 	case AP_RESPONSE_CHECKSTOPPED:
@@ -1699,7 +1701,7 @@ static int vfio_ap_mdev_reset_queues(struct ap_queue_table *qtable)
 	struct vfio_ap_queue *q;
 
 	hash_for_each(qtable->queues, loop_cursor, q, mdev_qnode) {
-		ret = vfio_ap_mdev_reset_queue(q, 1);
+		ret = vfio_ap_mdev_reset_queue(q);
 		/*
 		 * Regardless whether a queue turns out to be busy, or
 		 * is not operational, we need to continue resetting
@@ -1944,7 +1946,7 @@ void vfio_ap_mdev_remove_queue(struct ap_device *apdev)
 		}
 	}
 
-	vfio_ap_mdev_reset_queue(q, 1);
+	vfio_ap_mdev_reset_queue(q);
 	dev_set_drvdata(&apdev->device, NULL);
 	kfree(q);
 	release_update_locks_for_mdev(matrix_mdev);
-- 
2.31.1

[PATCH v2 4/6] s390/vfio_ap: verify ZAPQ completion after return of response code zero

[Tony Krowiak]

Verification that the asynchronous ZAPQ function has completed only needs
to be done when the response code indicates the function was successfully
initiated; so, let's call the apq_reset_check function immediately after
the response code zero is returned from the ZAPQ.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a443ee5f7789..e1b05e749570 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1656,6 +1656,9 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 	switch (status.response_code) {
 	case AP_RESPONSE_NORMAL:
 		ret = 0;
+		/* if the reset has not completed, wait for it to take effect */
+		if (!status.queue_empty || status.irq_enabled)
+			ret = apq_reset_check(q);
 		break;
 	case AP_RESPONSE_RESET_IN_PROGRESS:
 		/*
@@ -1685,10 +1688,6 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 		return -EIO;
 	}
 
-	/* wait for the reset to take effect */
-	if (!(status.queue_empty && !status.irq_enabled))
-		ret = apq_reset_check(q);
-
 free_resources:
 	vfio_ap_free_aqic_resources(q);
 
-- 
2.31.1

[PATCH v2 5/6] s390/vfio_ap: fix handling of error response codes

[Tony Krowiak]

Some response codes returned from the queue reset function are not being
handled correctly; this patch fixes them:

1. Response code 3, AP queue deconfigured: Deconfiguring an AP adapter
   resets all of its queues, so this is handled by indicating the reset
   verification completed successfully.

2. For all response codes other than 0 (normal reset completion), 2
   (queue reset in progress) and 3 (AP deconfigured), the -EIO error will
   be returned from the vfio_ap_mdev_reset_queue() function. In all cases,
   all fields of the status word other than the response code will be
   set to zero, so it makes no sense to check status bits.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e1b05e749570..68be34362680 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1670,17 +1670,15 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 		if (ret)
 			break;
 		goto retry_zapq;
-	case AP_RESPONSE_Q_NOT_AVAIL:
 	case AP_RESPONSE_DECONFIGURED:
-	case AP_RESPONSE_CHECKSTOPPED:
-		WARN_ONCE(status.irq_enabled,
-			  "PQAP/ZAPQ for %02x.%04x failed with rc=%u while IRQ enabled",
-			  AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
-			  status.response_code);
-		ret = -EBUSY;
-		goto free_resources;
+		/*
+		 * When an AP adapter is deconfigured, the associated
+		 * queues are reset, so let's return a value indicating the reset
+		 * completed successfully.
+		 */
+		ret = 0;
+		break;
 	default:
-		/* things are really broken, give up */
 		WARN(true,
 		     "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
 		     AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn),
@@ -1688,7 +1686,6 @@ static int vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q)
 		return -EIO;
 	}
 
-free_resources:
 	vfio_ap_free_aqic_resources(q);
 
 	return ret;
-- 
2.31.1

[PATCH v2 6/6] s390/vfio_ap: increase max wait time for reset verification

[Tony Krowiak]

Increase the maximum time to wait for verification of a queue reset
operation to 200ms.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 68be34362680..d665e1bc494a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -30,6 +30,9 @@
 #define AP_QUEUE_UNASSIGNED "unassigned"
 #define AP_QUEUE_IN_USE "in use"
 
+#define MAX_RESET_CHECK_WAIT	200	/* Sleep max 200ms for reset check	*/
+#define AP_RESET_INTERVAL		20	/* Reset sleep interval (20ms)		*/
+
 static int vfio_ap_mdev_reset_queues(struct ap_queue_table *qtable);
 static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 static const struct vfio_device_ops vfio_ap_matrix_dev_ops;
@@ -1626,11 +1629,12 @@ static int apq_status_check(int apqn, struct ap_queue_status *status)
 
 static int apq_reset_check(struct vfio_ap_queue *q)
 {
-	int iters = 2, ret;
+	int ret;
+	int iters = MAX_RESET_CHECK_WAIT / AP_RESET_INTERVAL;
 	struct ap_queue_status status;
 
-	while (iters--) {
-		msleep(20);
+	for (; iters > 0; iters--) {
+		msleep(AP_RESET_INTERVAL);
 		status = ap_tapq(q->apqn, NULL);
 		ret = apq_status_check(q->apqn, &status);
 		if (ret != -EBUSY)
-- 
2.31.1

Re: [PATCH v2 0/6] improve AP queue reset processing

[Christian Borntraeger]

Am 18.01.23 um 21:31 schrieb Tony Krowiak:
> This series introduces several improvements to the function that performs
> AP queue resets:
> 
> * Breaks up reset processing into multiple smaller, more concise functions.
> 
> * Use TAPQ to verify completion of a reset in progress rather than mulitple
>    invocations of ZAPQ.
> 
> * Check TAPQ response codes when verifying successful completion of ZAPQ.
> 
> * Fix erroneous handling of some error response codes.
> 
> * Increase the maximum amount of time to wait for successful completion of
>    ZAPQ.
>   
> Change log v1 => v2:
> -------------------
> Remove patch 7/7 to restore original behavior since we don't know whether
> interrupts are disabled when an unexpected response code is returned from
> ZAPQ. (Halil)
> 
> Tony Krowiak (6):
>    s390/vfio-ap: verify reset complete in separate function
>    s390/vfio_ap: check TAPQ response code when waiting for queue reset
>    s390/vfio_ap: use TAPQ to verify reset in progress completes
>    s390/vfio_ap: verify ZAPQ completion after return of response code
>      zero
>    s390/vfio_ap: fix handling of error response codes
>    s390/vfio_ap: increase max wait time for reset verification
> 
>   drivers/s390/crypto/vfio_ap_ops.c | 104 +++++++++++++++++++++---------
>   1 file changed, 72 insertions(+), 32 deletions(-)
> 

Thanks applied and queued for CI and regression runs. Will likely go via s390 tree.

Re: [PATCH v2 0/6] improve AP queue reset processing

[Anthony Krowiak]

On 1/20/23 5:25 AM, Christian Borntraeger wrote:
> Am 18.01.23 um 21:31 schrieb Tony Krowiak:
>> This series introduces several improvements to the function that 
>> performs
>> AP queue resets:
>>
>> * Breaks up reset processing into multiple smaller, more concise 
>> functions.
>>
>> * Use TAPQ to verify completion of a reset in progress rather than 
>> mulitple
>>    invocations of ZAPQ.
>>
>> * Check TAPQ response codes when verifying successful completion of 
>> ZAPQ.
>>
>> * Fix erroneous handling of some error response codes.
>>
>> * Increase the maximum amount of time to wait for successful 
>> completion of
>>    ZAPQ.
>>   Change log v1 => v2:
>> -------------------
>> Remove patch 7/7 to restore original behavior since we don't know 
>> whether
>> interrupts are disabled when an unexpected response code is returned 
>> from
>> ZAPQ. (Halil)
>>
>> Tony Krowiak (6):
>>    s390/vfio-ap: verify reset complete in separate function
>>    s390/vfio_ap: check TAPQ response code when waiting for queue reset
>>    s390/vfio_ap: use TAPQ to verify reset in progress completes
>>    s390/vfio_ap: verify ZAPQ completion after return of response code
>>      zero
>>    s390/vfio_ap: fix handling of error response codes
>>    s390/vfio_ap: increase max wait time for reset verification
>>
>>   drivers/s390/crypto/vfio_ap_ops.c | 104 +++++++++++++++++++++---------
>>   1 file changed, 72 insertions(+), 32 deletions(-)
>>
>
> Thanks applied and queued for CI and regression runs. Will likely go 
> via s390 tree.


Got it, thanks.