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

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

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

On 6/26/2025 7:59 PM, Parav Pandit wrote:

>> From: Zhu Lingshan<lingshan.zhu@amd.com>
>> 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<lingshan.zhu@amd.com>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>> 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?

I believe you totally ignored my replies in the last thread.
There are no rules forbid using "SHOULD" in any non-normative sections.
Now here I copy the reply here again:

In the spec section 1.3 Terminology, it says:

The key words “MUST”, “MUST NOT”, “REQUIRED”, “SHALL”, “SHALL NOT”, “SHOULD”, “SHOULD NOT”, “RECOMMENDED”, “NOT RECOMMENDED”, “MAY”, and “OPTIONAL” in this document are to be interpreted as described in [RFC2119] and [RFC8174] when, and only when, they appear in all capitals, as shown here.

RFC 2119 says:

SHOULD This word, or the adjective "RECOMMENDED", mean that there
    may exist valid reasons in particular circumstances to ignore a
    particular item, but the full implications must be understood and
    carefully weighed before choosing a different course.


Here this "SHOULD" exactly conforms to the definition.

"SHOULD" has already been used in non-normative sections, for example:
2.5.4 Legacy Interfaces: when using the legacy interface, drivers SHOULD read these fields multiple times until two reads generate a consistent result.

The spec does not say: “SHOULD” can only be used in driver or device requirements section.

@MST, we need your input

>
>> +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.
same as above
>
>
>> +\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?

patch-2 can be fixed for sure

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