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

Zhu Lingshan
2024-06-11T08:20:51Z

On 6/11/2024 12:04 AM, Cornelia Huck wrote:
> 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...
>
> (...)
will add change log in next version.
>
>> +\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?
Yes, this can be improved

How about:
If not, this indicating the device is not able to handle SUSPEND at the moment.
>
>> +\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"?
OK
>
>> +\item The driver MUST NOT access Device Configuration Space.
> ...except for the status field, if it is part of the config space?
Yes, that is the original design, then I am convinced that the status fields is
not a part of the config space because it has a dedicated section in the spec.

I will add this sentence in the next version.
>
>> +\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.
Yes
>
>> +
>> +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"?
I am a native speaker, but "subsequently" sounds like clearing SUSPEND right after "setting SUSPEND", I guess there can be
some operations during 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...
The device should not present SUSPEND bit in the device status until it is suspended,
so I am afraid there is not a SUSPEND bit to clear if the device fails to suspend itself.

I think there can be two indications of a failed suspend:
1) The driver sets SUSPEND, but the device does not present SUSPEND after a while, depends
on the driver tolerance, the driver may assume it is a failed SUSPEND, like how we handle FEATURES_OK
2)The device runs into errors, presenting NEEDS_RESET.

>
>> +
>> \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"?
Yes
>
>> + See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>> \end{description}
>>
>> \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}