[PATCH v3 0/3] s390/vfio-ap: a couple of corrections to the IRQ enablement function

[PATCH v3 0/3] s390/vfio-ap: a couple of corrections to the IRQ enablement function

[Tony Krowiak]

This series corrects two issues related to enablement of interrupts in 
response to interception of the PQAP(AQIC) command:

1. Returning a status response code 06 (Invalid address of AP-queue 
   notification byte) when the call to register a guest ISC fails makes no
   sense.
   
2. The pages containing the interrupt notification-indicator byte are not
   freed after a failure to register the guest ISC fails.


Anthony Krowiak (2):
  s390/vfio-ap: unpin pages on gisc registration failure
  s390/vfio-ap: set status response code to 06 on gisc registration
    failure

Tony Krowiak (1):
  s390/vfio-ap: improve reaction to response code 07 from PQAP(AQIC)
    command

 drivers/s390/crypto/vfio_ap_ops.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH v3 1/3] s390/vfio-ap: unpin pages on gisc registration failure

[Tony Krowiak]

From: Anthony Krowiak <akrowiak@linux.ibm.com>

In the vfio_ap_irq_enable function, after the page containing the
notification indicator byte (NIB) is pinned, the function attempts
to register the guest ISC. If registration fails, the function sets the
status response code and returns without unpinning the page containing
the NIB. In order to avoid a memory leak, the NIB should be unpinned before
returning from the vfio_ap_irq_enable function.

Co-developed-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Fixes: 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
Cc: <stable@vger.kernel.org>
---
 drivers/s390/crypto/vfio_ap_ops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 4db538a55192..9cb28978c186 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -457,6 +457,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		VFIO_AP_DBF_WARN("%s: gisc registration failed: nisc=%d, isc=%d, apqn=%#04x\n",
 				 __func__, nisc, isc, q->apqn);
 
+		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
 		status.response_code = AP_RESPONSE_INVALID_GISA;
 		return status;
 	}
-- 
2.41.0

[PATCH v3 2/3] s390/vfio-ap: set status response code to 06 on gisc registration failure

[Tony Krowiak]

From: Anthony Krowiak <akrowiak@linux.ibm.com>

The interception handler for the PQAP(AQIC) command calls the
kvm_s390_gisc_register function to register the guest ISC with the channel
subsystem. If that call fails, the status response code 08 - indicating
Invalid ZONE/GISA designation - is returned to the guest. This response
code is not valid because setting the ZONE/GISA values is the
responsibility of the hypervisor controlling the guest and there is nothing
that can be done from the guest perspective to correct that problem.

The likelihood of GISC registration failure is nil and there is no status
response code to indicate an invalid ISC value, so let's set the response
code to 06 indicating 'Invalid address of AP-queue notification byte'.
While this is not entirely accurate, it is better than setting a response
code which makes no sense for the guest.

Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Suggested-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 9cb28978c186..25d7ce2094f8 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -393,8 +393,8 @@ static int ensure_nib_shared(unsigned long addr, struct gmap *gmap)
  * Register the guest ISC to GIB interface and retrieve the
  * host ISC to issue the host side PQAP/AQIC
  *
- * Response.status may be set to AP_RESPONSE_INVALID_ADDRESS in case the
- * vfio_pin_pages failed.
+ * status.response_code may be set to AP_RESPONSE_INVALID_ADDRESS in case the
+ * vfio_pin_pages or kvm_s390_gisc_register failed.
  *
  * Otherwise return the ap_queue_status returned by the ap_aqic(),
  * all retry handling will be done by the guest.
@@ -458,7 +458,7 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 				 __func__, nisc, isc, q->apqn);
 
 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
-		status.response_code = AP_RESPONSE_INVALID_GISA;
+		status.response_code = AP_RESPONSE_INVALID_ADDRESS;
 		return status;
 	}
 
-- 
2.41.0

[PATCH v3 3/3] s390/vfio-ap: improve reaction to response code 07 from PQAP(AQIC) command

[Tony Krowiak]

Let's improve the vfio_ap driver's reaction to reception of response code
07 from the PQAP(AQIC) command when enabling interrupts on behalf of a
guest:

* Unregister the guest's ISC before the pages containing the notification
  indicator bytes are unpinned.

* Capture the return code from the kvm_s390_gisc_unregister function and
  log a DBF warning if it fails.

Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 25d7ce2094f8..4e80c211ba47 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -476,8 +476,11 @@ static struct ap_queue_status vfio_ap_irq_enable(struct vfio_ap_queue *q,
 		break;
 	case AP_RESPONSE_OTHERWISE_CHANGED:
 		/* We could not modify IRQ settings: clear new configuration */
+		ret = kvm_s390_gisc_unregister(kvm, isc);
+		if (ret)
+			VFIO_AP_DBF_WARN("%s: kvm_s390_gisc_unregister: rc=%d isc=%d, apqn=%#04x\n",
+					 __func__, ret, isc, q->apqn);
 		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
-		kvm_s390_gisc_unregister(kvm, isc);
 		break;
 	default:
 		pr_warn("%s: apqn %04x: response: %02x\n", __func__, q->apqn,
-- 
2.41.0

Re: [PATCH v3 0/3] s390/vfio-ap: a couple of corrections to the IRQ enablement function

[Alexander Gordeev]

On Thu, Nov 09, 2023 at 11:44:19AM -0500, Tony Krowiak wrote:
> This series corrects two issues related to enablement of interrupts in 
> response to interception of the PQAP(AQIC) command:
...

Hi Tony!

Via which tree this series is to be pulled?

Thanks!

Re: [PATCH v3 0/3] s390/vfio-ap: a couple of corrections to the IRQ enablement function

[Christian Borntraeger]

Am 15.11.23 um 15:29 schrieb Alexander Gordeev:
> On Thu, Nov 09, 2023 at 11:44:19AM -0500, Tony Krowiak wrote:
>> This series corrects two issues related to enablement of interrupts in
>> response to interception of the PQAP(AQIC) command:
> ...
> 
> Hi Tony!
> 
> Via which tree this series is to be pulled?
> 
> Thanks!

I wanted to create a topic repository/branch for s390-specfic vfio code when we expect conflicts or touch kvm or vfio base code.
I started that but I have not finished my preparations.

Given the diffstat:
drivers/s390/crypto/vfio_ap_ops.c | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)

This looks pretty much isolated to s390, so unless Paolo or Alex complain,
we should simply carry that via the s390 tree.
Alexander, can you take those patches?
Tony, I assume none if the patches is urgent and can wait for the next merge window, correct?

Christian

Re: [PATCH v3 0/3] s390/vfio-ap: a couple of corrections to the IRQ enablement function

[Alexander Gordeev]

On Fri, Nov 17, 2023 at 04:31:49PM +0100, Christian Borntraeger wrote:
> Alexander, can you take those patches?

Applied, thanks!

> Christian

Re: [PATCH v3 0/3] s390/vfio-ap: a couple of corrections to the IRQ enablement function

[Tony Krowiak]

On 11/17/23 10:31, Christian Borntraeger wrote:
> Am 15.11.23 um 15:29 schrieb Alexander Gordeev:
>> On Thu, Nov 09, 2023 at 11:44:19AM -0500, Tony Krowiak wrote:
>>> This series corrects two issues related to enablement of interrupts in
>>> response to interception of the PQAP(AQIC) command:
>> ...
>>
>> Hi Tony!
>>
>> Via which tree this series is to be pulled?
>>
>> Thanks!
> 
> I wanted to create a topic repository/branch for s390-specfic vfio code 
> when we expect conflicts or touch kvm or vfio base code.
> I started that but I have not finished my preparations.
> 
> Given the diffstat:
> drivers/s390/crypto/vfio_ap_ops.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> This looks pretty much isolated to s390, so unless Paolo or Alex complain,
> we should simply carry that via the s390 tree.
> Alexander, can you take those patches?
> Tony, I assume none if the patches is urgent and can wait for the next 
> merge window, correct?

It is not urgent.

> 
> Christian