| Message ID | 20231129143529.260264-1-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: handle response code 01 on queue reset | expand |
Am 29.11.23 um 15:35 schrieb Tony Krowiak: > In the current implementation, response code 01 (AP queue number not valid) > is handled as a default case along with other response codes returned from > a queue reset operation that are not handled specifically. Barring a bug, > response code 01 will occur only when a queue has been externally removed > from the host's AP configuration; nn this case, the queue must > be reset by the machine in order to avoid leaking crypto data if/when the > queue is returned to the host's configuration. The response code 01 case > will be handled specifically by logging a WARN message followed by cleaning > up the IRQ resources. > To me it looks like this can be triggered by the LPAR admin, correct? So it is not desireable but possible. In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. WARN can be a disruptive event if panic_on_warn is set. > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> > --- > drivers/s390/crypto/vfio_ap_ops.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c > index 4db538a55192..91d6334574d8 100644 > --- a/drivers/s390/crypto/vfio_ap_ops.c > +++ b/drivers/s390/crypto/vfio_ap_ops.c > @@ -1652,6 +1652,21 @@ static int apq_status_check(int apqn, struct ap_queue_status *status) > * a value indicating a reset needs to be performed again. > */ > return -EAGAIN; > + case AP_RESPONSE_Q_NOT_AVAIL: > + /* > + * This response code indicates the queue is not available. > + * Barring a bug, response code 01 will occur only when a queue > + * has been externally removed from the host's AP configuration; > + * in which case, the queue must be reset by the machine in > + * order to avoid leaking crypto data if/when the queue is > + * returned to the host's configuration. In this case, let's go > + * ahead and log a warning message and return 0 so the AQIC > + * resources get cleaned up by the caller. > + */ > + WARN(true, > + "Unable to reset queue %02x.%04x: not in host AP configuration\n", > + AP_QID_CARD(apqn), AP_QID_QUEUE(apqn)); > + return 0; > default: > WARN(true, > "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n", > @@ -1736,6 +1751,22 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q) > q->reset_status.response_code = 0; > vfio_ap_free_aqic_resources(q); > break; > + case AP_RESPONSE_Q_NOT_AVAIL: > + /* > + * This response code indicates the queue is not available. > + * Barring a bug, response code 01 will occur only when a queue > + * has been externally removed from the host's AP configuration; > + * in which case, the queue must be reset by the machine in > + * order to avoid leaking crypto data if/when the queue is > + * returned to the host's configuration. In this case, let's go > + * ahead and log a warning message then clean up the AQIC > + * resources. > + */ > + WARN(true, > + "Unable to reset queue %02x.%04x: not in host AP configuration\n", > + AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn)); > + vfio_ap_free_aqic_resources(q); > + break; > default: > WARN(true, > "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
On Wed, 29 Nov 2023 09:35:24 -0500 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > In the current implementation, response code 01 (AP queue number not valid) > is handled as a default case along with other response codes returned from > a queue reset operation that are not handled specifically. Barring a bug, > response code 01 will occur only when a queue has been externally removed > from the host's AP configuration; nn this case, the queue must > be reset by the machine in order to avoid leaking crypto data if/when the > queue is returned to the host's configuration. s/if\/when/at latest before/ I would argue that some of the cleanups need to happen before even 01 is reflected... The code comments may also require a similar rewording. With that fixed: Reviewed-by: Halil Pasic <pasic@linux.ibm.com> Regards, Halil > The response code 01 case > will be handled specifically by logging a WARN message followed by cleaning > up the IRQ resources. > > Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
On 11/29/23 12:12, Christian Borntraeger wrote: > Am 29.11.23 um 15:35 schrieb Tony Krowiak: >> In the current implementation, response code 01 (AP queue number not >> valid) >> is handled as a default case along with other response codes returned >> from >> a queue reset operation that are not handled specifically. Barring a bug, >> response code 01 will occur only when a queue has been externally removed >> from the host's AP configuration; nn this case, the queue must >> be reset by the machine in order to avoid leaking crypto data if/when the >> queue is returned to the host's configuration. The response code 01 case >> will be handled specifically by logging a WARN message followed by >> cleaning >> up the IRQ resources. >> > > To me it looks like this can be triggered by the LPAR admin, correct? So it > is not desireable but possible. > In that case I prefer to not use WARN, maybe use dev_warn or dev_err > instead. > WARN can be a disruptive event if panic_on_warn is set. Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. > > >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> >> --- >> drivers/s390/crypto/vfio_ap_ops.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/drivers/s390/crypto/vfio_ap_ops.c >> b/drivers/s390/crypto/vfio_ap_ops.c >> index 4db538a55192..91d6334574d8 100644 >> --- a/drivers/s390/crypto/vfio_ap_ops.c >> +++ b/drivers/s390/crypto/vfio_ap_ops.c >> @@ -1652,6 +1652,21 @@ static int apq_status_check(int apqn, struct >> ap_queue_status *status) >> * a value indicating a reset needs to be performed again. >> */ >> return -EAGAIN; >> + case AP_RESPONSE_Q_NOT_AVAIL: >> + /* >> + * This response code indicates the queue is not available. >> + * Barring a bug, response code 01 will occur only when a queue >> + * has been externally removed from the host's AP configuration; >> + * in which case, the queue must be reset by the machine in >> + * order to avoid leaking crypto data if/when the queue is >> + * returned to the host's configuration. In this case, let's go >> + * ahead and log a warning message and return 0 so the AQIC >> + * resources get cleaned up by the caller. >> + */ >> + WARN(true, >> + "Unable to reset queue %02x.%04x: not in host AP >> configuration\n", >> + AP_QID_CARD(apqn), AP_QID_QUEUE(apqn)); >> + return 0; >> default: >> WARN(true, >> "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n", >> @@ -1736,6 +1751,22 @@ static void vfio_ap_mdev_reset_queue(struct >> vfio_ap_queue *q) >> q->reset_status.response_code = 0; >> vfio_ap_free_aqic_resources(q); >> break; >> + case AP_RESPONSE_Q_NOT_AVAIL: >> + /* >> + * This response code indicates the queue is not available. >> + * Barring a bug, response code 01 will occur only when a queue >> + * has been externally removed from the host's AP configuration; >> + * in which case, the queue must be reset by the machine in >> + * order to avoid leaking crypto data if/when the queue is >> + * returned to the host's configuration. In this case, let's go >> + * ahead and log a warning message then clean up the AQIC >> + * resources. >> + */ >> + WARN(true, >> + "Unable to reset queue %02x.%04x: not in host AP >> configuration\n", >> + AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn)); >> + vfio_ap_free_aqic_resources(q); >> + break; >> default: >> WARN(true, >> "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
Am 04.12.23 um 15:53 schrieb Tony Krowiak: > > > On 11/29/23 12:12, Christian Borntraeger wrote: >> Am 29.11.23 um 15:35 schrieb Tony Krowiak: >>> In the current implementation, response code 01 (AP queue number not valid) >>> is handled as a default case along with other response codes returned from >>> a queue reset operation that are not handled specifically. Barring a bug, >>> response code 01 will occur only when a queue has been externally removed >>> from the host's AP configuration; nn this case, the queue must >>> be reset by the machine in order to avoid leaking crypto data if/when the >>> queue is returned to the host's configuration. The response code 01 case >>> will be handled specifically by logging a WARN message followed by cleaning >>> up the IRQ resources. >>> >> >> To me it looks like this can be triggered by the LPAR admin, correct? So it >> is not desireable but possible. >> In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. >> WARN can be a disruptive event if panic_on_warn is set. > > Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. Ok, please use pr_warn then.
On Mon, 4 Dec 2023 16:16:31 +0100 Christian Borntraeger <borntraeger@linux.ibm.com> wrote: > Am 04.12.23 um 15:53 schrieb Tony Krowiak: > > > > > > On 11/29/23 12:12, Christian Borntraeger wrote: > >> Am 29.11.23 um 15:35 schrieb Tony Krowiak: > >>> In the current implementation, response code 01 (AP queue number not valid) > >>> is handled as a default case along with other response codes returned from > >>> a queue reset operation that are not handled specifically. Barring a bug, > >>> response code 01 will occur only when a queue has been externally removed > >>> from the host's AP configuration; nn this case, the queue must > >>> be reset by the machine in order to avoid leaking crypto data if/when the > >>> queue is returned to the host's configuration. The response code 01 case > >>> will be handled specifically by logging a WARN message followed by cleaning > >>> up the IRQ resources. > >>> > >> > >> To me it looks like this can be triggered by the LPAR admin, correct? So it > >> is not desireable but possible. > >> In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. > >> WARN can be a disruptive event if panic_on_warn is set. > > > > Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. > > Ok, please use pr_warn then. Shouldn't we rather make this an 'info'. I mean we probably do not want people complaining about this condition. Yes it should be a best practice to coordinate such things with the guest, and ideally remove the resource from the guest first. But AFAIU our stack is supposed to be able to handle something like this. IMHO issuing a warning is excessive measure. I know Reinhard and Tony probably disagree with the last sentence though. Regards, Halil
On 12/4/23 11:15, Halil Pasic wrote: > On Mon, 4 Dec 2023 16:16:31 +0100 > Christian Borntraeger <borntraeger@linux.ibm.com> wrote: > >> Am 04.12.23 um 15:53 schrieb Tony Krowiak: >>> >>> >>> On 11/29/23 12:12, Christian Borntraeger wrote: >>>> Am 29.11.23 um 15:35 schrieb Tony Krowiak: >>>>> In the current implementation, response code 01 (AP queue number not valid) >>>>> is handled as a default case along with other response codes returned from >>>>> a queue reset operation that are not handled specifically. Barring a bug, >>>>> response code 01 will occur only when a queue has been externally removed >>>>> from the host's AP configuration; nn this case, the queue must >>>>> be reset by the machine in order to avoid leaking crypto data if/when the >>>>> queue is returned to the host's configuration. The response code 01 case >>>>> will be handled specifically by logging a WARN message followed by cleaning >>>>> up the IRQ resources. >>>>> >>>> >>>> To me it looks like this can be triggered by the LPAR admin, correct? So it >>>> is not desireable but possible. >>>> In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. >>>> WARN can be a disruptive event if panic_on_warn is set. >>> >>> Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. >> >> Ok, please use pr_warn then. > > Shouldn't we rather make this an 'info'. I mean we probably do not want > people complaining about this condition. Yes it should be a best practice > to coordinate such things with the guest, and ideally remove the resource > from the guest first. But AFAIU our stack is supposed to be able to > handle something like this. IMHO issuing a warning is excessive measure. > I know Reinhard and Tony probably disagree with the last sentence > though. I don't feel strongly one way or the other. Anybody else? > > Regards, > Halil
On 12/4/23 07:10, Halil Pasic wrote: > On Wed, 29 Nov 2023 09:35:24 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> In the current implementation, response code 01 (AP queue number not valid) >> is handled as a default case along with other response codes returned from >> a queue reset operation that are not handled specifically. Barring a bug, >> response code 01 will occur only when a queue has been externally removed >> from the host's AP configuration; nn this case, the queue must >> be reset by the machine in order to avoid leaking crypto data if/when the >> queue is returned to the host's configuration. > > s/if\/when/at latest before/ > > I would argue that some of the cleanups need to happen before even 01 is > reflected... To what cleanups are you referring? > > The code comments may also require a similar rewording. With that fixed: > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > > Regards, > Halil > >> The response code 01 case >> will be handled specifically by logging a WARN message followed by cleaning >> up the IRQ resources. >> >> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
On Mon, 4 Dec 2023 12:51:49 -0500 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > > s/if\/when/at latest before/ > > > > I would argue that some of the cleanups need to happen before even 01 is > > reflected... > > To what cleanups are you referring? Event notification and interruption disablement for starters. Otherwise OS has no way to figure out when is GISA and NIB safe to deallocate. Those actions are part of the reset process. I.e. some of the reset stuff can be deferred at most until the queue is made accessible again, some not so much. Regards, Halil
On 2023-12-04 17:15, Halil Pasic wrote: > On Mon, 4 Dec 2023 16:16:31 +0100 > Christian Borntraeger <borntraeger@linux.ibm.com> wrote: > >> Am 04.12.23 um 15:53 schrieb Tony Krowiak: >> > >> > >> > On 11/29/23 12:12, Christian Borntraeger wrote: >> >> Am 29.11.23 um 15:35 schrieb Tony Krowiak: >> >>> In the current implementation, response code 01 (AP queue number not valid) >> >>> is handled as a default case along with other response codes returned from >> >>> a queue reset operation that are not handled specifically. Barring a bug, >> >>> response code 01 will occur only when a queue has been externally removed >> >>> from the host's AP configuration; nn this case, the queue must >> >>> be reset by the machine in order to avoid leaking crypto data if/when the >> >>> queue is returned to the host's configuration. The response code 01 case >> >>> will be handled specifically by logging a WARN message followed by cleaning >> >>> up the IRQ resources. >> >>> >> >> >> >> To me it looks like this can be triggered by the LPAR admin, correct? So it >> >> is not desireable but possible. >> >> In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. >> >> WARN can be a disruptive event if panic_on_warn is set. >> > >> > Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. >> >> Ok, please use pr_warn then. > > Shouldn't we rather make this an 'info'. I mean we probably do not want > people complaining about this condition. Yes it should be a best > practice > to coordinate such things with the guest, and ideally remove the > resource > from the guest first. But AFAIU our stack is supposed to be able to > handle something like this. IMHO issuing a warning is excessive > measure. > I know Reinhard and Tony probably disagree with the last sentence > though. Halil, Tony, the thing about about info versus warning versus error is our own stuff. Keep in mind that these messages end up in the "debug feature" as FFDC data. So it comes to the point which FFDC data do you/Tony want to see there ? It should be enough to explain to a customer what happened without the need to "recreate with higher debug level" if something serious happened. So my private decision table is: 1) is it something serious, something exceptional, something which may not come up again if tried to recreate ? Yes -> make it visible on the first occurrence as error msg. 2) is it something you want to read when a customer hits it and you tell him to extract and examine the debug feature data ? Yes -> make it a warning and make sure your debug feature by default records warnings. 3) still serious, but may flood the debug feature. Good enough and high probability to reappear on a recreate ? Yes -> make it an info message and live with the risk that you may not be able to explain to a customer what happened without a recreate and higher debug level. 4) not 1-3, -> maybe a debug msg but still think about what happens when a customer enables "debug feature" with highest level. Does it squeeze out more important stuff ? Maybe make it dynamic debug with pr_debug() (see kernel docu admin-guide/dynamic-debug-howto.rst). > > Regards, > Halil
On Tue, 05 Dec 2023 09:04:23 +0100 Harald Freudenberger <freude@linux.ibm.com> wrote: > On 2023-12-04 17:15, Halil Pasic wrote: > > On Mon, 4 Dec 2023 16:16:31 +0100 > > Christian Borntraeger <borntraeger@linux.ibm.com> wrote: > > > >> Am 04.12.23 um 15:53 schrieb Tony Krowiak: > >> > > >> > > >> > On 11/29/23 12:12, Christian Borntraeger wrote: > >> >> Am 29.11.23 um 15:35 schrieb Tony Krowiak: > >> >>> In the current implementation, response code 01 (AP queue number not valid) > >> >>> is handled as a default case along with other response codes returned from > >> >>> a queue reset operation that are not handled specifically. Barring a bug, > >> >>> response code 01 will occur only when a queue has been externally removed > >> >>> from the host's AP configuration; nn this case, the queue must > >> >>> be reset by the machine in order to avoid leaking crypto data if/when the > >> >>> queue is returned to the host's configuration. The response code 01 case > >> >>> will be handled specifically by logging a WARN message followed by cleaning > >> >>> up the IRQ resources. > >> >>> > >> >> > >> >> To me it looks like this can be triggered by the LPAR admin, correct? So it > >> >> is not desireable but possible. > >> >> In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. > >> >> WARN can be a disruptive event if panic_on_warn is set. > >> > > >> > Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. > >> > >> Ok, please use pr_warn then. > > > > Shouldn't we rather make this an 'info'. I mean we probably do not want > > people complaining about this condition. Yes it should be a best > > practice > > to coordinate such things with the guest, and ideally remove the > > resource > > from the guest first. But AFAIU our stack is supposed to be able to > > handle something like this. IMHO issuing a warning is excessive > > measure. > > I know Reinhard and Tony probably disagree with the last sentence > > though. > > Halil, Tony, the thing about about info versus warning versus error is > our > own stuff. Keep in mind that these messages end up in the "debug > feature" > as FFDC data. So it comes to the point which FFDC data do you/Tony want > to > see there ? It should be enough to explain to a customer what happened > without the need to "recreate with higher debug level" if something > serious > happened. So my private decision table is: > 1) is it something serious, something exceptional, something which may > not > come up again if tried to recreate ? Yes -> make it visible on the > first > occurrence as error msg. > 2) is it something you want to read when a customer hits it and you tell > him > to extract and examine the debug feature data ? Yes -> make it a > warning > and make sure your debug feature by default records warnings. > 3) still serious, but may flood the debug feature. Good enough and high > probability to reappear on a recreate ? Yes -> make it an info > message > and live with the risk that you may not be able to explain to a > customer > what happened without a recreate and higher debug level. > 4) not 1-3, -> maybe a debug msg but still think about what happens when > a > customer enables "debug feature" with highest level. Does it squeeze > out > more important stuff ? Maybe make it dynamic debug with pr_debug() > (see > kernel docu admin-guide/dynamic-debug-howto.rst). AFAIU the default log level of the S390 Debug Feature is 3 that is error. So warnings do not help us there by default. And if we are already asking the reporter to crank up the loglevel of the debug feature, we can as the reporter to crank it up to 5, assumed there is not too much stuff that log level 5 in that area... How much info stuff do we have for the 'ap' debug facility (I hope that is the facility used by vfio_ap)? I think log levels are supposed to be primarily about severity, and and I'm not sure that a queue becoming unavailable in G1 without fist re-configuring the G2 so that it no more has access to the given queue is not really a warning severity thing. IMHO if we really do want people complaining about this should they ever see it, yes it should be a warning. If not then probably not. Regards, Halil
On 12/6/23 12:17 PM, Halil Pasic wrote: > On Tue, 05 Dec 2023 09:04:23 +0100 > Harald Freudenberger <freude@linux.ibm.com> wrote: > >> On 2023-12-04 17:15, Halil Pasic wrote: >>> On Mon, 4 Dec 2023 16:16:31 +0100 >>> Christian Borntraeger <borntraeger@linux.ibm.com> wrote: >>> >>>> Am 04.12.23 um 15:53 schrieb Tony Krowiak: >>>>> >>>>> On 11/29/23 12:12, Christian Borntraeger wrote: >>>>>> Am 29.11.23 um 15:35 schrieb Tony Krowiak: >>>>>>> In the current implementation, response code 01 (AP queue number not valid) >>>>>>> is handled as a default case along with other response codes returned from >>>>>>> a queue reset operation that are not handled specifically. Barring a bug, >>>>>>> response code 01 will occur only when a queue has been externally removed >>>>>>> from the host's AP configuration; nn this case, the queue must >>>>>>> be reset by the machine in order to avoid leaking crypto data if/when the >>>>>>> queue is returned to the host's configuration. The response code 01 case >>>>>>> will be handled specifically by logging a WARN message followed by cleaning >>>>>>> up the IRQ resources. >>>>>>> >>>>>> To me it looks like this can be triggered by the LPAR admin, correct? So it >>>>>> is not desireable but possible. >>>>>> In that case I prefer to not use WARN, maybe use dev_warn or dev_err instead. >>>>>> WARN can be a disruptive event if panic_on_warn is set. >>>>> Yes, it can be triggered by the LPAR admin. I can't use dev_warn here because we don't have a reference to any device, but I can use pr_warn if that suffices. >>>> Ok, please use pr_warn then. >>> Shouldn't we rather make this an 'info'. I mean we probably do not want >>> people complaining about this condition. Yes it should be a besNo info logging is done via the S390 Debug Feature in vfio_ap. >>> There are a few warning messages logged solely in the handle_pqap >>> and vfio_ap_irq_enable functions. The question is, why are we >>> talking about the S390 Debug Feature? We are talking about using >>> pr_warn verses pr_info. What am I missing here?t >>> practice >>> to coordinate such things with the guest, and ideally remove the >>> resource >>> from the guest first. But AFAIU our stack is supposed to be able to >>> handle something like this. IMHO issuing a warning is excessive >>> measure. >>> I know Reinhard and Tony probably disagree with the last sentence >>> though. >> Halil, Tony, the thing about about info versus warning versus error is >> our >> own stuff. Keep in mind that these messages end up in the "debug >> feature" >> as FFDC data. So it comes to the point which FFDC data do you/Tony want >> to >> see there ? It should be enough to explain to a customer what happened >> without the need to "recreate with higher debug level" if something >> serious >> happened. So my private decision table is: >> 1) is it something serious, something exceptional, something which may >> not >> come up again if tried to recreate ? Yes -> make it visible on the >> first >> occurrence as error msg. >> 2) is it something you want to read when a customer hits it and you tell >> him >> to extract and examine the debug feature data ? Yes -> make it a >> warning >> and make sure your debug feature by default records warnings. >> 3) still serious, but may flood the debug feature. Good enough and high >> probability to reappear on a recreate ? Yes -> make it an info >> message >> and live with the risk that you may not be able to explain to a >> customer >> what happened without a recreate and higher debug level. >> 4) not 1-3, -> maybe a debug msg but still think about what happens when >> a >> customer enables "debug feature" with highest level. Does it squeeze >> out >> more important stuff ? Maybe make it dynamic debug with pr_debug() >> (see >> kernel docu admin-guide/dynamic-debug-howto.rst). > AFAIU the default log level of the S390 Debug Feature is 3 that is > error. So warnings do not help us there by default. And if we are > already asking the reporter to crank up the loglevel of the debug > feature, we can as the reporter to crank it up to 5, assumed there > is not too much stuff that log level 5 in that area... How much > info stuff do we have for the 'ap' debug facility (I hope > that is the facility used by vfio_ap)? No info logging is done via the S390 Debug Feature in vfio_ap. There are a few warning messages logged solely in the handle_pqap and vfio_ap_irq_enable functions. The question is, why are we talking about the S390 Debug Feature given the discussion is about using pr_warn verses pr_info. What am I missing here? > > I think log levels are supposed to be primarily about severity, and > and I'm not sure that a queue becoming unavailable in G1 without > fist re-configuring the G2 so that it no more has access to the > given queue is not really a warning severity thing. IMHO if we > really do want people complaining about this should they ever see it, > yes it should be a warning. If not then probably not. > > Regards, > Halil
On 12/4/23 5:05 PM, Halil Pasic wrote: > On Mon, 4 Dec 2023 12:51:49 -0500 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >>> s/if\/when/at latest before/ >>> >>> I would argue that some of the cleanups need to happen before even 01 is >>> reflected... >> To what cleanups are you referring? > Event notification and interruption disablement for starters. Otherwise > OS has no way to figure out when is GISA and NIB safe to deallocate. > Those actions are part of the reset process. I.e. some of the reset stuff > can be deferred at most until the queue is made accessible again, some > not so much. How do you propose we disable interrupts if the PQAP(AQIC) will likely fail with response code 01 which is the subject of this patch? Do you think we should not free up the AQIC resources as we do in this patch? > > > Regards, > Halil
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c index 4db538a55192..91d6334574d8 100644 --- a/drivers/s390/crypto/vfio_ap_ops.c +++ b/drivers/s390/crypto/vfio_ap_ops.c @@ -1652,6 +1652,21 @@ static int apq_status_check(int apqn, struct ap_queue_status *status) * a value indicating a reset needs to be performed again. */ return -EAGAIN; + case AP_RESPONSE_Q_NOT_AVAIL: + /* + * This response code indicates the queue is not available. + * Barring a bug, response code 01 will occur only when a queue + * has been externally removed from the host's AP configuration; + * in which case, the queue must be reset by the machine in + * order to avoid leaking crypto data if/when the queue is + * returned to the host's configuration. In this case, let's go + * ahead and log a warning message and return 0 so the AQIC + * resources get cleaned up by the caller. + */ + WARN(true, + "Unable to reset queue %02x.%04x: not in host AP configuration\n", + AP_QID_CARD(apqn), AP_QID_QUEUE(apqn)); + return 0; default: WARN(true, "failed to verify reset of queue %02x.%04x: TAPQ rc=%u\n", @@ -1736,6 +1751,22 @@ static void vfio_ap_mdev_reset_queue(struct vfio_ap_queue *q) q->reset_status.response_code = 0; vfio_ap_free_aqic_resources(q); break; + case AP_RESPONSE_Q_NOT_AVAIL: + /* + * This response code indicates the queue is not available. + * Barring a bug, response code 01 will occur only when a queue + * has been externally removed from the host's AP configuration; + * in which case, the queue must be reset by the machine in + * order to avoid leaking crypto data if/when the queue is + * returned to the host's configuration. In this case, let's go + * ahead and log a warning message then clean up the AQIC + * resources. + */ + WARN(true, + "Unable to reset queue %02x.%04x: not in host AP configuration\n", + AP_QID_CARD(q->apqn), AP_QID_QUEUE(q->apqn)); + vfio_ap_free_aqic_resources(q); + break; default: WARN(true, "PQAP/ZAPQ for %02x.%04x failed with invalid rc=%u\n",
In the current implementation, response code 01 (AP queue number not valid) is handled as a default case along with other response codes returned from a queue reset operation that are not handled specifically. Barring a bug, response code 01 will occur only when a queue has been externally removed from the host's AP configuration; nn this case, the queue must be reset by the machine in order to avoid leaking crypto data if/when the queue is returned to the host's configuration. The response code 01 case will be handled specifically by logging a WARN message followed by cleaning up the IRQ resources. Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_ops.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)