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

["Michael S. Tsirkin" <mst@redhat.com>]

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