public inbox for virtio-comment@lists.linux.dev
 help / color / mirror / Atom feed
* [PATCH v3 0/3] Implement virtio SUSPEND and RESUME feature
@ 2025-06-23  8:36 Zhu Lingshan
  2025-06-23  8:36 ` [PATCH v3 1/3] virtio: re-order device status bits Zhu Lingshan
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Zhu Lingshan @ 2025-06-23  8:36 UTC (permalink / raw)
  To: mst, cohuck, jasowang; +Cc: virtio-comment, Ray.Huang, Zhu Lingshan

This series implements SUSPEND and RESUME feature
for virtio devices. This feature could be used
to perform live migration or take snapshots
of the devices.

This series re-orders the device status
bits, and documents feature bit 42.

Github issue link:
https://github.com/oasis-tcs/virtio-spec/issues/229

Zhu Lingshan (3):
  virtio: re-order device status bits
  virtio: document feature bit 42
  virtio: introduce SUSPEND and RESUME feature

 content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 8 deletions(-)

-- 
2.49.0


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/3] virtio: re-order device status bits
  2025-06-23  8:36 [PATCH v3 0/3] Implement virtio SUSPEND and RESUME feature Zhu Lingshan
@ 2025-06-23  8:36 ` Zhu Lingshan
  2025-06-26 11:39   ` Parav Pandit
  2025-06-23  8:36 ` [PATCH v3 2/3] virtio: document feature bit 42 Zhu Lingshan
  2025-06-23  8:36 ` [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature Zhu Lingshan
  2 siblings, 1 reply; 17+ messages in thread
From: Zhu Lingshan @ 2025-06-23  8:36 UTC (permalink / raw)
  To: mst, cohuck, jasowang; +Cc: virtio-comment, Ray.Huang, Zhu Lingshan

This commit re-orders the device status bits.

Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 content.tex | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/content.tex b/content.tex
index d3fc6a4..1efc2a5 100644
--- a/content.tex
+++ b/content.tex
@@ -36,19 +36,19 @@ \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[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
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 2/3] virtio: document feature bit 42
  2025-06-23  8:36 [PATCH v3 0/3] Implement virtio SUSPEND and RESUME feature Zhu Lingshan
  2025-06-23  8:36 ` [PATCH v3 1/3] virtio: re-order device status bits Zhu Lingshan
@ 2025-06-23  8:36 ` Zhu Lingshan
  2025-06-26 11:45   ` Parav Pandit
  2025-06-23  8:36 ` [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature Zhu Lingshan
  2 siblings, 1 reply; 17+ messages in thread
From: Zhu Lingshan @ 2025-06-23  8:36 UTC (permalink / raw)
  To: mst, cohuck, jasowang; +Cc: virtio-comment, Ray.Huang, Zhu Lingshan

This commit documents feture bit 42
VIRTIO_NET_F_GUEST_RSC6

Fixes: 94384142 ("content: Declare virtio-net legacy feature bits 41-42")
Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
---
 content.tex | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/content.tex b/content.tex
index 1efc2a5..2e8da46 100644
--- a/content.tex
+++ b/content.tex
@@ -99,10 +99,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 42] 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[43 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-23  8:36 [PATCH v3 0/3] Implement virtio SUSPEND and RESUME feature Zhu Lingshan
  2025-06-23  8:36 ` [PATCH v3 1/3] virtio: re-order device status bits Zhu Lingshan
  2025-06-23  8:36 ` [PATCH v3 2/3] virtio: document feature bit 42 Zhu Lingshan
@ 2025-06-23  8:36 ` Zhu Lingshan
  2025-06-26 11:59   ` Parav Pandit
  2 siblings, 1 reply; 17+ messages in thread
From: Zhu Lingshan @ 2025-06-23  8:36 UTC (permalink / raw)
  To: mst, cohuck, jasowang; +Cc: virtio-comment, Ray.Huang, Zhu Lingshan

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


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 1/3] virtio: re-order device status bits
  2025-06-23  8:36 ` [PATCH v3 1/3] virtio: re-order device status bits Zhu Lingshan
