Add device reset timeout field

PATCH In review Parav Pandit 3 participants 8 messages Sep 30, 2021 – Oct 6, 2021

Share & tools

Parav Pandit

author

2021-09-30 09:18 UTC #0 lore raw

PATCH 1/1 Add device reset timeout field

Motivation:
==========
Currently when driver initiates a device reset operation, a device may not be able finish the reset operation immediately. In such case driver waits for an arbitrary amount of time in a hope that device will finish the reset. Depending on the device type and its backend implementation a device timeout can be different. Trying to wait for all device type in same value may not be efficient or adequate.

Proposal:
========
Hence, enhance the specification to let device report a device reset timeout value in milliseconds. Such hint helps driver to know how long should it wait for device reset to finish.

This proposal introduces optional device reset timeout field for MMIO and PCI transports. Such transports have a use case where virtio devices are implemented in hardware and made available to the guest.

A device reset timeout field has following attributes.
(a) It is an optional field to maintain backward compatibility.
(b) It is valid only when device advertises a new feature bit VIRTIO_F_DEV_RESET_TO
(b) When it is not advertised, this field is invalid and driver should choose the reasonable reset timeout.
(c) It is a 16-bit field covering wide range of timeout from 10 milliseconds to several hundred seconds.

This proposal is an improvement over a past proposal [1]. Instead of just passing a flag for wait, this proposal offers upper bound wait time making the device reset timeout and overall device initialization more predictable.

[1] https://lists.oasis-open.org/archives/virtio-dev/202108/msg00161.html

Signed-off-by Parav Pandit <[email protected]>

Reviewed-by Max Gurtovoy <[email protected]>

---
changelog:
v1->v2:
- address comments from Max
- added comment that it is device specific for the new field
- added description to say that device reset timeout of zero is invalid
- fixed unit of the field in description
v0->v1:
- address comments from Shahaf, Max, Oren
- several grammar corrections
- changed timer granularity from 1msec to 10mec
- changed device reset timeout field from le32 to le16
- added VIRTIO_F_DEV_RESET_TO device feature bit

---
 content.tex | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

content.tex +43 -0

index 37c45d3..d52daf9 100644
@@ -73,6 +73,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 recover by issuing a reset.
 \end{note}
 
+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.
+Such driver initialization sequence specified in
+\ref{sec:General Initialization And Device Operation / Device
+Initialization}.
+
 \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.
+
 \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.
+
 \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
 
 Device configuration space is generally used for rarely-changing or

@@ -859,6 +877,8 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         le64 queue_driver;              /* read-write */
         le64 queue_device;              /* read-write */
         le16 queue_notify_data;         /* read-only for driver */
+        /* About the whole device. */
+        le16 device_reset_timeout;      /* read-only for driver */
 };
 \end{lstlisting}

@@ -938,6 +958,15 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         may benefit from providing another value, for example an internal virtqueue
         identifier, or an internal offset related to the virtqueue number.
         \end{note}
+
+\item[\field{device_reset_timeout}]
+        This field exists only if the VIRTIO_F_DEV_RESET_TO is advertised by the device.
+        This field provides the hint to the driver indicating how much maximum time a
+        device can take to complete the reset once the driver initiates the device reset
+        operation. This field unit is in 10 milliseconds. For example, a field value of
+        3 indicates device reset timeout is 30 milliseconds. When VIRTIO_F_DEV_RESET_TO
+	feature is set by the device this field must contain a non zero value.
+
 \end{description}
 
 \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}

@@ -1804,6 +1833,15 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     accesses apply to the queue selected by writing to \field{QueueSel}.
   }
   \hline 
