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

[Cornelia Huck <cohuck@redhat.com>]

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...

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