@ 2025-06-26 11:39   ` Parav Pandit
  0 siblings, 0 replies; 17+ messages in thread
From: Parav Pandit @ 2025-06-26 11:39 UTC (permalink / raw)
  To: Zhu Lingshan, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com


> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: 23 June 2025 02:07 PM
> 
> This commit re-orders the device status bits.

A better message would be that defines their ordering,

"Rearranged the bit definitions to list them in ascending order."

> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> ---
Anyways,
Reviewed-by: Parav Pandit <parav@nvidia.com>

>  content.tex | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index d3fc6a4..1efc2a5 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -36,19 +36,19 @@ \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[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
> --
> 2.49.0
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 2/3] virtio: document feature bit 42
  2025-06-23  8:36 ` [PATCH v3 2/3] virtio: document feature bit 42 Zhu Lingshan
@ 2025-06-26 11:45   ` Parav Pandit
  2025-06-26 13:06     ` Zhu, Lingshan
  0 siblings, 1 reply; 17+ messages in thread
From: Parav Pandit @ 2025-06-26 11:45 UTC (permalink / raw)
  To: Zhu Lingshan, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com


> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: 23 June 2025 02:07 PM
> 
> This commit documents feture bit 42
> VIRTIO_NET_F_GUEST_RSC6
> 
> Fixes: 94384142 ("content: Declare virtio-net legacy feature bits 41-42")
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> ---
>  content.tex | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index 1efc2a5..2e8da46 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -99,10 +99,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 42] Feature bits reserved for extensions to the queue and
>    feature negotiation mechanisms, see \ref{sec:Reserved Feature Bits}
> 
This does not look accurate.
Bits 41 and 42 are used by the device specific type and not generic queue related mechanism.
In this case it is used by the net device.

So right documentation should be,

0 to 23, 41, 42 and 50 to 127 Feature bits for the specific device type

24 to 40 Feature bits reserved for extensions to the queue and feature negotiation mechanisms

43 to 49, and 128 and above Feature bits reserved for future extensions.

> -\item[42 to 49, and 128 and above] Feature bits reserved for future
> extensions.
> +\item[43 to 49, and 128 and above] Feature bits reserved for future
> extensions.
>  \end{description}
> 
>  \begin{note}
> --
> 2.49.0
> 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-23  8:36 ` [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature Zhu Lingshan
@ 2025-06-26 11:59   ` Parav Pandit
  2025-06-26 13:25     ` Zhu, Lingshan
  2025-06-27  4:33     ` Parav Pandit
  0 siblings, 2 replies; 17+ messages in thread
From: Parav Pandit @ 2025-06-26 11:59 UTC (permalink / raw)
  To: Zhu Lingshan, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com


> 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?

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 2/3] virtio: document feature bit 42
  2025-06-26 11:45   ` Parav Pandit
@ 2025-06-26 13:06     ` Zhu, Lingshan
  0 siblings, 0 replies; 17+ messages in thread
From: Zhu, Lingshan @ 2025-06-26 13:06 UTC (permalink / raw)
  To: Parav Pandit, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

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

>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: 23 June 2025 02:07 PM
>>
>> This commit documents feture bit 42
>> VIRTIO_NET_F_GUEST_RSC6
>>
>> Fixes: 94384142 ("content: Declare virtio-net legacy feature bits 41-42")
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>> ---
>>   content.tex | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index 1efc2a5..2e8da46 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -99,10 +99,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 42] Feature bits reserved for extensions to the queue and
>>     feature negotiation mechanisms, see \ref{sec:Reserved Feature Bits}
>>
> This does not look accurate.
> Bits 41 and 42 are used by the device specific type and not generic queue related mechanism.
> In this case it is used by the net device.
>
> So right documentation should be,
>
> 0 to 23, 41, 42 and 50 to 127 Feature bits for the specific device type
>
> 24 to 40 Feature bits reserved for extensions to the queue and feature negotiation mechanisms
>
> 43 to 49, and 128 and above Feature bits reserved for future extensions.
OK, this looks better
>
>> -\item[42 to 49, and 128 and above] Feature bits reserved for future
>> extensions.
>> +\item[43 to 49, and 128 and above] Feature bits reserved for future
>> extensions.
>>   \end{description}
>>
>>   \begin{note}
>> --
>> 2.49.0
>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 11:59   ` Parav Pandit
@ 2025-06-26 13:25     ` Zhu, Lingshan
  2025-06-26 13:40       ` Parav Pandit
                         ` (2 more replies)
  2025-06-27  4:33     ` Parav Pandit
  1 sibling, 3 replies; 17+ messages in thread
From: Zhu, Lingshan @ 2025-06-26 13:25 UTC (permalink / raw)
  To: Parav Pandit, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@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 --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 13:25     ` Zhu, Lingshan
