[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH] virtio: introduce SUSPEND bit in device status
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. > > > > 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. > > > >>> --- > >>> 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. 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. 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. -David
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]