[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-dev] Re: [PATCH v5 1/1] Add SUSPEND bit to device status
On 2/29/2024 11:20 AM, David Stevens wrote:
On Wed, Feb 28, 2024 at 3:45âPM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:On 2/28/2024 9:18 AM, David Stevens wrote:On Tue, Feb 27, 2024 at 5:52âPM Zhu, Lingshan <lingshan.zhu@intel.com> wrote: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.If it's a u8, then the driver will be writing all 8 bits at the same time. So I don't see how it's possible for the driver to set one bit without also setting/clearing all the other bits at the same time. When you say the driver can re-set DRIVER_OK, it can either write 0x1F or 0xF. If we don't allow clearing the suspend bit, then it has to write 0x1F. But that's exactly what the driver wrote to suspend the device in the first place - how is the device supposed to tell the difference? I guess we could add something to the spec saying that's how the device is supposed to interpret it. But at that point, that's really the same as allowing the driver to clear the suspend bit, just with more complexity.Thanks for the explanation, I get your point now. I agree allowing clear a bit can resume the device running. however this makes an exception and require the device to examine every bits that the driver set.It needs to do this anyway to detect the SUSPEND bit being set in the first place, doesn't it? Why is detecting the SUSPEND bit being cleared more difficult than detecting the SUSPEND set being set?
as explained below, it does examine every bits in device status.
Letting the device clear the SUSPEND bit also require the device to examine every bits. Based on the device status(Init, Running or SUSPENDED), the device can behave differently: +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND +and resumes operation upon DRIVER_OK. But no need to make exceptions in the spec. Maybe this is a little less complex?That is also adding an exception, though? It introduces a single situation where the behavior of bits is tied together and a single situation where the device instead of the driver is allowed to clear bits. It would also complicate adding bit to device status that can be set when the device is suspended, since it would be ambiguous as to whether a driver writing 0x3F wants to enable bit 0x20 or wants to enable 0x20 and also disable SUSPEND. I fail to see how that is simpler than just letting the driver clear the SUSPEND bit.
Not adding a new exception, quite the opposite, this implementation tries to re-use current virtio mechanism. Let setting DRIVER_OK to bring the device back to alive operational: "Set the DRIVER_OK status bit. At this point the device is live" "DRIVER_OK is set: device operation begins." The device should behave differently based on its state machine, examples:"the device with the VIRTIO_BLK_Z_HM zone model MUST fail to set the FEATURES_OK device status bit when the driver writes the Device Status field." "The device MUST NOT consume buffers or send any used buffer notifications to the driver before DRIVER_OK."
And please take this piece of code as another example:
void virtio_add_status(struct virtio_device *dev, unsigned int status)
{
ÂÂ Âmight_sleep();
ÂÂ Âdev->config->set_status(dev, dev->config->get_status(dev) | status);
}
Here the code tries to add a status to the device,
the driver first read the device status, then add a new status,
at last set the new status to the device.
That means it is quite common to re-write the same status bits to the
device, and the device can behave differently based on its
device state machine.
So IMHO, this implementation does not introduce more complexities,
it tries to re-use existing virtio mechanism.
Thanks
Zhu Lingshan
-David --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]