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

[David Stevens <stevensd@chromium.org>]

On Sun, Feb 18, 2024 at 11:11âPM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Feb 18, 2024 at 09:23:06PM +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.
> >
> > This SUSPEND bit is transport-independent.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
>
> Could we get some kind of dscription how this has taken into
> consideration the proposal from David Stevens?
>
> I find it really tiring when there are competing patches with authors
> ignoring each other's work and leaving it up to reviewers to
> figure out how do the patches compare.

This patch looks like it could be used to implement my use case.
However, parts of it are a bit vague and imprecise, so it's hard to
actually say whether my use case would actually be covered by a
specific implementation of this proposal.

To give specifics on what I'm trying to do, I need to allow a guest
with virtio-pci devices (including stateful devices like virtio-fs and
virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
to wake up the guest (currently via an ACPI GPE, but eventually via a
native PCI PME). This is all done on a consumer device, so there is no
need for snapshotting or for live migration.

> > ---
> >  content.tex | 34 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 0a62dce..3d656b5 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)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > +  device has been suspended by the driver.
> >  \end{description}
> >
> >  The \field{device status} field starts out as 0, and is reinitialized to 0 by
> > @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >  recover by issuing a reset.
> >  \end{note}
> >
> > +The driver SHOULD NOT set SUSPEND if FEATURES_OK is not set.
> > +
> > +When setting SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> > +
> >  \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 +89,25 @@ \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.
> >
> > +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> > +
> > +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> > +
> > +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
> > +
> > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
> > +and resumes operation upon DRIVER_OK.
> > +
> > +If VIRTIO_F_SUSPEND is negotiated, 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 consuming buffers of any virtqueues and mark all finished descriptors as used.
> > +\item Wait until all descriptors that being processed to finish and mark them as used.
> > +\item Flush all used buffer and send used buffer notifications to the driver.

What's the difference between the previous three lines? They might be
trying to say different things, but there is a lot of overlap in how
they're phrased. That makes it hard to figure out exactly what they're
mandating. Is it something like: "stop processing new buffers",
"finish processing any buffers that are currently in flight", "mark
all finished buffers as used and send a used buffer notification"?

Also, what does this mean for devices where the driver places empty
buffers into the virtqueue that the device then holds on to for an
indeterminate period of time (e.g. network receiveq, balloon statsq,
etc)?

> > +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}

What does "Pause its operation except device status" mean? The word
"operation" is vague, what exactly does it include? Also what does
"operate its device status" mean?

-David