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

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

On Thu, Aug 01, 2024 at 07:35:16PM +0800, Zhu Lingshan wrote:
> +\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.

Actually, it has no effect before DRIVER_OK, no? I would forbid that
then.

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


This is still vague, I commented on this several times.
I think what you mean is that until it reads status with
SUSPEND as 1, it does not consider SUSPEND set.


But, what happens if driver clears SUSPEND before it reads it as set?
It would seem that should cancel suspend (which is a useful thing to
support)? But this creates a problem as it breaks read/modify/write that
some hypervisors assumed to be safe. I guess we need an extra
SUSPEND_IN_PROGRESS bit then? A little too much for status, at this
stage - maybe we need an extra register for this.


> +\item The driver MUST NOT make any more buffers available to the device.
> +\item The driver MUST NOT access any virtqueues or send notifications for any virtqueues.
> +\item The driver MUST NOT access Device Configuration 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 for any virtqeuues,
> +access any virtqueues, or modify any fields in
> +its Configuration Space while suspended.
> +
> +If changes occur in the Configuration Space while the SUSPEND bit is set,
> +the device MUST NOT send any configuration change notifications.
> +Instead, the device MUST send the notification after the SUSPEND bit has been cleared.
> +
> +When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend.
> +
> +If SUSPEND is set in \field{device status}, when the driver 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 that the SUSPEND bit is set to 1 in the \field{device status}:

what does "before presenting" mean? does it return SUSPEND as 0
after driver wrote 1 there and before it completed these
actions?

> +
> +\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}
> +
>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>  
>  Virtio can use various different buses, thus the standard is split
> @@ -872,6 +923,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
> +   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}
> -- 
> 2.45.2