Re: [PATCH v4 3/3] virtio: introduce SUSPEND and RESUME feature

["Zhu, Lingshan" <lingshan.zhu@amd.com>]

[-- Attachment #1: Type: text/plain, Size: 7124 bytes --]

On 7/2/2025 7:53 PM, Parav Pandit wrote:

>
>> From: Zhu Lingshan<lingshan.zhu@amd.com>
>> Sent: 02 July 2025 02:24 PM
>>
>> 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.
>>
>> Signed-off-by: Zhu Lingshan<lingshan.zhu@amd.com>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> Fixes:https://github.com/oasis-tcs/virtio-spec/issues/229
>> ---
>>   content.tex | 59
>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 57 insertions(+), 2 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 9213dc0..32cabfa 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -42,6 +42,9 @@ \section{\field{Device Status} Field}\label{sec:Basic
>> Facilities of a Virtio Dev  \item[FEATURES_OK (8)] Indicates that the driver has
>> acknowledged all the
>>     features it understands, and feature negotiation is complete.
>>
>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that
>> +the
>> +  device has been suspended by the driver.
>> +
>>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>     an error from which it can't recover.
>>
>> @@ -99,10 +102,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a
>> Virtio Device / Feature B  \begin{description}
>>   \item[0 to 23, 41, 42 and 50 to 127] Feature bits for the specific device type
>>
>> -\item[24 to 40] Feature bits reserved for extensions to the queue and
>> +\item[24 to 40, and 43] Feature bits reserved for extensions to the
>> +queue and
>>     feature negotiation mechanisms, see \ref{sec:Reserved Feature Bits}
>>
>> -\item[43 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,54 @@ \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 sets 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 sets 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.
>> +\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}
>> +
> I missed to suggest this before or suggested, don't recall exactly.
>
> You should add the similar normative here to indicate what are the requirements on:
>
> Once the driver sets DRIVER_OK to 1 when suspended, the driver must wait for SUSPEND bit to turn 0 and DRIVER_OK to 1.

OK, since MST suggest we repeat and emphasize some key requirements in 
the normative sections. And I think what you mean is: "When the device 
has been suspended, once the driver sets DRIVER_OK bit in \field{device 
status} to 1, the driver MUST wait for the SUSPEND bit in \field{device 
status} to turn 0 and DRIVER_OK bit in \field{device_status} to turn 1 
before any normal operations." Thanks

>
> (you have described this in the description, just the normative is missing).
> Though it is not a blocker to me.
>
>> +\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 SHOULD ignore any write access to its Configuration Space
>> +while suspended, except for \field{device status} if it is part of the
>> Configuration Space.
>> +
>> +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.
>> +
>> +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 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.
>> +
>> +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
>> +923,10 @@ \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
>>
> If you happen to send v5, please add above suggested requirement, if not it can be added later as well right after this as additional patch for same github issue 229.
>
> Reviewed-by: Parav Pandit<parav@nvidia.com>

[-- Attachment #2: Type: text/html, Size: 7989 bytes --]