[virtio-dev] [PATCH v5 0/1] Define a low power mode for devices

[virtio-dev] [PATCH v5 0/1] Define a low power mode for devices

[David Stevens]

The Linux patch [1] added support for suspending virtio devices using
native PCI power management. However, Linux does PCI power management
during the noirq phase of suspend and resume. Without a mechanism to
suspend/resume virtio devices when the driver is suspended/resumed in
the early phase of suspend/late phase of resume, there is a window where
interrupts can be lost.

This revision of the patch tries to reconcile with the similar proposal
[2], which was pulled out of one of a patch series attempting to add
live migration support to virtio [3].

[1] https://lore.kernel.org/lkml/20231208070754.3132339-1-stevensd@chromium.org/
[2] https://lore.kernel.org/all/20240218132306.83456-1-lingshan.zhu@intel.com/
[3] https://lore.kernel.org/all/20231103103437.72784-1-lingshan.zhu@intel.com/

v4 -> v5:
 - Use a device status bit instead of a pci capability.
 - Reconcile differences with [2] by specifying that devices should
   finish processing buffers before suspending and that devices
   shouldn't access virtqueues while suspended.
 - Add more details about state transition acknowledgment.
v3 -> v4:
 - Define virtio-pci specific power management that can be used in
   conjunction with PCI power management.
v2 -> v3:
 - Use different words for some concepts to avoid conflicts with other
   parts of the spec.
 - Rewrite various sentences to improve clarity.
v1 -> v2:
 - Define virtio-pci support on top of PCI power management.
 - Add more conformance requirements.

David Stevens (1):
  Add SUSPEND bit to device status

 content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 5 deletions(-)

-- 
2.44.0.rc0.258.g7320e95886-goog


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] [PATCH v5 1/1] Add SUSPEND bit to device status

[David Stevens]

Add a SUSPEND bit to the device status field to allow drivers to suspend
virtio devices. On systems where drivers don't directly manage interrupt
routing (e.g. Linux), this allows the drivers to suspend their devices
and prevent interrupts from being sent when the interrupt routing system
does not expect interrupts.
---
 content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 5 deletions(-)

diff --git a/content.tex b/content.tex
index 0a62dce5f65f..2183c63c45ea 100644
--- a/content.tex
+++ b/content.tex
@@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 
 \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
   an error from which it can't recover.
+
+\item[SUSPEND (16)] Indicates that the device has been suspended by the
+  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
 \end{description}
 
 The \field{device status} field starts out as 0, and is reinitialized to 0 by
@@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 initialization sequence specified in
 \ref{sec:General Initialization And Device Operation / Device
 Initialization}.
-The driver MUST NOT clear a
-\field{device status} bit.  If the driver sets the FAILED bit,
-the driver MUST later reset the device before attempting to re-initialize.
+
+The driver MUST NOT clear a \field{device status} bit, except for the
+SUSPEND bit as described in \ref{sec:General Initialization And Device
+Operation / Device Suspend}. If the driver sets the FAILED bit, the
+driver MUST later reset the device before attempting to re-initialize.
 
 The driver SHOULD NOT rely on completion of operations of a
 device if DEVICE_NEEDS_RESET is set.
@@ -73,6 +78,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 recover by issuing a reset.
 \end{note}
 
+If VIRTIO_F_SUSPEND is negotiated, the driver MUST manage the SUSPEND bit
+as described in \ref{sec:General Initialization And Device Operation /
+Device Suspend}. Otherwise, the driver MUST NOT set the SUSPEND bit.
+
 \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
 
 The device MUST NOT consume buffers or send any used buffer
@@ -82,6 +91,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
 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.
 
+If VIRTIO_F_SUSPEND is negotiated, the device MUST manage the SUSPEND bit
+as described in \ref{sec:General Initialization And Device Operation /
+Device Suspend}. Otherwise, the device MUST ignore the SUSPEND bit.
+
 \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
 
 Each virtio device offers all the features it understands.  During
@@ -99,10 +112,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
 
-\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}
@@ -629,6 +642,67 @@ \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}
+
+When the VIRTIO_F_SUSPEND feature is negotiated, the driver can set the
+SUSPEND bit in \field{device status} to suspend a live device, and can
+clear the SUSPEND bit to resume a suspended device. A suspended device
+should pause its operation, but it must maintain it state such that it
+can immediately continue operation upon being resumed.
+
+Suspending a device via the SUSPEND bit is a seperate process from any
+transport-specific suspend mechanism.
+
+\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
+
+The driver MUST NOT set the SUSPEND bit if the DRIVER_OK bit is not set.
+
+After writing a new value to the SUSPEND bit, the driver MUST wait for
+the device to acknowledged the transition by reading from \field{device
+status} until the returned value of the SUSPEND bit matches the written
+value. During this period, the driver MAY abort the transition by writing
+a new value to the SUSPEND bit or by resetting the device.
+
+A driver MUST NOT access the device configuration space of a suspended
+device, except for \field{device status}.
+
+A driver MAY suspend a device that has buffers in its virtqueues. While
+the device is suspended, a driver MUST NOT modify any available buffers
+or their descriptors.
+
+A driver MUST NOT make any new buffers available to a suspended device.
+
+If a transport-specific suspend mechanism is available, the driver SHOULD
+use it to put the device into a deeper suspend state after setting the
+SUSPEND bit.
+
+\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
+
+A device MUST ignore writes to the SUSPEND bit if the DRIVER_OK bit is
+not set.
+
+A device MUST maintain its state while suspended such that all driver
+visible state after resuming exactly matches driver visible state
+before suspending.
+
+A device MUST ignore all writes to its configuration space while
+suspended, except for writes to \field{device status}.
+
+A device MUST NOT send notifications, access any virtqueues, or modify
+any fields in its configuration space while suspended.
+
+A device MAY send notifications, access any virtqueues, or modify its
+configuration space after the driver writes the SUSPEND bit but before
+the device acknowledges the transition by returning a \field{device
+status} value with the SUSPEND bit set. A device SHOULD finish processing
+and send the used buffer notification for any buffers it is able to
+before acknowledging the transition, but MAY retain buffers that cannot
+be immiedately processed (e.g. empty buffers in a network recieveq).
+
+A device SHOULD take steps to minimize its resource consumption while
+suspended, although what this involves is specific to the particular
+device implementation.
+
 \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
 
 Virtio can use various different buses, thus the standard is split
