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

Pierre Morel
2018-06-05T13:02:06Z

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@linux.vnet.ibm.com>
>> ---
>> 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