Skip to contentSkip to conversation

a couple of corrections to the IRQ enablement function

PATCH Superseded Tony Krowiak 2 participants 7 messages Sep 13, 2023 – Sep 15, 2023

Subsystems (5)
Stable branch KVM s390 s390 zcrypt and pkey driver and AP bus +1 more subsystems

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Tony Krowiak

author maintainer
#0 lore raw

cover a couple of corrections to the IRQ enablement function

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

 drivers/s390/crypto/vfio_ap_ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Signature · 1 line
2.41.0
author maintainer
#1 lore raw

PATCH 1/2 s390/vfio-ap: unpin pages on gisc registration failure

From: Anthony Krowiak <[email protected]>

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.

Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
Signed-off-by Janosch Frank <[email protected]>
Signed-off-by Anthony Krowiak <[email protected]>
Cc <[email protected]>
---
 drivers/s390/crypto/vfio_ap_ops.c | 1 +
 1 file changed, 1 insertion(+)
drivers/s390/crypto/vfio_ap_ops.c +1 -0
index 4db538a55192..9cb28978c186 100644
@@ -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;
 	}
Signature · 1 line
2.41.0
Reviewed-by on v1 1/2
#2 lore raw

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

Quoted 9 lines · Tony Krowiak

From: Anthony Krowiak <[email protected]>

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.

Quoted 5 lines
Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
Signed-off-by Janosch Frank <[email protected]>
Signed-off-by Anthony Krowiak <[email protected]>
Cc <[email protected]>

Oops, good find.

Reviewed-by Matthew Rosato <[email protected]>
Quoted 16 lines
---
 drivers/s390/crypto/vfio_ap_ops.c | 1 +
 1 file changed, 1 insertion(+)
drivers/s390/crypto/vfio_ap_ops.c +1 -0
index 4db538a55192..9cb28978c186 100644
@@ -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;
 	}
author maintainer
#3 lore raw

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

Quoted 10 lines · Matthew Rosato
Quoted 9 lines · Tony Krowiak

From: Anthony Krowiak <[email protected]>

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.

Quoted 7 lines
Quoted 5 lines
Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
Signed-off-by Janosch Frank <[email protected]>
Signed-off-by Anthony Krowiak <[email protected]>
Cc <[email protected]>

Oops, good find.

Yes, thanks to Janosch/

Quoted 20 lines
Reviewed-by Matthew Rosato <[email protected]>
Quoted 16 lines
---
  drivers/s390/crypto/vfio_ap_ops.c | 1 +
  1 file changed, 1 insertion(+)
drivers/s390/crypto/vfio_ap_ops.c +1 -0
index 4db538a55192..9cb28978c186 100644
@@ -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;
  	}
author maintainer
#4 lore raw

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

In reply to Tony Krowiak

From: Anthony Krowiak <[email protected]>

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 does not make sense because the non-zero return code from the kvm_s390_gisc_register function can be due one of two things: Either the ISC passed as a parameter by the guest to the PQAP(AQIC) command is greater than the maximum ISC value allowed, or the guest is not using a GISA.

Since this scenario is very unlikely to happen and there is no status response code to indicate an invalid ISC value, 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 indicating that the ZONE/GISA designation is invalid which is something the guest can do nothing about since those values are set by the hypervisor.

Signed-off-by Anthony Krowiak <[email protected]>
Suggested-by Halil Pasic <[email protected]>
---
 drivers/s390/crypto/vfio_ap_ops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/s390/crypto/vfio_ap_ops.c +2 -2
index 9cb28978c186..e7e4dbbf5ad3 100644
@@ -394,7 +394,7 @@ static int ensure_nib_shared(unsigned long addr, struct gmap *gmap)
  * 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.
+ * 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;
 	}
Signature · 1 line
2.41.0
#5 lore raw
In reply to Tony Krowiak
Quoted 10 lines · 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.

Hi Tony,

3. Since you're already making changes related to gisc registration, you might consider a 3rd patch that looks at the return code for kvm_s390_gisc_unregister and tags the unexpected error rc somehow. This came up in a recent conversation I had with Michael, see this conversation towards the bottom:

https://lore.kernel.org/linux-s390/[email protected]/

4. While looking at patch 1 I also had a question re: the AP_RESPONSE_OTHERWISE_CHANGED path in vfio_ap_irq_enable. Here's a snippet of the current code:

	case AP_RESPONSE_OTHERWISE_CHANGED:
		/* We could not modify IRQ settings: clear new configuration */
		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
		kvm_s390_gisc_unregister(kvm, isc);
		break;

Is it safe to unpin the page before unregistering the gisc in this case? Or shouldn't the unpin happen after we have unregistered the gisc / set the IAM?

Quoted 8 lines

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

 drivers/s390/crypto/vfio_ap_ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
author maintainer
#6 lore raw
Quoted 12 lines · Matthew Rosato
Quoted 10 lines · 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.

Quoted 5 lines

Hi Tony,

3. Since you're already making changes related to gisc registration, you might consider a 3rd patch that looks at the return code for kvm_s390_gisc_unregister and tags the unexpected error rc somehow. This came up in a recent conversation I had with Michael, see this conversation towards the bottom:

https://lore.kernel.org/linux-s390/[email protected]/

When we receive a non-zero return code from kvm_s390_gisc_register, we log a DBF warning message. We can do the same for a non-zero rc from kvm_s390_gisc_unregister.

Quoted 10 lines

4. While looking at patch 1 I also had a question re: the AP_RESPONSE_OTHERWISE_CHANGED path in vfio_ap_irq_enable. Here's a snippet of the current code:

	case AP_RESPONSE_OTHERWISE_CHANGED:
		/* We could not modify IRQ settings: clear new configuration */
		vfio_unpin_pages(&q->matrix_mdev->vdev, nib, 1);
		kvm_s390_gisc_unregister(kvm, isc);
		break;

Is it safe to unpin the page before unregistering the gisc in this case? Or shouldn't the unpin happen after we have unregistered the gisc / set the IAM?

I don't know the answer to the question, but it makes logical sense; so, I'll go ahead and create a third patch as you suggested.

Quoted 10 lines
Quoted 9 lines

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

  drivers/s390/crypto/vfio_ap_ops.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)