@@ -872,6 +946,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(42)] This feature indicates that the driver can
+    suspend the device via the SUSPEND bit in \field{device status} (see
+    \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}).
+
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
2.44.0.rc0.258.g7320e95886-goog


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[Zhu, Lingshan]

On 2/27/2024 9:53 AM, David Stevens wrote:
> Add a SUSPEND bit to the device status field to allow drivers to suspend
> virtio devices. On systems where drivers don't directly manage interrupt
> routing (e.g. Linux), this allows the drivers to suspend their devices
> and prevent interrupts from being sent when the interrupt routing system
> does not expect interrupts.
> ---
>   content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 84 insertions(+), 5 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 0a62dce5f65f..2183c63c45ea 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>   
>   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>     an error from which it can't recover.
> +
> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
>   \end{description}
>   
>   The \field{device status} field starts out as 0, and is reinitialized to 0 by
> @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>   initialization sequence specified in
>   \ref{sec:General Initialization And Device Operation / Device
>   Initialization}.
> -The driver MUST NOT clear a
> -\field{device status} bit.  If the driver sets the FAILED bit,
> -the driver MUST later reset the device before attempting to re-initialize.
> +
> +The driver MUST NOT clear a \field{device status} bit, except for the
> +SUSPEND bit as described in \ref{sec:General Initialization And Device
> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
> +driver MUST later reset the device before attempting to re-initialize.
Comparing to add a new exception, why not just re-setting DRIVER_OK to 
let the device
clearing SUSPEND? This issue has been addressed when Eugenio working on 
a similar STOP_BIT

https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html

Not sure it is my email client bug, it shows the authorship has been 
reset and
all sign-off-by are removed. We have been working on this since 2021, 
how about
just include our updated patch(WIP) in a new series?

Thanks
>   
>   The driver SHOULD NOT rely on completion of operations of a
>   device if DEVICE_NEEDS_RESET is set.
> @@ -73,6 +78,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>   recover by issuing a reset.
>   \end{note}
>   
> +If VIRTIO_F_SUSPEND is negotiated, the driver MUST manage the SUSPEND bit
> +as described in \ref{sec:General Initialization And Device Operation /
> +Device Suspend}. Otherwise, the driver MUST NOT set the SUSPEND bit.
> +
>   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>   
>   The device MUST NOT consume buffers or send any used buffer
> @@ -82,6 +91,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>   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.
>   
> +If VIRTIO_F_SUSPEND is negotiated, the device MUST manage the SUSPEND bit
> +as described in \ref{sec:General Initialization And Device Operation /
> +Device Suspend}. Otherwise, the device MUST ignore the SUSPEND bit.
> +
>   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>   
>   Each virtio device offers all the features it understands.  During
> @@ -99,10 +112,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
>   
> -\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}
> @@ -629,6 +642,67 @@ \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}
> +
> +When the VIRTIO_F_SUSPEND feature is negotiated, the driver can set the
> +SUSPEND bit in \field{device status} to suspend a live device, and can
> +clear the SUSPEND bit to resume a suspended device. A suspended device
> +should pause its operation, but it must maintain it state such that it
> +can immediately continue operation upon being resumed.
> +
> +Suspending a device via the SUSPEND bit is a seperate process from any
> +transport-specific suspend mechanism.
> +
> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> +
> +The driver MUST NOT set the SUSPEND bit if the DRIVER_OK bit is not set.
> +
> +After writing a new value to the SUSPEND bit, the driver MUST wait for
> +the device to acknowledged the transition by reading from \field{device
> +status} until the returned value of the SUSPEND bit matches the written
> +value. During this period, the driver MAY abort the transition by writing
> +a new value to the SUSPEND bit or by resetting the device.
> +
> +A driver MUST NOT access the device configuration space of a suspended
> +device, except for \field{device status}.
> +
> +A driver MAY suspend a device that has buffers in its virtqueues. While
> +the device is suspended, a driver MUST NOT modify any available buffers
> +or their descriptors.
> +
> +A driver MUST NOT make any new buffers available to a suspended device.
> +
> +If a transport-specific suspend mechanism is available, the driver SHOULD
> +use it to put the device into a deeper suspend state after setting the
> +SUSPEND bit.
> +
> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> +
> +A device MUST ignore writes to the SUSPEND bit if the DRIVER_OK bit is
> +not set.
> +
> +A device MUST maintain its state while suspended such that all driver
> +visible state after resuming exactly matches driver visible state
> +before suspending.
> +
> +A device MUST ignore all writes to its configuration space while
> +suspended, except for writes to \field{device status}.
> +
> +A device MUST NOT send notifications, access any virtqueues, or modify
> +any fields in its configuration space while suspended.
> +
> +A device MAY send notifications, access any virtqueues, or modify its
> +configuration space after the driver writes the SUSPEND bit but before
> +the device acknowledges the transition by returning a \field{device
> +status} value with the SUSPEND bit set. A device SHOULD finish processing
> +and send the used buffer notification for any buffers it is able to
> +before acknowledging the transition, but MAY retain buffers that cannot
> +be immiedately processed (e.g. empty buffers in a network recieveq).
> +
> +A device SHOULD take steps to minimize its resource consumption while
> +suspended, although what this involves is specific to the particular
> +device implementation.
> +
>   \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>   
>   Virtio can use various different buses, thus the standard is split
> @@ -872,6 +946,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(42)] This feature indicates that the driver can
> +    suspend the device via the SUSPEND bit in \field{device status} (see
> +    \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}).
> +
> +
>   \end{description}
>   
>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[David Stevens]

