Re: [virtio-comment] Public review comment for virtio-v1.0-csprd02: 2.1 Device Status Field

[Rusty Russell <rusty@au1.ibm.com>]

Rusty Russell <rusty@au1.ibm.com> writes:
> Fam Zheng <famz@redhat.com> writes:
>> DEVICE_NEEDS_RESET (16)
>> Indicate that the device is in an unrecoverable failed state, and need reset.

Minor point, I chose 64 (closer to FAILED, which is 128) instead.

>> The driver MUST reset the device upon seeing a set DEVICE_NEEDS_RESET before
>> doing other operations on this device.

This implies an extra read on each operation, so I think it's too
strict.  How about:

        The driver SHOULD NOT rely on completion of operations of a
        device if DEVICE_NEEDS_RESET is set.

        Example: The driver can't assume requests in flight will be
        completed if DEVICE_NEEDS_RESET is set, nor can it assume that
        they have not been completed.  A good implementation will try to
        recover by issuing a reset.

>> The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state that a
>> reset is needed, and MAY send a notification of device configuration changes to
>> driver.

I think we can be stricter here, however:

        The device SHOULD set DEVICE_NEEDS_RESET when it enters an error
        state that a reset is needed.  After it sets DEVICE_NEEDS_RESET,
        the device MUST send a device configuration change notification
        to the driver.

Here's what I ended up with:

diff --git a/content.tex b/content.tex
index 58c3ebc..5b1c500 100644
--- a/content.tex
+++ b/content.tex
@@ -40,6 +40,9 @@ following bits are defined:
 \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
   drive the device.
 
+\item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
+  an error from which it can't recover.
+
 \item[FAILED (128)] Indicates that something went wrong in the guest,
   and it has given up on the device. This could be an internal
   error, or the driver didn't like the device for some reason, or
@@ -56,11 +59,24 @@ The driver MUST NOT clear a
 \field{device status} bit.  If the driver sets the FAILED bit,
 the driver MUST later reset the device before attempting to re-initialize.
 
+The driver SHOULD NOT rely on completion of operations of a
+device if DEVICE_NEEDS_RESET is set.
+\begin{note}
+For example, the driver can't assume requests in flight will be
+completed if DEVICE_NEEDS_RESET is set, nor can it assume that
+they have not been completed.  A good implementation will try to
+recover by issuing a reset.
+\end{note}
+
 \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
 The device MUST initialize \field{device status} to 0 upon reset.
 
 The device MUST NOT consume buffers or notify the driver before DRIVER_OK.
 
+\label{sec:Basic Facilities of a Virtio Device / Device Status Field / DEVICE_NEEDS_RESET}The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state
+that a reset is needed.  After it sets DEVICE_NEEDS_RESET, the device
+MUST send a device configuration change notification to the driver.
+
 \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
 
 Each virtio device offers all the features it understands.  During
@@ -899,6 +915,9 @@ for (;;) {
 For devices where the device-specific configuration information can be changed, an
 interrupt is delivered when a device-specific configuration change occurs.
 
+In addition, this interrupt is triggered by the device setting
+DEVICE_NEEDS_RESET (see \ref{sec:Basic Facilities of a Virtio Device / Device Status Field / DEVICE_NEEDS_RESET}).
+
 \section{Device Cleanup}\label{sec:General Initialization And Device Operation / Device Cleanup}
 
 Once the driver has set the DRIVER_OK status bit, all the configured