[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: 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 <lingshan.zhu@intel.com> 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.htmlI 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.htmlNot 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.
-DavidThanksThe 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}
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]