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

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 2/20/2024 1:09 PM, David Stevens wrote:
> On Tue, Feb 20, 2024 at 1:06âPM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > On 2/19/2024 2:46 PM, David Stevens wrote:
> > > 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.
> > I am on vacation till this Friday. Shall we co-work on this and
> > post something new together?
> That works for me.
Cool! How about two patches in a new series:
1) a general virtio suspending patch describing suspending behaviors for all
virtio devices from me, an updating version of this patch
2) a PCI transport suspending patch from you, describing PCI specific 
behaviors.

Dose this sound good to you?
> > > 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.
> > Suspending is not dedicated only for live migration.
> > For your use case, shall we add a new PCI section like saying: when entering
> > SUSPEND, the device should get into S1/S3 and other PCI specific steps
> > in PCI transport charter.
> >
> > That is because PCI is a transport layer of virtio, controlling virtio
> > states by PCI sounds like a layer violation.
> I don't know if it's really necessary to mandate that in the spec.
> Sure, most systems are probably going to want to put the virtio-pci
> device into S1/S3 after suspending virtio operation. But nothing
> really breaks or goes wrong if we don't do that.
Yes, maybe not S1/S3, I remember you want to manage power in PCI
transport when suspend in a cap, S1/S3 is just an example.

> > > > > ---
> > > > >    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"?
> > sure, the "mark used" parts can merge.
> > > 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)?
> > The device doesn't process them.
> > Oh, I should add: The driver should not make any more buffers available
> > to the device.
> > > > > +\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?
> > Because we need to resume the device after suspending it by setting
> > DRIVER_OK, so we need the device status
> > filed keel alive.
> This doesn't answer what "Pause its operation" means. If the
> specification is not precise, then the best that can be implemented is
> for a specific device to be compatible with a specific driver. If we
> actually want portable, spec-compliant devices, we need explicitly
> defined MUST/MUST NOT requirements. Presumably we want something like:
>
> A device MUST not send notifications while suspended.
The device should send a last used buffer notification if
it has buffers under processing before presenting SUSPEND
in the device status, but the device should not process
any new buffers. These are in devicenormative section,
but they may be vague, I can add more details to make
it precise, thanks for the tips
> A device MUST ignore writes to its device configuration while
> suspended, except for writes to the device status byte.
> A driver MUST NOT write to a suspended device's configuration space,
> except for the device status byte.
sure, we can make it more precise than only pausing.
> We also need to specify what a suspended device is allowed to do with
> buffers it owns. For my use case of just suspending/resuming a VM, I
> don't think it particularly matters if a suspended device writes to
> buffers or descriptors while it is suspended. But based on what you
> wrote about finishing any in-flight processing of buffers and not
> processing any empty buffers, presumably you would prefer that the
> device not be allowed to access buffer/descriptors while it is
> suspended? That approach is probably cleaner from a pure specification
> standpoint, but it's also more restrictive for devices and thus harder
> to properly implement.
When the driver sets SUSPEND, the device should finished all buffers that
already under processing, include the in-flight buffers(or it may lose 
the buffers).
And the device should not touch the buffers once it present SUSPEND in the
device status field, because once it presents SUSPEND, the device state 
should
be stable, people may use SUSPEND to debug the device.

The device still can process the buffer before it presenting SUSPEND.

Thanks
Zhu Lingshan
> -David