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

[Parav Pandit <parav@nvidia.com>]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Tuesday, October 5, 2021 12:18 PM
> 
> On Tue, Oct 05 2021, Parav Pandit <parav@nvidia.com> wrote:
> 
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Monday, October 4, 2021 9:09 PM
> >
> >
> >> On Fri, Oct 01 2021, Parav Pandit <parav@nvidia.com> wrote:
> >>
> >> >> 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:
> >> >> > +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?
> >>
> >> The sentence seems to be missing a verb?
> > Ah, right. I will add "is" and modify it as " Such driver initialization sequence is
> specified in 3.1."
> 
> Maybe
> 
> "Refer to the driver initialization sequence specified in..."
> 
Device reset section already has reference to initialization sequence.
So I will skip mentioning this as part of device reset timeout explanation.

> ?
> >> >> > -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.
> >>
> >> Yes; therefore my question. Should we agree about putting it into the
> >> config space, or does it make more sense to leave the actual
> >> mechanism to the transport?
> > I think its worth to add into the config space like proposed.
> > I don't see it any different than queue_notify_data field.
> 
> The queue_notify_data field does not seem to have been specified for anything
> but PCI so far. In particular, CCW does not yet provide that facility, and I'm not
> sure whether it should go into the config space, or use a dedicated mechanism.
> It might be worth standardizing on using the config space for variables such as
> the notification data and the reset timeout, or it might make things awkward
> for some transports.
> 
> Halil, do you have any opinion on this, especially for CCW?
> 
> >
> >>
> >> >
> >> >  > > +
> >> >> >  \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.
> >>
> >> Yes, it does make sense use a timeout; but should we note explicitly
> >> that there's no requirement for the driver to use any kind of timeout
> >> mechanism if it didn't negotiate the feature? Or drop that statement
> >> completely, and let the driver continue to do whatever it is doing right now?
> >>
> > Letting driver doing what it is doing right now = wait infinitely is certainly not
> good.
> > We want to improve the driver stack to have determinism.
> > Hence, the proposal to improve.
> 
> I think the driver had already been free to implement some timeout if it
> wanted to?
Sure when this feature is not supported, driver can choose its own timeout value.
Spec is providing the guidance to driver of what to do when this field is not present.

> 
> >
> > Also in other thread Michael also mentioned that current code is broken in
> the hotplug scenario where, device is infinitely waiting for reset to finish.
> > And this reset never finishes, because device is already on its way to hot
> unplug.
> > We also experienced this in the field.
> 
> I don't question the usefulness of the timeout; my concern is that we should not
> cause confusion whether old drivers are compliant or not. 
Old driver will follow old spec. New driver will follow new spec.
Old driver unaware of this new spec changes cannot do anything anyways.

> If we say that the
> driver SHOULD timeout with a self-chosen value, it might read as a new
> requirement, even though the driver is not forced to implement it. No
> statement at all might be less confusing.
I think yes, it is a new requirement as part of the new spec release.
An old driver doesn't need to implement new spec recommendations.

Guidance provided in newer spec is anyway good for driver to make decision, so I think we should think from forward spec perspective.