Re: [PATCH v2] virtio: introduce SUSPEND and RESUME feature

On 6/6/2025 1:52 PM, Parav Pandit wrote:
> Hi,
>
>> From: Zhu Lingshan <[email protected]>
>> Sent: Thursday, May 29, 2025 1:54 PM
>>
>> This commit allows the driver to suspend the device through a new device
>> status bit SUSPEND and resume the device running by re-setting DRIVER_OK
>> bit in device status.
>>
>> This commit re-orders the device status bits.
>>
> I overall like this proposal. Few small comments to fix more below.
> It is lot cleaner as it eliminates the previous bit flipping complexities.
>
> Can you please add one or two line on a use case/motivation,
>
> For example, 
>
> Presently when an OS is suspended, the driver must fully stop and reset the device, and on resume the driver must re-initialize the device.
> This can be time consuming task to recreate large number of VQs.
>
> Instead, extend the specification to allow drivers to suspend and resume the device, without recreating all the resources.
> ...

Hello Parav

Here I think the spec provides mechanisms, not a policy. The user may employ this feature for other tasks
other than live migration which requires OS suspending. 

>
>  
>> Signed-off-by: Zhu Lingshan <[email protected]>
>> Signed-off-by: Jason Wang <[email protected]>
>> ---
>>  content.tex | 74 ++++++++++++++++++++++++++++++++++++++++++++++-----
>> --
>>  1 file changed, 65 insertions(+), 9 deletions(-)
>>
>> diff --git a/content.tex b/content.tex
>> index d3fc6a4..caef5d0 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[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that
>> +the
>> +  device has been suspended by the driver.
>>
>>  \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[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}
>>
> A new bit SUSPEND introduction should be done at the end without re-organizing rest of the bits.
> Please add at the end.
>
> Is the intention to rearrange the fields in ascending order of the bit number?
> If yes, its good change too but than it missed re-arranging FEATURE_OK.

oh yes, good catch, will fix

>
> If you want to re-arrange, please have the pre-patch in this series.
>
> Either way is ok to me,
> 1. Either as pre-patch for re-arrangement + addition of SUSPEND in middle.
> Or
> 2. adding SUSPEND bit definition at the end 
>
> #1 is better to see in ascending order.
I was required to re-arrange these status bits, but this is a trivial change,
so lets keep them in a single patch for easier review
>
>>  The \field{device status} field starts out as 0, and is reinitialized to 0 by @@ -
>> 99,10 +102,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 43] Feature bits reserved for extensions to the queue and
>>    feature negotiation mechanisms, see \ref{sec:Reserved Feature Bits}
>>
> Only one feature bit is used by this proposal.
> So it should be 24 to 42 and use bit 42.
>
> Did you use bit 43 due to existence of VIRTIO_NET_F_GUEST_RSC6?

Yes, bit 42 is taken for VIRTIO_NET_F_GUEST_RSC6.

>
>> -\item[42 to 49, and 128 and above] Feature bits reserved for future
>> extensions.
>> +\item[44 to 49, and 128 and above] Feature bits reserved for future
>> extensions.
>>  \end{description}
>>
>>  \begin{note}
>> @@ -629,6 +632,54 @@ \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}
>> +
>> +If VIRTIO_F_SUSPEND is negotiated, the driver is eligible to suspend the
>> device by setting the SUSPEND bit in \field{device status} to 1, and the device
>> SHOULD set the DRIVER_OK bit to 0 once it has been suspended.
>> +
> This is not a normative section so please avoid using SHOULD.
> Better to word it as informative theory of operation text.
> Such as,

why "SHOULD" can only be used in normative sections?

>
> If X is not negotiated, the....
> And the device sets the DRIVER_OK bit to 0 once it has been suspend.
>
>> +If the device has been suspended, the driver can resume the device running
>> by setting the DRIVER_OK bit in \field{device status} to 1, and the device
>> should set the SUSPEND bit to 0 once it resumes running.
>> +
> The device sets the SUSPEND bit to 0 once the device is running.

What is the difference in semantic?

>> +\drivernormative{\subsection}{Device Suspend}{General Initialization
>> +And Device Operation / Device Suspend}
>> +
>> +The driver SHOULD NOT set SUSPEND bit if DRIVER_OK is not set or
>> VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +Once the driver sets SUSPEND bit in \field{device status} to 1:
>> +\begin{itemize}
>> +\item The driver MUST verify whether the device has been suspended by
>> re-reading \field{device status}, examining whether the SUSPEND bit is set to
>> 1 and the DRIVER_OK bit is set to 0.
>> +\item The driver MUST NOT make any more buffers available to the device.
>> +\item The driver MUST NOT send notifications for any virtqueues.
>> +\item The driver MUST NOT make any changes to Device Configuration
>> Space except for \field{device status} if it is part of the Configuration Space.
>> +\end{itemize}
>> +
>> +\devicenormative{\subsection}{Device Suspend}{General Initialization
>> +And Device Operation / Device Suspend}
>> +
>> +The device MUST ignore any operations on the SUSPEND bit from the
>> +driver if the device has not been completely initialized by the
>> +procedures in \ref{sec:General Initialization And Device Operation /
>> +Device Initialization}
>> +
>> +The device SHOULD ignore any write 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 virtqueues, access any
>> +virtqueues, or modify any fields in its Configuration Space while
>> +suspended.
>> +
>> +If changes occur in the Configuration Space during suspended period,
>> +the device MUST NOT send any configuration change notifications.
>> +Instead, the device MUST send the notification when it resumes running.
>> +
>> +If the driver sets the SUSPEND bit in \field{device status} to 1, the
>> +device MUST either suspend itself or set the DEVICE_NEEDS_RESET bit in
>> \field{device status} to 1 when it fails to suspend.
>> +
> Even though you have the driver normative, it is good to have device normative for suspend and resume operation:
>
> Something like,
>
> When the suspend operation completes in the device, the device MUST set
> SUSPEND bit to 1 and DRIVERK_OK to 0.
>
> When the suspended device is resumed, the device MUST set SUSPEND bit to 0 and DRIVER_OK to 1.

I think we already have similar descriptions in the beginning of this section

>
>
>> +If the device has been suspended and the driver resumes the device
>> +running by setting the DRIVER_OK bit in \field{device status} to 1, the
>> device MUST either resume normal operation or set the
>> DEVICE_NEEDS_RESET bit in \field{device status} to 1 when it fails to resume.
>> +
>> +When the driver sets the SUSPEND bit to 1, the device SHOULD perform
> It should be MUST.
> Otherwise it can crash the system accessing memory that may not be any more accessible.

Not sure how this can crash system memory, but "MUST" is OK

>
>> +the following actions before presenting the SUSPEND bit as 1 and
>> DRIVER_OK bit as 0 in the \field{device status}:
>> +
>> +\begin{itemize}
>> +\item Stop consuming 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,11 @@ \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(43)] This feature indicates that the driver can
>> +   suspend the device by set the SUSPEND bit to 1.
>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>> +
>>  \end{description}
>>
>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> --
>> 2.49.0
>>
> Apart from these small comments, please add the line in v3 commit log:
>
> https://github.com/oasis-tcs/virtio-spec/issues/<issue_number>
>
> This will help to link the two sides.

sure