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

[Pierre Morel <pmorel@linux.ibm.com>]

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