RE: [virtio-dev] [PATCH] Add device reset timeout field

[Parav Pandit <parav@nvidia.com>]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Friday, October 1, 2021 5:31 PM
> 
> On Thu, Sep 30 2021, Parav Pandit <parav@nvidia.com> wrote:
> 
[..]

> > +Once the driver initiates a reset, the device may not be able to
> > +finish the reset immediately. Such device provides a hint to the
> > +driver about how long a device may take to complete the reset. Driver
> > +should wait at least for the time specified by the device to let
> > +device finish the reset or if the device status field to become 0 within the
> specified time interval.
> 
> Would such a timeout also apply to the virtqueue reset that is discussed
> elsewhere on the virtio lists?
> 
No. I haven't read that thread fully but device reset timeout is not intent to solve all the timeouts.
It is intent to have the specific purpose described in this proposal.

> Can transports specify what can actually time out? For example, for an
> asynchronous transport like ccw, it might make sense to have the timeout
> apply to the interrupt that is supposed to come in for the RESET command (the
> driver then can try to terminate the running command, if desired.)
> 
> > +Such driver initialization sequence specified in \ref{sec:General
> > +Initialization And Device Operation / Device Initialization}.
> 
> I'm sorry, I don't parse that statement.
It is similar statement that exists in "Driver Requirements" section as first line.
Do you mean I should rewrite above sentence or?

> 
> > +
> >  \devicenormative{\subsection}{Device Status Field}{Basic Facilities
> > of a Virtio Device / Device Status Field}
> >
> >  The device MUST NOT consume buffers or send any used buffer @@
> > -210,11 +219,20 @@ \section{Device Reset}\label{sec:Basic Facilities
> > of a Virtio Device / Device Re  indicating completion of the reset by
> > reinitializing \field{device status}  to 0, until the driver re-initializes the
> device.
> >
> > +A device may not be able to complete reset action when the driver initiates
> a reset operation.
> > +Such a device should provide the hint as to how long a device may take to
> finish the reset operation.
> > +This hint is provided by the device using device reset timeout field in units
> of 10 milliseconds.
> 
> Is this supposed to go into the device config space? Or can each transport
> decide how to expose the value?
>
Isn't each transport define how a config space is accessible?
In this proposal for PCI it is part of the struct virtio_pci_common_cfg.

 > > +
> >  \drivernormative{\subsection}{Device Reset}{Basic Facilities of a
> > Virtio Device / Device Reset}
> >
> >  The driver SHOULD consider a driver-initiated reset complete when it
> > reads \field{device status} as 0.
> >
> > +When a device reports a non-zero device reset timeout value, the
> > +driver SHOULD wait for the device to finish the reset action during
> > +the initialization sequence before giving up on the device. When
> > +device reset timeout is not provided by the device, driver SHOULD choose a
> reasonable timeout value for reset operation to complete.
> 
> Is the driver allowed to not time the reset action out at all?
> 
Which is what is being done by the Linux driver today.
Not sure I follow the question.
As virtio spec follows RFC 2119, driver is recommended to time the reset with above text description.

> What does "giving up on the device" imply? 
I should probably better write it as " action during the initialization sequence before declaring the device as unable_to_reset".

> Can the driver perform some
> recovery actions (e.g for ccw, the driver may be able to terminate the running
> reset command and re-try)?
>
Will that retry help to recover the device? If so, what is the new operation that helps in recovery?
 
> What about a reset that is done outside of the initialization sequence?
In general reset leads to device re-initialization. Isn't it?
Can a virtio device be reset without initialization or re-initialization?

> 
> (...)
> 
> > @@ -6673,6 +6711,11 @@ \chapter{Reserved Feature
> Bits}\label{sec:Reserved Feature Bits}
> >    transport specific.
> >    For more details about driver notifications over PCI see \ref{sec:Virtio
> Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device
> Operation / Available Buffer Notifications}.
> >
> > +  \item[VIRTIO_F_DEV_RESET_TO(40)] This feature indicates that the
> > + device
> 
> I think we should spell this out as VIRTIO_F_DEV_RESET_TIMEOUT 
Ok. I will revise it in v2.