Re: [PATCH 7/7] s390/vfio_ap: always clean up IRQ resources

[Halil Pasic]

On Tue, 20 Dec 2022 09:33:03 -0500
Anthony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:

> On 12/19/22 9:10 AM, Halil Pasic wrote:
> > On Tue, 13 Dec 2022 10:44:37 -0500
> > Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> >  
> >> Clean up IRQ resources even when a PQAP(ZAPQ) function fails with an error
> >> not handled by a case statement.  
> > Why?  
> 
> 
> If the ZAPQ failed, then instructions submitted to the same queue will 
> likewise fail. Are you saying it's not safe to assume, therefore, that 
> interrupts will not be occurring?

Right. We are talking about the default branch here, and I suppose, the
codes where we know that it is safe to assume that no reset is needed
handled separately (AP_RESPONSE_DECONFIGURED).

I'm not convinced that if we take the default branch we can safely
assume, that we won't see any interrupts.

For example consider hot-unplug as done by KVM. We modify the
CRYCB/APCB with all vCPUS take out of SIE, but we don't keep
the vCPUs out of SIE until the resets of the unpugged queues
are done, and we don't do any extra interrupt disablement
with all vCPUs keept out of SIE. So I believe currently there
may be a window where the guest can observe a 01 but the
interrupts are still live. That may be a bug, but IMHO it ain't clear
cut.

But it is not just about interrupts. Before we returned an error
code, which gets propagated to the userspace if this reset was
triggered via the ioctl.

With this change, ret seems to be uninitialized when returned 
if we take the code path which you change here. So we would
end up logging a warning and returning garbage?

One could also debate, whether RCs introduced down the road
can affect the logic here (even if the statement "if we
see an RC other that 00 and 02, we don't need to pursue a
reset any further, and interrpts are disabled" were to be
guaranteed to be true now, new RCs could theoretically mess
this up).

 
> 
> 
> >
> > I'm afraid this is a step in the wrong direction...  
> 
> 
> Please explain why.
> 

Sorry, I kept this brief because IMHO it is your job to tell us why
this needs to be changed. But I gave in, as you see.

Regards,
Halil