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

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

On Tue, Jun 11, 2024 at 05:17:32AM +0000, Parav Pandit wrote:
> Hi Lingshan, David,
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Monday, June 10, 2024 9:34 PM
> > 
> > On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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: Zhu Lingshan <lingshan.zhu@amd.com>
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > ---
> > >  content.tex | 69
> > > +++++++++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 59 insertions(+), 10 deletions(-)
> > 
> > Can you please add a changelog? Especially as some of the previous discussion
> > has been lost due to the broken old mailing lists...
> > 
> > (...)
> > 
> > > +\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 not, the device does not support the SUSPEND feature.
> > 
> > That sentence is a bit weird: I'd expect the device to not offer
> > VIRTIO_F_SUSPEND in the first place in that case... could this rather happen if
> > the device is not able to handle the request at a specific point in time?
> > 
> > > +\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 notify
> > any virtqueues.
> > 
> > "send notifications for any virtqueues"?
> > 
> > > +\item The driver MUST NOT access Device Configuration Space.
> > 
> > ...except for the status field, if it is part of 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.
> > > +
> > > +If SUSPEND is set in \field{device status}, when the driver clears
> > > +SUSPEND,
> > 
> > "subsequently 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}
> > 
> > So, is there any opportunity for the device to fail setting SUSPEND? I mean, if
> > the driver is supposed to look whether it sticks, there should be conditions for
> > when the device might clear it again...
> > 
> Additionally, a suspend operation usually involves saving things to a slow memory (or media).
> This is because the device implementation wouldn’t know when exactly the device will be resumed.
> Few examples, are:
> a. A gpu device with 128MB of video RAM when suspended, QEMU needs to store this into a (for example) rotating hard disk as 1msec IO latency.
> 
> b. a NIC may need to store its RSS, queues, flow filters configuration for several tens of KBs to some slow memory.
> 
> c. A block device may prefer to complete some IOs to a threshold level instead of maintaining large list of outstanding IOs in some suspended memory.
> 
> d. May be more in future.
> 
> Additionally, suspend operation needs to be synchronized with certain data path hardware engines to suspend DMA operations (read and write both) which are inflight.
> (without causing DMA errors). Same would apply to the sw backend implementations too.
> 
> Therefore, the suspend operation that is initiated by the driver, should get the acknowledgement back from the device that it has been suspend.
> 
> Some of the good examples if you prefer to follow, a driver<->device interface needs a suspend register which should behave like below queue reset register.
> 
> Spec snippet:
> "The device MUST reset the queue when 1 is written to queue_reset. The device MUST continue to present
> 1 in queue_reset as long as the queue reset is ongoing. The device MUST present 0 in both queue_reset
> and queue_enable when queue reset has completed."
> 
> At minimum, we need, something implementable like,
> 
> The device MUST suspend the device when 1 is written to the device_suspend_ctlr. The device MUST continue to present 1 in device_suspend_ctrl as the suspend operation is ongoing in the device.
> The device MUST present 0 in the device_suspend_ctlr register when device has completely suspended the device.
> 
> The device MUST resume the device when 2 is written to the device_suspend_ctrl. The device MUST continue to present 2 in the device_suspend_ctrl as the resume operation may not have yet completed.
> The device MUST present 0 in the device_suspend_ctrl register when the device has completely resumed the device.
> At this point, the driver may resume notifying the device and accessing the configuration space.
> 
> Can you please enhance this part?

Yea, this gives me pause too.


> 
> > > +
> > >  \chapter{Virtio Transport Options}\label{sec:Virtio Transport
> > > Options}
> > >
> > >  Virtio can use various different buses, thus the standard is split @@
> > > -872,6 +917,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
> > > +   set SUSPEND to the device.
> > 
> > "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}
> > 
>