On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> On 2/27/2024 9:53 AM, David Stevens wrote:
> > Add a SUSPEND bit to the device status field to allow drivers to suspend
> > virtio devices. On systems where drivers don't directly manage interrupt
> > routing (e.g. Linux), this allows the drivers to suspend their devices
> > and prevent interrupts from being sent when the interrupt routing system
> > does not expect interrupts.
> > ---
> >   content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 file changed, 84 insertions(+), 5 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 0a62dce5f65f..2183c63c45ea 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >
> >   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >     an error from which it can't recover.
> > +
> > +\item[SUSPEND (16)] Indicates that the device has been suspended by the
> > +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
> >   \end{description}
> >
> >   The \field{device status} field starts out as 0, and is reinitialized to 0 by
> > @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >   initialization sequence specified in
> >   \ref{sec:General Initialization And Device Operation / Device
> >   Initialization}.
> > -The driver MUST NOT clear a
> > -\field{device status} bit.  If the driver sets the FAILED bit,
> > -the driver MUST later reset the device before attempting to re-initialize.
> > +
> > +The driver MUST NOT clear a \field{device status} bit, except for the
> > +SUSPEND bit as described in \ref{sec:General Initialization And Device
> > +Operation / Device Suspend}. If the driver sets the FAILED bit, the
> > +driver MUST later reset the device before attempting to re-initialize.
> Comparing to add a new exception, why not just re-setting DRIVER_OK to
> let the device
> clearing SUSPEND? This issue has been addressed when Eugenio working on
> a similar STOP_BIT
>
> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html

I don't think the device status bit is bit-addressable, so it's not
possible to re-set DRIVER_OK without also either re-setting or
clearing SUSPEND.

The linked series seems to effectively do the same thing as this patch
does. Just rather than an exception for SUSPEND, it explicitly lists
the bits which shouldn't be cleared. Although on the patch doing that,
there was feedback suggesting that it be done the way this patch does
it [1]. Personally, I think either an allowlist or a blocklist is
fine.

[1] https://lists.oasis-open.org/archives/virtio-dev/202111/msg00025.html

> Not sure it is my email client bug, it shows the authorship has been
> reset and
> all sign-off-by are removed. We have been working on this since 2021,
> how about
> just include our updated patch(WIP) in a new series?

Oh, sorry. I'm not very familiar with the process of collaboration
over email like this. I wasn't sure about adding Signed-off-by for
other people that haven't ready the patch yet, but I can add them
going forward if that's what's expected. And I'll fix up authorship
and add myself as Co-authored-by next time.

-David

> Thanks
> >
> >   The driver SHOULD NOT rely on completion of operations of a
> >   device if DEVICE_NEEDS_RESET is set.
> > @@ -73,6 +78,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >   recover by issuing a reset.
> >   \end{note}
> >
> > +If VIRTIO_F_SUSPEND is negotiated, the driver MUST manage the SUSPEND bit
> > +as described in \ref{sec:General Initialization And Device Operation /
> > +Device Suspend}. Otherwise, the driver MUST NOT set the SUSPEND bit.
> > +
> >   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> >
> >   The device MUST NOT consume buffers or send any used buffer
> > @@ -82,6 +91,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >   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.
> >
> > +If VIRTIO_F_SUSPEND is negotiated, the device MUST manage the SUSPEND bit
> > +as described in \ref{sec:General Initialization And Device Operation /
> > +Device Suspend}. Otherwise, the device MUST ignore the SUSPEND bit.
> > +
> >   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> >
> >   Each virtio device offers all the features it understands.  During
> > @@ -99,10 +112,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
> >
> > -\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}
> > @@ -629,6 +642,67 @@ \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}
> > +
> > +When the VIRTIO_F_SUSPEND feature is negotiated, the driver can set the
> > +SUSPEND bit in \field{device status} to suspend a live device, and can
> > +clear the SUSPEND bit to resume a suspended device. A suspended device
> > +should pause its operation, but it must maintain it state such that it
> > +can immediately continue operation upon being resumed.
> > +
> > +Suspending a device via the SUSPEND bit is a seperate process from any
> > +transport-specific suspend mechanism.
> > +
> > +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> > +
> > +The driver MUST NOT set the SUSPEND bit if the DRIVER_OK bit is not set.
> > +
> > +After writing a new value to the SUSPEND bit, the driver MUST wait for
> > +the device to acknowledged the transition by reading from \field{device
> > +status} until the returned value of the SUSPEND bit matches the written
> > +value. During this period, the driver MAY abort the transition by writing
> > +a new value to the SUSPEND bit or by resetting the device.
> > +
> > +A driver MUST NOT access the device configuration space of a suspended
> > +device, except for \field{device status}.
> > +
> > +A driver MAY suspend a device that has buffers in its virtqueues. While
> > +the device is suspended, a driver MUST NOT modify any available buffers
> > +or their descriptors.
> > +
> > +A driver MUST NOT make any new buffers available to a suspended device.
> > +
> > +If a transport-specific suspend mechanism is available, the driver SHOULD
> > +use it to put the device into a deeper suspend state after setting the
> > +SUSPEND bit.
> > +
> > +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> > +
> > +A device MUST ignore writes to the SUSPEND bit if the DRIVER_OK bit is
> > +not set.
> > +
> > +A device MUST maintain its state while suspended such that all driver
> > +visible state after resuming exactly matches driver visible state
> > +before suspending.
> > +
> > +A device MUST ignore all writes to its configuration space while
> > +suspended, except for writes to \field{device status}.
> > +
> > +A device MUST NOT send notifications, access any virtqueues, or modify
> > +any fields in its configuration space while suspended.
> > +
> > +A device MAY send notifications, access any virtqueues, or modify its
> > +configuration space after the driver writes the SUSPEND bit but before
> > +the device acknowledges the transition by returning a \field{device
> > +status} value with the SUSPEND bit set. A device SHOULD finish processing
> > +and send the used buffer notification for any buffers it is able to
> > +before acknowledging the transition, but MAY retain buffers that cannot
> > +be immiedately processed (e.g. empty buffers in a network recieveq).
> > +
> > +A device SHOULD take steps to minimize its resource consumption while
> > +suspended, although what this involves is specific to the particular
> > +device implementation.
> > +
> >   \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
> >
> >   Virtio can use various different buses, thus the standard is split
> > @@ -872,6 +946,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(42)] This feature indicates that the driver can
> > +    suspend the device via the SUSPEND bit in \field{device status} (see
> > +    \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}).
> > +
> > +
> >   \end{description}
> >
> >   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[Zhu, Lingshan]

