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

[Zhu Lingshan <lingshan.zhu@amd.com>]

On 7/3/2024 5:01 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 03, 2024 at 11:10:57AM +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.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>> Signed-off-by: David Stevens <stevensd@chromium.org>
> Your signature would normally be last.
>
>
>> ---
>>  content.tex | 71 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 61 insertions(+), 10 deletions(-)
> At a high level, I think this is somewhat overspecified.
> Generally e.g. if driver is forbidden from accessing
> a field, then we do not also add specific requirements for
> devices - with no driver doing this, such functionality will
> remain untested and unused, and when we finally decide we need
> it it's not going to be there.
> Similarly for when device is not touching a field - no
> reason for driver not to touch it.
>
>
>> diff --git a/content.tex b/content.tex
>> index 0a62dce..8c974d3 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -36,19 +36,22 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>      this bit.  For example, under Linux, drivers can be loadable modules.
>>    \end{note}
>>  
>> -\item[FAILED (128)] Indicates that something went wrong in the guest,
>> -  and it has given up on the device. This could be an internal
>> -  error, or the driver didn't like the device for some reason, or
>> -  even a fatal error during device operation.
>> +\item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>> +  drive the device.
>>  
>>  \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
>>    features it understands, and feature negotiation is complete.
>>  
>> -\item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>> -  drive the device.
>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>> +  device has been suspended by the driver.
>>  
>>  \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>    an error from which it can't recover.
>> +
>> +\item[FAILED (128)] Indicates that something went wrong in the guest,
>> +  and it has given up on the device. This could be an internal
>> +  error, or the driver didn't like the device for some reason, or
>> +  even a fatal error during device operation.
>>  \end{description}
>>  
>>  The \field{device status} field starts out as 0, and is reinitialized to 0 by
>> @@ -60,8 +63,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>  initialization sequence specified in
>>  \ref{sec:General Initialization And Device Operation / Device
>>  Initialization}.
>> -The driver MUST NOT clear a
>> -\field{device status} bit.  If the driver sets the FAILED bit,
>> +The driver MUST NOT clear a \field{device status} bit other than SUSPEND
>> +except when setting \field{device status} to 0 as a transport-specific way to
>> +initiate a reset. If the driver sets the FAILED bit,
>>  the driver MUST later reset the device before attempting to re-initialize.
>>  
>>  The driver SHOULD NOT rely on completion of operations of a
>> @@ -99,10 +103,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
>>  \begin{description}
>>  \item[0 to 23, and 50 to 127] Feature bits for the specific device type
>>  
>> -\item[24 to 41] Feature bits reserved for extensions to the queue and
>> +\item[24 to 42] Feature bits reserved for extensions to the queue and
>>    feature negotiation mechanisms
>>  
>> -\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
>> +\item[43 to 49, and 128 and above] Feature bits reserved for future extensions.
>>  \end{description}
>>  
>>  \begin{note}
>> @@ -629,6 +633,49 @@ \section{Device Cleanup}\label{sec:General Initialization And Device Operation /
>>  
>>  Thus a driver MUST ensure a virtqueue isn't live (by device reset) before removing exposed buffers.
>>  
>> +\section{Device Suspend}\label{sec:General Initialization And Device Operation / Device Suspend}
>> +
>> +When VIRTIO_F_SUSPEND is negotiated, the driver can set the
>> +SUSPEND bit in \field{device status} to suspend a device, and can
>> +clear the SUSPEND bit to resume a suspended device.
>> +
>> +\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 it's not set, then what? Device error, have to reset?
below in this patch:
"When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend"
>
>
>> +\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 send notifications for any virtqueues.
> what are these "fields"? the area in memory where the vq lives? why is
> doing that a problem - you want device to be able to write something
> there?
To stabilize the vq states once suspended.

So how about change "fields" to "Descriptor Area and Driver Area", or just "virtqueues"?
>
>> +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>> +if it is implemented in the Config Space.
> what is "the Config Space"? device status is never in
> a device configuration space, it is part of the common configuration.
> so what exactly are you forbidding here?
"common configuration" is PCI specific and actually "device status" and "device configuration space"
are two separate sections in the spec.

And also for PCI, the device status is implemented in "common configuration".

Do you suggest we fall back to the last version:"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.
> what does must ignore SUSPEND mean? what do you want the device to do and when?
Do nothing and "ignore" is existing language in this spec:

The device MUST ignore the write-only flag (flags&VIRTQ_DESC_F_WRITE) in the descriptor that refers
to an indirect table.
>
>
>> +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.
> again what does ignore mean? Configuration Space here means
> Device Configuration Space?
>
>> +
>> +A device MUST NOT send any notifications, access any virtqueues, or modify
>> +any fields in its Configuration Space while suspended.
> If a tree falls in a forest, and no one is around to hear it, does
> it still make a sound?
> I think what matters is that if a configuration space changes while
> SUSPEND is set, then the device must not send a configuration change
> notification, and it must send a configuration change notification
> after SUSPEND bit has been cleared.
I will add this in V7.
>
>
>> +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 SUSPEND bit in the \field{device status}:
> what does "before presenting" mean here? that a driver reading status
> will see 0 in this bit?
Presenting SUSPEND means setting the bit to 1. This is also existing spec language:
"device_feature The device uses this to report which feature bits it is offering to the driver: the driver writes to device_feature_select to select which feature bits are presented"

Is it clear if we say "presenting 1 in the SUSPEND bit"?
>
>> +\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}
> Is there anything here that has not been said before, but as a MUST?
> Saying same thing twice once a MUST once a SHOULD is confusing IMHO.
We have similar statements in the driver parts, but here is device normative section.
>
>
>> +
>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>  
>>  Virtio can use various different buses, thus the standard is split
>> @@ -872,6 +919,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