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

Parav Pandit
2024-06-11T05:17:38Z

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?

> > +
> > \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}
>