* [PATCH 0/2] a couple of corrections to the IRQ enablement function
@ 2023-09-13 13:06 Tony Krowiak
2023-09-13 13:06 ` [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure Tony Krowiak
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Tony Krowiak @ 2023-09-13 13:06 UTC (permalink / raw
To: linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david
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(-)
--
2.41.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure
2023-09-13 13:06 [PATCH 0/2] a couple of corrections to the IRQ enablement function Tony Krowiak
@ 2023-09-13 13:06 ` Tony Krowiak
2023-09-13 18:10 ` Matthew Rosato
2023-09-13 13:06 ` [PATCH 2/2] s390/vfio-ap: set status response code to 06 " Tony Krowiak
2023-09-13 18:13 ` [PATCH 0/2] a couple of corrections to the IRQ enablement function Matthew Rosato
2 siblings, 1 reply; 7+ messages in thread
From: Tony Krowiak @ 2023-09-13 13:06 UTC (permalink / raw
To: linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david, Anthony Krowiak, stable
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.
Fixes: 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] s390/vfio-ap: set status response code to 06 on gisc registration failure
2023-09-13 13:06 [PATCH 0/2] a couple of corrections to the IRQ enablement function Tony Krowiak
2023-09-13 13:06 ` [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure Tony Krowiak
@ 2023-09-13 13:06 ` Tony Krowiak
2023-09-13 18:13 ` [PATCH 0/2] a couple of corrections to the IRQ enablement function Matthew Rosato
2 siblings, 0 replies; 7+ messages in thread
From: Tony Krowiak @ 2023-09-13 13:06 UTC (permalink / raw
To: linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david, Anthony 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 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 <akrowiak@linux.ibm.com>
Suggested-by: Halil Pasic <pasic@linux.ibm.com>
---
drivers/s390/crypto/vfio_ap_ops.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 9cb28978c186..e7e4dbbf5ad3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -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;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure
2023-09-13 13:06 ` [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure Tony Krowiak
@ 2023-09-13 18:10 ` Matthew Rosato
2023-09-15 13:16 ` Tony Krowiak
0 siblings, 1 reply; 7+ messages in thread
From: Matthew Rosato @ 2023-09-13 18:10 UTC (permalink / raw
To: Tony Krowiak, linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david, stable
On 9/13/23 9:06 AM, Tony Krowiak wrote:
> 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.
>
> Fixes: 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
> Cc: <stable@vger.kernel.org>
Oops, good find.
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
> 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;
> }
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] a couple of corrections to the IRQ enablement function
2023-09-13 13:06 [PATCH 0/2] a couple of corrections to the IRQ enablement function Tony Krowiak
2023-09-13 13:06 ` [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure Tony Krowiak
2023-09-13 13:06 ` [PATCH 2/2] s390/vfio-ap: set status response code to 06 " Tony Krowiak
@ 2023-09-13 18:13 ` Matthew Rosato
2023-09-15 14:04 ` Tony Krowiak
2 siblings, 1 reply; 7+ messages in thread
From: Matthew Rosato @ 2023-09-13 18:13 UTC (permalink / raw
To: Tony Krowiak, linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david, Michael Mueller
On 9/13/23 9:06 AM, Tony Krowiak wrote:
> 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/0ddf808c-e929-c975-1b39-5ebc1f2fab62@linux.ibm.com/
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?
> 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(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure
2023-09-13 18:10 ` Matthew Rosato
@ 2023-09-15 13:16 ` Tony Krowiak
0 siblings, 0 replies; 7+ messages in thread
From: Tony Krowiak @ 2023-09-15 13:16 UTC (permalink / raw
To: Matthew Rosato, linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david, stable
On 9/13/23 14:10, Matthew Rosato wrote:
> On 9/13/23 9:06 AM, Tony Krowiak wrote:
>> 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.
>>
>> Fixes: 783f0a3ccd79 ("s390/vfio-ap: add s390dbf logging to the vfio_ap_irq_enable function")
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> Signed-off-by: Anthony Krowiak <akrowiak@linux.ibm.com>
>> Cc: <stable@vger.kernel.org>
>
> Oops, good find.
Yes, thanks to Janosch/
>
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>
>> ---
>> 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;
>> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 0/2] a couple of corrections to the IRQ enablement function
2023-09-13 18:13 ` [PATCH 0/2] a couple of corrections to the IRQ enablement function Matthew Rosato
@ 2023-09-15 14:04 ` Tony Krowiak
0 siblings, 0 replies; 7+ messages in thread
From: Tony Krowiak @ 2023-09-15 14:04 UTC (permalink / raw
To: Matthew Rosato, linux-s390, linux-kernel, kvm
Cc: jjherne, pasic, alex.williamson, borntraeger, kwankhede, frankja,
imbrenda, david, Michael Mueller
On 9/13/23 14:13, Matthew Rosato wrote:
> On 9/13/23 9:06 AM, Tony Krowiak wrote:
>> 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/0ddf808c-e929-c975-1b39-5ebc1f2fab62@linux.ibm.com/
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.
>
> 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.
>
>> 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(-)
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-09-15 14:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-13 13:06 [PATCH 0/2] a couple of corrections to the IRQ enablement function Tony Krowiak
2023-09-13 13:06 ` [PATCH 1/2] s390/vfio-ap: unpin pages on gisc registration failure Tony Krowiak
2023-09-13 18:10 ` Matthew Rosato
2023-09-15 13:16 ` Tony Krowiak
2023-09-13 13:06 ` [PATCH 2/2] s390/vfio-ap: set status response code to 06 " Tony Krowiak
2023-09-13 18:13 ` [PATCH 0/2] a couple of corrections to the IRQ enablement function Matthew Rosato
2023-09-15 14:04 ` Tony Krowiak
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.