[virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status

On 2/27/2024 3:59 PM, David Stevens wrote:
> On Tue, Feb 27, 2024 at 11:22 AM Zhu, Lingshan <[email protected]> wrote:
>> On 2/27/2024 9:53 AM, David Stevens wrote:
>>> Add a SUSPEND bit to the device status field to allow drivers to suspend
>>> virtio devices. On systems where drivers don't directly manage interrupt
>>> routing (e.g. Linux), this allows the drivers to suspend their devices
>>> and prevent interrupts from being sent when the interrupt routing system
>>> does not expect interrupts.
>>> ---
>>>    content.tex | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>    1 file changed, 84 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/content.tex b/content.tex
>>> index 0a62dce5f65f..2183c63c45ea 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>
>>>    \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>      an error from which it can't recover.
>>> +
>>> +\item[SUSPEND (16)] Indicates that the device has been suspended by the
>>> +  driver. Only valid when the VIRTIO_F_SUSPEND feature bit is negotiated.
>>>    \end{description}
>>>
>>>    The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>> @@ -60,9 +63,11 @@ \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 later reset the device before attempting to re-initialize.
>>> +
>>> +The driver MUST NOT clear a \field{device status} bit, except for the
>>> +SUSPEND bit as described in \ref{sec:General Initialization And Device
>>> +Operation / Device Suspend}. If the driver sets the FAILED bit, the
>>> +driver MUST later reset the device before attempting to re-initialize.
>> Comparing to add a new exception, why not just re-setting DRIVER_OK to
>> let the device
>> clearing SUSPEND? This issue has been addressed when Eugenio working on
>> a similar STOP_BIT
>>
>> https://lists.oasis-open.org/archives/virtio-dev/202111/msg00020.html
> I don't think the device status bit is bit-addressable, so it's not
> possible to re-set DRIVER_OK without also either re-setting or
> clearing SUSPEND.
Please correct me if I misunderstand your point,
I think it can be addressed, for example, in PCI it is an u8,
the device can clear a bit in it.


It may be easier to let the device to clear the SUSPEND bit.
>
> The linked series seems to effectively do the same thing as this patch
> does. Just rather than an exception for SUSPEND, it explicitly lists
> the bits which shouldn't be cleared. Although on the patch doing that,
> there was feedback suggesting that it be done the way this patch does
> it [1]. Personally, I think either an allowlist or a blocklist is
> fine.
>
> [1] https://lists.oasis-open.org/archives/virtio-dev/202111/msg00025.html
>
>> Not sure it is my email client bug, it shows the authorship has been
>> reset and
>> all sign-off-by are removed. We have been working on this since 2021,
>> how about
>> just include our updated patch(WIP) in a new series?
> Oh, sorry. I'm not very familiar with the process of collaboration
> over email like this. I wasn't sure about adding Signed-off-by for
> other people that haven't ready the patch yet, but I can add them
> going forward if that's what's expected. And I'll fix up authorship
> and add myself as Co-authored-by next time.
This maps to my Intel assignment.
So please let me send out a V2 and once the patch passed review process,
we can add more PCI patches into the series. I will add your sign-off-by
there.
>
> -David
>
>> Thanks
>>>    The driver SHOULD NOT rely on completion of operations of a
>>>    device if DEVICE_NEEDS_RESET is set.
>>> @@ -73,6 +78,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>    recover by issuing a reset.
>>>    \end{note}
>>>
>>> +If VIRTIO_F_SUSPEND is negotiated, the driver MUST manage the SUSPEND bit
>>> +as described in \ref{sec:General Initialization And Device Operation /
>>> +Device Suspend}. Otherwise, the driver MUST NOT set the SUSPEND bit.
>>> +
>>>    \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
>>>
>>>    The device MUST NOT consume buffers or send any used buffer
>>> @@ -82,6 +91,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>    that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
>>>    MUST send a device configuration change notification to the driver.
>>>
>>> +If VIRTIO_F_SUSPEND is negotiated, the device MUST manage the SUSPEND bit
>>> +as described in \ref{sec:General Initialization And Device Operation /
>>> +Device Suspend}. Otherwise, the device MUST ignore the SUSPEND bit.
>>> +
>>>    \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
>>>
>>>    Each virtio device offers all the features it understands.  During
>>> @@ -99,10 +112,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 +642,67 @@ \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 the VIRTIO_F_SUSPEND feature is negotiated, the driver can set the
>>> +SUSPEND bit in \field{device status} to suspend a live device, and can
>>> +clear the SUSPEND bit to resume a suspended device. A suspended device
>>> +should pause its operation, but it must maintain it state such that it
>>> +can immediately continue operation upon being resumed.
>>> +
>>> +Suspending a device via the SUSPEND bit is a seperate process from any
>>> +transport-specific suspend mechanism.
>>> +
>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>> +
>>> +The driver MUST NOT set the SUSPEND bit if the DRIVER_OK bit is not set.
>>> +
>>> +After writing a new value to the SUSPEND bit, the driver MUST wait for
>>> +the device to acknowledged the transition by reading from \field{device
>>> +status} until the returned value of the SUSPEND bit matches the written
>>> +value. During this period, the driver MAY abort the transition by writing
>>> +a new value to the SUSPEND bit or by resetting the device.
>>> +
>>> +A driver MUST NOT access the device configuration space of a suspended
>>> +device, except for \field{device status}.
>>> +
>>> +A driver MAY suspend a device that has buffers in its virtqueues. While
>>> +the device is suspended, a driver MUST NOT modify any available buffers
>>> +or their descriptors.
>>> +
>>> +A driver MUST NOT make any new buffers available to a suspended device.
>>> +
>>> +If a transport-specific suspend mechanism is available, the driver SHOULD
>>> +use it to put the device into a deeper suspend state after setting the
>>> +SUSPEND bit.
>>> +
>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>> +
>>> +A device MUST ignore writes to the SUSPEND bit if the DRIVER_OK bit is
>>> +not set.
>>> +
>>> +A device MUST maintain its state while suspended such that all driver
>>> +visible state after resuming exactly matches driver visible state
>>> +before suspending.
>>> +
>>> +A device MUST ignore all writes to its configuration space while
>>> +suspended, except for writes to \field{device status}.
>>> +
>>> +A device MUST NOT send notifications, access any virtqueues, or modify
>>> +any fields in its configuration space while suspended.
>>> +
>>> +A device MAY send notifications, access any virtqueues, or modify its
>>> +configuration space after the driver writes the SUSPEND bit but before
>>> +the device acknowledges the transition by returning a \field{device
>>> +status} value with the SUSPEND bit set. A device SHOULD finish processing
>>> +and send the used buffer notification for any buffers it is able to
>>> +before acknowledging the transition, but MAY retain buffers that cannot
>>> +be immiedately processed (e.g. empty buffers in a network recieveq).
>>> +
>>> +A device SHOULD take steps to minimize its resource consumption while
>>> +suspended, although what this involves is specific to the particular
>>> +device implementation.
>>> +
>>>    \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>>
>>>    Virtio can use various different buses, thus the standard is split
>>> @@ -872,6 +946,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(42)] This feature indicates that the driver can
>>> +    suspend the device via the SUSPEND bit in \field{device status} (see
>>> +    \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}).
>>> +
>>> +
>>>    \end{description}
>>>
>>>    \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]