Re: [PATCH] [RFC] virtio: Limit the retries on a virtio device reset

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

On 24/08/2017 13:07, Cornelia Huck wrote:
> On Wed, 23 Aug 2017 18:33:02 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Reseting a device can sometime fail, even a virtual device.
>> If the device is not reseted after a while the driver should
>> abandon the retries.
>> This is the change proposed for the modern virtio_pci.
>>
>> More generally, when this happens,the virtio driver can set the
>> VIRTIO_CONFIG_S_FAILED status flag to advertise the caller.
>>
>> The virtio core can test if the reset was succesful by testing
>> this flag after a reset.
>>
>> This behavior is backward compatible with existing drivers.
>> This behavior seems to me compatible with Virtio-1.0 specifications,
>> Chapters 2.1 Device Status Field.
>> There I definitively need your opinion: Is it right?
> 
> Will have to double check with the spec.
> 
>>
>> This patch also lead to another question:
>> do we care if a device provided by the hypervisor is buggy?
> 
> Getting into a hang because of a broken device is not nice, but I'm not
> sure we need to plan for this. Have you seen this in the wild?

Yes, with virtio-pci on S390.


> 
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> ---
>>   drivers/virtio/virtio.c            |  4 ++++
>>   drivers/virtio/virtio_pci_modern.c | 11 ++++++++++-
>>   2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> index 48230a5..6255dc4 100644
>> --- a/drivers/virtio/virtio.c
>> +++ b/drivers/virtio/virtio.c
>> @@ -324,6 +324,8 @@ int register_virtio_device(struct virtio_device *dev)
>>   	/* We always start by resetting the device, in case a previous
>>   	 * driver messed it up.  This also tests that code path a little. */
>>   	dev->config->reset(dev);
>> +	if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
>> +		return -EIO;
>>   
>>   	/* Acknowledge that we've seen the device. */
>>   	virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> @@ -373,6 +375,8 @@ int virtio_device_restore(struct virtio_device *dev)
>>   	/* We always start by resetting the device, in case a previous
>>   	 * driver messed it up. */
>>   	dev->config->reset(dev);
>> +	if (dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED)
>> +		return -EIO;
> 
> virtio-ccw prior to rev 2 won't ever see this (as the read command did
> not exist then), but this is not really a problem.
> 
>>   
>>   	/* Acknowledge that we've seen the device. */
>>   	virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index 2555d80..bfc5fc1 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
>> @@ -270,6 +270,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 status)
>>   static void vp_reset(struct virtio_device *vdev)
>>   {
>>   	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> +	int retry_count = 10;
> 
> When you're touching this anyway, it would be a good time to add an
> extra blank line :)

Yes, I like blank lines too.

> 
>>   	/* 0 status means a reset. */
>>   	vp_iowrite8(0, &vp_dev->common->device_status);
>>   	/* After writing 0 to device_status, the driver MUST wait for a read of
>> @@ -277,8 +278,16 @@ static void vp_reset(struct virtio_device *vdev)
>>   	 * This will flush out the status write, and flush in device writes,
>>   	 * including MSI-X interrupts, if any.
>>   	 */
>> -	while (vp_ioread8(&vp_dev->common->device_status))
>> +	while (vp_ioread8(&vp_dev->common->device_status) && retry_count--)
>>   		msleep(1);
>> +	/* If the read did not return 0 before the timeout consider that
>> +	 * the device failed.
>> +	 */
>> +	if (retry_count <= 0) {
>> +		virtio_add_status(vdev, VIRTIO_CONFIG_S_FAILED);
>> +		return;
>> +	}
>> +	virtio_add_status(vdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> 
> Adding ACK here seems wrong?

Exact, I forgot to remove this from a previous test.
I wait a little and post a v2

Thanks for reviewing.

Pierre

> 
>>   	/* Flush pending VQ/configuration callbacks. */
>>   	vp_synchronize_vectors(vdev);
>>   }
> 


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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization