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

[Cornelia Huck <cohuck@redhat.com>]

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..."

?
>> >> > -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?

>
> 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. 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.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org