Re: [PATCH v2 10/10] vfio: ccw: Let user wait when busy on IO

[Pierre Morel]

On 25/05/2018 16:04, Heiko Carstens wrote:
> On Fri, May 25, 2018 at 12:21:18PM +0200, Pierre Morel wrote:
> > In the current implementation, we do not want to start a new SSCH
> > command before the last one ends.
> >
> > Currently the user needs to poll on the -EBUSY error to
> > wait before sending a new request.
> >
> > Let's be friendly with global warming and let the user sleep
> > until he may send a new request.
> >
> > Let's make the caller wait until the last SSCH ends.
> >
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/s390/cio/vfio_ccw_fsm.c     | 4 ++++
> >   drivers/s390/cio/vfio_ccw_ops.c     | 6 ++++++
> >   drivers/s390/cio/vfio_ccw_private.h | 1 +
> >   3 files changed, 11 insertions(+)
> >
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> > index c37052d..97b74a1 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -200,6 +200,10 @@ static int fsm_irq(struct vfio_ccw_private *private)
> >   
> >   	if (private->io_trigger)
> >   		eventfd_signal(private->io_trigger, 1);
> > +
> > +	if (private->io_completion)
> > +		complete(private->io_completion);
> > +
> >   	return VFIO_CCW_STATE_IDLE;
> >   }
> >   
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> > index b202e73..39beb6e 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -183,6 +183,7 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
> >   	struct vfio_ccw_private *private;
> >   	struct ccw_io_region *region;
> >   	union scsw *scsw;
> > +	DECLARE_COMPLETION_ONSTACK(completion);
> >   
> >   	if (*ppos + count > sizeof(*region))
> >   		return -EINVAL;
> > @@ -196,6 +197,11 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device *mdev,
> >   	scsw = (union scsw *) &region->scsw_area;
> >   	switch (scsw->cmd.fctl) {
> >   	case SCSW_FCTL_START_FUNC:
> > +		if (private->state == VFIO_CCW_STATE_BUSY) {
> > +			private->io_completion = &completion;
> > +			if (wait_for_completion_interruptible(&completion))
> > +				return -EINTR;
> > +		}
> What prevents a state change between checking the state and before
> private->io_completion is set? If that happens you would end with an
> endless wait.
>
> Similarly, you would have memory corruption if the task would be
> interrupted and if the function would be left, ending up with a stale
> private->io_completion completion pointer.
> The complete(private->io_completion) call will then write to a memory
> location that might already be reused.
>
> Just my 0.02 after having a very very short look ;)

Right, completely false, I should pay a little more (at least) attention.

Thanks to have had a very very short (but sharp)look.

Pierre


--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany