Add device reset timeout field

PATCH Superseded Parav Pandit 2 participants 3 messages Oct 5, 2021 – Oct 6, 2021

Share & tools

Parav Pandit

author

2021-10-05 17: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:
v0->v1:
- v0: https://lists.oasis-open.org/archives/virtio-dev/202109/msg00133.html
- Addressed below comments from Cornelia Huck
- renamed VIRTIO_F_DEV_RESET_TO to VIRTIO_F_DEV_RESET_TIMEOUT
- removed references to device initialization sequence during device reset flow
- rewrote 'giving up on device'

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

content.tex +40 -0

index 37c45d3..c87833d 100644
@@ -73,6 +73,12 @@ \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.
+
 \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 +216,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 before considering the reset operation to have failed. 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 +874,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 +955,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_TIMEOUT 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_TIMEOUT
+	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 +1830,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_TIMEOUT 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_TIMEOUT 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 +6708,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_TIMEOUT(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 · 1 line

2.31.1

Cornelia Huck

2021-10-06 09:25 UTC #1 lore raw

Quoted 56 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:
v0->v1:
- v0: https://lists.oasis-open.org/archives/virtio-dev/202109/msg00133.html
- Addressed below comments from Cornelia Huck
- renamed VIRTIO_F_DEV_RESET_TO to VIRTIO_F_DEV_RESET_TIMEOUT
- removed references to device initialization sequence during device reset flow
- rewrote 'giving up on device'
---
 content.tex | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
content.tex +0 -0
index 37c45d3..c87833d 100644

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

Quoted 6 lines

content.tex +3 -0

@@ -73,6 +73,12 @@ \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.

Maybe

"To accommodate that situation, the device can provide a hint to the driver about how long it might take the device to complete the reset."

Quoted 1 line

Driver should wait at least for the

s/Driver/The driver/

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

Quoted 2 lines

content.tex +2 -0

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

Hm, status == 0 is supposed to signal completion of the reset anyway, no? What about rewording this to

"...for the device status field to become 0, signifying completion of the reset operation."

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

Quoted 9 lines

content.tex +2 -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 +216,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.

"...may not be able to complete the reset action immediately..." ?

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

Quoted 1 line

content.tex +1 -0

+Such a device should provide the hint as to how long a device may take to finish the reset operation.

s/the hint/a hint/

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

Quoted 1 line

content.tex +1 -0

+This hint is provided by the device using device reset timeout field in units of 10 milliseconds.

Maybe

"This hint is provided by the device via a device reset timeout value in units of 10 milliseconds."

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

Quoted 8 lines

content.tex +3 -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 before considering the reset operation to have failed. When

"...the driver SHOULD NOT consider the reset operation to have failed unless the specified timeout has expired." ?

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

Quoted 1 line

content.tex +1 -0

+device reset timeout is not provided by the device, driver SHOULD choose a reasonable timeout

Maybe "When no device reset timeout is provided..." ? s/driver/the driver/

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

Quoted 1 line

content.tex +1 -0

+value for reset operation to complete.

s/reset operation/the reset operation/

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 878

Quoted 10 lines

content.tex +3 -0

+
 \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 +874,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 */

Is a 16 bit value reasonable for all devices? It very likely is (that's 10 minutes or so), but having to extend it later may be annoying. What values would you expect for the devices you know of today?

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 960

Quoted 10 lines

content.tex +3 -0

 };
 \end{lstlisting}

@@ -938,6 +955,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_TIMEOUT is advertised by the device.

s/if the/if/

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 965

Quoted 5 lines

content.tex +5 -0

+        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_TIMEOUT
+	feature is set by the device this field must contain a non zero value.

s/feature is set/is offered/ ?

Quoted 5 lines

content.tex +1 -0

+
 \end{description}
 
 \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}

content.tex \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi · line 1839

Quoted 10 lines

content.tex +7 -0

@@ -1804,6 +1830,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_TIMEOUT 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_TIMEOUT feature is set by the device
+    this field must contain a non zero value.

Same nits here as for the PCI definition above.

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

Quoted 11 lines

content.tex +4 -0

+  }
+  \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 +6708,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_TIMEOUT(40)] This feature indicates that the device
+  advertises a maximum reset timeout which the driver should use during device reset stage.

"...advertises a timeout value for the device reset operation." ?

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

Quoted 6 lines

content.tex +3 -0

+  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}

