[RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan]

This commit allows the driver to suspend the device by
introducing a new status bit SUSPEND in device_status.

This commit also introduce a new feature bit VIRTIO_F_SUSPEND
which indicating whether the device support SUSPEND.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
Signed-off-by: David Stevens <stevensd@chromium.org>
---
 content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 61 insertions(+), 10 deletions(-)

diff --git a/content.tex b/content.tex
index 0a62dce..8c974d3 100644
--- a/content.tex
+++ b/content.tex
@@ -36,19 +36,22 @@ \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[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.
+
+\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
@@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
+except when setting \field{device status} to 0 as a transport-specific way to
+initiate a reset. 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
@@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
+SUSPEND bit in \field{device status} to suspend a device, and can
+clear the SUSPEND bit to resume a suspended device.
+
+\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
+
+The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
+
+Once the driver sets SUSPEND to \field{device status} of the device:
+\begin{itemize}
+\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
+\item The driver MUST NOT make any more buffers available to the device.
+\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
+\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
+if it is implemented in the Config Space.
+\end{itemize}
+
+\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
+
+The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
+
+The device MUST ignore all 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, access any virtqueues, or modify
+any fields in its Configuration Space while suspended.
+
+When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend.
+
+If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
+the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
+
+When the driver sets SUSPEND,
+the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
+
+\begin{itemize}
+\item Stop processing 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 +919,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(42)] This feature indicates that the driver can
+   trigger suspending the device via the SUSPEND flag
+   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
2.45.2

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Michael S. Tsirkin]

On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
> This commit allows the driver to suspend the device by
> introducing a new status bit SUSPEND in device_status.
> 
> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> which indicating whether the device support SUSPEND.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> Signed-off-by: David Stevens <stevensd@chromium.org>

Your signature would normally be last.


> ---
>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 61 insertions(+), 10 deletions(-)

At a high level, I think this is somewhat overspecified.
Generally e.g. if driver is forbidden from accessing
a field, then we do not also add specific requirements for
devices - with no driver doing this, such functionality will
remain untested and unused, and when we finally decide we need
it it's not going to be there.
Similarly for when device is not touching a field - no
reason for driver not to touch it.


> diff --git a/content.tex b/content.tex
> index 0a62dce..8c974d3 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -36,19 +36,22 @@ \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[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.
> +
> +\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
> @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
> +except when setting \field{device status} to 0 as a transport-specific way to
> +initiate a reset. 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
> @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
> +SUSPEND bit in \field{device status} to suspend a device, and can
> +clear the SUSPEND bit to resume a suspended device.
> +
> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> +
> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> +
> +Once the driver sets SUSPEND to \field{device status} of the device:
> +\begin{itemize}
> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.

If it's not set, then what? Device error, have to reset?


> +\item The driver MUST NOT make any more buffers available to the device.
> +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.

what are these "fields"? the area in memory where the vq lives? why is
doing that a problem - you want device to be able to write something
there?

> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
> +if it is implemented in the Config Space.

what is "the Config Space"? device status is never in
a device configuration space, it is part of the common configuration.
so what exactly are you forbidding here?

> +\end{itemize}
> +
> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> +
> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.

what does must ignore SUSPEND mean? what do you want the device to do and when?


> +The device MUST ignore all access to its Configuration Space while
> +suspended, except for \field{device status} if it is part of the Configuration Space.

again what does ignore mean? Configuration Space here means
Device Configuration Space?

> +
> +A device MUST NOT send any notifications, access any virtqueues, or modify
> +any fields in its Configuration Space while suspended.

If a tree falls in a forest, and no one is around to hear it, does
it still make a sound?
I think what matters is that if a configuration space changes while
SUSPEND is set, then the device must not send a configuration change
notification, and it must send a configuration change notification
after SUSPEND bit has been cleared.


> +When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend.
> +
> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
> +
> +When the driver sets SUSPEND,
> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:

what does "before presenting" mean here? that a driver reading status
will see 0 in this bit?

> +\begin{itemize}
> +\item Stop processing 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}

Is there anything here that has not been said before, but as a MUST?
Saying same thing twice once a MUST once a SHOULD is confusing IMHO.


> +
>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>  
>  Virtio can use various different buses, thus the standard is split
> @@ -872,6 +919,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(42)] This feature indicates that the driver can
> +   trigger suspending the device via the SUSPEND flag
> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> +
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> -- 
> 2.45.2

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[David Stevens]

On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
> > This commit allows the driver to suspend the device by
> > introducing a new status bit SUSPEND in device_status.
> >
> > This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> > which indicating whether the device support SUSPEND.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> > Signed-off-by: David Stevens <stevensd@chromium.org>
>
> Your signature would normally be last.
>
>
> > ---
> >  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 61 insertions(+), 10 deletions(-)
>
> At a high level, I think this is somewhat overspecified.
> Generally e.g. if driver is forbidden from accessing
> a field, then we do not also add specific requirements for
> devices - with no driver doing this, such functionality will
> remain untested and unused, and when we finally decide we need
> it it's not going to be there.
> Similarly for when device is not touching a field - no
> reason for driver not to touch it.
>
>
> > diff --git a/content.tex b/content.tex
> > index 0a62dce..8c974d3 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -36,19 +36,22 @@ \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[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.
> > +
> > +\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
> > @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
> > +except when setting \field{device status} to 0 as a transport-specific way to
> > +initiate a reset. 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
> > @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
> > +SUSPEND bit in \field{device status} to suspend a device, and can
> > +clear the SUSPEND bit to resume a suspended device.
> > +
> > +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> > +
> > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> > +
> > +Once the driver sets SUSPEND to \field{device status} of the device:
> > +\begin{itemize}
> > +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
>
> If it's not set, then what? Device error, have to reset?
>
>
> > +\item The driver MUST NOT make any more buffers available to the device.
> > +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
>
> what are these "fields"? the area in memory where the vq lives? why is
> doing that a problem - you want device to be able to write something
> there?
>
> > +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
> > +if it is implemented in the Config Space.
>
> what is "the Config Space"? device status is never in
> a device configuration space, it is part of the common configuration.
> so what exactly are you forbidding here?

IIUC, referring to the common configuration here wouldn't make sense.
The common configuration is part of the PCI transport specification,
so in this part of the specification it should only be referred to via
the "transport specific" phrasing. This wording appears to be taken
from Cornelia's feedback on v5. Specifically:

> > +\item The driver MUST NOT access Device Configuration Space.
> ...except for the status field, if it is part of the config space?

I asked for clarification [1], because Cornelia's feedback seems to
contradict your feedback here and on the (unfortunately unarchived) v3
patch. What is the definitive statement agreed upon by both editors of
the virtio specification as to whether or not the device status field
is part of the device configuration space?

[1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/

-David

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Michael S. Tsirkin]

On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
> > > This commit allows the driver to suspend the device by
> > > introducing a new status bit SUSPEND in device_status.
> > >
> > > This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> > > which indicating whether the device support SUSPEND.
> > >
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> >
> > Your signature would normally be last.
> >
> >
> > > ---
> > >  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 61 insertions(+), 10 deletions(-)
> >
> > At a high level, I think this is somewhat overspecified.
> > Generally e.g. if driver is forbidden from accessing
> > a field, then we do not also add specific requirements for
> > devices - with no driver doing this, such functionality will
> > remain untested and unused, and when we finally decide we need
> > it it's not going to be there.
> > Similarly for when device is not touching a field - no
> > reason for driver not to touch it.
> >
> >
> > > diff --git a/content.tex b/content.tex
> > > index 0a62dce..8c974d3 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -36,19 +36,22 @@ \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[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.
> > > +
> > > +\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
> > > @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
> > > +except when setting \field{device status} to 0 as a transport-specific way to
> > > +initiate a reset. 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
> > > @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
> > > +SUSPEND bit in \field{device status} to suspend a device, and can
> > > +clear the SUSPEND bit to resume a suspended device.
> > > +
> > > +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> > > +
> > > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> > > +
> > > +Once the driver sets SUSPEND to \field{device status} of the device:
> > > +\begin{itemize}
> > > +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> >
> > If it's not set, then what? Device error, have to reset?
> >
> >
> > > +\item The driver MUST NOT make any more buffers available to the device.
> > > +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
> >
> > what are these "fields"? the area in memory where the vq lives? why is
> > doing that a problem - you want device to be able to write something
> > there?
> >
> > > +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
> > > +if it is implemented in the Config Space.
> >
> > what is "the Config Space"? device status is never in
> > a device configuration space, it is part of the common configuration.
> > so what exactly are you forbidding here?
> 
> IIUC, referring to the common configuration here wouldn't make sense.
> The common configuration is part of the PCI transport specification,
> so in this part of the specification it should only be referred to via
> the "transport specific" phrasing.

Good point.  We can easily fix this for MMIO though.

 
MMIO virtio devices provide a set of memory mapped control
registers followed by a device-specific configuration space,
described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.

->

MMIO virtio devices provide memory mapped control
including a set of common configuration
registers followed by a device-specific configuration space,
described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.



Less sure about CCW.
Maybe:

In addition to the basic channel commands, virtio-ccw defines a
set of channel commands related to configuration and operation of
virtio:

\begin{lstlisting}
#define CCW_CMD_SET_VQ 0x13
#define CCW_CMD_VDEV_RESET 0x33
#define CCW_CMD_SET_IND 0x43
#define CCW_CMD_SET_CONF_IND 0x53
#define CCW_CMD_SET_IND_ADAPTER 0x73
#define CCW_CMD_READ_FEAT 0x12
#define CCW_CMD_WRITE_FEAT 0x11
#define CCW_CMD_READ_CONF 0x22
#define CCW_CMD_WRITE_CONF 0x21
#define CCW_CMD_WRITE_STATUS 0x31
#define CCW_CMD_READ_VQ_CONF 0x32
#define CCW_CMD_SET_VIRTIO_REV 0x83
#define CCW_CMD_READ_STATUS 0x72
\end{lstlisting}

->


In addition to the basic channel commands, virtio-ccw defines
channel commands related to configuration and operation of
virtio - a set of commands to access common configuration of
the device:

\begin{lstlisting}
#define CCW_CMD_SET_VQ 0x13
#define CCW_CMD_VDEV_RESET 0x33
#define CCW_CMD_SET_IND 0x43
#define CCW_CMD_SET_CONF_IND 0x53
#define CCW_CMD_SET_IND_ADAPTER 0x73
#define CCW_CMD_READ_FEAT 0x12
#define CCW_CMD_WRITE_FEAT 0x11
#define CCW_CMD_WRITE_STATUS 0x31
#define CCW_CMD_READ_VQ_CONF 0x32
#define CCW_CMD_SET_VIRTIO_REV 0x83
#define CCW_CMD_READ_STATUS 0x72
\end{lstlisting}

and additionally, two commands to access the device
specification configuration space:

\begin{lstlisting}
#define CCW_CMD_READ_CONF 0x22
#define CCW_CMD_WRITE_CONF 0x21
\end{lstlisting}


And now we have common configuration defined for all transports.
Cornelia, WDYT?


> This wording appears to be taken
> from Cornelia's feedback on v5. Specifically:
> 
> > > +\item The driver MUST NOT access Device Configuration Space.
> > ...except for the status field, if it is part of the config space?
> 
> I asked for clarification [1], because Cornelia's feedback seems to
> contradict your feedback here and on the (unfortunately unarchived) v3
> patch. What is the definitive statement agreed upon by both editors of
> the virtio specification as to whether or not the device status field
> is part of the device configuration space?
> 
> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
> 
> -David

I don't know what did Cornelia mean. Cornelia?

-- 
MST

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan]

On 7/4/2024 4:55 PM, Michael S. Tsirkin wrote:

> On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
>> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
>>>> This commit allows the driver to suspend the device by
>>>> introducing a new status bit SUSPEND in device_status.
>>>>
>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>> which indicating whether the device support SUSPEND.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> Your signature would normally be last.
>>>
>>>
>>>> ---
>>>>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 61 insertions(+), 10 deletions(-)
>>> At a high level, I think this is somewhat overspecified.
>>> Generally e.g. if driver is forbidden from accessing
>>> a field, then we do not also add specific requirements for
>>> devices - with no driver doing this, such functionality will
>>> remain untested and unused, and when we finally decide we need
>>> it it's not going to be there.
>>> Similarly for when device is not touching a field - no
>>> reason for driver not to touch it.
>>>
>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 0a62dce..8c974d3 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -36,19 +36,22 @@ \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[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.
>>>> +
>>>> +\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
>>>> @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
>>>> +except when setting \field{device status} to 0 as a transport-specific way to
>>>> +initiate a reset. 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
>>>> @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
>>>> +SUSPEND bit in \field{device status} to suspend a device, and can
>>>> +clear the SUSPEND bit to resume a suspended device.
>>>> +
>>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>>> +
>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>>>> +
>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>> +\begin{itemize}
>>>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
>>> If it's not set, then what? Device error, have to reset?
>>>
>>>
>>>> +\item The driver MUST NOT make any more buffers available to the device.
>>>> +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
>>> what are these "fields"? the area in memory where the vq lives? why is
>>> doing that a problem - you want device to be able to write something
>>> there?
>>>
>>>> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>>>> +if it is implemented in the Config Space.
>>> what is "the Config Space"? device status is never in
>>> a device configuration space, it is part of the common configuration.
>>> so what exactly are you forbidding here?
>> IIUC, referring to the common configuration here wouldn't make sense.
>> The common configuration is part of the PCI transport specification,
>> so in this part of the specification it should only be referred to via
>> the "transport specific" phrasing.
> Good point.  We can easily fix this for MMIO though.
>
>  
> MMIO virtio devices provide a set of memory mapped control
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>
> ->
>
> MMIO virtio devices provide memory mapped control
> including a set of common configuration
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
IMHO this may be a bit vague, PCI as a section about common configuration and a struct "struct virtio_pci_common_cfg "
listed all contents. But what does CCW/MMIO common configuration contains? Shall we add new sections for them?
>
>
>
> Less sure about CCW.
> Maybe:
>
> In addition to the basic channel commands, virtio-ccw defines a
> set of channel commands related to configuration and operation of
> virtio:
>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> ->
>
>
> In addition to the basic channel commands, virtio-ccw defines
> channel commands related to configuration and operation of
> virtio - a set of commands to access common configuration of
> the device:
>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> and additionally, two commands to access the device
> specification configuration space:
>
> \begin{lstlisting}
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> \end{lstlisting}
>
>
> And now we have common configuration defined for all transports.
> Cornelia, WDYT?
>
>
>> This wording appears to be taken
>> from Cornelia's feedback on v5. Specifically:
>>
>>>> +\item The driver MUST NOT access Device Configuration Space.
>>> ...except for the status field, if it is part of the config space?
>> I asked for clarification [1], because Cornelia's feedback seems to
>> contradict your feedback here and on the (unfortunately unarchived) v3
>> patch. What is the definitive statement agreed upon by both editors of
>> the virtio specification as to whether or not the device status field
>> is part of the device configuration space?
>>
>> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
>>
>> -David
> I don't know what did Cornelia mean. Cornelia?
>

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Michael S. Tsirkin]

On Fri, Jul 12, 2024 at 05:04:48PM +0800, Zhu Lingshan wrote:
> On 7/4/2024 4:55 PM, Michael S. Tsirkin wrote:
> 
> > On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
> >> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>> On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
> >>>> This commit allows the driver to suspend the device by
> >>>> introducing a new status bit SUSPEND in device_status.
> >>>>
> >>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> >>>> which indicating whether the device support SUSPEND.
> >>>>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>> Your signature would normally be last.
> >>>
> >>>
> >>>> ---
> >>>>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>  1 file changed, 61 insertions(+), 10 deletions(-)
> >>> At a high level, I think this is somewhat overspecified.
> >>> Generally e.g. if driver is forbidden from accessing
> >>> a field, then we do not also add specific requirements for
> >>> devices - with no driver doing this, such functionality will
> >>> remain untested and unused, and when we finally decide we need
> >>> it it's not going to be there.
> >>> Similarly for when device is not touching a field - no
> >>> reason for driver not to touch it.
> >>>
> >>>
> >>>> diff --git a/content.tex b/content.tex
> >>>> index 0a62dce..8c974d3 100644
> >>>> --- a/content.tex
> >>>> +++ b/content.tex
> >>>> @@ -36,19 +36,22 @@ \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[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.
> >>>> +
> >>>> +\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
> >>>> @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
> >>>> +except when setting \field{device status} to 0 as a transport-specific way to
> >>>> +initiate a reset. 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
> >>>> @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
> >>>> +SUSPEND bit in \field{device status} to suspend a device, and can
> >>>> +clear the SUSPEND bit to resume a suspended device.
> >>>> +
> >>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >>>> +
> >>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> >>>> +
> >>>> +Once the driver sets SUSPEND to \field{device status} of the device:
> >>>> +\begin{itemize}
> >>>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> >>> If it's not set, then what? Device error, have to reset?
> >>>
> >>>
> >>>> +\item The driver MUST NOT make any more buffers available to the device.
> >>>> +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
> >>> what are these "fields"? the area in memory where the vq lives? why is
> >>> doing that a problem - you want device to be able to write something
> >>> there?
> >>>
> >>>> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
> >>>> +if it is implemented in the Config Space.
> >>> what is "the Config Space"? device status is never in
> >>> a device configuration space, it is part of the common configuration.
> >>> so what exactly are you forbidding here?
> >> IIUC, referring to the common configuration here wouldn't make sense.
> >> The common configuration is part of the PCI transport specification,
> >> so in this part of the specification it should only be referred to via
> >> the "transport specific" phrasing.
> > Good point.  We can easily fix this for MMIO though.
> >
> >  
> > MMIO virtio devices provide a set of memory mapped control
> > registers followed by a device-specific configuration space,
> > described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
> >
> > ->
> >
> > MMIO virtio devices provide memory mapped control
> > including a set of common configuration
> > registers followed by a device-specific configuration space,
> > described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
> IMHO this may be a bit vague, PCI as a section about common configuration and a struct "struct virtio_pci_common_cfg "
> listed all contents. But what does CCW/MMIO common configuration contains? Shall we add new sections for them?

I think it's clear: everything that is not device-specific is common,
and we have a table documenting that.

And again CCW does not have registers, it has commands.
And below I suggested listing the common configuration commands.

> >
> >
> >
> > Less sure about CCW.
> > Maybe:
> >
> > In addition to the basic channel commands, virtio-ccw defines a
> > set of channel commands related to configuration and operation of
> > virtio:
> >
> > \begin{lstlisting}
> > #define CCW_CMD_SET_VQ 0x13
> > #define CCW_CMD_VDEV_RESET 0x33
> > #define CCW_CMD_SET_IND 0x43
> > #define CCW_CMD_SET_CONF_IND 0x53
> > #define CCW_CMD_SET_IND_ADAPTER 0x73
> > #define CCW_CMD_READ_FEAT 0x12
> > #define CCW_CMD_WRITE_FEAT 0x11
> > #define CCW_CMD_READ_CONF 0x22
> > #define CCW_CMD_WRITE_CONF 0x21
> > #define CCW_CMD_WRITE_STATUS 0x31
> > #define CCW_CMD_READ_VQ_CONF 0x32
> > #define CCW_CMD_SET_VIRTIO_REV 0x83
> > #define CCW_CMD_READ_STATUS 0x72
> > \end{lstlisting}
> >
> > ->
> >
> >
> > In addition to the basic channel commands, virtio-ccw defines
> > channel commands related to configuration and operation of
> > virtio - a set of commands to access common configuration of
> > the device:
> >
> > \begin{lstlisting}
> > #define CCW_CMD_SET_VQ 0x13
> > #define CCW_CMD_VDEV_RESET 0x33
> > #define CCW_CMD_SET_IND 0x43
> > #define CCW_CMD_SET_CONF_IND 0x53
> > #define CCW_CMD_SET_IND_ADAPTER 0x73
> > #define CCW_CMD_READ_FEAT 0x12
> > #define CCW_CMD_WRITE_FEAT 0x11
> > #define CCW_CMD_WRITE_STATUS 0x31
> > #define CCW_CMD_READ_VQ_CONF 0x32
> > #define CCW_CMD_SET_VIRTIO_REV 0x83
> > #define CCW_CMD_READ_STATUS 0x72
> > \end{lstlisting}
> >
> > and additionally, two commands to access the device
> > specification configuration space:
> >
> > \begin{lstlisting}
> > #define CCW_CMD_READ_CONF 0x22
> > #define CCW_CMD_WRITE_CONF 0x21
> > \end{lstlisting}
> >
> >
> > And now we have common configuration defined for all transports.
> > Cornelia, WDYT?
> >
> >
> >> This wording appears to be taken
> >> from Cornelia's feedback on v5. Specifically:
> >>
> >>>> +\item The driver MUST NOT access Device Configuration Space.
> >>> ...except for the status field, if it is part of the config space?
> >> I asked for clarification [1], because Cornelia's feedback seems to
> >> contradict your feedback here and on the (unfortunately unarchived) v3
> >> patch. What is the definitive statement agreed upon by both editors of
> >> the virtio specification as to whether or not the device status field
> >> is part of the device configuration space?
> >>
> >> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
> >>
> >> -David
> > I don't know what did Cornelia mean. Cornelia?
> >

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan]

On 7/12/2024 7:18 PM, Michael S. Tsirkin wrote:
> On Fri, Jul 12, 2024 at 05:04:48PM +0800, Zhu Lingshan wrote:
>> On 7/4/2024 4:55 PM, Michael S. Tsirkin wrote:
>>
>>> On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
>>>> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
>>>>>> This commit allows the driver to suspend the device by
>>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>>
>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>>> which indicating whether the device support SUSPEND.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>> Your signature would normally be last.
>>>>>
>>>>>
>>>>>> ---
>>>>>>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>  1 file changed, 61 insertions(+), 10 deletions(-)
>>>>> At a high level, I think this is somewhat overspecified.
>>>>> Generally e.g. if driver is forbidden from accessing
>>>>> a field, then we do not also add specific requirements for
>>>>> devices - with no driver doing this, such functionality will
>>>>> remain untested and unused, and when we finally decide we need
>>>>> it it's not going to be there.
>>>>> Similarly for when device is not touching a field - no
>>>>> reason for driver not to touch it.
>>>>>
>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 0a62dce..8c974d3 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -36,19 +36,22 @@ \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[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.
>>>>>> +
>>>>>> +\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
>>>>>> @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
>>>>>> +except when setting \field{device status} to 0 as a transport-specific way to
>>>>>> +initiate a reset. 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
>>>>>> @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
>>>>>> +SUSPEND bit in \field{device status} to suspend a device, and can
>>>>>> +clear the SUSPEND bit to resume a suspended device.
>>>>>> +
>>>>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>>>>> +
>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>>>>>> +
>>>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>>>> +\begin{itemize}
>>>>>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
>>>>> If it's not set, then what? Device error, have to reset?
>>>>>
>>>>>
>>>>>> +\item The driver MUST NOT make any more buffers available to the device.
>>>>>> +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
>>>>> what are these "fields"? the area in memory where the vq lives? why is
>>>>> doing that a problem - you want device to be able to write something
>>>>> there?
>>>>>
>>>>>> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>>>>>> +if it is implemented in the Config Space.
>>>>> what is "the Config Space"? device status is never in
>>>>> a device configuration space, it is part of the common configuration.
>>>>> so what exactly are you forbidding here?
>>>> IIUC, referring to the common configuration here wouldn't make sense.
>>>> The common configuration is part of the PCI transport specification,
>>>> so in this part of the specification it should only be referred to via
>>>> the "transport specific" phrasing.
>>> Good point.  We can easily fix this for MMIO though.
>>>
>>>  
>>> MMIO virtio devices provide a set of memory mapped control
>>> registers followed by a device-specific configuration space,
>>> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>>>
>>> ->
>>>
>>> MMIO virtio devices provide memory mapped control
>>> including a set of common configuration
>>> registers followed by a device-specific configuration space,
>>> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>> IMHO this may be a bit vague, PCI as a section about common configuration and a struct "struct virtio_pci_common_cfg "
>> listed all contents. But what does CCW/MMIO common configuration contains? Shall we add new sections for them?
> I think it's clear: everything that is not device-specific is common,
> and we have a table documenting that.
>
> And again CCW does not have registers, it has commands.
> And below I suggested listing the common configuration commands.
OK, so would you like to add this editorial change or shall I work on it?

Therefore, in V7 I will rephrase fallback to V5: "The driver MUST NOT access Device Configuration Space" Any comments? @Michael @Cornelia Thanks Zhu Lingshan
>
>>>
>>>
>>> Less sure about CCW.
>>> Maybe:
>>>
>>> In addition to the basic channel commands, virtio-ccw defines a
>>> set of channel commands related to configuration and operation of
>>> virtio:
>>>
>>> \begin{lstlisting}
>>> #define CCW_CMD_SET_VQ 0x13
>>> #define CCW_CMD_VDEV_RESET 0x33
>>> #define CCW_CMD_SET_IND 0x43
>>> #define CCW_CMD_SET_CONF_IND 0x53
>>> #define CCW_CMD_SET_IND_ADAPTER 0x73
>>> #define CCW_CMD_READ_FEAT 0x12
>>> #define CCW_CMD_WRITE_FEAT 0x11
>>> #define CCW_CMD_READ_CONF 0x22
>>> #define CCW_CMD_WRITE_CONF 0x21
>>> #define CCW_CMD_WRITE_STATUS 0x31
>>> #define CCW_CMD_READ_VQ_CONF 0x32
>>> #define CCW_CMD_SET_VIRTIO_REV 0x83
>>> #define CCW_CMD_READ_STATUS 0x72
>>> \end{lstlisting}
>>>
>>> ->
>>>
>>>
>>> In addition to the basic channel commands, virtio-ccw defines
>>> channel commands related to configuration and operation of
>>> virtio - a set of commands to access common configuration of
>>> the device:
>>>
>>> \begin{lstlisting}
>>> #define CCW_CMD_SET_VQ 0x13
>>> #define CCW_CMD_VDEV_RESET 0x33
>>> #define CCW_CMD_SET_IND 0x43
>>> #define CCW_CMD_SET_CONF_IND 0x53
>>> #define CCW_CMD_SET_IND_ADAPTER 0x73
>>> #define CCW_CMD_READ_FEAT 0x12
>>> #define CCW_CMD_WRITE_FEAT 0x11
>>> #define CCW_CMD_WRITE_STATUS 0x31
>>> #define CCW_CMD_READ_VQ_CONF 0x32
>>> #define CCW_CMD_SET_VIRTIO_REV 0x83
>>> #define CCW_CMD_READ_STATUS 0x72
>>> \end{lstlisting}
>>>
>>> and additionally, two commands to access the device
>>> specification configuration space:
>>>
>>> \begin{lstlisting}
>>> #define CCW_CMD_READ_CONF 0x22
>>> #define CCW_CMD_WRITE_CONF 0x21
>>> \end{lstlisting}
>>>
>>>
>>> And now we have common configuration defined for all transports.
>>> Cornelia, WDYT?
>>>
>>>
>>>> This wording appears to be taken
>>>> from Cornelia's feedback on v5. Specifically:
>>>>
>>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>>> ...except for the status field, if it is part of the config space?
>>>> I asked for clarification [1], because Cornelia's feedback seems to
>>>> contradict your feedback here and on the (unfortunately unarchived) v3
>>>> patch. What is the definitive statement agreed upon by both editors of
>>>> the virtio specification as to whether or not the device status field
>>>> is part of the device configuration space?
>>>>
>>>> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
>>>>
>>>> -David
>>> I don't know what did Cornelia mean. Cornelia?
>>>

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Cornelia Huck]

On Thu, Jul 04 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
>> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >
>> > On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
>> > > +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>> > > +if it is implemented in the Config Space.
>> >
>> > what is "the Config Space"? device status is never in
>> > a device configuration space, it is part of the common configuration.
>> > so what exactly are you forbidding here?
>> 
>> IIUC, referring to the common configuration here wouldn't make sense.
>> The common configuration is part of the PCI transport specification,
>> so in this part of the specification it should only be referred to via
>> the "transport specific" phrasing.
>
> Good point.  We can easily fix this for MMIO though.
>
>  
> MMIO virtio devices provide a set of memory mapped control
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>
> ->
>
> MMIO virtio devices provide memory mapped control
> including a set of common configuration
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>
>
>
> Less sure about CCW.
> Maybe:
>
> In addition to the basic channel commands, virtio-ccw defines a
> set of channel commands related to configuration and operation of
> virtio:
>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> ->
>
>
> In addition to the basic channel commands, virtio-ccw defines
> channel commands related to configuration and operation of
> virtio - a set of commands to access common configuration of
> the device:

Hm, I'm not sure whether "common configuration" is the best wording
here; resetting a device, for example, is not something I'd call
configuration. If we don't manage to come up with anything better, I can
live with it, though.

>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> and additionally, two commands to access the device
> specification configuration space:
>
> \begin{lstlisting}
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> \end{lstlisting}
>
>
> And now we have common configuration defined for all transports.
> Cornelia, WDYT?
>
>
>> This wording appears to be taken
>> from Cornelia's feedback on v5. Specifically:
>> 
>> > > +\item The driver MUST NOT access Device Configuration Space.
>> > ...except for the status field, if it is part of the config space?
>> 
>> I asked for clarification [1], because Cornelia's feedback seems to
>> contradict your feedback here and on the (unfortunately unarchived) v3
>> patch. What is the definitive statement agreed upon by both editors of
>> the virtio specification as to whether or not the device status field
>> is part of the device configuration space?
>> 
>> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
>> 
>> -David
>
> I don't know what did Cornelia mean. Cornelia?

What I meant (I think...) was that accessing the status field is fine,
even if it is implemented as part of the config space.

Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan]

