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

> From: Zhu Lingshan <[email protected]>
> Sent: Monday, June 9, 2025 1:31 PM
> 
> On 6/7/2025 10:11 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <[email protected]>
> >> Sent: Friday, June 6, 2025 4:01 PM
> >>
> >> 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.
> >>
> > Its not about some policy.
> > Sure user may use the feature in N different use cases.
> >
> > The intent is very simple. For the reviewers to review the patch, why
> something is needed should be presented.
> > So motivation/usecase/need is worth the entry.
> > Some reviewers do rely on the commit logs to understand the design intent
> behind it.
> >
> > Without the need of the patch, I am not sure why would even one write
> and review the spec patch.
> > Please add known use cases. I am familiar with one that I suggested above.
> 
> we can add these descriptions in the cover letter.

Yes, cover letter covers for the series as you will have 3 patches and commit log of individual patch will have one two sentences would be good.

> 
> >
> >>>
> >>>> 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
> > Having logically separate patches are always good. Re-arrangement is pure
> cosmetic vs this one that adds new functionality.
> 
> ok, we can split this patch into a series if this is preferred.
> 
Sounds good.

> >
> >>>>  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.
> > So the current spec wordings  is incorrect 0 to 23, and 50 to 127
> > Feature bits for the specific device type
> 
> current spec missed documenting bit 42.
> 
> >
> > We need to says,
> >
> > 0 to 23, 41 to 42 and 50 to 127 Feature bits for the specific device
> > type This should also be pre-patch, so that it can be backported to master
> branch.
> > Another reason for the small patch to be separate.
> >
> > If you want me to write up this pre-patch, please let me know.
> > You can include in the series.
> 
> There could a a separate patch in a series says:
> 41 to 42 Feature bits reserved for extensions to the queue and feature
> negotiation mechanisms
> 43 to 49, and 128 and above Feature bits reserved for future extensions.
>
41-42 are used by the device specific types. So they are not reserved.
They are in_use by device specific type. It should be worded accordingly.


> which fixes commit 9438414
> 
> >
> >>>> -\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?
> >>
> > Because:
> >
> > 1. To clarify the intent:
> > These keywords carry precise semantic weight. When a spec says MUST, it
> means compliance is not optional. Using such terms in general or explanatory
> text risks misinterpreting guidance as a requirement.
> >
> > 2. Avoiding Ambiguity:
> > If normative language is used in non-normative (informative) sections, it
> becomes unclear whether the reader is required to follow it or if it's just
> background or suggestion.
> >
> > 3. Informative sections explain context, rationale, or examples. Normative
> sections define rules. Mixing the two muddies the structure and weakens the
> authority of the normative part.
> 
> There are no such rules.
>
It is not about the rule.
It is the way virtio spec is structured today.
What I explained above : is what happens when one writes theory of operation as normative tone.
Description/theory of operation etc sections are written with simple present declarative tone.
And requirements are written as normative language tone following the RFC 1.3.
 
So please avoid normative wordings in description.

> In the spec section 1.3 Terminology, it says:
> 
> The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
> "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
> "MAY", and "OPTIONAL" in this document are to be interpreted as described
> in [RFC2119] and [RFC8174] when, and only when, they appear in all capitals,
> as shown here.
> 
> RFC 2119 says:
> 
> SHOULD This word, or the adjective "RECOMMENDED", mean that there
>    may exist valid reasons in particular circumstances to ignore a
>    particular item, but the full implications must be understood and
>    carefully weighed before choosing a different course.
> 
> 
> Here this "SHOULD" exactly conforms to the definition.
> 
> "SHOULD" has already been used in non-normative sections, for example:
> 2.5.4 Legacy Interfaces: when using the legacy interface, drivers SHOULD read
> these fields multiple times until two reads generate a consistent result.
> 
> The spec does not say: "SHOULD" can only be used in driver or device
> requirements section.
> 
It follows the style to not have normative words in descriptions as I explained above.

> >
> >>> 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?
> >>
> > "resumes running" does not read correct.
> > And normative "should" is used...
> > Hence the suggestion to re-word it.
> 
> Looking into a dictionary
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
> .merriam-
> webster.com%2Fdictionary%2Fresume&data=05%7C02%7Cparav%40nvidia.c
> om%7C2676f0bbbacd46d2944608dda72bc149%7C43083d15727340c1b7db3
> 9efd9ccc17a%7C0%7C0%7C638850528606897234%7CUnknown%7CTWFpbG
> Zsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiI
> sIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=FftxcTQOXxj
> hvCNeIs3%2FHzphS5Q1DVUn2VjLJAt75oA%3D&reserved=0
> There is quite a lot of examples of resume + noun.
>
Running is not a good noun in this context.
It can be worded better which clarifies things more crisply as proposed.
 
> And here, "SHOULD" still conforms to RFC 2119.
> 
It does not belong in descriptive section. It belongs to requirements section.

> >
> >>>> +\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
> >>
> > I didn't find it in the normative device section.
> 
> +\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 b
> it to 0 once it has been suspended.
> +
> +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.
>
As I explained, it is not the requirements section of the device. It is the description/theory of operation section.
And you gave the good example that creates this confusion of mixing the normative tone in description.
:)

> >
> >>>
> >>>> +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
> >>
> > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >>>
> th%2F&data=05%7C02%7Cparav%40nvidia.com%7C2676f0bbbacd46d29446
> 08dda7
> >>>
> 2bc149%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C6388505286
> 069551
> >>>
> 99%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjA
> uMDAwM
> >>>
> CIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&
> sda
> >>>
> ta=JM7RqfmpPNS3eVrlq0ky93nh6NGLhKsm97QZkSCnq%2B4%3D&reserved=
> 0
> >>> ub.com%2Foasis-tcs%2Fvirtio-
> >> spec%2Fissues%2F&data=05%7C02%7Cparav%40nv
> >>
> idia.com%7C0ef2b01694d942339bb308dda4e5365c%7C43083d15727340c1b
> >> 7db39ef
> >>
> d9ccc17a%7C0%7C0%7C638848026619639978%7CUnknown%7CTWFpbGZsb
> >> 3d8eyJFbXB0
> >>
> eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpb
> >> CIsIl
> >>
> dUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=0st2osYsAxascHHmHj0RgSyYAn9l
> >> yQRKqxOl
> >>> aIjE8dk%3D&reserved=0<issue_number>
> >>>
> >>> This will help to link the two sides.
> >> sure
> >>
> > Thanks.
> 

Please send v3 with these short fixes and 2 pre-patch.
For fixing feature bits if you want me to write up the patch, please let me know.