a couple of corrections to the IRQ enablement function

Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")

---
 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

Christian Borntraeger

reviewer

2023-10-26 12:19 UTC #2 lore raw

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

Am 18.10.23 um 15:38 schrieb Tony Krowiak:

Quoted 10 lines

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.
Signed-off-by Janosch Frank <[email protected]>

Where is Janoschs signed off coming from here?

Quoted 20 lines

Signed-off-by Anthony Krowiak <[email protected]>
Reviewed-by Matthew Rosato <[email protected]>
Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
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;
  	}

Tony Krowiak

author maintainer

2023-10-26 13:16 UTC #3 lore raw

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

Quoted 12 lines · Christian Borntraeger

Am 18.10.23 um 15:38 schrieb Tony Krowiak:
Quoted 9 lines
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 3 lines

Quoted 1 line
Signed-off-by Janosch Frank <[email protected]>
Where is Janoschs signed off coming from here?

Janosch found this and composed the patch originally. I just tweaked the description and posted it.

Quoted 21 lines

Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")

---
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;
       }

Christian Borntraeger

reviewer

2023-10-26 13:25 UTC #4 lore raw

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

Am 26.10.23 um 15:16 schrieb Tony Krowiak:

Quoted 15 lines · Christian Borntraeger

Quoted 12 lines
Am 18.10.23 um 15:38 schrieb Tony Krowiak:
Quoted 9 lines
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

Quoted 3 lines
Quoted 1 line
Signed-off-by Janosch Frank <[email protected]>
Where is Janoschs signed off coming from here?
Janosch found this and composed the patch originally. I just tweaked the description and posted it.

So we should add

Co-developed-by Janosch Frank <[email protected]>

in front of Janoschs signoff.

Quoted 21 lines

Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")

---
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;
       }

Tony Krowiak

author maintainer

2023-10-26 14:20 UTC #5 lore raw

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

Quoted 22 lines · Christian Borntraeger

Am 26.10.23 um 15:16 schrieb Tony Krowiak:
Quoted 19 lines · Christian Borntraeger
Quoted 15 lines
Am 18.10.23 um 15:38 schrieb Tony Krowiak:
Quoted 10 lines
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.
Signed-off-by Janosch Frank <[email protected]>
Where is Janoschs signed off coming from here?

Quoted 7 lines

Quoted 1 line
Janosch found this and composed the patch originally. I just tweaked the description and posted it.
So we should add
Co-developed-by Janosch Frank <[email protected]>
in front of Janoschs signoff.

Will do.

Quoted 23 lines

Fixes 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")

---
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;
       }

Tony Krowiak

author maintainer

2023-10-18 13:43 UTC #6 lore raw

PATCH v2 2/3 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.

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

drivers/s390/crypto/vfio_ap_ops.c +3 -3

index 9cb28978c186..25d7ce2094f8 100644
@@ -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;
 	}

Signature · 1 line

2.41.0

Halil Pasic

maintainer

2023-10-27 10:56 UTC #7 lore raw

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

Quoted 10 lines · On Wed, 18 Oct 2023 09:38:24 -0400 Tony Krowiak <[email protected]> wrote:

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.

The "ISC passed as a parameter by the guest to the PQAP(AQIC) command is greater than the maximum ISC value allowed" is not possible. The isc is 3 bits wide and all 8 values that can be represented on 3 bits are valid.

This is only possible if the hypervisor was to mess up, or if the machine was broken.

Quoted 40 lines

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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/s390/crypto/vfio_ap_ops.c +3 -3
index 9cb28978c186..25d7ce2094f8 100644
@@ -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;
 	}
 

A message that is not in the archive (sent off-list or not yet received): [email protected]

Tony Krowiak

author maintainer

2023-10-27 17:16 UTC #10 lore raw

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

Quoted 29 lines · Halil Pasic

Quoted 26 lines · On Fri, 27 Oct 2023 09:36:26 -0400 Tony Krowiak <[email protected]> wrote:
Quoted 9 lines
Quoted 8 lines
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.
Quoted 6 lines
The "ISC passed as a parameter by the guest to the PQAP(AQIC) command is greater than the maximum ISC value allowed" is not possible. The isc is 3 bits wide and all 8 values that can be represented on 3 bits are valid.
This is only possible if the hypervisor was to mess up, or if the machine was broken.
kvm_s390_gisc_register(struct kvm *kvm, u32 gisc)
{
	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;

	if (!gi->origin)
		return -ENODEV;
	if (gisc > MAX_ISC)
		return -ERANGE;
...

Quoted 8 lines

Quoted 1 line
Just quoting what is in the code.
Right! But it is not the guest that calls this function directly. This function is called by the vfio_ap code.
The guest passes ISC in bits 61, 62 and 63 of GR1.
So the guest can't give you an invalid value.

Yes, I got it the first time you said that.

Quoted 3 lines

Regards,
Halil

Tony Krowiak

author maintainer

2023-10-18 13:38 UTC #11 lore raw

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

In reply to 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.

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_ops.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/s390/crypto/vfio_ap_ops.c +4 -1

index 25d7ce2094f8..4e80c211ba47 100644
@@ -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,

Signature · 1 line

2.41.0

Matthew Rosato

Reviewed-by on v2 3/3

2023-10-26 14:15 UTC #12 lore raw

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

Quoted 4 lines · 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:

Quoted 8 lines

* 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 <[email protected]>
Signed-off-by Tony Krowiak <[email protected]>