On 7/3/2024 5:01 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
>> This commit allows the driver to suspend the device by
>> introducing a new status bit SUSPEND in device_status.
>>
>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>> which indicating whether the device support SUSPEND.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>> Signed-off-by: David Stevens <stevensd@chromium.org>
> Your signature would normally be last.
>
>
>> ---
>>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 61 insertions(+), 10 deletions(-)
> At a high level, I think this is somewhat overspecified.
> Generally e.g. if driver is forbidden from accessing
> a field, then we do not also add specific requirements for
> devices - with no driver doing this, such functionality will
> remain untested and unused, and when we finally decide we need
> it it's not going to be there.
> Similarly for when device is not touching a field - no
> reason for driver not to touch it.
>
>
>> diff --git a/content.tex b/content.tex
>> index 0a62dce..8c974d3 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -36,19 +36,22 @@ \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[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.
>> +
>> +\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
>> @@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
>> +except when setting \field{device status} to 0 as a transport-specific way to
>> +initiate a reset. 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
>> @@ -99,10 +103,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 +633,49 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
>> +SUSPEND bit in \field{device status} to suspend a device, and can
>> +clear the SUSPEND bit to resume a suspended device.
>> +
>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +Once the driver sets SUSPEND to \field{device status} of the device:
>> +\begin{itemize}
>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> If it's not set, then what? Device error, have to reset?
below in this patch:
"When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend"
>
>
>> +\item The driver MUST NOT make any more buffers available to the device.
>> +\item The driver MUST NOT access any fields of any virtqueues or send notifications for any virtqueues.
> what are these "fields"? the area in memory where the vq lives? why is
> doing that a problem - you want device to be able to write something
> there?
To stabilize the vq states once suspended.

So how about change "fields" to "Descriptor Area and Driver Area", or just "virtqueues"?
>
>> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>> +if it is implemented in the Config Space.
> what is "the Config Space"? device status is never in
> a device configuration space, it is part of the common configuration.
> so what exactly are you forbidding here?
"common configuration" is PCI specific and actually "device status" and "device configuration space"
are two separate sections in the spec.

And also for PCI, the device status is implemented in "common configuration".

Do you suggest we fall back to the last version:"The driver MUST NOT access Device Configuration Space"?
>
>> +\end{itemize}
>> +
>> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> what does must ignore SUSPEND mean? what do you want the device to do and when?
Do nothing and "ignore" is existing language in this spec:

The device MUST ignore the write-only flag (flags&VIRTQ_DESC_F_WRITE) in the descriptor that refers
to an indirect table.
>
>
>> +The device MUST ignore all access to its Configuration Space while
>> +suspended, except for \field{device status} if it is part of the Configuration Space.
> again what does ignore mean? Configuration Space here means
> Device Configuration Space?
>
>> +
>> +A device MUST NOT send any notifications, access any virtqueues, or modify
>> +any fields in its Configuration Space while suspended.
> If a tree falls in a forest, and no one is around to hear it, does
> it still make a sound?
> I think what matters is that if a configuration space changes while
> SUSPEND is set, then the device must not send a configuration change
> notification, and it must send a configuration change notification
> after SUSPEND bit has been cleared.
I will add this in V7.
>
>
>> +When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend.
>> +
>> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
>> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
>> +
>> +When the driver sets SUSPEND,
>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
> what does "before presenting" mean here? that a driver reading status
> will see 0 in this bit?
Presenting SUSPEND means setting the bit to 1. This is also existing spec language:
"device_feature The device uses this to report which feature bits it is offering to the driver: the driver writes to device_feature_select to select which feature bits are presented"

Is it clear if we say "presenting 1 in the SUSPEND bit"?
>
>> +\begin{itemize}
>> +\item Stop processing 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}
> Is there anything here that has not been said before, but as a MUST?
> Saying same thing twice once a MUST once a SHOULD is confusing IMHO.
We have similar statements in the driver parts, but here is device normative section.
>
>
>> +
>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>  
>>  Virtio can use various different buses, thus the standard is split
>> @@ -872,6 +919,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(42)] This feature indicates that the driver can
>> +   trigger suspending the device via the SUSPEND flag
>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>>  \end{description}
>>  
>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> -- 
>> 2.45.2