Re: [PATCH] s390/vfio-ap: handle response code 01 on queue reset

[Anthony Krowiak]

On 12/6/23 12:17 PM, Halil Pasic wrote:
> On Tue, 05 Dec 2023 09:04:23 +0100
> Harald Freudenberger <freude@xxxxxxxxxxxxx> wrote:
>
> > On 2023-12-04 17:15, Halil Pasic wrote:
> > > On Mon, 4 Dec 2023 16:16:31 +0100
> > > Christian Borntraeger <borntraeger@xxxxxxxxxxxxx> 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