RE: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature

> From: Zhu Lingshan <[email protected]>
> Sent: 23 June 2025 02:07 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 <[email protected]>
> Signed-off-by: Jason Wang <[email protected]>
> Fixes:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> ub.com%2Foasis-tcs%2Fvirtio-
> spec%2Fissues%2F229&data=05%7C02%7Cparav%40nvidia.com%7C3085ab
> 378f264b5c845808ddb2313478%7C43083d15727340c1b7db39efd9ccc17
> a%7C0%7C0%7C638862646595099882%7CUnknown%7CTWFpbGZsb3d8e
> yJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIj
> oiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Pf2ub%2FwVzdmf
> A1JjScOJ7D%2B%2BNea9XgVFYdvvDQxym40%3D&reserved=0
> ---
>  content.tex | 59
> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 2e8da46..c987334c 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, and 50 to 127] Feature bits for the specific device type
> 
> -\item[24 to 42] 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}
> 
Ignoring this delta as the fix of patch-2 will change this.

> -\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
> SHOULD set the DRIVER_OK bit to 0 once it has been suspended.
> +
You ignored the inputs.
I do not agree to add the "SHOULD" normative wording in the general description area.
The reasoning is explained already.
Please adapt to the existing style of this spec to keep the normative in requirements section.

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.

Is this really that hard to write above way?

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

Similarly,

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

Rest of the requirements and description looks good to me.

Can you please fix these 2 small things in the general description and patch-2?