Not quite sure yet how to model this for CCW, but it should not present any problems (I'm not aware of any devices addressed via CCW that would currently need this anyway... an advertised timeout value for *any* CCW operation that involves channel I/O would make more sense, I think. But that's beyond the scope of this proposal, and I'm not sure if we would be able to come up with anything sane considering the other variables like delays on the channel etc. involved, anyway.)

So on the whole, I think this basically looks good.

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

Parav Pandit

author

2021-10-06 12:03 UTC #2 lore raw

Quoted 65 lines

From: Cornelia Huck <[email protected]>
Sent: Wednesday, October 6, 2021 2:55 PM
Quoted 60 lines
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:
v0->v1:
- v0:
https://lists.oasis-open.org/archives/virtio-dev/202109/msg00133.html
- Addressed below comments from Cornelia Huck
- renamed VIRTIO_F_DEV_RESET_TO to VIRTIO_F_DEV_RESET_TIMEOUT
- removed references to device initialization sequence during device reset flow
- rewrote 'giving up on device'
---
 content.tex | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
content.tex +0 -0
@@ -73,6 +73,12 @@ \section{\field{Device Status}
Field}\label{sec:Basic Facilities of a Virtio Dev recover by issuing a reset.
\end{note}

Quoted 9 lines

content.tex
Quoted 3 lines
content.tex +3 -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.
Maybe
"To accommodate that situation, the device can provide a hint to the driver about how long it might take the device to complete the reset."

Ack.

Quoted 4 lines

Quoted 1 line
Driver should wait at least for the
s/Driver/The driver/

Ack.

Quoted 8 lines

content.tex
Quoted 2 lines
content.tex +2 -0
+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.
Hm, status == 0 is supposed to signal completion of the reset anyway, no? What about rewording this to
"...for the device status field to become 0, signifying completion of the reset operation."

Yes, but we probably don't need to repeat here what is the significance of device status zero mean. It signify completion of the reset is already captured in the reset description.
So I think this paragraph should just say that let it become zero.

Quoted 11 lines

?
Quoted 8 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 +216,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

Quoted 7 lines

device.
Quoted 2 lines
+A device may not be able to complete reset action when the driver initiates
a reset operation.
"...may not be able to complete the reset action immediately..." ?

Ok.

Quoted 5 lines

Quoted 1 line
+Such a device should provide the hint as to how long a device may take to
finish the reset operation.
s/the hint/a hint/

Ack.

Quoted 10 lines

Quoted 1 line
+This hint is provided by the device using device reset timeout field in units
of 10 milliseconds.
Maybe
"This hint is provided by the device via a device reset timeout value in units of 10 milliseconds."
?

Sounds equally fine.

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

content.tex
Quoted 3 lines
content.tex +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 before
+considering the reset operation to have failed. When
"...the driver SHOULD NOT consider the reset operation to have failed unless the specified timeout has expired." ?

I think it is better to have positive sentence as "driver should wait for the device to finish" compare to saying Should not considered failed" which contains two negative.
How about below version?

"When a device reports a non-zero device reset timeout value, the
driver SHOULD wait for the device to finish the reset action.
Once the specified timeout has expired, driver should consider that reset operation has failed".

Quoted 4 lines

content.tex
Quoted 2 lines
content.tex +2 -0
+device reset timeout is not provided by the device, driver SHOULD
+choose a reasonable timeout
Maybe "When no device reset timeout is provided..." ?

I think "device reset timeout is not provided" reads better to me, compare to saying "no d_r_t provided". Here the sentence refers to "device reset timeout" field being absent.
Saying "no" and provided in same sentence is confusing me.

Quoted 2 lines

s/driver/the driver/

Ack.

Quoted 4 lines

Quoted 1 line
+value for reset operation to complete.
s/reset operation/the reset operation/

Ack.

Quoted 13 lines

content.tex \subsubsection{Common configuration structure
Quoted 6 lines
+
\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
content.tex +0 -0
@@ -859,6 +874,8 @@ \subsubsection{Common configuration structure
layout}\label{sec:Virtio Transport
content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 878
Quoted 5 lines
content.tex +2 -0
         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 */

Quoted 4 lines

Is a 16 bit value reasonable for all devices? It very likely is (that's 10 minutes or so), but having to extend it later may be annoying. What values would you expect for the devices you know of today?

Devices that I know needs 2 to 3 minutes of time in worst cases. We initially considered it be 32-bit value with granularity of 1 msec. But during review, we felt that 10 minutes is high enough.

We waned to here from community if this should be le32 or le16 is good enough?

Quoted 7 lines

content.tex \subsubsection{Common configuration structure
Quoted 4 lines
content.tex +0 -0
 };
 \end{lstlisting}
 
@@ -938,6 +955,15 @@ \subsubsection{Common configuration structure
layout}\label{sec:Virtio Transport
content.tex \subsubsection{Common configuration structure · line 955
Quoted 1 line
content.tex +0 -0
         may benefit from providing another value, for example an internal
virtqueue

Quoted 9 lines

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
Quoted 5 lines
content.tex +3 -0
         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_TIMEOUT is
advertised by the device.
s/if the/if/

Ack.

Quoted 12 lines

content.tex \subsubsection{Common configuration structure · line 961
Quoted 1 line
content.tex +1 -0
+        This field provides the hint to the driver indicating how much maximum
time a
content.tex \subsubsection{Common configuration structure · line 962
Quoted 1 line
content.tex +1 -0
+        device can take to complete the reset once the driver initiates the
device reset
content.tex \subsubsection{Common configuration structure · line 963
Quoted 1 line
content.tex +1 -0
+        operation. This field unit is in 10 milliseconds. For example, a field
value of
content.tex \subsubsection{Common configuration structure · line 964
Quoted 1 line
content.tex +1 -0
+        3 indicates device reset timeout is 30 milliseconds. When
VIRTIO_F_DEV_RESET_TIMEOUT
content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 965
Quoted 1 line
content.tex +1 -0
+	feature is set by the device this field must contain a non zero value.
s/feature is set/is offered/ ?

Ack.

Quoted 16 lines

Quoted 5 lines
content.tex +1 -0
+
 \end{description}
 
 \devicenormative{\paragraph}{Common configuration structure
layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout /
Common configuration structure layout} @@ -1804,6 +1830,15 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
content.tex
Quoted 5 lines
accesses apply to the queue selected by writing to \field{QueueSel}.
}
\hline
content.tex +2 -0
+  \mmioreg{DeviceResetTimeout}{Device reset timeout}{0x04c}{R}{%
+    This register exists only if the VIRTIO_F_DEV_RESET_TIMEOUT is
advertised by the device.
content.tex
Quoted 2 lines
content.tex +2 -0
+    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

Quoted 9 lines

reset operation.
Quoted 1 line
+ This field unit is in 10 milliseconds. For example, a field value of 3
indicates device
Quoted 1 line
+ reset timeout is 30 milliseconds. When VIRTIO_F_DEV_RESET_TIMEOUT
feature is set by the device
Quoted 1 line
+ this field must contain a non zero value.
Same nits here as for the PCI definition above.

Ok. will change.

Quoted 9 lines

content.tex \chapter{Reserved Feature
Quoted 6 lines
content.tex +2 -0
+  }
+  \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 +6708,11 @@ \chapter{Reserved Feature
Bits}\label{sec:Reserved Feature Bits}
content.tex \chapter{Reserved Feature · line 6709
Quoted 2 lines
content.tex +0 -0
   transport specific.
   For more details about driver notifications over PCI see \ref{sec:Virtio

Quoted 9 lines

Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
content.tex \chapter{Reserved Feature · line 6711
Quoted 3 lines
content.tex +2 -0
+  \item[VIRTIO_F_DEV_RESET_TIMEOUT(40)] This feature indicates that
+ the device  advertises a maximum reset timeout which the driver should
use during device reset stage.
"...advertises a timeout value for the device reset operation." ?

Ack.
Timeout is anyway for the maximum. :-)

Quoted 19 lines

content.tex \chapter{Reserved Feature · line 6712
Quoted 1 line
content.tex +1 -0
+  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}.
content.tex \chapter{Reserved Feature · line 6713
Quoted 1 line
content.tex +1 -0
+  For more details about device reset timeout over MMIO see \ref{sec:Virtio
Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
Quoted 5 lines
content.tex +1 -0
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
 Bits}
Not quite sure yet how to model this for CCW, but it should not present any problems (I'm not aware of any devices addressed via CCW that would currently need this anyway... an advertised timeout value for *any* CCW operation that involves channel I/O would make more sense, I think. But that's beyond the scope of this proposal, and I'm not sure if we would be able to come up with anything sane considering the other variables like delays on the channel etc. involved, anyway.)

Quoted 1 line

So on the whole, I think this basically looks good.

Ok. Thanks a lot for te review. I will send v2 fixing above comments.