On 2/27/2024 3:59 PM, David Stevens wrote:
> On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>> On 2/27/2024 9:53 AM, David Stevens wrote:
>>> Add a SUSPEND bit to the device status field to allow drivers to suspend
>>> virtio devices. On systems where drivers don't directly manage interrupt
>>> routing (e.g. Linux), this allows the drivers to suspend their devices
>>> and prevent interrupts from being sent when the interrupt routing system
>>> does not expect interrupts.
>>> ---
>>>    content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>    1 file changed, 84 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/content.tex b/content.tex
>>> index 0a62dce5f65f..2183c63c45ea 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>
>>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>      an error from which it can't recover.
>>> +
>>> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
>>> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
>>>    \end{description}
>>>
>>>    The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>> @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>    initialization sequence specified in
>>>    \ref{sec:General Initialization And Device Operation / Device
>>>    Initialization}.
>>> -The driver MUST NOT clear a
>>> -\field{device status} bit.  If the driver sets the FAILED bit,
>>> -the driver MUST later reset the device before attempting to re-initialize.
>>> +
>>> +The driver MUST NOT clear a \field{device status} bit, except for the
>>> +SUSPEND bit as described in \ref{sec:General Initialization And Device
>>> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
>>> +driver MUST later reset the device before attempting to re-initialize.
>> Comparing to add a new exception, why not just re-setting DRIVER_OK to
>> let the device
>> clearing SUSPEND? This issue has been addressed when Eugenio working on
>> a similar STOP_BIT
>>
>> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> I don't think the device status bit is bit-addressable, so it's not
> possible to re-set DRIVER_OK without also either re-setting or
> clearing SUSPEND.
Please correct me if I misunderstand your point,
I think it can be addressed, for example, in PCI it is an u8,
the device can clear a bit in it.


It may be easier to let the device to clear the SUSPEND bit.
>
> The linked series seems to effectively do the same thing as this patch
> does. Just rather than an exception for SUSPEND, it explicitly lists
> the bits which shouldn't be cleared. Although on the patch doing that,
> there was feedback suggesting that it be done the way this patch does
> it [1]. Personally, I think either an allowlist or a blocklist is
> fine.
>
> [1] https://lists.oasis-open.org/archives/virtio-dev/202111/msg00025.html
>
>> Not sure it is my email client bug, it shows the authorship has been
>> reset and
>> all sign-off-by are removed. We have been working on this since 2021,
>> how about
>> just include our updated patch(WIP) in a new series?
> Oh, sorry. I'm not very familiar with the process of collaboration
> over email like this. I wasn't sure about adding Signed-off-by for
> other people that haven't ready the patch yet, but I can add them
> going forward if that's what's expected. And I'll fix up authorship
> and add myself as Co-authored-by next time.
This maps to my Intel assignment.
So please let me send out a V2 and once the patch passed review process,
we can add more PCI patches into the series. I will add your sign-off-by
there.
>
> -David
>
>> Thanks
>>>    The driver SHOULD NOT rely on completion of operations of a
>>>    device if DEVICE_NEEDS_RESET is set.
>>> @@ -73,6 +78,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>    recover by issuing a reset.
>>>    \end{note}
>>>
>>> +If VIRTIO_F_SUSPEND is negotiated, the driver MUST manage the SUSPEND bit
>>> +as described in \ref{sec:General Initialization And Device Operation /
>>> +Device Suspend}. Otherwise, the driver MUST NOT set the SUSPEND bit.
>>> +
>>>    \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>
>>>    The device MUST NOT consume buffers or send any used buffer
>>> @@ -82,6 +91,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>    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.
>>>
>>> +If VIRTIO_F_SUSPEND is negotiated, the device MUST manage the SUSPEND bit
>>> +as described in \ref{sec:General Initialization And Device Operation /
>>> +Device Suspend}. Otherwise, the device MUST ignore the SUSPEND bit.
>>> +
>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>
>>>    Each virtio device offers all the features it understands.  During
>>> @@ -99,10 +112,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
>>>
>>> -\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}
>>> @@ -629,6 +642,67 @@ \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}
>>> +
>>> +When the VIRTIO_F_SUSPEND feature is negotiated, the driver can set the
>>> +SUSPEND bit in \field{device status} to suspend a live device, and can
>>> +clear the SUSPEND bit to resume a suspended device. A suspended device
>>> +should pause its operation, but it must maintain it state such that it
>>> +can immediately continue operation upon being resumed.
>>> +
>>> +Suspending a device via the SUSPEND bit is a seperate process from any
>>> +transport-specific suspend mechanism.
>>> +
>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>> +
>>> +The driver MUST NOT set the SUSPEND bit if the DRIVER_OK bit is not set.
>>> +
>>> +After writing a new value to the SUSPEND bit, the driver MUST wait for
>>> +the device to acknowledged the transition by reading from \field{device
>>> +status} until the returned value of the SUSPEND bit matches the written
>>> +value. During this period, the driver MAY abort the transition by writing
>>> +a new value to the SUSPEND bit or by resetting the device.
>>> +
>>> +A driver MUST NOT access the device configuration space of a suspended
>>> +device, except for \field{device status}.
>>> +
>>> +A driver MAY suspend a device that has buffers in its virtqueues. While
>>> +the device is suspended, a driver MUST NOT modify any available buffers
>>> +or their descriptors.
>>> +
>>> +A driver MUST NOT make any new buffers available to a suspended device.
>>> +
>>> +If a transport-specific suspend mechanism is available, the driver SHOULD
>>> +use it to put the device into a deeper suspend state after setting the
>>> +SUSPEND bit.
>>> +
>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>> +
>>> +A device MUST ignore writes to the SUSPEND bit if the DRIVER_OK bit is
>>> +not set.
>>> +
>>> +A device MUST maintain its state while suspended such that all driver
>>> +visible state after resuming exactly matches driver visible state
>>> +before suspending.
>>> +
>>> +A device MUST ignore all writes to its configuration space while
>>> +suspended, except for writes to \field{device status}.
>>> +
>>> +A device MUST NOT send notifications, access any virtqueues, or modify
>>> +any fields in its configuration space while suspended.
>>> +
>>> +A device MAY send notifications, access any virtqueues, or modify its
>>> +configuration space after the driver writes the SUSPEND bit but before
>>> +the device acknowledges the transition by returning a \field{device
>>> +status} value with the SUSPEND bit set. A device SHOULD finish processing
>>> +and send the used buffer notification for any buffers it is able to
>>> +before acknowledging the transition, but MAY retain buffers that cannot
>>> +be immiedately processed (e.g. empty buffers in a network recieveq).
>>> +
>>> +A device SHOULD take steps to minimize its resource consumption while
>>> +suspended, although what this involves is specific to the particular
>>> +device implementation.
>>> +
>>>    \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>>
>>>    Virtio can use various different buses, thus the standard is split
>>> @@ -872,6 +946,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(42)] This feature indicates that the driver can
>>> +    suspend the device via the SUSPEND bit in \field{device status} (see
>>> +    \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}).
>>> +
>>> +
>>>    \end{description}
>>>
>>>    \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[David Stevens]

