Re: [RFC PATCH v2 0/4] vfio-ccw: Fix interrupt handling for HALT/CLEAR

Cornelia Huck
2020-05-19T11:23:17Z

On Mon, 18 May 2020 17:57:39 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> On 5/18/20 12:09 PM, Cornelia Huck wrote:
> > On Wed, 13 May 2020 16:29:30 +0200
> > Eric Farman <farman@linux.ibm.com> wrote:
> >
> >> Hi Conny,
> >>
> >> Back in January, I suggested a small patch [1] to try to clean up
> >> the handling of HSCH/CSCH interrupts, especially as it relates to
> >> concurrent SSCH interrupts. Here is a new attempt to address this.
> >>
> >> There was some suggestion earlier about locking the FSM, but I'm not
> >> seeing any problems with that. Rather, what I'm noticing is that the
> >> flow between a synchronous START and asynchronous HALT/CLEAR have
> >> different impacts on the FSM state. Consider:
> >>
> >> CPU 1 CPU 2
> >>
> >> SSCH (set state=CP_PENDING)
> >> INTERRUPT (set state=IDLE)
> >> CSCH (no change in state)
> >> SSCH (set state=CP_PENDING)
> >
> > This is the transition I do not understand. When we get a request via
> > the I/O area, we go to CP_PROCESSING and start doing translations.
> > However, we only transition to CP_PENDING if we actually do a SSCH with
> > cc 0 -- which shouldn't be possible in the flow you outline... unless
> > it really is something that can be taken care of with locking (state
> > machine transitioning due to an interrupt without locking, so we go to
> > IDLE without other parts noticing.)
>
> I'm only going by what the (existing and my temporary) tea leaves in
> s390dbf are telling us. :)

/me makes a note to try tea leaves :)

>
> >
> >> INTERRUPT (set state=IDLE)
>
> Part of the problem is that this is actually comprised of these elements:
>
> if (irb_is_final && state == CP_PENDING)
> cp_free()
>
> lock io_mutex
> copy irb to io_region
> unlock io_mutex
>
> if (irb_is_final)
> state = IDLE
>
> The CP_PENDING check will protect us if a SSCH is still being built at
> the time we execute this code. But if we got to CP_PENDING first
> (between fsm_irq() stacking to the workqueue and us unstacking
> vfio_ccw_sch_io_todo()), we would free an unrelated operation. (This was
> the scenario in the first version of my fix back in January.)
>
> We can't add a CP_PENDING check after the io_mutex barrier, because if a
> second SSCH is being processed, we will hang on the lock acquisition and
> will DEFINITELY be in CP_PENDING state when we come back. But by that
> point, we will have skipped freeing the (now active) CP but are back in
> an IDLE state.

That's all very ugly :(

>
>
> >> INTERRUPT (set state=IDLE)
> >
> > But taking a step back (and ignoring your series and the discussion,
> > sorry about that):
>
> No apologies necessary.
>
> >
> > We need to do something (creating a local translation of the guest's
> > channel program) that does not have any relation to the process in the
> > architecture at all, but is only something that needs to be done
> > because of what vfio-ccw is trying to do (issuing a channel program on
> > behalf of another entity.) Trying to sort that out by poking at actl
> > and fctl bits does not seem like the best way; especially as keeping
> > the bits up-to-date via STSCH is an exercise in futility.
>
> I am coming to strongly agree with this sentiment.

Thank you for making me feel like I'm not completely out in the weeds :)

>
> >
> > What about the following (and yes, I had suggested something vaguely in
> > that direction before):
> >
> > - Detach the cp from the subchannel (or better, remove the 1:1
> > relationship). By that I mean building the cp as a separately
> > allocated structure (maybe embedding a kref, but that might not be
> > needed), and appending it to a list after SSCH with cc=0. Discard it
> > if cc!=0.
> > - Remove the CP_PENDING state. The state is either IDLE after any
> > successful SSCH/HSCH/CSCH, or a new state in that case. But no
> > special state for SSCH.
> > - A successful CSCH removes the first queued request, if any.
> > - A final interrupt removes the first queued request, if any.
> >
> > Thoughts?
> >
>
> I'm cautiously optimistic, for exactly the reason I mention above. If we
> always expect to be in IDLE state once an interrupt arrives, we can just
> rely on determining if the interrupt is in relation to an actual
> operation we're waiting on. I'll give this a try and report back.

Great, good luck!