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

[Cornelia Huck <cohuck@redhat.com>]

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?

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). 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...
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?

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