Re: [PATCH v1] virtio: introduce SUSPEND and RESUME feature

On 5/21/2025 6:30 PM, Michael S. Tsirkin wrote:
> On Wed, May 21, 2025 at 06:20:49PM +0800, Zhu Lingshan wrote:
>> This commit allows the driver to suspend the
>> device through a new device status bit SUSPEND
>> and resume the device running by re-setting
>> DRIVER_OK bit in device status.
>>
>> This commit re-orders the device status bits.
>>
>> Signed-off-by: Zhu Lingshan <[email protected]>
> this looks good. yet something to improve.
>
>> ---
>>  content.tex | 72 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 63 insertions(+), 9 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index d3fc6a4..38b4529 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -36,19 +36,22 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>      this bit.  For example, under Linux, drivers can be loadable modules.
>>    \end{note}
>>  
>> -\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
>> -  even a fatal error during device operation.
>> +\item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>> +  drive the device.
>> +
>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>> +  device has been suspended by the driver.
>>  
>>  \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
>>    features it understands, and feature negotiation is complete.
>>  
>> -\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
>> +  even a fatal error during device operation.
>>  \end{description}
>>  
>>  The \field{device status} field starts out as 0, and is reinitialized to 0 by
>> @@ -99,10 +102,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
>>  \begin{description}
>>  \item[0 to 23, and 50 to 127] Feature bits for the specific device type
>>  
>> -\item[24 to 41] Feature bits reserved for extensions to the queue and
>> +\item[24 to 43] Feature bits reserved for extensions to the queue and
>>    feature negotiation mechanisms, see \ref{sec:Reserved Feature Bits}
>>  
>> -\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
>> +\item[44 to 49, and 128 and above] Feature bits reserved for future extensions.
>>  \end{description}
>>  
>>  \begin{note}
>> @@ -629,6 +632,52 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation /
>>  
>>  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers.
>>  
>> +\section{Device Suspend}\label{sec:General Initialization And Device Operation / Device Suspend}
>> +
>> +If VIRTIO_F_SUSPEND is negotiated, the driver is eligible to suspend the device by setting the SUSPEND bit in \field{device status} to 1, and the device SHOULD set the DRIVER_OK bit to 0 once it has been suspended.
>> +
>> +If the device has been suspended, the driver can resume the device running by setting the DRIVER_OK bit in \field{device status} to 1, and the device should set the SUSPEND bit to 0 once it resumes running.
>> +
>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The driver SHOULD NOT set SUSPEND bit if DRIVER_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +Once the driver sets SUSPEND bit in \field{device status} to 1:
>> +\begin{itemize}
>> +\item The driver MUST verify whether the device has been suspended by re-reading \field{device status}, examining whether the SUSPEND bit is set to 1 and the DRIVER_OK bit is set to 0.
> should driver poll for DEVICE_NEEDS_RESET here? if yes say so.

I think the driver can rely on a config interrupt, the spec says:
The device SHOULD set DEVICE_NEEDS_RESET when it enters an error state that a reset is needed.
If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device MUST send a device configuration change notification to the driver. 

>
>
>> +\item The driver MUST NOT make any more buffers available to the device.
>> +\item The driver MUST NOT send notifications for any virtqueues.
>> +\item The driver MUST NOT make any changes to Device Configuration Space except for \field{device status} if it is part of the Configuration Space.
>> +\end{itemize}
>> +
>> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The device MUST ignore any operations on the SUSPEND bit from the driver if the device has not been completely initialized by the procedures in \ref{sec:General Initialization And Device Operation / Device Initialization}
>> +
>> +The device MUST ignore any write access to its Configuration Space while
>> +suspended, except for \field{device status} if it is part of the Configuration Space.
> I am inclined to relax it to SHOULD, even just so the drivers do not get
> lazy and do not violate their side of the bargain.

sure!

>
>> +
>> +A device MUST NOT send any notifications for any virtqueues,
>> +access any virtqueues, or modify any fields in
>> +its Configuration Space while suspended.
>> +
>> +If changes occur in the Configuration Space during suspended period,
>> +the device MUST NOT send any configuration change notifications.
>> +Instead, the device MUST send the notification when it resumes running.
>> +
>> +When the driver sets SUSPEND, the device MUST either suspend itself or sets DEVICE_NEEDS_RESET if failed to suspend.
> or set
>
>
> pls make it clear: what to set and when.

OK, v2 will say: If the driver sets the SUSPEND bit in \field{device status} to 1, the device MUST either suspend itself or set the DEVICE_NEEDS_RESET bit in \field{device status} to 1 when it fails to suspend.

>
>
>> +
>> +If the device has been suspended and the driver resumes the device running by setting DRIVER_OK to 1, the device MUST either resume normal operation or sets DEVICE_NEEDS_RESET if it fails to resume.
>> +
> pls make it clear: what to set and when.

v2 will say: If the device has been suspended and the driver resumes the device running by setting the DRIVER_OK bit in \field{device status} to 1, the device MUST either resume normal operation or set the DEVICE_NEEDS_RESET bit in \field{device status} to 1 when it fails to resume. Thanks Zhu Lingshan

>
>> +When the driver sets the SUSPEND bit to 1,
>> +the device SHOULD perform the following actions before presenting the SUSPEND bit as 1 and DRIVER_OK bit as 0 in the \field{device status}:
>> +
>> +\begin{itemize}
>> +\item Stop consuming more buffers of any virtqueues.
>> +\item Wait until all buffers that are being processed have been used.
>> +\item Send used buffer notifications to the driver.
>> +\end{itemize}
>> +
>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>  
>>  Virtio can use various different buses, thus the standard is split
>> @@ -872,6 +921,11 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
>>  	handling features reserved for future use.
>>  
>> +  \item[VIRTIO_F_SUSPEND(43)] This feature indicates that the driver can
>> +   suspend the device by set the SUSPEND bit to 1.
>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>> +
>>  \end{description}
>>  
>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> -- 
>> 2.49.0