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

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?
>
> 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.
>
>>> ---
>>>   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.

Thanks,
Zhu Lingshan
>
> -David


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