On Tue, Feb 27, 2024 at 5:52 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> On 2/27/2024 3:59 PM, David Stevens wrote:
> > On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >> On 2/27/2024 9:53 AM, David Stevens wrote:
> >>> Add a SUSPEND bit to the device status field to allow drivers to suspend
> >>> virtio devices. On systems where drivers don't directly manage interrupt
> >>> routing (e.g. Linux), this allows the drivers to suspend their devices
> >>> and prevent interrupts from being sent when the interrupt routing system
> >>> does not expect interrupts.
> >>> ---
> >>>    content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>>    1 file changed, 84 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 0a62dce5f65f..2183c63c45ea 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>
> >>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >>>      an error from which it can't recover.
> >>> +
> >>> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
> >>> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
> >>>    \end{description}
> >>>
> >>>    The \field{device status} field starts out as 0, and is reinitialized to 0 by
> >>> @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>    initialization sequence specified in
> >>>    \ref{sec:General Initialization And Device Operation / Device
> >>>    Initialization}.
> >>> -The driver MUST NOT clear a
> >>> -\field{device status} bit.  If the driver sets the FAILED bit,
> >>> -the driver MUST later reset the device before attempting to re-initialize.
> >>> +
> >>> +The driver MUST NOT clear a \field{device status} bit, except for the
> >>> +SUSPEND bit as described in \ref{sec:General Initialization And Device
> >>> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
> >>> +driver MUST later reset the device before attempting to re-initialize.
> >> Comparing to add a new exception, why not just re-setting DRIVER_OK to
> >> let the device
> >> clearing SUSPEND? This issue has been addressed when Eugenio working on
> >> a similar STOP_BIT
> >>
> >> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> > I don't think the device status bit is bit-addressable, so it's not
> > possible to re-set DRIVER_OK without also either re-setting or
> > clearing SUSPEND.
> Please correct me if I misunderstand your point,
> I think it can be addressed, for example, in PCI it is an u8,
> the device can clear a bit in it.
>
> It may be easier to let the device to clear the SUSPEND bit.

If it's a u8, then the driver will be writing all 8 bits at the same
time. So I don't see how it's possible for the driver to set one bit
without also setting/clearing all the other bits at the same time.
When you say the driver can re-set DRIVER_OK, it can either write 0x1F
or 0xF. If we don't allow clearing the suspend bit, then it has to
write 0x1F. But that's exactly what the driver wrote to suspend the
device in the first place - how is the device supposed to tell the
difference? I guess we could add something to the spec saying that's
how the device is supposed to interpret it. But at that point, that's
really the same as allowing the driver to clear the suspend bit, just
with more complexity.

-David

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[Zhu, Lingshan]

On 2/28/2024 9:18 AM, David Stevens wrote:
> On Tue, Feb 27, 2024 at 5:52 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>> On 2/27/2024 3:59 PM, David Stevens wrote:
>>> On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>> On 2/27/2024 9:53 AM, David Stevens wrote:
>>>>> Add a SUSPEND bit to the device status field to allow drivers to suspend
>>>>> virtio devices. On systems where drivers don't directly manage interrupt
>>>>> routing (e.g. Linux), this allows the drivers to suspend their devices
>>>>> and prevent interrupts from being sent when the interrupt routing system
>>>>> does not expect interrupts.
>>>>> ---
>>>>>     content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>     1 file changed, 84 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index 0a62dce5f65f..2183c63c45ea 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>
>>>>>     \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>       an error from which it can't recover.
>>>>> +
>>>>> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
>>>>> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
>>>>>     \end{description}
>>>>>
>>>>>     The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>> @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>     initialization sequence specified in
>>>>>     \ref{sec:General Initialization And Device Operation / Device
>>>>>     Initialization}.
>>>>> -The driver MUST NOT clear a
>>>>> -\field{device status} bit.  If the driver sets the FAILED bit,
>>>>> -the driver MUST later reset the device before attempting to re-initialize.
>>>>> +
>>>>> +The driver MUST NOT clear a \field{device status} bit, except for the
>>>>> +SUSPEND bit as described in \ref{sec:General Initialization And Device
>>>>> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
>>>>> +driver MUST later reset the device before attempting to re-initialize.
>>>> Comparing to add a new exception, why not just re-setting DRIVER_OK to
>>>> let the device
>>>> clearing SUSPEND? This issue has been addressed when Eugenio working on
>>>> a similar STOP_BIT
>>>>
>>>> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
>>> I don't think the device status bit is bit-addressable, so it's not
>>> possible to re-set DRIVER_OK without also either re-setting or
>>> clearing SUSPEND.
>> Please correct me if I misunderstand your point,
>> I think it can be addressed, for example, in PCI it is an u8,
>> the device can clear a bit in it.
>>
>> It may be easier to let the device to clear the SUSPEND bit.
> If it's a u8, then the driver will be writing all 8 bits at the same
> time. So I don't see how it's possible for the driver to set one bit
> without also setting/clearing all the other bits at the same time.
> When you say the driver can re-set DRIVER_OK, it can either write 0x1F
> or 0xF. If we don't allow clearing the suspend bit, then it has to
> write 0x1F. But that's exactly what the driver wrote to suspend the
> device in the first place - how is the device supposed to tell the
> difference? I guess we could add something to the spec saying that's
> how the device is supposed to interpret it. But at that point, that's
> really the same as allowing the driver to clear the suspend bit, just
> with more complexity.
Thanks for the explanation, I get your point now.
I agree allowing clear a bit can resume the device running. however this
makes an exception and require the device to examine every bits that the 
driver set.

Letting the device clear the SUSPEND bit also require the device to examine
every bits. Based on the device status(Init, Running or SUSPENDED), the 
device
can behave differently:
+If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD 
clear SUSPEND
+and resumes operation upon DRIVER_OK.

But no need to make exceptions in the spec. Maybe this is a little less 
complex?

Allowing clear a bit by the driver also needs to deal with reset:
https://lists.oasis-open.org/archives/virtio-dev/202111/msg00025.html

Thanks

>
> -David


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[David Stevens]

On Wed, Feb 28, 2024 at 3:45 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> On 2/28/2024 9:18 AM, David Stevens wrote:
> > On Tue, Feb 27, 2024 at 5:52 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >> On 2/27/2024 3:59 PM, David Stevens wrote:
> >>> On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> On 2/27/2024 9:53 AM, David Stevens wrote:
> >>>>> Add a SUSPEND bit to the device status field to allow drivers to suspend
> >>>>> virtio devices. On systems where drivers don't directly manage interrupt
> >>>>> routing (e.g. Linux), this allows the drivers to suspend their devices
> >>>>> and prevent interrupts from being sent when the interrupt routing system
> >>>>> does not expect interrupts.
> >>>>> ---
> >>>>>     content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>>>>     1 file changed, 84 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/content.tex b/content.tex
> >>>>> index 0a62dce5f65f..2183c63c45ea 100644
> >>>>> --- a/content.tex
> >>>>> +++ b/content.tex
> >>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>>>
> >>>>>     \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >>>>>       an error from which it can't recover.
> >>>>> +
> >>>>> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
> >>>>> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
> >>>>>     \end{description}
> >>>>>
> >>>>>     The \field{device status} field starts out as 0, and is reinitialized to 0 by
> >>>>> @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>>>     initialization sequence specified in
> >>>>>     \ref{sec:General Initialization And Device Operation / Device
> >>>>>     Initialization}.
> >>>>> -The driver MUST NOT clear a
> >>>>> -\field{device status} bit.  If the driver sets the FAILED bit,
> >>>>> -the driver MUST later reset the device before attempting to re-initialize.
> >>>>> +
> >>>>> +The driver MUST NOT clear a \field{device status} bit, except for the
> >>>>> +SUSPEND bit as described in \ref{sec:General Initialization And Device
> >>>>> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
> >>>>> +driver MUST later reset the device before attempting to re-initialize.
> >>>> Comparing to add a new exception, why not just re-setting DRIVER_OK to
> >>>> let the device
> >>>> clearing SUSPEND? This issue has been addressed when Eugenio working on
> >>>> a similar STOP_BIT
> >>>>
> >>>> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> >>> I don't think the device status bit is bit-addressable, so it's not
> >>> possible to re-set DRIVER_OK without also either re-setting or
> >>> clearing SUSPEND.
> >> Please correct me if I misunderstand your point,
> >> I think it can be addressed, for example, in PCI it is an u8,
> >> the device can clear a bit in it.
> >>
> >> It may be easier to let the device to clear the SUSPEND bit.
> > If it's a u8, then the driver will be writing all 8 bits at the same
> > time. So I don't see how it's possible for the driver to set one bit
> > without also setting/clearing all the other bits at the same time.
> > When you say the driver can re-set DRIVER_OK, it can either write 0x1F
> > or 0xF. If we don't allow clearing the suspend bit, then it has to
> > write 0x1F. But that's exactly what the driver wrote to suspend the
> > device in the first place - how is the device supposed to tell the
> > difference? I guess we could add something to the spec saying that's
> > how the device is supposed to interpret it. But at that point, that's
> > really the same as allowing the driver to clear the suspend bit, just
> > with more complexity.
> Thanks for the explanation, I get your point now.
> I agree allowing clear a bit can resume the device running. however this
> makes an exception and require the device to examine every bits that the
> driver set.

It needs to do this anyway to detect the SUSPEND bit being set in the
first place, doesn't it? Why is detecting the SUSPEND bit being
cleared more difficult than detecting the SUSPEND set being set?

> Letting the device clear the SUSPEND bit also require the device to examine
> every bits. Based on the device status(Init, Running or SUSPENDED), the
> device
> can behave differently:
> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD
> clear SUSPEND
> +and resumes operation upon DRIVER_OK.
>
> But no need to make exceptions in the spec. Maybe this is a little less
> complex?

That is also adding an exception, though? It introduces a single
situation where the behavior of bits is tied together and a single
situation where the device instead of the driver is allowed to clear
bits. It would also complicate adding bit to device status that can be
set when the device is suspended, since it would be ambiguous as to
whether a driver writing 0x3F wants to enable bit 0x20 or wants to
enable 0x20 and also disable SUSPEND. I fail to see how that is
simpler than just letting the driver clear the SUSPEND bit.

-David

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

[Zhu, Lingshan]

On 2/29/2024 11:20 AM, David Stevens wrote:
> On Wed, Feb 28, 2024 at 3:45 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>> On 2/28/2024 9:18 AM, David Stevens wrote:
>>> On Tue, Feb 27, 2024 at 5:52 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>> On 2/27/2024 3:59 PM, David Stevens wrote:
>>>>> On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>> On 2/27/2024 9:53 AM, David Stevens wrote:
>>>>>>> Add a SUSPEND bit to the device status field to allow drivers to suspend
>>>>>>> virtio devices. On systems where drivers don't directly manage interrupt
>>>>>>> routing (e.g. Linux), this allows the drivers to suspend their devices
>>>>>>> and prevent interrupts from being sent when the interrupt routing system
>>>>>>> does not expect interrupts.
>>>>>>> ---
>>>>>>>      content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>      1 file changed, 84 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>> index 0a62dce5f65f..2183c63c45ea 100644
>>>>>>> --- a/content.tex
>>>>>>> +++ b/content.tex
>>>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>>
>>>>>>>      \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>>>        an error from which it can't recover.
>>>>>>> +
>>>>>>> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
>>>>>>> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
>>>>>>>      \end{description}
>>>>>>>
>>>>>>>      The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>>>> @@ -60,9 +63,11 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>>      initialization sequence specified in
>>>>>>>      \ref{sec:General Initialization And Device Operation / Device
>>>>>>>      Initialization}.
>>>>>>> -The driver MUST NOT clear a
>>>>>>> -\field{device status} bit.  If the driver sets the FAILED bit,
>>>>>>> -the driver MUST later reset the device before attempting to re-initialize.
>>>>>>> +
>>>>>>> +The driver MUST NOT clear a \field{device status} bit, except for the
>>>>>>> +SUSPEND bit as described in \ref{sec:General Initialization And Device
>>>>>>> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
>>>>>>> +driver MUST later reset the device before attempting to re-initialize.
>>>>>> Comparing to add a new exception, why not just re-setting DRIVER_OK to
>>>>>> let the device
>>>>>> clearing SUSPEND? This issue has been addressed when Eugenio working on
>>>>>> a similar STOP_BIT
>>>>>>
>>>>>> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
>>>>> I don't think the device status bit is bit-addressable, so it's not
>>>>> possible to re-set DRIVER_OK without also either re-setting or
>>>>> clearing SUSPEND.
>>>> Please correct me if I misunderstand your point,
>>>> I think it can be addressed, for example, in PCI it is an u8,
>>>> the device can clear a bit in it.
>>>>
>>>> It may be easier to let the device to clear the SUSPEND bit.
>>> If it's a u8, then the driver will be writing all 8 bits at the same
>>> time. So I don't see how it's possible for the driver to set one bit
>>> without also setting/clearing all the other bits at the same time.
>>> When you say the driver can re-set DRIVER_OK, it can either write 0x1F
>>> or 0xF. If we don't allow clearing the suspend bit, then it has to
>>> write 0x1F. But that's exactly what the driver wrote to suspend the
>>> device in the first place - how is the device supposed to tell the
>>> difference? I guess we could add something to the spec saying that's
>>> how the device is supposed to interpret it. But at that point, that's
>>> really the same as allowing the driver to clear the suspend bit, just
>>> with more complexity.
>> Thanks for the explanation, I get your point now.
>> I agree allowing clear a bit can resume the device running. however this
>> makes an exception and require the device to examine every bits that the
>> driver set.
> It needs to do this anyway to detect the SUSPEND bit being set in the
> first place, doesn't it? Why is detecting the SUSPEND bit being
> cleared more difficult than detecting the SUSPEND set being set?
as explained below, it does examine every bits in device status.
>
>> Letting the device clear the SUSPEND bit also require the device to examine
>> every bits. Based on the device status(Init, Running or SUSPENDED), the
>> device
>> can behave differently:
>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD
>> clear SUSPEND
>> +and resumes operation upon DRIVER_OK.
>>
>> But no need to make exceptions in the spec. Maybe this is a little less
>> complex?
> That is also adding an exception, though? It introduces a single
> situation where the behavior of bits is tied together and a single
> situation where the device instead of the driver is allowed to clear
> bits. It would also complicate adding bit to device status that can be
> set when the device is suspended, since it would be ambiguous as to
> whether a driver writing 0x3F wants to enable bit 0x20 or wants to
> enable 0x20 and also disable SUSPEND. I fail to see how that is
> simpler than just letting the driver clear the SUSPEND bit.
Not adding a new exception, quite the opposite, this implementation
tries to re-use current virtio mechanism.

Let setting DRIVER_OK to bring the device back to alive operational:
"Set the DRIVER_OK status bit. At this point the device is live"
"DRIVER_OK is set: device operation begins."

The device should behave differently based on its state machine, examples:
"the device with the VIRTIO_BLK_Z_HM zone model MUST fail to set the 
FEATURES_OK device status bit when the driver writes the Device Status 
field."
"The device MUST NOT consume buffers or send any used buffer 
notifications to the driver before DRIVER_OK."


And please take this piece of code as another example:

void virtio_add_status(struct virtio_device *dev, unsigned int status)
{
     might_sleep();
     dev->config->set_status(dev, dev->config->get_status(dev) | status);
}

Here the code tries to add a status to the device,
the driver first read the device status, then add a new status,
at last set the new status to the device.

That means it is quite common to re-write the same status bits to the
device, and the device can behave differently based on its
device state machine.

So IMHO, this implementation does not introduce more complexities,
it tries to re-use existing virtio mechanism.

Thanks
Zhu Lingshan
>
> -David
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org