@ 2025-06-26 13:40       ` Parav Pandit
  2025-06-27  1:57         ` Zhu, Lingshan
  2025-06-26 15:42       ` Cornelia Huck
  2025-06-27 11:47       ` Michael S. Tsirkin
  2 siblings, 1 reply; 17+ messages in thread
From: Parav Pandit @ 2025-06-26 13:40 UTC (permalink / raw)
  To: Zhu, Lingshan, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

> From: Zhu, Lingshan <lingshan.zhu@amd.com> 
> Sent: 26 June 2025 06:56 PM

[....]

I wont be able to reply to this non text format inline.
So please find the reply here.

I will not be able to take RFC 2119 keywords sprinkled in general description into the spec.

Even worse, they are sprinkled non-consistently sometimes "SHOULD" and sometimes "should" in your patch.

Virtio spec restricts usage of those keywords only in the requirements section.
Existing spec is an example of it in all the sections.

Please refer to the rest of the spec for such nomenclature.
The reasons are also explained in [1]. Please refer [1].

You didn't answer 'why is it so hard for your patch to follow the current spec nomenclature.' 
i.e. to change 4 words...
If there is no reason, I do not want to debate on this anymore.
Both of us have more important things to do in virtio and otherwise. :)

And I didn't ignore your comments, I replied with [2], which the last email in the thread.

[1] https://lore.kernel.org/virtio-comment/CY8PR12MB71957FB8A0337FB62A1CDB41DC6AA@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m3ea175d50e7ffc9816a322384e6766492215469f
[2] https://lore.kernel.org/virtio-comment/CY8PR12MB71957FB8A0337FB62A1CDB41DC6AA@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5d09faabdaf91a97b5844b040a19f5700c377870

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 13:25     ` Zhu, Lingshan
  2025-06-26 13:40       ` Parav Pandit
@ 2025-06-26 15:42       ` Cornelia Huck
  2025-06-26 16:01         ` Parav Pandit
  2025-06-27  1:46         ` Zhu, Lingshan
  2025-06-27 11:47       ` Michael S. Tsirkin
  2 siblings, 2 replies; 17+ messages in thread
From: Cornelia Huck @ 2025-06-26 15:42 UTC (permalink / raw)
  To: Zhu, Lingshan, Parav Pandit, mst@redhat.com, jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

On Thu, Jun 26 2025, "Zhu, Lingshan" <lingshan.zhu@amd.com> wrote:

> On 6/26/2025 7:59 PM, Parav Pandit wrote:
>
>>> From: Zhu Lingshan<lingshan.zhu@amd.com>
>>> Sent: 23 June 2025 02:07 PM
>>> @@ -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
>

Not MST, but you can have my input.

We refer to the RFC for the definitions of SHOULD and friends, it does
not say where to use them.