+  \mmioreg{DeviceResetTimeout}{Device reset timeout}{0x04c}{R}{%
+    This register exists only if the VIRTIO_F_DEV_RESET_TO is advertised by the device.
+    It provides the hint to the driver indicating how much maximum time a
+    device can take to complete the reset once the driver initiates the device reset operation.
+    This field unit is in 10 milliseconds. For example, a field value of 3 indicates device
+    reset timeout is 30 milliseconds. When VIRTIO_F_DEV_RESET_TO feature is set by the device
+    this field must contain a non zero value.
+  }
+  \hline
   \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
     Writing a value to this register notifies the device that
     there are new buffers to process in a queue.

@@ -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
+  advertises a maximum reset timeout which the driver should use during device reset stage.
+  For more details about device reset timeout over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}.
+  For more details about device reset timeout over MMIO see \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

Signature · 6 lines

2.31.1


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Cornelia Huck

2021-10-01 12:01 UTC #1 lore raw

Quoted 60 lines · Parav Pandit

Motivation:
==========
Currently when driver initiates a device reset operation, a device may not be able finish the reset operation immediately. In such case driver waits for an arbitrary amount of time in a hope that device will finish the reset. Depending on the device type and its backend implementation a device timeout can be different. Trying to wait for all device type in same value may not be efficient or adequate.
Proposal:
========
Hence, enhance the specification to let device report a device reset timeout value in milliseconds. Such hint helps driver to know how long should it wait for device reset to finish.
This proposal introduces optional device reset timeout field for MMIO and PCI transports. Such transports have a use case where virtio devices are implemented in hardware and made available to the guest.
A device reset timeout field has following attributes.
(a) It is an optional field to maintain backward compatibility.
(b) It is valid only when device advertises a new feature bit VIRTIO_F_DEV_RESET_TO
(b) When it is not advertised, this field is invalid and driver should choose the reasonable reset timeout.
(c) It is a 16-bit field covering wide range of timeout from 10 milliseconds to several hundred seconds.
This proposal is an improvement over a past proposal [1]. Instead of just passing a flag for wait, this proposal offers upper bound wait time making the device reset timeout and overall device initialization more predictable.
[1] https://lists.oasis-open.org/archives/virtio-dev/202108/msg00161.html
Signed-off-by Parav Pandit <[email protected]>
Reviewed-by Max Gurtovoy <[email protected]>
---
changelog:
v1->v2:
- address comments from Max
- added comment that it is device specific for the new field
- added description to say that device reset timeout of zero is invalid
- fixed unit of the field in description
v0->v1:
- address comments from Shahaf, Max, Oren
- several grammar corrections
- changed timer granularity from 1msec to 10mec
- changed device reset timeout field from le32 to le16
- added VIRTIO_F_DEV_RESET_TO device feature bit
---
 content.tex | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)
content.tex +0 -0
index 37c45d3..d52daf9 100644

content.tex \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev · line 80

Quoted 8 lines

content.tex +5 -0

@@ -73,6 +73,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 recover by issuing a reset.
 \end{note}
 
+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?

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

content.tex \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev · line 83

Quoted 3 lines

content.tex +3 -0

+Such driver initialization sequence specified in
+\ref{sec:General Initialization And Device Operation / Device
+Initialization}.

I'm sorry, I don't parse that statement.

content.tex \section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Re · line 224

Quoted 11 lines

content.tex +4 -0

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

content.tex \section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Re · line 234

Quoted 10 lines

content.tex +5 -0

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

What does "giving up on the device" imply? 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)?

What about a reset that is done outside of the initialization sequence?

(...)

content.tex \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} · line 6714

Quoted 5 lines

content.tex +1 -0

@@ -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 (RESET_TO makes me wonder what we try to reset the device to.)

content.tex \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} · line 6721

Quoted 7 lines

content.tex +4 -0

+  advertises a maximum reset timeout which the driver should use during device reset stage.
+  For more details about device reset timeout over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}.
+  For more details about device reset timeout over MMIO see \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Parav Pandit

author

2021-10-01 12:25 UTC #2 lore raw

Quoted 6 lines

From: Cornelia Huck <[email protected]>
Sent: Friday, October 1, 2021 5:31 PM
On Thu, Sep 30 2021, Parav Pandit <[email protected]> wrote:

[..]

Quoted 10 lines

Review
Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 9 lines

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.)
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?

Quoted 12 lines

Quoted 9 lines
+
\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.
Quoted 2 lines
+A device may not be able to complete reset action when the driver initiates

Quoted 9 lines

a reset operation.
Quoted 1 line
+Such a device should provide the hint as to how long a device may take to
finish the reset operation.
Quoted 1 line
+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.

Quoted 7 lines

Quoted 7 lines
+
\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.

Quoted 8 lines

Review
Quoted 4 lines
(quoted diff) +4 -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.

Quoted 1 line

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

Quoted 4 lines

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?

Quoted 1 line

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?

Quoted 7 lines

(...)
Review \chapter{Reserved Feature
Quoted 1 line
(quoted diff) +0 -0
@@ -6673,6 +6711,11 @@ \chapter{Reserved Feature
Bits}\label{sec:Reserved Feature Bits}
Review \chapter{Reserved Feature · line 6712
Quoted 2 lines
(quoted diff) +0 -0
   transport specific.
   For more details about driver notifications over PCI see \ref{sec:Virtio

Quoted 7 lines

Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
Review \chapter{Reserved Feature · line 6714
Quoted 3 lines
(quoted diff) +2 -0
+  \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.

Cornelia Huck

2021-10-04 15:39 UTC #3 lore raw

Quoted 14 lines · Parav Pandit

Quoted 5 lines
From: Cornelia Huck <[email protected]>
Sent: Friday, October 1, 2021 5:31 PM
On Thu, Sep 30 2021, Parav Pandit <[email protected]> wrote:
[..]
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5 -0
+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.

Quoted 5 lines

Quoted 3 lines
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.

Ok. I guess if we want to specify some more generic timeout handling in the future, the device is free to use the same timeout everywhere it makes sense.

Quoted 6 lines

Quoted 6 lines
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.)

Quoted 6 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?

Quoted 16 lines

Quoted 16 lines
Quoted 10 lines
+
\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.
Quoted 2 lines
+A device may not be able to complete reset action when the driver initiates
a reset operation.
Quoted 1 line
+Such a device should provide the hint as to how long a device may take to
finish the reset operation.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
+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?

Quoted 14 lines

Quoted 14 lines
Review
Quoted 12 lines
+
\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.
(quoted diff) +4 -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.

Quoted 5 lines

Quoted 2 lines
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?

Quoted 3 lines

Quoted 2 lines
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".

Maybe "before considering the reset operation to have failed"?

Quoted 6 lines

Quoted 5 lines
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?

It is about the communication mechanism, not the action; we can abort the communication mechanism for the command and re-try the same command again, if there's anything that caused the command to not be properly transmitted or so. The mechanism for that is probably different enough from the read/write operations used by pci or mmio.

Quoted 4 lines

Quoted 2 lines
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?

I don't think we say anywhere that the driver must start to re-initialize the device again if it did a reset. It's probably best to simply drop any references to the initialization sequence; the timeout should apply to any reset initiated by the driver, regardless of its purpose/usefulness.

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Parav Pandit

author

2021-10-05 03:16 UTC #4 lore raw

Quoted 26 lines

From: Cornelia Huck <[email protected]>
Sent: Monday, October 4, 2021 9:09 PM
Quoted 18 lines · Parav Pandit
Quoted 5 lines
From: Cornelia Huck <[email protected]>
Sent: Friday, October 1, 2021 5:31 PM
On Thu, Sep 30 2021, Parav Pandit <[email protected]> wrote:
[..]
Quoted 11 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+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?

Quoted 7 lines

Quoted 1 line
No. I haven't read that thread fully but device reset timeout is not intent to
solve all the timeouts.
Quoted 1 line
It is intent to have the specific purpose described in this proposal.
Ok. I guess if we want to specify some more generic timeout handling in the future, the device is free to use the same timeout everywhere it makes sense.

I think it is better to not overload device reset timeout for queue reset timeout. A queue in a device might recover a lot faster depending on the device type. It would be incorrect to define single field that may not serve both the purpose. So lets keep the scope of device reset timeout to device reset level. And not mix with other IO timeout or queue timeout.

Quoted 11 lines

Quoted 11 lines
Quoted 11 lines
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.)
Review
Quoted 2 lines
(quoted diff) +2 -0
+Such driver initialization sequence specified in \ref{sec:General
+Initialization And Device Operation / Device Initialization}.
I'm sorry, I don't parse that statement.

Quoted 5 lines

Quoted 1 line
It is similar statement that exists in "Driver Requirements" section as first
line.
Quoted 1 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."

Quoted 27 lines

Quoted 27 lines
Quoted 27 lines
Quoted 7 lines
+
\devicenormative{\subsection}{Device Status Field}{Basic
Facilities of a Virtio Device / Device Status Field}
Quoted 5 lines
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.
Review
Quoted 3 lines
(quoted diff) +2 -0
+A device may not be able to complete reset action when the driver
+initiates
a reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+Such a device should provide the hint as to how long a device may
+take to
finish the reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?

Quoted 6 lines

Quoted 2 lines
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.

Quoted 21 lines

Quoted 16 lines
Quoted 16 lines
Quoted 9 lines
+
\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.
Review
Quoted 5 lines
(quoted diff) +5 -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.
Quoted 5 lines
Quoted 2 lines
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

Quoted 7 lines

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.

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.

Quoted 7 lines

Quoted 3 lines
Quoted 2 lines
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".
Maybe "before considering the reset operation to have failed"?

Sounds good. Will modify.

Quoted 15 lines

Quoted 6 lines
Quoted 5 lines
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?
It is about the communication mechanism, not the action; we can abort the communication mechanism for the command and re-try the same command again, if there's anything that caused the command to not be properly transmitted or so. The mechanism for that is probably different enough from the read/write operations used by pci or mmio.
Quoted 1 line

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
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?
I don't think we say anywhere that the driver must start to re-initialize the device again if it did a reset. It's probably best to simply drop any references to the initialization sequence; the timeout should apply to any reset initiated by the driver, regardless of its purpose/usefulness.

The proposed spec suggest the device reset timeout handling during device reset stage.
It doesn't suggest device reinitialization.
Proposed changes does refer to device initialization section, because it is applicable in usual device reset flow as part of the device init sequence. But I agree that this may not be necessary to overdo.
I will remove this reference.
Thanks.

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]

Cornelia Huck

2021-10-05 06:47 UTC #5 lore raw

Quoted 14 lines · Parav Pandit

Quoted 14 lines
From: Cornelia Huck <[email protected]>
Sent: Monday, October 4, 2021 9:09 PM
Quoted 8 lines · Parav Pandit
Quoted 8 lines
From: Cornelia Huck <[email protected]>
Sent: Friday, October 1, 2021 5:31 PM
Review
Quoted 2 lines · Parav Pandit
(quoted diff) +2 -0
+Such driver initialization sequence specified in \ref{sec:General
+Initialization And Device Operation / Device Initialization}.
I'm sorry, I don't parse that statement.

Quoted 6 lines

Quoted 5 lines
Quoted 1 line
It is similar statement that exists in "Driver Requirements" section as first
line.
Quoted 1 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..."

Quoted 19 lines

Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 4 lines
-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.
Review
Quoted 3 lines
(quoted diff) +2 -0
+A device may not be able to complete reset action when the driver
+initiates
a reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+Such a device should provide the hint as to how long a device may
+take to
finish the reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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?

Quoted 24 lines

Quoted 24 lines
Quoted 17 lines
Quoted 17 lines
Quoted 10 lines
+
\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.
Review
Quoted 5 lines
(quoted diff) +5 -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.
Quoted 5 lines
Quoted 2 lines
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.

Quoted 8 lines

Quoted 5 lines
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?

Quoted 4 lines

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: [email protected] For additional commands, e-mail: [email protected]

Parav Pandit

author

2021-10-05 08:42 UTC #6 lore raw

Quoted 26 lines

From: Cornelia Huck <[email protected]>
Sent: Tuesday, October 5, 2021 12:18 PM
Quoted 4 lines · Parav Pandit
Quoted 2 lines
From: Cornelia Huck <[email protected]>
Sent: Monday, October 4, 2021 9:09 PM
Quoted 8 lines
Quoted 8 lines · Parav Pandit
Quoted 7 lines
Quoted 7 lines
From: Cornelia Huck <[email protected]>
Sent: Friday, October 1, 2021 5:31 PM
Quoted 2 lines · Parav Pandit
+Such driver initialization sequence specified in +\ref{sec:General Initialization And Device Operation / Device
Initialization}.
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
I'm sorry, I don't parse that statement.
It is similar statement that exists in "Driver Requirements" section as first
line.
Quoted 1 line
Do you mean I should rewrite above sentence or?

Quoted 8 lines

Quoted 2 lines
Quoted 1 line
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.

Quoted 69 lines

?
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 4 lines
-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.
Review
Quoted 3 lines
(quoted diff) +2 -0
+A device may not be able to complete reset action when the
+driver initiates
a reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+Such a device should provide the hint as to how long a device
+may take to
finish the reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
+
\drivernormative{\subsection}{Device Reset}{Basic Facilities of
a Virtio Device / Device Reset}
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 6 lines
The driver SHOULD consider a driver-initiated reset complete
when it reads \field{device status} as 0.
(quoted diff) +3 -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.
Quoted 17 lines
Quoted 17 lines
Quoted 10 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?

Quoted 7 lines

Quoted 1 line
Letting driver doing what it is doing right now = wait infinitely is certainly not
good.
Quoted 2 lines
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.

Quoted 10 lines

Quoted 3 lines
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.
Quoted 1 line
And this reset never finishes, because device is already on its way to hot
unplug.
Quoted 1 line
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.

Quoted 4 lines

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.

Max Gurtovoy

2021-10-06 08:35 UTC #7 lore raw

Quoted 96 lines · Parav Pandit

Quoted 21 lines
From: Cornelia Huck <[email protected]>
Sent: Tuesday, October 5, 2021 12:18 PM
Quoted 11 lines · Parav Pandit
Quoted 11 lines
From: Cornelia Huck <[email protected]>
Sent: Monday, October 4, 2021 9:09 PM
Quoted 6 lines · Parav Pandit
Quoted 6 lines
From: Cornelia Huck <[email protected]>
Sent: Friday, October 1, 2021 5:31 PM
Quoted 2 lines · Parav Pandit
+Such driver initialization sequence specified in +\ref{sec:General Initialization And Device Operation / Device
Initialization}.
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line
I'm sorry, I don't parse that statement.
It is similar statement that exists in "Driver Requirements" section as first
line.
Quoted 9 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 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.
Quoted 63 lines
?
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 18 lines
Quoted 4 lines
-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.
Review
Quoted 2 lines
(quoted diff) +2 -0
+A device may not be able to complete reset action when the
+driver initiates
a reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+Such a device should provide the hint as to how long a device
+may take to
finish the reset operation.
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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?
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Review
Quoted 10 lines
+
\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.
(quoted diff) +3 -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.
Quoted 17 lines
Quoted 17 lines
Quoted 10 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+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?

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Letting driver doing what it is doing right now = wait infinitely is certainly not
good.
Quoted 2 lines
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.

Well, probably not. I've send a patch to fix that in the current driver and it wasn't accepted because it's not according to the spec.

Quoted 19 lines

Quoted 8 lines
Quoted 2 lines
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.
Quoted 1 line
And this reset never finishes, because device is already on its way to hot
unplug.
Quoted 1 line
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.
Quoted 4 lines
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.