Re: [RFC PATCH v4 2/4] vfio-ccw: Check workqueue before doing START

[Eric Farman <farman@linux.ibm.com>]

On Thu, 2021-04-15 at 12:51 +0200, Cornelia Huck wrote:
> On Tue, 13 Apr 2021 20:24:08 +0200
> Eric Farman <farman@linux.ibm.com> wrote:
> 
> > When an interrupt is received via the IRQ, the bulk of the work
> > is stacked on a workqueue for later processing. Which means that
> > a concurrent START or HALT/CLEAR operation (via the async_region)
> > will race with this process and require some serialization.
> > 
> > Once we have all our locks acquired, let's just look to see if
> > we're
> > in a window where the process has been started from the IRQ, but
> > not
> > yet picked up by vfio-ccw to clean up an I/O. If there is, mark the
> > request as BUSY so it can be redriven.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_fsm.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c
> > b/drivers/s390/cio/vfio_ccw_fsm.c
> > index 23e61aa638e4..92d638f10b27 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -28,6 +28,11 @@ static int fsm_io_helper(struct vfio_ccw_private
> > *private)
> >  
> >  	spin_lock_irqsave(sch->lock, flags);
> >  
> > +	if (work_pending(&private->io_work)) {
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> >  	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
> >  	if (!orb) {
> >  		ret = -EIO;
> 
> I'm wondering what condition we can consider this situation
> equivalent
> to. I'd say that the virtual version of the subchannel is basically
> status pending already, even though userspace may not have retrieved
> that information yet; so probably cc 1?

Yes, I guess cc1 is a more natural fit, since there is status pending
rather than an active start/halt/clear that would expect get the cc2.

> 
> Following the code path further along, it seems we return -EBUSY both
> for cc 1 and cc 2 conditions we receive from the device (i.e. they
> are
> not distinguishable from userspace). 

Yeah. :/

> I don't think we can change that,
> as it is an existing API (QEMU maps -EBUSY to cc 2.) So this change
> looks fine so far.
> 
> I'm wondering what we should do for hsch. We probably want to return
> -EBUSY for a pending condition as well, if I read the PoP
> correctly...

Ah, yes...  I agree that to maintain parity with ssch and pops, the
same cc1/-EBUSY would be applicable here. Will make that change in next
version.

> the only problem is that QEMU seems to match everything to 0; but
> that
> is arguably not the kernel's problem.
> 
> For clear, we obviously don't have busy conditions. Should we clean
> up
> any pending conditions?

By doing anything other than issuing the csch to the subchannel?  I
don't think so, that should be more than enough to get the css and
vfio-ccw in sync with each other.

> 
> [It feels like we have discussed this before, but any information has
> vanished from my cache :/]
> 

It has vanished from mine too, and looking over the old threads and
notes doesn't page anything useful in, so here we are. Sorry. :(

Eric