The intention is to use them in normative statements only, so that
implementers have clear guidelines on the requirements. The legacy
sections are arguably a special case (for implementers who want to
support legacy implementations.) I don't think we want the all-caps key
words leaking into other sections (any more than what might have
happened already.)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 15:42       ` Cornelia Huck
@ 2025-06-26 16:01         ` Parav Pandit
  2025-06-27  1:46         ` Zhu, Lingshan
  1 sibling, 0 replies; 17+ messages in thread
From: Parav Pandit @ 2025-06-26 16:01 UTC (permalink / raw)
  To: Cornelia Huck, Zhu, Lingshan, mst@redhat.com, jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: 26 June 2025 09:13 PM


> On Thu, Jun 26 2025, "Zhu, Lingshan" <lingshan.zhu@amd.com> wrote:
> 
> > On 6/26/2025 7:59 PM, Parav Pandit wrote:
> >
> >>> From: Zhu Lingshan<lingshan.zhu@amd.com>
> >>> Sent: 23 June 2025 02:07 PM
> >>> @@ -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
> >
> 
> Not MST, but you can have my input.
> 
> We refer to the RFC for the definitions of SHOULD and friends, it does not say
> where to use them.
> 
> The intention is to use them in normative statements only, so that
> implementers have clear guidelines on the requirements. The legacy sections
> are arguably a special case (for implementers who want to support legacy
> implementations.) I don't think we want the all-caps key words leaking into
> other sections (any more than what might have happened already.)

+1.
Let me send a short patch to update our "terminology" section to provide this guidance.
It will be useful in general for everyone.
Sending soon.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 15:42       ` Cornelia Huck
  2025-06-26 16:01         ` Parav Pandit
@ 2025-06-27  1:46         ` Zhu, Lingshan
  2025-06-27  8:27           ` Cornelia Huck
  1 sibling, 1 reply; 17+ messages in thread
From: Zhu, Lingshan @ 2025-06-27  1:46 UTC (permalink / raw)
  To: Cornelia Huck, Parav Pandit, mst@redhat.com, jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

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

On 6/26/2025 11:42 PM, Cornelia Huck wrote:

> On Thu, Jun 26 2025, "Zhu, Lingshan"<lingshan.zhu@amd.com> wrote:
>
>> On 6/26/2025 7:59 PM, Parav Pandit wrote:
>>
>>>> From: Zhu Lingshan<lingshan.zhu@amd.com>
>>>> Sent: 23 June 2025 02:07 PM
>>>> @@ -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
>>
> Not MST, but you can have my input.
>
> We refer to the RFC for the definitions of SHOULD and friends, it does
> not say where to use them.

exactly

>
> The intention is to use them in normative statements only, so that
> implementers have clear guidelines on the requirements. The legacy
> sections are arguably a special case (for implementers who want to
> support legacy implementations.) I don't think we want the all-caps key
> words leaking into other sections (any more than what might have
> happened already.)

so this is a new requirement, we need to document these guidance

>

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 13:40       ` Parav Pandit
@ 2025-06-27  1:57         ` Zhu, Lingshan
  0 siblings, 0 replies; 17+ messages in thread
From: Zhu, Lingshan @ 2025-06-27  1:57 UTC (permalink / raw)
  To: Parav Pandit, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

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

On 6/26/2025 9:40 PM, Parav Pandit wrote:

>> From: Zhu, Lingshan<lingshan.zhu@amd.com> 
>> Sent: 26 June 2025 06:56 PM
> [....]
>
> I wont be able to reply to this non text format inline.
> So please find the reply here.
>
> I will not be able to take RFC 2119 keywords sprinkled in general description into the spec.
>
> Even worse, they are sprinkled non-consistently sometimes "SHOULD" and sometimes "should" in your patch.
>
> Virtio spec restricts usage of those keywords only in the requirements section.
> Existing spec is an example of it in all the sections.

do you find any sections documenting this restriction?
If we want this rule, we should document it first in the spec.

>
> Please refer to the rest of the spec for such nomenclature.
> The reasons are also explained in [1]. Please refer [1].
>
> You didn't answer 'why is it so hard for your patch to follow the current spec nomenclature.'
> i.e. to change 4 words...
> If there is no reason, I do not want to debate on this anymore.
> Both of us have more important things to do in virtio and otherwise. :)
>
> And I didn't ignore your comments, I replied with [2], which the last email in the thread.
>
> [1]https://lore.kernel.org/virtio-comment/CY8PR12MB71957FB8A0337FB62A1CDB41DC6AA@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m3ea175d50e7ffc9816a322384e6766492215469f
> [2]https://lore.kernel.org/virtio-comment/CY8PR12MB71957FB8A0337FB62A1CDB41DC6AA@CY8PR12MB7195.namprd12.prod.outlook.com/T/#m5d09faabdaf91a97b5844b040a19f5700c377870

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 11:59   ` Parav Pandit
  2025-06-26 13:25     ` Zhu, Lingshan
@ 2025-06-27  4:33     ` Parav Pandit
  1 sibling, 0 replies; 17+ messages in thread
