[virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status

On 2/25/2024 4:52 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
>>
>> On 2/20/2024 1:09 PM, David Stevens wrote:
>>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <[email protected]> wrote:
>>>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <[email protected]> wrote:
>>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>>>> This commit allows the driver to suspend the device by
>>>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>>>
>>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>>>> which indicating whether the device support SUSPEND.
>>>>>>>
>>>>>>> This SUSPEND bit is transport-independent.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <[email protected]>
>>>>>>> Signed-off-by: Jason Wang <[email protected]>
>>>>>>> Signed-off-by: Eugenio Pérez <[email protected]>
>>>>>> Could we get some kind of dscription how this has taken into
>>>>>> consideration the proposal from David Stevens?
>>>>>>
>>>>>> I find it really tiring when there are competing patches with authors
>>>>>> ignoring each other's work and leaving it up to reviewers to
>>>>>> figure out how do the patches compare.
>>>>> This patch looks like it could be used to implement my use case.
>>>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>>>> actually say whether my use case would actually be covered by a
>>>>> specific implementation of this proposal.
>>>> I am on vacation till this Friday. Shall we co-work on this and
>>>> post something new together?
>>> That works for me.
>> Cool! How about two patches in a new series:
>> 1) a general virtio suspending patch describing suspending behaviors for all
>> virtio devices from me, an updating version of this patch
>> 2) a PCI transport suspending patch from you, describing PCI specific
>> behaviors.
>>
>> Dose this sound good to you?
> Well David's patch was more precise than yours, and also better worded.
> So think if there's an agreement you guys would really start with that,
> move the functionality to the status bit and make other changes as needed.
I think we plan to cooperate on a new series including both the status 
bit and
PCI transport, collaborations on both of our patches.

Let's see whether this looks good to David

Thanks
>
>
>
>>>>> To give specifics on what I'm trying to do, I need to allow a guest
>>>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>>>> native PCI PME). This is all done on a consumer device, so there is no
>>>>> need for snapshotting or for live migration.
>>>> Suspending is not dedicated only for live migration.
>>>> For your use case, shall we add a new PCI section like saying: when entering
>>>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>>>> in PCI transport charter.
>>>>
>>>> That is because PCI is a transport layer of virtio, controlling virtio
>>>> states by PCI sounds like a layer violation.
>>> I don't know if it's really necessary to mandate that in the spec.
>>> Sure, most systems are probably going to want to put the virtio-pci
>>> device into S1/S3 after suspending virtio operation. But nothing
>>> really breaks or goes wrong if we don't do that.
>> Yes, maybe not S1/S3, I remember you want to manage power in PCI
>> transport when suspend in a cap, S1/S3 is just an example.
>>
>>>>>>> ---
>>>>>>>     content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>>>>     1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>> index 0a62dce..3d656b5 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)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>>>> +  device has been suspended by the driver.
>>>>>>>     \end{description}
>>>>>>>
>>>>>>>     The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>>>> @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>>     recover by issuing a reset.
>>>>>>>     \end{note}
>>>>>>>
>>>>>>> +The driver SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>>>> +
>>>>>>> +When setting SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
>>>>>>> +
>>>>>>>     \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 +89,25 @@ \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.
>>>>>>>
>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set.
>>>>>>> +
>>>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>>>> +
>>>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>>>> +
>>>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>>>> +and resumes operation upon DRIVER_OK.
>>>>>>> +
>>>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>>>> +
>>>>>>> +\begin{itemize}
>>>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>>>> What's the difference between the previous three lines? They might be
>>>>> trying to say different things, but there is a lot of overlap in how
>>>>> they're phrased. That makes it hard to figure out exactly what they're
>>>>> mandating. Is it something like: "stop processing new buffers",
>>>>> "finish processing any buffers that are currently in flight", "mark
>>>>> all finished buffers as used and send a used buffer notification"?
>>>> sure, the "mark used" parts can merge.
>>>>> Also, what does this mean for devices where the driver places empty
>>>>> buffers into the virtqueue that the device then holds on to for an
>>>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>>>> etc)?
>>>> The device doesn't process them.
>>>> Oh, I should add: The driver should not make any more buffers available
>>>> to the device.
>>>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>>> What does "Pause its operation except device status" mean? The word
>>>>> "operation" is vague, what exactly does it include? Also what does
>>>>> "operate its device status" mean?
>>>> Because we need to resume the device after suspending it by setting
>>>> DRIVER_OK, so we need the device status
>>>> filed keel alive.
>>> This doesn't answer what "Pause its operation" means. If the
>>> specification is not precise, then the best that can be implemented is
>>> for a specific device to be compatible with a specific driver. If we
>>> actually want portable, spec-compliant devices, we need explicitly
>>> defined MUST/MUST NOT requirements. Presumably we want something like:
>>>
>>> A device MUST not send notifications while suspended.
>> The device should send a last used buffer notification if
>> it has buffers under processing before presenting SUSPEND
>> in the device status, but the device should not process
>> any new buffers. These are in devicenormative section,
>> but they may be vague, I can add more details to make
>> it precise, thanks for the tips
>>> A device MUST ignore writes to its device configuration while
>>> suspended, except for writes to the device status byte.
>>> A driver MUST NOT write to a suspended device's configuration space,
>>> except for the device status byte.
>> sure, we can make it more precise than only pausing.
>>> We also need to specify what a suspended device is allowed to do with
>>> buffers it owns. For my use case of just suspending/resuming a VM, I
>>> don't think it particularly matters if a suspended device writes to
>>> buffers or descriptors while it is suspended. But based on what you
>>> wrote about finishing any in-flight processing of buffers and not
>>> processing any empty buffers, presumably you would prefer that the
>>> device not be allowed to access buffer/descriptors while it is
>>> suspended? That approach is probably cleaner from a pure specification
>>> standpoint, but it's also more restrictive for devices and thus harder
>>> to properly implement.
>> When the driver sets SUSPEND, the device should finished all buffers that
>> already under processing, include the in-flight buffers(or it may lose the
>> buffers).
>> And the device should not touch the buffers once it present SUSPEND in the
>> device status field, because once it presents SUSPEND, the device state
>> should
>> be stable, people may use SUSPEND to debug the device.
>>
>> The device still can process the buffer before it presenting SUSPEND.
>>
>> Thanks
>> Zhu Lingshan
>>> -David


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