From: Parav Pandit @ 2025-06-27  4:33 UTC (permalink / raw)
  To: Parav Pandit, Zhu Lingshan, mst@redhat.com, cohuck@redhat.com,
	jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

Hi Lingshan,

> From: Parav Pandit <parav@nvidia.com>
> Sent: 26 June 2025 05:30 PM

[..]
 
> 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?

This series in my view is reaching nearly good state, when you post v4 with these small changes,
Can you please add below line to the commit log?

This helps to connect github issue, patches and subsequently voting ballot.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/229

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-27  1:46         ` Zhu, Lingshan
@ 2025-06-27  8:27           ` Cornelia Huck
  0 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2025-06-27  8:27 UTC (permalink / raw)
  To: Zhu, Lingshan, Parav Pandit, mst@redhat.com, jasowang@redhat.com
  Cc: virtio-comment@lists.linux.dev, Ray.Huang@amd.com

On Fri, Jun 27 2025, "Zhu, Lingshan" <lingshan.zhu@amd.com> wrote:

> On 6/26/2025 11:42 PM, Cornelia Huck wrote:
>
>> The intention is to use them in normative statements only, so that
>> implementers have clear guidelines on the requirements. The legacy
>> sections are arguably a special case (for implementers who want to
>> support legacy implementations.) I don't think we want the all-caps key
>> words leaking into other sections (any more than what might have
>> happened already.)
>
> so this is a new requirement, we need to document these guidance

Not new, but not documented, either. I see that Parav is already
tackling the latter part (thanks!)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature
  2025-06-26 13:25     ` Zhu, Lingshan
  2025-06-26 13:40       ` Parav Pandit
  2025-06-26 15:42       ` Cornelia Huck
@ 2025-06-27 11:47       ` Michael S. Tsirkin
  2 siblings, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2025-06-27 11:47 UTC (permalink / raw)
  To: Zhu, Lingshan
  Cc: Parav Pandit, cohuck@redhat.com, jasowang@redhat.com,
	virtio-comment@lists.linux.dev, Ray.Huang@amd.com

On Thu, Jun 26, 2025 at 09:25:42PM +0800, Zhu, Lingshan wrote:
> 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.


While true, it is confusing to the reader, and tends to make writers
sloopy about adding confirmance statements.

Our recommendation (which we do not always remembered to follow, sadly)
is to do it like this:

In the non confirmance sections, avoid these keywords, describing the
functionality in a detailed way but without putting too much
emphasis on who does what and the specific level or requirement
and without using keywords.

Additionally, repeat the requirement in a short but formal way in the
confirmance section. For details of the operation, reader has the
non-confirmance sections.



Hope that helps.




^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2025-06-27 11:47 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-23  8:36 [PATCH v3 0/3] Implement virtio SUSPEND and RESUME feature Zhu Lingshan
2025-06-23  8:36 ` [PATCH v3 1/3] virtio: re-order device status bits Zhu Lingshan
2025-06-26 11:39   ` Parav Pandit
2025-06-23  8:36 ` [PATCH v3 2/3] virtio: document feature bit 42 Zhu Lingshan
2025-06-26 11:45   ` Parav Pandit
2025-06-26 13:06     ` Zhu, Lingshan
2025-06-23  8:36 ` [PATCH v3 3/3] virtio: introduce SUSPEND and RESUME feature Zhu Lingshan
2025-06-26 11:59   ` Parav Pandit
2025-06-26 13:25     ` Zhu, Lingshan
2025-06-26 13:40       ` Parav Pandit
2025-06-27  1:57         ` Zhu, Lingshan
2025-06-26 15:42       ` Cornelia Huck
2025-06-26 16:01         ` Parav Pandit
2025-06-27  1:46         ` Zhu, Lingshan
2025-06-27  8:27           ` Cornelia Huck
2025-06-27 11:47       ` Michael S. Tsirkin
2025-06-27  4:33     ` Parav Pandit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox