[PATCH v5] virtio: introduce SUSPEND bit in device status

[PATCH v5] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan]

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.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Signed-off-by: David Stevens <stevensd@chromium.org>
---
 content.tex | 69 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 59 insertions(+), 10 deletions(-)

diff --git a/content.tex b/content.tex
index 0a62dce..350b815 100644
--- a/content.tex
+++ b/content.tex
@@ -36,19 +36,22 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
     this bit.  For example, under Linux, drivers can be loadable modules.
   \end{note}
 
-\item[FAILED (128)] Indicates that something went wrong in the guest,
-  and it has given up on the device. This could be an internal
-  error, or the driver didn't like the device for some reason, or
-  even a fatal error during device operation.
+\item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
+  drive the device.
 
 \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
   features it understands, and feature negotiation is complete.
 
-\item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
-  drive the device.
+\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
+  device has been suspended by the driver.
 
 \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
   an error from which it can't recover.
+
+\item[FAILED (128)] Indicates that something went wrong in the guest,
+  and it has given up on the device. This could be an internal
+  error, or the driver didn't like the device for some reason, or
+  even a fatal error during device operation.
 \end{description}
 
 The \field{device status} field starts out as 0, and is reinitialized to 0 by
@@ -60,8 +63,9 @@ \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 NOT clear a \field{device status} bit other than SUSPEND
+except when setting \field{device status} to 0 as a transport-specific way to
+initiate a reset. If the driver sets the FAILED bit,
 the driver MUST later reset the device before attempting to re-initialize.
 
 The driver SHOULD NOT rely on completion of operations of a
@@ -99,10 +103,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 +633,47 @@ \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 VIRTIO_F_SUSPEND is negotiated, the driver can set the
+SUSPEND bit in \field{device status} to suspend a device, and can
+clear the SUSPEND bit to resume a suspended device.
+
+\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
+
+The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
+
+Once the driver sets SUSPEND to \field{device status} of the device:
+\begin{itemize}
+\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
+If not, the device does not support the SUSPEND feature.
+\item The driver MUST NOT make any more buffers available to the device.
+\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.
+\item The driver MUST NOT access Device Configuration Space.
+\end{itemize}
+
+\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
+
+The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
+
+The device MUST ignore all access to its Configuration Space while
+suspended, except for \field{device status}.
+
+A device MUST NOT send any notifications, access any virtqueues, or modify
+any fields in its configuration space while suspended.
+
+If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
+the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
+
+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 processing more buffers of any virtqueues
+\item Wait until all buffers that are being processed have been used.
+\item Send used buffer notifications to the driver.
+\end{itemize}
+
 \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
 
 Virtio can use various different buses, thus the standard is split
@@ -872,6 +917,10 @@ \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
+   set SUSPEND to the device.
+   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
2.45.1

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

[Cornelia Huck]

On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  content.tex | 69 +++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 59 insertions(+), 10 deletions(-)

Can you please add a changelog? Especially as some of the previous
discussion has been lost due to the broken old mailing lists...

(...)

> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> +
> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> +
> +Once the driver sets SUSPEND to \field{device status} of the device:
> +\begin{itemize}
> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> +If not, the device does not support the SUSPEND feature.

That sentence is a bit weird: I'd expect the device to not offer
VIRTIO_F_SUSPEND in the first place in that case... could this rather
happen if the device is not able to handle the request at a specific
point in time?

> +\item The driver MUST NOT make any more buffers available to the device.
> +\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.

"send notifications for any virtqueues"?

> +\item The driver MUST NOT access Device Configuration Space.

...except for the status field, if it is part of the config space?

> +\end{itemize}
> +
> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> +
> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> +
> +The device MUST ignore all access to its Configuration Space while
> +suspended, except for \field{device status}.

...if it is part of the configuration space.

> +
> +A device MUST NOT send any notifications, access any virtqueues, or modify
> +any fields in its configuration space while suspended.
> +
> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,

"subsequently clears SUSPEND"?

> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
> +
> +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 processing more buffers of any virtqueues
> +\item Wait until all buffers that are being processed have been used.
> +\item Send used buffer notifications to the driver.
> +\end{itemize}

So, is there any opportunity for the device to fail setting SUSPEND? I
mean, if the driver is supposed to look whether it sticks, there should
be conditions for when the device might clear it again...

> +
>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>  
>  Virtio can use various different buses, thus the standard is split
> @@ -872,6 +917,10 @@ \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
> +   set SUSPEND to the device.

"that the driver can trigger suspending the device via the SUSPEND flag"?

> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> +
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

Hi Lingshan, David,

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, June 10, 2024 9:34 PM
> 
> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> > ---
> >  content.tex | 69
> > +++++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 59 insertions(+), 10 deletions(-)
> 
> Can you please add a changelog? Especially as some of the previous discussion
> has been lost due to the broken old mailing lists...
> 
> (...)
> 
> > +\drivernormative{\subsection}{Device Suspend}{General Initialization
> > +And Device Operation / Device Suspend}
> > +
> > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
> VIRTIO_F_SUSPEND is not negotiated.
> > +
> > +Once the driver sets SUSPEND to \field{device status} of the device:
> > +\begin{itemize}
> > +\item The driver MUST re-read \field{device status} to verify whether the
> SUSPEND bit is set.
> > +If not, the device does not support the SUSPEND feature.
> 
> That sentence is a bit weird: I'd expect the device to not offer
> VIRTIO_F_SUSPEND in the first place in that case... could this rather happen if
> the device is not able to handle the request at a specific point in time?
> 
> > +\item The driver MUST NOT make any more buffers available to the device.
> > +\item The driver MUST NOT access any fields of any virtqueues or notify
> any virtqueues.
> 
> "send notifications for any virtqueues"?
> 
> > +\item The driver MUST NOT access Device Configuration Space.
> 
> ...except for the status field, if it is part of the config space?
> 
> > +\end{itemize}
> > +
> > +\devicenormative{\subsection}{Device Suspend}{General Initialization
> > +And Device Operation / Device Suspend}
> > +
> > +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> VIRTIO_F_SUSPEND is not negotiated.
> > +
> > +The device MUST ignore all access to its Configuration Space while
> > +suspended, except for \field{device status}.
> 
> ...if it is part of the configuration space.
> 
> > +
> > +A device MUST NOT send any notifications, access any virtqueues, or
> > +modify any fields in its configuration space while suspended.
> > +
> > +If SUSPEND is set in \field{device status}, when the driver clears
> > +SUSPEND,
> 
> "subsequently clears SUSPEND"?
> 
> > +the device MUST either resume normal operation or set
> DEVICE_NEEDS_RESET.
> > +
> > +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 processing more buffers of any virtqueues \item Wait until
> > +all buffers that are being processed have been used.
> > +\item Send used buffer notifications to the driver.
> > +\end{itemize}
> 
> So, is there any opportunity for the device to fail setting SUSPEND? I mean, if
> the driver is supposed to look whether it sticks, there should be conditions for
> when the device might clear it again...
> 
Additionally, a suspend operation usually involves saving things to a slow memory (or media).
This is because the device implementation wouldn’t know when exactly the device will be resumed.
Few examples, are:
a. A gpu device with 128MB of video RAM when suspended, QEMU needs to store this into a (for example) rotating hard disk as 1msec IO latency.

b. a NIC may need to store its RSS, queues, flow filters configuration for several tens of KBs to some slow memory.

c. A block device may prefer to complete some IOs to a threshold level instead of maintaining large list of outstanding IOs in some suspended memory.

d. May be more in future.

Additionally, suspend operation needs to be synchronized with certain data path hardware engines to suspend DMA operations (read and write both) which are inflight.
(without causing DMA errors). Same would apply to the sw backend implementations too.

Therefore, the suspend operation that is initiated by the driver, should get the acknowledgement back from the device that it has been suspend.

Some of the good examples if you prefer to follow, a driver<->device interface needs a suspend register which should behave like below queue reset register.

Spec snippet:
"The device MUST reset the queue when 1 is written to queue_reset. The device MUST continue to present
1 in queue_reset as long as the queue reset is ongoing. The device MUST present 0 in both queue_reset
and queue_enable when queue reset has completed."

At minimum, we need, something implementable like,

The device MUST suspend the device when 1 is written to the device_suspend_ctlr. The device MUST continue to present 1 in device_suspend_ctrl as the suspend operation is ongoing in the device.
The device MUST present 0 in the device_suspend_ctlr register when device has completely suspended the device.

The device MUST resume the device when 2 is written to the device_suspend_ctrl. The device MUST continue to present 2 in the device_suspend_ctrl as the resume operation may not have yet completed.
The device MUST present 0 in the device_suspend_ctrl register when the device has completely resumed the device.
At this point, the driver may resume notifying the device and accessing the configuration space.

Can you please enhance this part?


> > +
> >  \chapter{Virtio Transport Options}\label{sec:Virtio Transport
> > Options}
> >
> >  Virtio can use various different buses, thus the standard is split @@
> > -872,6 +917,10 @@ \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
> > +   set SUSPEND to the device.
> 
> "that the driver can trigger suspending the device via the SUSPEND flag"?
> 
> > +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> > +
> >  \end{description}
> >
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
> > Bits}
> 

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

[Zhu Lingshan]

On 6/11/2024 12:04 AM, Cornelia Huck wrote:
> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> Signed-off-by: David Stevens <stevensd@chromium.org>
>> ---
>>  content.tex | 69 +++++++++++++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 59 insertions(+), 10 deletions(-)
> Can you please add a changelog? Especially as some of the previous
> discussion has been lost due to the broken old mailing lists...
>
> (...)
will add change log in next version.
>
>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +Once the driver sets SUSPEND to \field{device status} of the device:
>> +\begin{itemize}
>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
>> +If not, the device does not support the SUSPEND feature.
> That sentence is a bit weird: I'd expect the device to not offer
> VIRTIO_F_SUSPEND in the first place in that case... could this rather
> happen if the device is not able to handle the request at a specific
> point in time?
Yes, this can be improved

How about:
If not, this indicating the device is not able to handle SUSPEND at the moment.
>
>> +\item The driver MUST NOT make any more buffers available to the device.
>> +\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.
> "send notifications for any virtqueues"?
OK
>
>> +\item The driver MUST NOT access Device Configuration Space.
> ...except for the status field, if it is part of the config space?
Yes, that is the original design, then I am convinced that the status fields is
not a part of the config space because it has a dedicated section in the spec.

I will add this sentence in the next version.
>
>> +\end{itemize}
>> +
>> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>> +
>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>> +
>> +The device MUST ignore all access to its Configuration Space while
>> +suspended, except for \field{device status}.
> ...if it is part of the configuration space.
Yes
>
>> +
>> +A device MUST NOT send any notifications, access any virtqueues, or modify
>> +any fields in its configuration space while suspended.
>> +
>> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
> "subsequently clears SUSPEND"?
I am a native speaker, but "subsequently" sounds like clearing SUSPEND right after "setting SUSPEND", I guess there can be
some operations during SUSPEND.

>
>> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
>> +
>> +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 processing more buffers of any virtqueues
>> +\item Wait until all buffers that are being processed have been used.
>> +\item Send used buffer notifications to the driver.
>> +\end{itemize}
> So, is there any opportunity for the device to fail setting SUSPEND? I
> mean, if the driver is supposed to look whether it sticks, there should
> be conditions for when the device might clear it again...
The device should not present SUSPEND bit in the device status until it is suspended,
so I am afraid there is not a SUSPEND bit to clear if the device fails to suspend itself.

I think there can be two indications of a failed suspend:
1) The driver sets SUSPEND, but the device does not present SUSPEND after a while, depends
on the driver tolerance, the driver may assume it is a failed SUSPEND, like how we handle FEATURES_OK
2)The device runs into errors, presenting NEEDS_RESET.


>
>> +
>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>  
>>  Virtio can use various different buses, thus the standard is split
>> @@ -872,6 +917,10 @@ \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
>> +   set SUSPEND to the device.
> "that the driver can trigger suspending the device via the SUSPEND flag"?
Yes
>
>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>> +
>>  \end{description}
>>  
>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

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

[Zhu Lingshan]

On 6/11/2024 1:17 PM, Parav Pandit wrote:
> Hi Lingshan, David,
>
>> From: Cornelia Huck <cohuck@redhat.com>
>> Sent: Monday, June 10, 2024 9:34 PM
>>
>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> ---
>>>  content.tex | 69
>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>> Can you please add a changelog? Especially as some of the previous discussion
>> has been lost due to the broken old mailing lists...
>>
>> (...)
>>
>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization
>>> +And Device Operation / Device Suspend}
>>> +
>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
>> VIRTIO_F_SUSPEND is not negotiated.
>>> +
>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>> +\begin{itemize}
>>> +\item The driver MUST re-read \field{device status} to verify whether the
>> SUSPEND bit is set.
>>> +If not, the device does not support the SUSPEND feature.
>> That sentence is a bit weird: I'd expect the device to not offer
>> VIRTIO_F_SUSPEND in the first place in that case... could this rather happen if
>> the device is not able to handle the request at a specific point in time?
>>
>>> +\item The driver MUST NOT make any more buffers available to the device.
>>> +\item The driver MUST NOT access any fields of any virtqueues or notify
>> any virtqueues.
>>
>> "send notifications for any virtqueues"?
>>
>>> +\item The driver MUST NOT access Device Configuration Space.
>> ...except for the status field, if it is part of the config space?
>>
>>> +\end{itemize}
>>> +
>>> +\devicenormative{\subsection}{Device Suspend}{General Initialization
>>> +And Device Operation / Device Suspend}
>>> +
>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
>> VIRTIO_F_SUSPEND is not negotiated.
>>> +
>>> +The device MUST ignore all access to its Configuration Space while
>>> +suspended, except for \field{device status}.
>> ...if it is part of the configuration space.
>>
>>> +
>>> +A device MUST NOT send any notifications, access any virtqueues, or
>>> +modify any fields in its configuration space while suspended.
>>> +
>>> +If SUSPEND is set in \field{device status}, when the driver clears
>>> +SUSPEND,
>> "subsequently clears SUSPEND"?
>>
>>> +the device MUST either resume normal operation or set
>> DEVICE_NEEDS_RESET.
>>> +
>>> +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 processing more buffers of any virtqueues \item Wait until
>>> +all buffers that are being processed have been used.
>>> +\item Send used buffer notifications to the driver.
>>> +\end{itemize}
>> So, is there any opportunity for the device to fail setting SUSPEND? I mean, if
>> the driver is supposed to look whether it sticks, there should be conditions for
>> when the device might clear it again...
>>
> Additionally, a suspend operation usually involves saving things to a slow memory (or media).
> This is because the device implementation wouldn’t know when exactly the device will be resumed.
> Few examples, are:
> a. A gpu device with 128MB of video RAM when suspended, QEMU needs to store this into a (for example) rotating hard disk as 1msec IO latency.
>
> b. a NIC may need to store its RSS, queues, flow filters configuration for several tens of KBs to some slow memory.
>
> c. A block device may prefer to complete some IOs to a threshold level instead of maintaining large list of outstanding IOs in some suspended memory.
>
> d. May be more in future.
>
> Additionally, suspend operation needs to be synchronized with certain data path hardware engines to suspend DMA operations (read and write both) which are inflight.
> (without causing DMA errors). Same would apply to the sw backend implementations too.
>
> Therefore, the suspend operation that is initiated by the driver, should get the acknowledgement back from the device that it has been suspend.
>
> Some of the good examples if you prefer to follow, a driver<->device interface needs a suspend register which should behave like below queue reset register.
>
> Spec snippet:
> "The device MUST reset the queue when 1 is written to queue_reset. The device MUST continue to present
> 1 in queue_reset as long as the queue reset is ongoing. The device MUST present 0 in both queue_reset
> and queue_enable when queue reset has completed."
>
> At minimum, we need, something implementable like,
>
> The device MUST suspend the device when 1 is written to the device_suspend_ctlr. The device MUST continue to present 1 in device_suspend_ctrl as the suspend operation is ongoing in the device.
> The device MUST present 0 in the device_suspend_ctlr register when device has completely suspended the device.
>
> The device MUST resume the device when 2 is written to the device_suspend_ctrl. The device MUST continue to present 2 in the device_suspend_ctrl as the resume operation may not have yet completed.
> The device MUST present 0 in the device_suspend_ctrl register when the device has completely resumed the device.
> At this point, the driver may resume notifying the device and accessing the configuration space.
>
> Can you please enhance this part?
Hi Parav

Yes they are necessary contents and we will address them in the following patches.

This patch focus on the basic facility virito suspending, not transport or device types, this is
to make this chageset focused and small.

Thanks,
Zhu Linghsan
>
>
>>> +
>>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport
>>> Options}
>>>
>>>  Virtio can use various different buses, thus the standard is split @@
>>> -872,6 +917,10 @@ \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
>>> +   set SUSPEND to the device.
>> "that the driver can trigger suspending the device via the SUSPEND flag"?
>>
>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>>> +
>>>  \end{description}
>>>
>>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
>>> Bits}

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Tuesday, June 11, 2024 1:57 PM
> 
> On 6/11/2024 1:17 PM, Parav Pandit wrote:
> > Hi Lingshan, David,
> >
> >> From: Cornelia Huck <cohuck@redhat.com>
> >> Sent: Monday, June 10, 2024 9:34 PM
> >>
> >> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >>>
> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>> ---
> >>>  content.tex | 69
> >>> +++++++++++++++++++++++++++++++++++++++++++++--------
> >>>  1 file changed, 59 insertions(+), 10 deletions(-)
> >> Can you please add a changelog? Especially as some of the previous
> >> discussion has been lost due to the broken old mailing lists...
> >>
> >> (...)
> >>
> >>> +\drivernormative{\subsection}{Device Suspend}{General
> >>> +Initialization And Device Operation / Device Suspend}
> >>> +
> >>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
> >> VIRTIO_F_SUSPEND is not negotiated.
> >>> +
> >>> +Once the driver sets SUSPEND to \field{device status} of the device:
> >>> +\begin{itemize}
> >>> +\item The driver MUST re-read \field{device status} to verify
> >>> +whether the
> >> SUSPEND bit is set.
> >>> +If not, the device does not support the SUSPEND feature.
> >> That sentence is a bit weird: I'd expect the device to not offer
> >> VIRTIO_F_SUSPEND in the first place in that case... could this rather
> >> happen if the device is not able to handle the request at a specific point in
> time?
> >>
> >>> +\item The driver MUST NOT make any more buffers available to the
> device.
> >>> +\item The driver MUST NOT access any fields of any virtqueues or
> >>> +notify
> >> any virtqueues.
> >>
> >> "send notifications for any virtqueues"?
> >>
> >>> +\item The driver MUST NOT access Device Configuration Space.
> >> ...except for the status field, if it is part of the config space?
> >>
> >>> +\end{itemize}
> >>> +
> >>> +\devicenormative{\subsection}{Device Suspend}{General
> >>> +Initialization And Device Operation / Device Suspend}
> >>> +
> >>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> >> VIRTIO_F_SUSPEND is not negotiated.
> >>> +
> >>> +The device MUST ignore all access to its Configuration Space while
> >>> +suspended, except for \field{device status}.
> >> ...if it is part of the configuration space.
> >>
> >>> +
> >>> +A device MUST NOT send any notifications, access any virtqueues, or
> >>> +modify any fields in its configuration space while suspended.
> >>> +
> >>> +If SUSPEND is set in \field{device status}, when the driver clears
> >>> +SUSPEND,
> >> "subsequently clears SUSPEND"?
> >>
> >>> +the device MUST either resume normal operation or set
> >> DEVICE_NEEDS_RESET.
> >>> +
> >>> +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 processing more buffers of any virtqueues \item Wait
> >>> +until all buffers that are being processed have been used.
> >>> +\item Send used buffer notifications to the driver.
> >>> +\end{itemize}
> >> So, is there any opportunity for the device to fail setting SUSPEND?
> >> I mean, if the driver is supposed to look whether it sticks, there
> >> should be conditions for when the device might clear it again...
> >>
> > Additionally, a suspend operation usually involves saving things to a slow
> memory (or media).
> > This is because the device implementation wouldn’t know when exactly the
> device will be resumed.
> > Few examples, are:
> > a. A gpu device with 128MB of video RAM when suspended, QEMU needs
> to store this into a (for example) rotating hard disk as 1msec IO latency.
> >
> > b. a NIC may need to store its RSS, queues, flow filters configuration for
> several tens of KBs to some slow memory.
> >
> > c. A block device may prefer to complete some IOs to a threshold level
> instead of maintaining large list of outstanding IOs in some suspended
> memory.
> >
> > d. May be more in future.
> >
> > Additionally, suspend operation needs to be synchronized with certain data
> path hardware engines to suspend DMA operations (read and write both)
> which are inflight.
> > (without causing DMA errors). Same would apply to the sw backend
> implementations too.
> >
> > Therefore, the suspend operation that is initiated by the driver, should get
> the acknowledgement back from the device that it has been suspend.
> >
> > Some of the good examples if you prefer to follow, a driver<->device
> interface needs a suspend register which should behave like below queue
> reset register.
> >
> > Spec snippet:
> > "The device MUST reset the queue when 1 is written to queue_reset. The
> > device MUST continue to present
> > 1 in queue_reset as long as the queue reset is ongoing. The device
> > MUST present 0 in both queue_reset and queue_enable when queue reset
> has completed."
> >
> > At minimum, we need, something implementable like,
> >
> > The device MUST suspend the device when 1 is written to the
> device_suspend_ctlr. The device MUST continue to present 1 in
> device_suspend_ctrl as the suspend operation is ongoing in the device.
> > The device MUST present 0 in the device_suspend_ctlr register when
> device has completely suspended the device.
> >
> > The device MUST resume the device when 2 is written to the
> device_suspend_ctrl. The device MUST continue to present 2 in the
> device_suspend_ctrl as the resume operation may not have yet completed.
> > The device MUST present 0 in the device_suspend_ctrl register when the
> device has completely resumed the device.
> > At this point, the driver may resume notifying the device and accessing the
> configuration space.
> >
> > Can you please enhance this part?
> Hi Parav
> 
> Yes they are necessary contents and we will address them in the following
> patches.
> 
The register polarity is critical.
In your reply to Cornelia, you descried that suspend bit cleared in the device after driver sets it (until the device is suspended).
This is ambiguous behavior of the register. Queue_reset register was like that before it was fixed in 1.2.

> This patch focus on the basic facility virito suspending, not transport or
> device types, this is to make this chageset focused and small.
>
If suspend bit is part of the device_status, it is already covered by the PCI transport.
So it is fine, but its polarity is an issue, that we need to fix.

i.e.
One driver writes 1, it needs to stay 1, while the suspend is ongoing.
When suspension is completed in the device, the device to turn it to zero.
 
If the device_status register does not fullfil this, a new register is worthwhile to add in this patch.

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

[Zhu Lingshan]

On 6/11/2024 4:48 PM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Tuesday, June 11, 2024 1:57 PM
>>
>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
>>> Hi Lingshan, David,
>>>
>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>> Sent: Monday, June 10, 2024 9:34 PM
>>>>
>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>> ---
>>>>>  content.tex | 69
>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>>> Can you please add a changelog? Especially as some of the previous
>>>> discussion has been lost due to the broken old mailing lists...
>>>>
>>>> (...)
>>>>
>>>>> +\drivernormative{\subsection}{Device Suspend}{General
>>>>> +Initialization And Device Operation / Device Suspend}
>>>>> +
>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>> +
>>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>>> +\begin{itemize}
>>>>> +\item The driver MUST re-read \field{device status} to verify
>>>>> +whether the
>>>> SUSPEND bit is set.
>>>>> +If not, the device does not support the SUSPEND feature.
>>>> That sentence is a bit weird: I'd expect the device to not offer
>>>> VIRTIO_F_SUSPEND in the first place in that case... could this rather
>>>> happen if the device is not able to handle the request at a specific point in
>> time?
>>>>> +\item The driver MUST NOT make any more buffers available to the
>> device.
>>>>> +\item The driver MUST NOT access any fields of any virtqueues or
>>>>> +notify
>>>> any virtqueues.
>>>>
>>>> "send notifications for any virtqueues"?
>>>>
>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>> ...except for the status field, if it is part of the config space?
>>>>
>>>>> +\end{itemize}
>>>>> +
>>>>> +\devicenormative{\subsection}{Device Suspend}{General
>>>>> +Initialization And Device Operation / Device Suspend}
>>>>> +
>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>> +
>>>>> +The device MUST ignore all access to its Configuration Space while
>>>>> +suspended, except for \field{device status}.
>>>> ...if it is part of the configuration space.
>>>>
>>>>> +
>>>>> +A device MUST NOT send any notifications, access any virtqueues, or
>>>>> +modify any fields in its configuration space while suspended.
>>>>> +
>>>>> +If SUSPEND is set in \field{device status}, when the driver clears
>>>>> +SUSPEND,
>>>> "subsequently clears SUSPEND"?
>>>>
>>>>> +the device MUST either resume normal operation or set
>>>> DEVICE_NEEDS_RESET.
>>>>> +
>>>>> +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 processing more buffers of any virtqueues \item Wait
>>>>> +until all buffers that are being processed have been used.
>>>>> +\item Send used buffer notifications to the driver.
>>>>> +\end{itemize}
>>>> So, is there any opportunity for the device to fail setting SUSPEND?
>>>> I mean, if the driver is supposed to look whether it sticks, there
>>>> should be conditions for when the device might clear it again...
>>>>
>>> Additionally, a suspend operation usually involves saving things to a slow
>> memory (or media).
>>> This is because the device implementation wouldn’t know when exactly the
>> device will be resumed.
>>> Few examples, are:
>>> a. A gpu device with 128MB of video RAM when suspended, QEMU needs
>> to store this into a (for example) rotating hard disk as 1msec IO latency.
>>> b. a NIC may need to store its RSS, queues, flow filters configuration for
>> several tens of KBs to some slow memory.
>>> c. A block device may prefer to complete some IOs to a threshold level
>> instead of maintaining large list of outstanding IOs in some suspended
>> memory.
>>> d. May be more in future.
>>>
>>> Additionally, suspend operation needs to be synchronized with certain data
>> path hardware engines to suspend DMA operations (read and write both)
>> which are inflight.
>>> (without causing DMA errors). Same would apply to the sw backend
>> implementations too.
>>> Therefore, the suspend operation that is initiated by the driver, should get
>> the acknowledgement back from the device that it has been suspend.
>>> Some of the good examples if you prefer to follow, a driver<->device
>> interface needs a suspend register which should behave like below queue
>> reset register.
>>> Spec snippet:
>>> "The device MUST reset the queue when 1 is written to queue_reset. The
>>> device MUST continue to present
>>> 1 in queue_reset as long as the queue reset is ongoing. The device
>>> MUST present 0 in both queue_reset and queue_enable when queue reset
>> has completed."
>>> At minimum, we need, something implementable like,
>>>
>>> The device MUST suspend the device when 1 is written to the
>> device_suspend_ctlr. The device MUST continue to present 1 in
>> device_suspend_ctrl as the suspend operation is ongoing in the device.
>>> The device MUST present 0 in the device_suspend_ctlr register when
>> device has completely suspended the device.
>>> The device MUST resume the device when 2 is written to the
>> device_suspend_ctrl. The device MUST continue to present 2 in the
>> device_suspend_ctrl as the resume operation may not have yet completed.
>>> The device MUST present 0 in the device_suspend_ctrl register when the
>> device has completely resumed the device.
>>> At this point, the driver may resume notifying the device and accessing the
>> configuration space.
>>> Can you please enhance this part?
>> Hi Parav
>>
>> Yes they are necessary contents and we will address them in the following
>> patches.
>>
> The register polarity is critical.
> In your reply to Cornelia, you descried that suspend bit cleared in the device after driver sets it (until the device is suspended).
> This is ambiguous behavior of the register. Queue_reset register was like that before it was fixed in 1.2.
once suspended, the driver should not access the configuration space except for the device status, so it should not reset any vqs through
configuration space.
>
>> This patch focus on the basic facility virito suspending, not transport or
>> device types, this is to make this chageset focused and small.
>>
> If suspend bit is part of the device_status, it is already covered by the PCI transport.
> So it is fine, but its polarity is an issue, that we need to fix.
Oh, I see your confusion.
All transport have their own implementation of configuration space, this is not PCI specific.
PCI config space != virtio configuration space. And this patch focuses on virtio layer.
>
> i.e.
> One driver writes 1, it needs to stay 1, while the suspend is ongoing.
> When suspension is completed in the device, the device to turn it to zero.
>  
> If the device_status register does not fullfil this, a new register is worthwhile to add in this patch.
The driver writes SUSPEND, once the device is suspened, it should present SUSPEND in its device status.

Thanks
Zhu Lingshan
>

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Tuesday, June 11, 2024 3:03 PM
> 
> On 6/11/2024 4:48 PM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Tuesday, June 11, 2024 1:57 PM
> >>
> >> On 6/11/2024 1:17 PM, Parav Pandit wrote:
> >>> Hi Lingshan, David,
> >>>
> >>>> From: Cornelia Huck <cohuck@redhat.com>
> >>>> Sent: Monday, June 10, 2024 9:34 PM
> >>>>
> >>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >>>>>
> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>>>> ---
> >>>>>  content.tex | 69
> >>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
> >>>> Can you please add a changelog? Especially as some of the previous
> >>>> discussion has been lost due to the broken old mailing lists...
> >>>>
> >>>> (...)
> >>>>
> >>>>> +\drivernormative{\subsection}{Device Suspend}{General
> >>>>> +Initialization And Device Operation / Device Suspend}
> >>>>> +
> >>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
> >>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>> +
> >>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
> >>>>> +\begin{itemize}
> >>>>> +\item The driver MUST re-read \field{device status} to verify
> >>>>> +whether the
> >>>> SUSPEND bit is set.
> >>>>> +If not, the device does not support the SUSPEND feature.
> >>>> That sentence is a bit weird: I'd expect the device to not offer
> >>>> VIRTIO_F_SUSPEND in the first place in that case... could this
> >>>> rather happen if the device is not able to handle the request at a
> >>>> specific point in
> >> time?
> >>>>> +\item The driver MUST NOT make any more buffers available to the
> >> device.
> >>>>> +\item The driver MUST NOT access any fields of any virtqueues or
> >>>>> +notify
> >>>> any virtqueues.
> >>>>
> >>>> "send notifications for any virtqueues"?
> >>>>
> >>>>> +\item The driver MUST NOT access Device Configuration Space.
> >>>> ...except for the status field, if it is part of the config space?
> >>>>
> >>>>> +\end{itemize}
> >>>>> +
> >>>>> +\devicenormative{\subsection}{Device Suspend}{General
> >>>>> +Initialization And Device Operation / Device Suspend}
> >>>>> +
> >>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> >>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>> +
> >>>>> +The device MUST ignore all access to its Configuration Space
> >>>>> +while suspended, except for \field{device status}.
> >>>> ...if it is part of the configuration space.
> >>>>
> >>>>> +
> >>>>> +A device MUST NOT send any notifications, access any virtqueues,
> >>>>> +or modify any fields in its configuration space while suspended.
> >>>>> +
> >>>>> +If SUSPEND is set in \field{device status}, when the driver
> >>>>> +clears SUSPEND,
> >>>> "subsequently clears SUSPEND"?
> >>>>
> >>>>> +the device MUST either resume normal operation or set
> >>>> DEVICE_NEEDS_RESET.
> >>>>> +
> >>>>> +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 processing more buffers of any virtqueues \item Wait
> >>>>> +until all buffers that are being processed have been used.
> >>>>> +\item Send used buffer notifications to the driver.
> >>>>> +\end{itemize}
> >>>> So, is there any opportunity for the device to fail setting SUSPEND?
> >>>> I mean, if the driver is supposed to look whether it sticks, there
> >>>> should be conditions for when the device might clear it again...
> >>>>
> >>> Additionally, a suspend operation usually involves saving things to
> >>> a slow
> >> memory (or media).
> >>> This is because the device implementation wouldn’t know when exactly
> >>> the
> >> device will be resumed.
> >>> Few examples, are:
> >>> a. A gpu device with 128MB of video RAM when suspended, QEMU needs
> >> to store this into a (for example) rotating hard disk as 1msec IO latency.
> >>> b. a NIC may need to store its RSS, queues, flow filters
> >>> configuration for
> >> several tens of KBs to some slow memory.
> >>> c. A block device may prefer to complete some IOs to a threshold
> >>> level
> >> instead of maintaining large list of outstanding IOs in some
> >> suspended memory.
> >>> d. May be more in future.
> >>>
> >>> Additionally, suspend operation needs to be synchronized with
> >>> certain data
> >> path hardware engines to suspend DMA operations (read and write both)
> >> which are inflight.
> >>> (without causing DMA errors). Same would apply to the sw backend
> >> implementations too.
> >>> Therefore, the suspend operation that is initiated by the driver,
> >>> should get
> >> the acknowledgement back from the device that it has been suspend.
> >>> Some of the good examples if you prefer to follow, a driver<->device
> >> interface needs a suspend register which should behave like below
> >> queue reset register.
> >>> Spec snippet:
> >>> "The device MUST reset the queue when 1 is written to queue_reset.
> >>> The device MUST continue to present
> >>> 1 in queue_reset as long as the queue reset is ongoing. The device
> >>> MUST present 0 in both queue_reset and queue_enable when queue reset
> >> has completed."
> >>> At minimum, we need, something implementable like,
> >>>
> >>> The device MUST suspend the device when 1 is written to the
> >> device_suspend_ctlr. The device MUST continue to present 1 in
> >> device_suspend_ctrl as the suspend operation is ongoing in the device.
> >>> The device MUST present 0 in the device_suspend_ctlr register when
> >> device has completely suspended the device.
> >>> The device MUST resume the device when 2 is written to the
> >> device_suspend_ctrl. The device MUST continue to present 2 in the
> >> device_suspend_ctrl as the resume operation may not have yet completed.
> >>> The device MUST present 0 in the device_suspend_ctrl register when
> >>> the
> >> device has completely resumed the device.
> >>> At this point, the driver may resume notifying the device and
> >>> accessing the
> >> configuration space.
> >>> Can you please enhance this part?
> >> Hi Parav
> >>
> >> Yes they are necessary contents and we will address them in the
> >> following patches.
> >>
> > The register polarity is critical.
> > In your reply to Cornelia, you descried that suspend bit cleared in the device
> after driver sets it (until the device is suspended).
> > This is ambiguous behavior of the register. Queue_reset register was like that
> before it was fixed in 1.2.
> once suspended, the driver should not access the configuration space except
> for the device status, so it should not reset any vqs through configuration
> space.
I am not talking about VQ reset at all.
I gave an example of how the VQ reset register polarity works (with was broken like how the proposed suspend bit is broken).

Suspend register needs to work the way the reset register works.
Can you please go through my exact text I replied in the previous email?

Basically, the bug is:
When driver has initiated the suspend (writing 1), the device continue to respond 0, while suspend is going on.

At this point, just by looking at the device_status register one cannot figure out what is going on in the device.
Is driver initiated suspend is ongoing, or it was never started.
It is ambiguous.

Hence, it needs to work like how the queue_reset works.
i.e. if suspend is ongoing, one should be able to read device_status without involving driver and understand that it is ongoing.

> >
> >> This patch focus on the basic facility virito suspending, not
> >> transport or device types, this is to make this chageset focused and small.
> >>
> > If suspend bit is part of the device_status, it is already covered by the PCI
> transport.
> > So it is fine, but its polarity is an issue, that we need to fix.
> Oh, I see your confusion.
> All transport have their own implementation of configuration space, this is not
> PCI specific.
> PCI config space != virtio configuration space. And this patch focuses on virtio
> layer.
> >
> > i.e.
> > One driver writes 1, it needs to stay 1, while the suspend is ongoing.
> > When suspension is completed in the device, the device to turn it to zero.
> >
> > If the device_status register does not fullfil this, a new register is worthwhile
> to add in this patch.
> The driver writes SUSPEND, once the device is suspened, it should present
> SUSPEND in its device status.

Yes, I understood what you are saying.
It is ambiguous like how I explained above.

The suspend bit needs to work like queue_reset register. Please go through the queue reset specification text.

I explained this in context of VQ reset, at [1].
Suspend bit needs to avoid similar ambiguity.

[1] https://github.com/oasis-tcs/virtio-spec/issues/139

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

[Zhu Lingshan]

On 6/11/2024 5:43 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Tuesday, June 11, 2024 3:03 PM
>>
>> On 6/11/2024 4:48 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Tuesday, June 11, 2024 1:57 PM
>>>>
>>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
>>>>> Hi Lingshan, David,
>>>>>
>>>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>>>> Sent: Monday, June 10, 2024 9:34 PM
>>>>>>
>>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>>>> ---
>>>>>>>  content.tex | 69
>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>>>>> Can you please add a changelog? Especially as some of the previous
>>>>>> discussion has been lost due to the broken old mailing lists...
>>>>>>
>>>>>> (...)
>>>>>>
>>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>> +
>>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>> +
>>>>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>>>>> +\begin{itemize}
>>>>>>> +\item The driver MUST re-read \field{device status} to verify
>>>>>>> +whether the
>>>>>> SUSPEND bit is set.
>>>>>>> +If not, the device does not support the SUSPEND feature.
>>>>>> That sentence is a bit weird: I'd expect the device to not offer
>>>>>> VIRTIO_F_SUSPEND in the first place in that case... could this
>>>>>> rather happen if the device is not able to handle the request at a
>>>>>> specific point in
>>>> time?
>>>>>>> +\item The driver MUST NOT make any more buffers available to the
>>>> device.
>>>>>>> +\item The driver MUST NOT access any fields of any virtqueues or
>>>>>>> +notify
>>>>>> any virtqueues.
>>>>>>
>>>>>> "send notifications for any virtqueues"?
>>>>>>
>>>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>>>> ...except for the status field, if it is part of the config space?
>>>>>>
>>>>>>> +\end{itemize}
>>>>>>> +
>>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>> +
>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>> +
>>>>>>> +The device MUST ignore all access to its Configuration Space
>>>>>>> +while suspended, except for \field{device status}.
>>>>>> ...if it is part of the configuration space.
>>>>>>
>>>>>>> +
>>>>>>> +A device MUST NOT send any notifications, access any virtqueues,
>>>>>>> +or modify any fields in its configuration space while suspended.
>>>>>>> +
>>>>>>> +If SUSPEND is set in \field{device status}, when the driver
>>>>>>> +clears SUSPEND,
>>>>>> "subsequently clears SUSPEND"?
>>>>>>
>>>>>>> +the device MUST either resume normal operation or set
>>>>>> DEVICE_NEEDS_RESET.
>>>>>>> +
>>>>>>> +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 processing more buffers of any virtqueues \item Wait
>>>>>>> +until all buffers that are being processed have been used.
>>>>>>> +\item Send used buffer notifications to the driver.
>>>>>>> +\end{itemize}
>>>>>> So, is there any opportunity for the device to fail setting SUSPEND?
>>>>>> I mean, if the driver is supposed to look whether it sticks, there
>>>>>> should be conditions for when the device might clear it again...
>>>>>>
>>>>> Additionally, a suspend operation usually involves saving things to
>>>>> a slow
>>>> memory (or media).
>>>>> This is because the device implementation wouldn’t know when exactly
>>>>> the
>>>> device will be resumed.
>>>>> Few examples, are:
>>>>> a. A gpu device with 128MB of video RAM when suspended, QEMU needs
>>>> to store this into a (for example) rotating hard disk as 1msec IO latency.
>>>>> b. a NIC may need to store its RSS, queues, flow filters
>>>>> configuration for
>>>> several tens of KBs to some slow memory.
>>>>> c. A block device may prefer to complete some IOs to a threshold
>>>>> level
>>>> instead of maintaining large list of outstanding IOs in some
>>>> suspended memory.
>>>>> d. May be more in future.
>>>>>
>>>>> Additionally, suspend operation needs to be synchronized with
>>>>> certain data
>>>> path hardware engines to suspend DMA operations (read and write both)
>>>> which are inflight.
>>>>> (without causing DMA errors). Same would apply to the sw backend
>>>> implementations too.
>>>>> Therefore, the suspend operation that is initiated by the driver,
>>>>> should get
>>>> the acknowledgement back from the device that it has been suspend.
>>>>> Some of the good examples if you prefer to follow, a driver<->device
>>>> interface needs a suspend register which should behave like below
>>>> queue reset register.
>>>>> Spec snippet:
>>>>> "The device MUST reset the queue when 1 is written to queue_reset.
>>>>> The device MUST continue to present
>>>>> 1 in queue_reset as long as the queue reset is ongoing. The device
>>>>> MUST present 0 in both queue_reset and queue_enable when queue reset
>>>> has completed."
>>>>> At minimum, we need, something implementable like,
>>>>>
>>>>> The device MUST suspend the device when 1 is written to the
>>>> device_suspend_ctlr. The device MUST continue to present 1 in
>>>> device_suspend_ctrl as the suspend operation is ongoing in the device.
>>>>> The device MUST present 0 in the device_suspend_ctlr register when
>>>> device has completely suspended the device.
>>>>> The device MUST resume the device when 2 is written to the
>>>> device_suspend_ctrl. The device MUST continue to present 2 in the
>>>> device_suspend_ctrl as the resume operation may not have yet completed.
>>>>> The device MUST present 0 in the device_suspend_ctrl register when
>>>>> the
>>>> device has completely resumed the device.
>>>>> At this point, the driver may resume notifying the device and
>>>>> accessing the
>>>> configuration space.
>>>>> Can you please enhance this part?
>>>> Hi Parav
>>>>
>>>> Yes they are necessary contents and we will address them in the
>>>> following patches.
>>>>
>>> The register polarity is critical.
>>> In your reply to Cornelia, you descried that suspend bit cleared in the device
>> after driver sets it (until the device is suspended).
>>> This is ambiguous behavior of the register. Queue_reset register was like that
>> before it was fixed in 1.2.
>> once suspended, the driver should not access the configuration space except
>> for the device status, so it should not reset any vqs through configuration
>> space.
> I am not talking about VQ reset at all.
> I gave an example of how the VQ reset register polarity works (with was broken like how the proposed suspend bit is broken).
>
> Suspend register needs to work the way the reset register works.
> Can you please go through my exact text I replied in the previous email?
>
> Basically, the bug is:
> When driver has initiated the suspend (writing 1), the device continue to respond 0, while suspend is going on.
>
> At this point, just by looking at the device_status register one cannot figure out what is going on in the device.
> Is driver initiated suspend is ongoing, or it was never started.
> It is ambiguous.
From the device perspective, there is only one user: the driver, and the driver sets or clears SUSPEND, it knows the status of the device.
Differs from vq reset, in this patch there is a section describing how the device should behave suspending. So once the driver sets SUSPEND,
the device should perform the required actions and suspend itself, during this process, SUSPEND bit is still 0, and the drivers know that, like
how we handle FEATURES_OK. Once the device suspended itself, it present SUSPEND == 1 in the device status.

1 bit controls whether SUSPEND-ed.
>
> Hence, it needs to work like how the queue_reset works.
> i.e. if suspend is ongoing, one should be able to read device_status without involving driver and understand that it is ongoing.
there are other users than the driver?
>
>>>> This patch focus on the basic facility virito suspending, not
>>>> transport or device types, this is to make this chageset focused and small.
>>>>
>>> If suspend bit is part of the device_status, it is already covered by the PCI
>> transport.
>>> So it is fine, but its polarity is an issue, that we need to fix.
>> Oh, I see your confusion.
>> All transport have their own implementation of configuration space, this is not
>> PCI specific.
>> PCI config space != virtio configuration space. And this patch focuses on virtio
>> layer.
>>> i.e.
>>> One driver writes 1, it needs to stay 1, while the suspend is ongoing.
>>> When suspension is completed in the device, the device to turn it to zero.
>>>
>>> If the device_status register does not fullfil this, a new register is worthwhile
>> to add in this patch.
>> The driver writes SUSPEND, once the device is suspened, it should present
>> SUSPEND in its device status.
> Yes, I understood what you are saying.
> It is ambiguous like how I explained above.
>
> The suspend bit needs to work like queue_reset register. Please go through the queue reset specification text.
>
> I explained this in context of VQ reset, at [1].
> Suspend bit needs to avoid similar ambiguity.
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/139
replied above

Thanks
Zhu Lingshan

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Tuesday, June 11, 2024 3:42 PM
> 
> On 6/11/2024 5:43 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Tuesday, June 11, 2024 3:03 PM
> >>
> >> On 6/11/2024 4:48 PM, Parav Pandit wrote:
> >>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>> Sent: Tuesday, June 11, 2024 1:57 PM
> >>>>
> >>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
> >>>>> Hi Lingshan, David,
> >>>>>
> >>>>>> From: Cornelia Huck <cohuck@redhat.com>
> >>>>>> Sent: Monday, June 10, 2024 9:34 PM
> >>>>>>
> >>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >>>>>>>
> >>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>>>>>> ---
> >>>>>>>  content.tex | 69
> >>>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
> >>>>>> Can you please add a changelog? Especially as some of the
> >>>>>> previous discussion has been lost due to the broken old mailing lists...
> >>>>>>
> >>>>>> (...)
> >>>>>>
> >>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
> >>>>>>> +Initialization And Device Operation / Device Suspend}
> >>>>>>> +
> >>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
> >>>>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>>>> +
> >>>>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
> >>>>>>> +\begin{itemize}
> >>>>>>> +\item The driver MUST re-read \field{device status} to verify
> >>>>>>> +whether the
> >>>>>> SUSPEND bit is set.
> >>>>>>> +If not, the device does not support the SUSPEND feature.
> >>>>>> That sentence is a bit weird: I'd expect the device to not offer
> >>>>>> VIRTIO_F_SUSPEND in the first place in that case... could this
> >>>>>> rather happen if the device is not able to handle the request at
> >>>>>> a specific point in
> >>>> time?
> >>>>>>> +\item The driver MUST NOT make any more buffers available to
> >>>>>>> +the
> >>>> device.
> >>>>>>> +\item The driver MUST NOT access any fields of any virtqueues
> >>>>>>> +or notify
> >>>>>> any virtqueues.
> >>>>>>
> >>>>>> "send notifications for any virtqueues"?
> >>>>>>
> >>>>>>> +\item The driver MUST NOT access Device Configuration Space.
> >>>>>> ...except for the status field, if it is part of the config space?
> >>>>>>
> >>>>>>> +\end{itemize}
> >>>>>>> +
> >>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
> >>>>>>> +Initialization And Device Operation / Device Suspend}
> >>>>>>> +
> >>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> >>>>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>>>> +
> >>>>>>> +The device MUST ignore all access to its Configuration Space
> >>>>>>> +while suspended, except for \field{device status}.
> >>>>>> ...if it is part of the configuration space.
> >>>>>>
> >>>>>>> +
> >>>>>>> +A device MUST NOT send any notifications, access any
> >>>>>>> +virtqueues, or modify any fields in its configuration space while
> suspended.
> >>>>>>> +
> >>>>>>> +If SUSPEND is set in \field{device status}, when the driver
> >>>>>>> +clears SUSPEND,
> >>>>>> "subsequently clears SUSPEND"?
> >>>>>>
> >>>>>>> +the device MUST either resume normal operation or set
> >>>>>> DEVICE_NEEDS_RESET.
> >>>>>>> +
> >>>>>>> +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 processing more buffers of any virtqueues \item Wait
> >>>>>>> +until all buffers that are being processed have been used.
> >>>>>>> +\item Send used buffer notifications to the driver.
> >>>>>>> +\end{itemize}
> >>>>>> So, is there any opportunity for the device to fail setting SUSPEND?
> >>>>>> I mean, if the driver is supposed to look whether it sticks,
> >>>>>> there should be conditions for when the device might clear it again...
> >>>>>>
> >>>>> Additionally, a suspend operation usually involves saving things
> >>>>> to a slow
> >>>> memory (or media).
> >>>>> This is because the device implementation wouldn't know when
> >>>>> exactly the
> >>>> device will be resumed.
> >>>>> Few examples, are:
> >>>>> a. A gpu device with 128MB of video RAM when suspended, QEMU
> needs
> >>>> to store this into a (for example) rotating hard disk as 1msec IO latency.
> >>>>> b. a NIC may need to store its RSS, queues, flow filters
> >>>>> configuration for
> >>>> several tens of KBs to some slow memory.
> >>>>> c. A block device may prefer to complete some IOs to a threshold
> >>>>> level
> >>>> instead of maintaining large list of outstanding IOs in some
> >>>> suspended memory.
> >>>>> d. May be more in future.
> >>>>>
> >>>>> Additionally, suspend operation needs to be synchronized with
> >>>>> certain data
> >>>> path hardware engines to suspend DMA operations (read and write
> >>>> both) which are inflight.
> >>>>> (without causing DMA errors). Same would apply to the sw backend
> >>>> implementations too.
> >>>>> Therefore, the suspend operation that is initiated by the driver,
> >>>>> should get
> >>>> the acknowledgement back from the device that it has been suspend.
> >>>>> Some of the good examples if you prefer to follow, a
> >>>>> driver<->device
> >>>> interface needs a suspend register which should behave like below
> >>>> queue reset register.
> >>>>> Spec snippet:
> >>>>> "The device MUST reset the queue when 1 is written to queue_reset.
> >>>>> The device MUST continue to present
> >>>>> 1 in queue_reset as long as the queue reset is ongoing. The device
> >>>>> MUST present 0 in both queue_reset and queue_enable when queue
> >>>>> reset
> >>>> has completed."
> >>>>> At minimum, we need, something implementable like,
> >>>>>
> >>>>> The device MUST suspend the device when 1 is written to the
> >>>> device_suspend_ctlr. The device MUST continue to present 1 in
> >>>> device_suspend_ctrl as the suspend operation is ongoing in the device.
> >>>>> The device MUST present 0 in the device_suspend_ctlr register when
> >>>> device has completely suspended the device.
> >>>>> The device MUST resume the device when 2 is written to the
> >>>> device_suspend_ctrl. The device MUST continue to present 2 in the
> >>>> device_suspend_ctrl as the resume operation may not have yet
> completed.
> >>>>> The device MUST present 0 in the device_suspend_ctrl register when
> >>>>> the
> >>>> device has completely resumed the device.
> >>>>> At this point, the driver may resume notifying the device and
> >>>>> accessing the
> >>>> configuration space.
> >>>>> Can you please enhance this part?
> >>>> Hi Parav
> >>>>
> >>>> Yes they are necessary contents and we will address them in the
> >>>> following patches.
> >>>>
> >>> The register polarity is critical.
> >>> In your reply to Cornelia, you descried that suspend bit cleared in
> >>> the device
> >> after driver sets it (until the device is suspended).
> >>> This is ambiguous behavior of the register. Queue_reset register was
> >>> like that
> >> before it was fixed in 1.2.
> >> once suspended, the driver should not access the configuration space
> >> except for the device status, so it should not reset any vqs through
> >> configuration space.
> > I am not talking about VQ reset at all.
> > I gave an example of how the VQ reset register polarity works (with was
> broken like how the proposed suspend bit is broken).
> >
> > Suspend register needs to work the way the reset register works.
> > Can you please go through my exact text I replied in the previous email?
> >
> > Basically, the bug is:
> > When driver has initiated the suspend (writing 1), the device continue to
> respond 0, while suspend is going on.
> >
> > At this point, just by looking at the device_status register one cannot figure
> out what is going on in the device.
> > Is driver initiated suspend is ongoing, or it was never started.
> > It is ambiguous.
> From the device perspective, there is only one user: the driver, and the driver
> sets or clears SUSPEND, it knows the status of the device.
It does not work elegantly when the hypervisor or other system is looking dealing with this register.

In current proposal, anyone else than the driver looking at the device_status, it is ambiguous.

For example,
suspend_bit: 0x1 suspended.
	     0x0 not suspended or suspend is ongoing. (ambiguous for hypervisor debugging this device)

For device migration, it requires yet another side bit in the device parts.

An elegant and non-ambiguous interface can be,

ctrl_register =	0x0 (a usual default)
		0x1, suspend the device
		0x2, resume the device.
status_register =	0x0, (still) running.
			0x1, suspended.

With this driver and hypervisor (both) has very clear view of what is going on and what is the status.
No more ambiguity for anyone.

Please consider pros and cons of both approaches.

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

[Michael S. Tsirkin]

On Tue, Jun 11, 2024 at 04:20:42PM +0800, Zhu Lingshan wrote:
> 
> 
> On 6/11/2024 12:04 AM, Cornelia Huck wrote:
> > On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> Signed-off-by: David Stevens <stevensd@chromium.org>
> >> ---
> >>  content.tex | 69 +++++++++++++++++++++++++++++++++++++++++++++--------
> >>  1 file changed, 59 insertions(+), 10 deletions(-)
> > Can you please add a changelog? Especially as some of the previous
> > discussion has been lost due to the broken old mailing lists...
> >
> > (...)
> will add change log in next version.
> >
> >> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >> +
> >> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> >> +
> >> +Once the driver sets SUSPEND to \field{device status} of the device:
> >> +\begin{itemize}
> >> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> >> +If not, the device does not support the SUSPEND feature.
> > That sentence is a bit weird: I'd expect the device to not offer
> > VIRTIO_F_SUSPEND in the first place in that case... could this rather
> > happen if the device is not able to handle the request at a specific
> > point in time?
> Yes, this can be improved
> 
> How about:
> If not, this indicating the device is not able to handle SUSPEND at the moment.


I don't get what this means, agrammatical and vague.
Please make it explicit. Do you mean if driver reads
status and SUSPEND is clear then devide failed to
enter suspend state? Might be problematic: time to 
suspend might be quite high.


> >> +\item The driver MUST NOT make any more buffers available to the device.
> >> +\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.
> > "send notifications for any virtqueues"?
> OK
> >
> >> +\item The driver MUST NOT access Device Configuration Space.
> > ...except for the status field, if it is part of the config space?
> Yes, that is the original design, then I am convinced that the status fields is
> not a part of the config space because it has a dedicated section in the spec.
> 
> I will add this sentence in the next version.
> >
> >> +\end{itemize}
> >> +
> >> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >> +
> >> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> >> +
> >> +The device MUST ignore all access to its Configuration Space while
> >> +suspended, except for \field{device status}.
> > ...if it is part of the configuration space.
> Yes
> >
> >> +
> >> +A device MUST NOT send any notifications, access any virtqueues, or modify
> >> +any fields in its configuration space while suspended.
> >> +
> >> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
> > "subsequently clears SUSPEND"?
> I am a native speaker, but "subsequently" sounds like clearing SUSPEND right after "setting SUSPEND", I guess there can be
> some operations during SUSPEND.
> 
> >
> >> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
> >> +
> >> +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 processing more buffers of any virtqueues
> >> +\item Wait until all buffers that are being processed have been used.
> >> +\item Send used buffer notifications to the driver.
> >> +\end{itemize}
> > So, is there any opportunity for the device to fail setting SUSPEND? I
> > mean, if the driver is supposed to look whether it sticks, there should
> > be conditions for when the device might clear it again...
> The device should not present SUSPEND bit in the device status until it is suspended,
> so I am afraid there is not a SUSPEND bit to clear if the device fails to suspend itself.
> 
> I think there can be two indications of a failed suspend:
> 1) The driver sets SUSPEND, but the device does not present SUSPEND after a while, depends
> on the driver tolerance, the driver may assume it is a failed SUSPEND, like how we handle FEATURES_OK


This is not how we handle FEATURES_OK.

> 2)The device runs into errors, presenting NEEDS_RESET.
> 

More reasonable.

> >
> >> +
> >>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
> >>  
> >>  Virtio can use various different buses, thus the standard is split
> >> @@ -872,6 +917,10 @@ \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
> >> +   set SUSPEND to the device.
> > "that the driver can trigger suspending the device via the SUSPEND flag"?
> Yes
> >
> >> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> >> +
> >>  \end{description}
> >>  
> >>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

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

[David Stevens]

> >> +\item The driver MUST NOT make any more buffers available to the device.
> >> +\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.
> > "send notifications for any virtqueues"?
> OK
> >
> >> +\item The driver MUST NOT access Device Configuration Space.
> > ...except for the status field, if it is part of the config space?
> Yes, that is the original design, then I am convinced that the status fields is
> not a part of the config space because it has a dedicated section in the spec.
>
> I will add this sentence in the next version.

In the discussion on v3 (unarchived due to the migration), Michael stated:

> by the way device status is not in device configuration space at all
> so this sentence does not make sense either.

Doesn't that assertion directly contradict Cornelia's feedback on this
series, which seems to be saying that the device status field can be
part of the config space? Can we get some clarity here on what exactly
the existing spec is?

-David

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

[Zhu Lingshan]

On 6/11/2024 6:37 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Tuesday, June 11, 2024 3:42 PM
>>
>> On 6/11/2024 5:43 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Tuesday, June 11, 2024 3:03 PM
>>>>
>>>> On 6/11/2024 4:48 PM, Parav Pandit wrote:
>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>> Sent: Tuesday, June 11, 2024 1:57 PM
>>>>>>
>>>>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
>>>>>>> Hi Lingshan, David,
>>>>>>>
>>>>>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>>>>>> Sent: Monday, June 10, 2024 9:34 PM
>>>>>>>>
>>>>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>>>>>> ---
>>>>>>>>>  content.tex | 69
>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>>>>>>> Can you please add a changelog? Especially as some of the
>>>>>>>> previous discussion has been lost due to the broken old mailing lists...
>>>>>>>>
>>>>>>>> (...)
>>>>>>>>
>>>>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
>>>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>>>> +
>>>>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>>>> +
>>>>>>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>>>>>>> +\begin{itemize}
>>>>>>>>> +\item The driver MUST re-read \field{device status} to verify
>>>>>>>>> +whether the
>>>>>>>> SUSPEND bit is set.
>>>>>>>>> +If not, the device does not support the SUSPEND feature.
>>>>>>>> That sentence is a bit weird: I'd expect the device to not offer
>>>>>>>> VIRTIO_F_SUSPEND in the first place in that case... could this
>>>>>>>> rather happen if the device is not able to handle the request at
>>>>>>>> a specific point in
>>>>>> time?
>>>>>>>>> +\item The driver MUST NOT make any more buffers available to
>>>>>>>>> +the
>>>>>> device.
>>>>>>>>> +\item The driver MUST NOT access any fields of any virtqueues
>>>>>>>>> +or notify
>>>>>>>> any virtqueues.
>>>>>>>>
>>>>>>>> "send notifications for any virtqueues"?
>>>>>>>>
>>>>>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>>>>>> ...except for the status field, if it is part of the config space?
>>>>>>>>
>>>>>>>>> +\end{itemize}
>>>>>>>>> +
>>>>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
>>>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>>>> +
>>>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>>>> +
>>>>>>>>> +The device MUST ignore all access to its Configuration Space
>>>>>>>>> +while suspended, except for \field{device status}.
>>>>>>>> ...if it is part of the configuration space.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +A device MUST NOT send any notifications, access any
>>>>>>>>> +virtqueues, or modify any fields in its configuration space while
>> suspended.
>>>>>>>>> +
>>>>>>>>> +If SUSPEND is set in \field{device status}, when the driver
>>>>>>>>> +clears SUSPEND,
>>>>>>>> "subsequently clears SUSPEND"?
>>>>>>>>
>>>>>>>>> +the device MUST either resume normal operation or set
>>>>>>>> DEVICE_NEEDS_RESET.
>>>>>>>>> +
>>>>>>>>> +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 processing more buffers of any virtqueues \item Wait
>>>>>>>>> +until all buffers that are being processed have been used.
>>>>>>>>> +\item Send used buffer notifications to the driver.
>>>>>>>>> +\end{itemize}
>>>>>>>> So, is there any opportunity for the device to fail setting SUSPEND?
>>>>>>>> I mean, if the driver is supposed to look whether it sticks,
>>>>>>>> there should be conditions for when the device might clear it again...
>>>>>>>>
>>>>>>> Additionally, a suspend operation usually involves saving things
>>>>>>> to a slow
>>>>>> memory (or media).
>>>>>>> This is because the device implementation wouldn't know when
>>>>>>> exactly the
>>>>>> device will be resumed.
>>>>>>> Few examples, are:
>>>>>>> a. A gpu device with 128MB of video RAM when suspended, QEMU
>> needs
>>>>>> to store this into a (for example) rotating hard disk as 1msec IO latency.
>>>>>>> b. a NIC may need to store its RSS, queues, flow filters
>>>>>>> configuration for
>>>>>> several tens of KBs to some slow memory.
>>>>>>> c. A block device may prefer to complete some IOs to a threshold
>>>>>>> level
>>>>>> instead of maintaining large list of outstanding IOs in some
>>>>>> suspended memory.
>>>>>>> d. May be more in future.
>>>>>>>
>>>>>>> Additionally, suspend operation needs to be synchronized with
>>>>>>> certain data
>>>>>> path hardware engines to suspend DMA operations (read and write
>>>>>> both) which are inflight.
>>>>>>> (without causing DMA errors). Same would apply to the sw backend
>>>>>> implementations too.
>>>>>>> Therefore, the suspend operation that is initiated by the driver,
>>>>>>> should get
>>>>>> the acknowledgement back from the device that it has been suspend.
>>>>>>> Some of the good examples if you prefer to follow, a
>>>>>>> driver<->device
>>>>>> interface needs a suspend register which should behave like below
>>>>>> queue reset register.
>>>>>>> Spec snippet:
>>>>>>> "The device MUST reset the queue when 1 is written to queue_reset.
>>>>>>> The device MUST continue to present
>>>>>>> 1 in queue_reset as long as the queue reset is ongoing. The device
>>>>>>> MUST present 0 in both queue_reset and queue_enable when queue
>>>>>>> reset
>>>>>> has completed."
>>>>>>> At minimum, we need, something implementable like,
>>>>>>>
>>>>>>> The device MUST suspend the device when 1 is written to the
>>>>>> device_suspend_ctlr. The device MUST continue to present 1 in
>>>>>> device_suspend_ctrl as the suspend operation is ongoing in the device.
>>>>>>> The device MUST present 0 in the device_suspend_ctlr register when
>>>>>> device has completely suspended the device.
>>>>>>> The device MUST resume the device when 2 is written to the
>>>>>> device_suspend_ctrl. The device MUST continue to present 2 in the
>>>>>> device_suspend_ctrl as the resume operation may not have yet
>> completed.
>>>>>>> The device MUST present 0 in the device_suspend_ctrl register when
>>>>>>> the
>>>>>> device has completely resumed the device.
>>>>>>> At this point, the driver may resume notifying the device and
>>>>>>> accessing the
>>>>>> configuration space.
>>>>>>> Can you please enhance this part?
>>>>>> Hi Parav
>>>>>>
>>>>>> Yes they are necessary contents and we will address them in the
>>>>>> following patches.
>>>>>>
>>>>> The register polarity is critical.
>>>>> In your reply to Cornelia, you descried that suspend bit cleared in
>>>>> the device
>>>> after driver sets it (until the device is suspended).
>>>>> This is ambiguous behavior of the register. Queue_reset register was
>>>>> like that
>>>> before it was fixed in 1.2.
>>>> once suspended, the driver should not access the configuration space
>>>> except for the device status, so it should not reset any vqs through
>>>> configuration space.
>>> I am not talking about VQ reset at all.
>>> I gave an example of how the VQ reset register polarity works (with was
>> broken like how the proposed suspend bit is broken).
>>> Suspend register needs to work the way the reset register works.
>>> Can you please go through my exact text I replied in the previous email?
>>>
>>> Basically, the bug is:
>>> When driver has initiated the suspend (writing 1), the device continue to
>> respond 0, while suspend is going on.
>>> At this point, just by looking at the device_status register one cannot figure
>> out what is going on in the device.
>>> Is driver initiated suspend is ongoing, or it was never started.
>>> It is ambiguous.
>> From the device perspective, there is only one user: the driver, and the driver
>> sets or clears SUSPEND, it knows the status of the device.
> It does not work elegantly when the hypervisor or other system is looking dealing with this register.
>
> In current proposal, anyone else than the driver looking at the device_status, it is ambiguous.
>
> For example,
> suspend_bit: 0x1 suspended.
> 	     0x0 not suspended or suspend is ongoing. (ambiguous for hypervisor debugging this device)
>
> For device migration, it requires yet another side bit in the device parts.
>
> An elegant and non-ambiguous interface can be,
>
> ctrl_register =	0x0 (a usual default)
> 		0x1, suspend the device
> 		0x2, resume the device.
> status_register =	0x0, (still) running.
> 			0x1, suspended.
>
> With this driver and hypervisor (both) has very clear view of what is going on and what is the status.
> No more ambiguity for anyone.
>
> Please consider pros and cons of both approaches.
The driver "owns" the device, other components access the device configurations through the driver, I am not sure there are other "owners" than the driver.

Thanks
Zhu Lingshan

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

[Zhu Lingshan]

On 6/12/2024 12:26 AM, Michael S. Tsirkin wrote:
> On Tue, Jun 11, 2024 at 04:20:42PM +0800, Zhu Lingshan wrote:
>>
>> On 6/11/2024 12:04 AM, Cornelia Huck wrote:
>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>> ---
>>>>  content.tex | 69 +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>> Can you please add a changelog? Especially as some of the previous
>>> discussion has been lost due to the broken old mailing lists...
>>>
>>> (...)
>> will add change log in next version.
>>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>>> +
>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>>>> +
>>>> +Once the driver sets SUSPEND to \field{device status} of the device:
>>>> +\begin{itemize}
>>>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
>>>> +If not, the device does not support the SUSPEND feature.
>>> That sentence is a bit weird: I'd expect the device to not offer
>>> VIRTIO_F_SUSPEND in the first place in that case... could this rather
>>> happen if the device is not able to handle the request at a specific
>>> point in time?
>> Yes, this can be improved
>>
>> How about:
>> If not, this indicating the device is not able to handle SUSPEND at the moment.
>
> I don't get what this means, agrammatical and vague.
> Please make it explicit. Do you mean if driver reads
> status and SUSPEND is clear then devide failed to
> enter suspend state? Might be problematic: time to 
> suspend might be quite high.
I think this is a common problem how the driver handle changes of the device status.
Even without this SUSPEND feature, spec section 3.1.1 Driver Requirements: Device Initialization says:
“Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable”
Other status changing may also be costly, so how long should the driver wait for the device responding to status changes?
This currently depends on the driver's tolerance.
What should the driver do when overdue? Shall it reset the device or set FAILED?

I think the device should either SUSPEND successfully or set NEEDS_RESET, but maybe not a good idea to set a time-out in the spec,
the driver can set FAILED or just reset the device if the operation exceeds the tolerance in pulling.

Any suggestions?

>
>
>>>> +\item The driver MUST NOT make any more buffers available to the device.
>>>> +\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.
>>> "send notifications for any virtqueues"?
>> OK
>>>> +\item The driver MUST NOT access Device Configuration Space.
>>> ...except for the status field, if it is part of the config space?
>> Yes, that is the original design, then I am convinced that the status fields is
>> not a part of the config space because it has a dedicated section in the spec.
>>
>> I will add this sentence in the next version.
>>>> +\end{itemize}
>>>> +
>>>> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
>>>> +
>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
>>>> +
>>>> +The device MUST ignore all access to its Configuration Space while
>>>> +suspended, except for \field{device status}.
>>> ...if it is part of the configuration space.
>> Yes
>>>> +
>>>> +A device MUST NOT send any notifications, access any virtqueues, or modify
>>>> +any fields in its configuration space while suspended.
>>>> +
>>>> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
>>> "subsequently clears SUSPEND"?
>> I am a native speaker, but "subsequently" sounds like clearing SUSPEND right after "setting SUSPEND", I guess there can be
>> some operations during SUSPEND.
>>
>>>> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
>>>> +
>>>> +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 processing more buffers of any virtqueues
>>>> +\item Wait until all buffers that are being processed have been used.
>>>> +\item Send used buffer notifications to the driver.
>>>> +\end{itemize}
>>> So, is there any opportunity for the device to fail setting SUSPEND? I
>>> mean, if the driver is supposed to look whether it sticks, there should
>>> be conditions for when the device might clear it again...
>> The device should not present SUSPEND bit in the device status until it is suspended,
>> so I am afraid there is not a SUSPEND bit to clear if the device fails to suspend itself.
>>
>> I think there can be two indications of a failed suspend:
>> 1) The driver sets SUSPEND, but the device does not present SUSPEND after a while, depends
>> on the driver tolerance, the driver may assume it is a failed SUSPEND, like how we handle FEATURES_OK
>
> This is not how we handle FEATURES_OK.
setting FEATURES_OK may not always be successful and can take longer time than expect. What should the driver
do when the pulling re-reads does not return FEATURES_OK?
>
>> 2)The device runs into errors, presenting NEEDS_RESET.
>>
> More reasonable.
OK, so the driver should re-read the status, it should see either SUSPEND or NEEDS_RESET, this works for me.

Thanks
>
>>>> +
>>>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
>>>>  
>>>>  Virtio can use various different buses, thus the standard is split
>>>> @@ -872,6 +917,10 @@ \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
>>>> +   set SUSPEND to the device.
>>> "that the driver can trigger suspending the device via the SUSPEND flag"?
>> Yes
>>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
>>>> +
>>>>  \end{description}
>>>>  
>>>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Wednesday, June 12, 2024 2:50 PM
> 
> On 6/11/2024 6:37 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Tuesday, June 11, 2024 3:42 PM
> >>
> >> On 6/11/2024 5:43 PM, Parav Pandit wrote:
> >>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>> Sent: Tuesday, June 11, 2024 3:03 PM
> >>>>
> >>>> On 6/11/2024 4:48 PM, Parav Pandit wrote:
> >>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>>> Sent: Tuesday, June 11, 2024 1:57 PM
> >>>>>>
> >>>>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
> >>>>>>> Hi Lingshan, David,
> >>>>>>>
> >>>>>>>> From: Cornelia Huck <cohuck@redhat.com>
> >>>>>>>> Sent: Monday, June 10, 2024 9:34 PM
> >>>>>>>>
> >>>>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>>>>>>>> ---
> >>>>>>>>>  content.tex | 69
> >>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
> >>>>>>>> Can you please add a changelog? Especially as some of the
> >>>>>>>> previous discussion has been lost due to the broken old mailing
> lists...
> >>>>>>>>
> >>>>>>>> (...)
> >>>>>>>>
> >>>>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
> >>>>>>>>> +Initialization And Device Operation / Device Suspend}
> >>>>>>>>> +
> >>>>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
> >>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>>>>>> +
> >>>>>>>>> +Once the driver sets SUSPEND to \field{device status} of the
> device:
> >>>>>>>>> +\begin{itemize}
> >>>>>>>>> +\item The driver MUST re-read \field{device status} to verify
> >>>>>>>>> +whether the
> >>>>>>>> SUSPEND bit is set.
> >>>>>>>>> +If not, the device does not support the SUSPEND feature.
> >>>>>>>> That sentence is a bit weird: I'd expect the device to not
> >>>>>>>> offer VIRTIO_F_SUSPEND in the first place in that case... could
> >>>>>>>> this rather happen if the device is not able to handle the
> >>>>>>>> request at a specific point in
> >>>>>> time?
> >>>>>>>>> +\item The driver MUST NOT make any more buffers available to
> >>>>>>>>> +the
> >>>>>> device.
> >>>>>>>>> +\item The driver MUST NOT access any fields of any virtqueues
> >>>>>>>>> +or notify
> >>>>>>>> any virtqueues.
> >>>>>>>>
> >>>>>>>> "send notifications for any virtqueues"?
> >>>>>>>>
> >>>>>>>>> +\item The driver MUST NOT access Device Configuration Space.
> >>>>>>>> ...except for the status field, if it is part of the config space?
> >>>>>>>>
> >>>>>>>>> +\end{itemize}
> >>>>>>>>> +
> >>>>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
> >>>>>>>>> +Initialization And Device Operation / Device Suspend}
> >>>>>>>>> +
> >>>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> >>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>>>>>> +
> >>>>>>>>> +The device MUST ignore all access to its Configuration Space
> >>>>>>>>> +while suspended, except for \field{device status}.
> >>>>>>>> ...if it is part of the configuration space.
> >>>>>>>>
> >>>>>>>>> +
> >>>>>>>>> +A device MUST NOT send any notifications, access any
> >>>>>>>>> +virtqueues, or modify any fields in its configuration space
> >>>>>>>>> +while
> >> suspended.
> >>>>>>>>> +
> >>>>>>>>> +If SUSPEND is set in \field{device status}, when the driver
> >>>>>>>>> +clears SUSPEND,
> >>>>>>>> "subsequently clears SUSPEND"?
> >>>>>>>>
> >>>>>>>>> +the device MUST either resume normal operation or set
> >>>>>>>> DEVICE_NEEDS_RESET.
> >>>>>>>>> +
> >>>>>>>>> +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 processing more buffers of any virtqueues \item
> >>>>>>>>> +Wait until all buffers that are being processed have been used.
> >>>>>>>>> +\item Send used buffer notifications to the driver.
> >>>>>>>>> +\end{itemize}
> >>>>>>>> So, is there any opportunity for the device to fail setting SUSPEND?
> >>>>>>>> I mean, if the driver is supposed to look whether it sticks,
> >>>>>>>> there should be conditions for when the device might clear it again...
> >>>>>>>>
> >>>>>>> Additionally, a suspend operation usually involves saving things
> >>>>>>> to a slow
> >>>>>> memory (or media).
> >>>>>>> This is because the device implementation wouldn't know when
> >>>>>>> exactly the
> >>>>>> device will be resumed.
> >>>>>>> Few examples, are:
> >>>>>>> a. A gpu device with 128MB of video RAM when suspended, QEMU
> >> needs
> >>>>>> to store this into a (for example) rotating hard disk as 1msec IO latency.
> >>>>>>> b. a NIC may need to store its RSS, queues, flow filters
> >>>>>>> configuration for
> >>>>>> several tens of KBs to some slow memory.
> >>>>>>> c. A block device may prefer to complete some IOs to a threshold
> >>>>>>> level
> >>>>>> instead of maintaining large list of outstanding IOs in some
> >>>>>> suspended memory.
> >>>>>>> d. May be more in future.
> >>>>>>>
> >>>>>>> Additionally, suspend operation needs to be synchronized with
> >>>>>>> certain data
> >>>>>> path hardware engines to suspend DMA operations (read and write
> >>>>>> both) which are inflight.
> >>>>>>> (without causing DMA errors). Same would apply to the sw backend
> >>>>>> implementations too.
> >>>>>>> Therefore, the suspend operation that is initiated by the
> >>>>>>> driver, should get
> >>>>>> the acknowledgement back from the device that it has been suspend.
> >>>>>>> Some of the good examples if you prefer to follow, a
> >>>>>>> driver<->device
> >>>>>> interface needs a suspend register which should behave like below
> >>>>>> queue reset register.
> >>>>>>> Spec snippet:
> >>>>>>> "The device MUST reset the queue when 1 is written to queue_reset.
> >>>>>>> The device MUST continue to present
> >>>>>>> 1 in queue_reset as long as the queue reset is ongoing. The
> >>>>>>> device MUST present 0 in both queue_reset and queue_enable when
> >>>>>>> queue reset
> >>>>>> has completed."
> >>>>>>> At minimum, we need, something implementable like,
> >>>>>>>
> >>>>>>> The device MUST suspend the device when 1 is written to the
> >>>>>> device_suspend_ctlr. The device MUST continue to present 1 in
> >>>>>> device_suspend_ctrl as the suspend operation is ongoing in the device.
> >>>>>>> The device MUST present 0 in the device_suspend_ctlr register
> >>>>>>> when
> >>>>>> device has completely suspended the device.
> >>>>>>> The device MUST resume the device when 2 is written to the
> >>>>>> device_suspend_ctrl. The device MUST continue to present 2 in the
> >>>>>> device_suspend_ctrl as the resume operation may not have yet
> >> completed.
> >>>>>>> The device MUST present 0 in the device_suspend_ctrl register
> >>>>>>> when the
> >>>>>> device has completely resumed the device.
> >>>>>>> At this point, the driver may resume notifying the device and
> >>>>>>> accessing the
> >>>>>> configuration space.
> >>>>>>> Can you please enhance this part?
> >>>>>> Hi Parav
> >>>>>>
> >>>>>> Yes they are necessary contents and we will address them in the
> >>>>>> following patches.
> >>>>>>
> >>>>> The register polarity is critical.
> >>>>> In your reply to Cornelia, you descried that suspend bit cleared
> >>>>> in the device
> >>>> after driver sets it (until the device is suspended).
> >>>>> This is ambiguous behavior of the register. Queue_reset register
> >>>>> was like that
> >>>> before it was fixed in 1.2.
> >>>> once suspended, the driver should not access the configuration
> >>>> space except for the device status, so it should not reset any vqs
> >>>> through configuration space.
> >>> I am not talking about VQ reset at all.
> >>> I gave an example of how the VQ reset register polarity works (with
> >>> was
> >> broken like how the proposed suspend bit is broken).
> >>> Suspend register needs to work the way the reset register works.
> >>> Can you please go through my exact text I replied in the previous email?
> >>>
> >>> Basically, the bug is:
> >>> When driver has initiated the suspend (writing 1), the device
> >>> continue to
> >> respond 0, while suspend is going on.
> >>> At this point, just by looking at the device_status register one
> >>> cannot figure
> >> out what is going on in the device.
> >>> Is driver initiated suspend is ongoing, or it was never started.
> >>> It is ambiguous.
> >> From the device perspective, there is only one user: the driver, and
> >> the driver sets or clears SUSPEND, it knows the status of the device.
> > It does not work elegantly when the hypervisor or other system is looking
> dealing with this register.
> >
> > In current proposal, anyone else than the driver looking at the device_status,
> it is ambiguous.
> >
> > For example,
> > suspend_bit: 0x1 suspended.
> > 	     0x0 not suspended or suspend is ongoing. (ambiguous for
> > hypervisor debugging this device)
> >
> > For device migration, it requires yet another side bit in the device parts.
> >
> > An elegant and non-ambiguous interface can be,
> >
> > ctrl_register =	0x0 (a usual default)
> > 		0x1, suspend the device
> > 		0x2, resume the device.
> > status_register =	0x0, (still) running.
> > 			0x1, suspended.
> >
> > With this driver and hypervisor (both) has very clear view of what is going on
> and what is the status.
> > No more ambiguity for anyone.
> >
> > Please consider pros and cons of both approaches.
> The driver "owns" the device, other components access the device
> configurations through the driver, I am not sure there are other "owners" than
> the driver.
Device migration and debug channel [1] plans to access the device from the hypervisor.
I don’t see any disadvantage of separate control and status register.
Do you see any?

[1] https://lore.kernel.org/virtio-comment/20240601145042.2074739-1-parav@nvidia.com/T/#m14b80fd996e8d2522723967d8af844b70e131a90

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

[Zhu Lingshan]

On 6/12/2024 6:07 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Wednesday, June 12, 2024 2:50 PM
>>
>> On 6/11/2024 6:37 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Tuesday, June 11, 2024 3:42 PM
>>>>
>>>> On 6/11/2024 5:43 PM, Parav Pandit wrote:
>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>> Sent: Tuesday, June 11, 2024 3:03 PM
>>>>>>
>>>>>> On 6/11/2024 4:48 PM, Parav Pandit wrote:
>>>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>>> Sent: Tuesday, June 11, 2024 1:57 PM
>>>>>>>>
>>>>>>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
>>>>>>>>> Hi Lingshan, David,
>>>>>>>>>
>>>>>>>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>>>>>>>> Sent: Monday, June 10, 2024 9:34 PM
>>>>>>>>>>
>>>>>>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>>>>>>>> ---
>>>>>>>>>>>  content.tex | 69
>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++--------
>>>>>>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>>>>>>>>> Can you please add a changelog? Especially as some of the
>>>>>>>>>> previous discussion has been lost due to the broken old mailing
>> lists...
>>>>>>>>>> (...)
>>>>>>>>>>
>>>>>>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
>>>>>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>>>>>> +
>>>>>>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
>>>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>>>>>> +
>>>>>>>>>>> +Once the driver sets SUSPEND to \field{device status} of the
>> device:
>>>>>>>>>>> +\begin{itemize}
>>>>>>>>>>> +\item The driver MUST re-read \field{device status} to verify
>>>>>>>>>>> +whether the
>>>>>>>>>> SUSPEND bit is set.
>>>>>>>>>>> +If not, the device does not support the SUSPEND feature.
>>>>>>>>>> That sentence is a bit weird: I'd expect the device to not
>>>>>>>>>> offer VIRTIO_F_SUSPEND in the first place in that case... could
>>>>>>>>>> this rather happen if the device is not able to handle the
>>>>>>>>>> request at a specific point in
>>>>>>>> time?
>>>>>>>>>>> +\item The driver MUST NOT make any more buffers available to
>>>>>>>>>>> +the
>>>>>>>> device.
>>>>>>>>>>> +\item The driver MUST NOT access any fields of any virtqueues
>>>>>>>>>>> +or notify
>>>>>>>>>> any virtqueues.
>>>>>>>>>>
>>>>>>>>>> "send notifications for any virtqueues"?
>>>>>>>>>>
>>>>>>>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>>>>>>>> ...except for the status field, if it is part of the config space?
>>>>>>>>>>
>>>>>>>>>>> +\end{itemize}
>>>>>>>>>>> +
>>>>>>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
>>>>>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>>>>>> +
>>>>>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
>>>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>>>>>> +
>>>>>>>>>>> +The device MUST ignore all access to its Configuration Space
>>>>>>>>>>> +while suspended, except for \field{device status}.
>>>>>>>>>> ...if it is part of the configuration space.
>>>>>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> +A device MUST NOT send any notifications, access any
>>>>>>>>>>> +virtqueues, or modify any fields in its configuration space
>>>>>>>>>>> +while
>>>> suspended.
>>>>>>>>>>> +
>>>>>>>>>>> +If SUSPEND is set in \field{device status}, when the driver
>>>>>>>>>>> +clears SUSPEND,
>>>>>>>>>> "subsequently clears SUSPEND"?
>>>>>>>>>>
>>>>>>>>>>> +the device MUST either resume normal operation or set
>>>>>>>>>> DEVICE_NEEDS_RESET.
>>>>>>>>>>> +
>>>>>>>>>>> +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 processing more buffers of any virtqueues \item
>>>>>>>>>>> +Wait until all buffers that are being processed have been used.
>>>>>>>>>>> +\item Send used buffer notifications to the driver.
>>>>>>>>>>> +\end{itemize}
>>>>>>>>>> So, is there any opportunity for the device to fail setting SUSPEND?
>>>>>>>>>> I mean, if the driver is supposed to look whether it sticks,
>>>>>>>>>> there should be conditions for when the device might clear it again...
>>>>>>>>>>
>>>>>>>>> Additionally, a suspend operation usually involves saving things
>>>>>>>>> to a slow
>>>>>>>> memory (or media).
>>>>>>>>> This is because the device implementation wouldn't know when
>>>>>>>>> exactly the
>>>>>>>> device will be resumed.
>>>>>>>>> Few examples, are:
>>>>>>>>> a. A gpu device with 128MB of video RAM when suspended, QEMU
>>>> needs
>>>>>>>> to store this into a (for example) rotating hard disk as 1msec IO latency.
>>>>>>>>> b. a NIC may need to store its RSS, queues, flow filters
>>>>>>>>> configuration for
>>>>>>>> several tens of KBs to some slow memory.
>>>>>>>>> c. A block device may prefer to complete some IOs to a threshold
>>>>>>>>> level
>>>>>>>> instead of maintaining large list of outstanding IOs in some
>>>>>>>> suspended memory.
>>>>>>>>> d. May be more in future.
>>>>>>>>>
>>>>>>>>> Additionally, suspend operation needs to be synchronized with
>>>>>>>>> certain data
>>>>>>>> path hardware engines to suspend DMA operations (read and write
>>>>>>>> both) which are inflight.
>>>>>>>>> (without causing DMA errors). Same would apply to the sw backend
>>>>>>>> implementations too.
>>>>>>>>> Therefore, the suspend operation that is initiated by the
>>>>>>>>> driver, should get
>>>>>>>> the acknowledgement back from the device that it has been suspend.
>>>>>>>>> Some of the good examples if you prefer to follow, a
>>>>>>>>> driver<->device
>>>>>>>> interface needs a suspend register which should behave like below
>>>>>>>> queue reset register.
>>>>>>>>> Spec snippet:
>>>>>>>>> "The device MUST reset the queue when 1 is written to queue_reset.
>>>>>>>>> The device MUST continue to present
>>>>>>>>> 1 in queue_reset as long as the queue reset is ongoing. The
>>>>>>>>> device MUST present 0 in both queue_reset and queue_enable when
>>>>>>>>> queue reset
>>>>>>>> has completed."
>>>>>>>>> At minimum, we need, something implementable like,
>>>>>>>>>
>>>>>>>>> The device MUST suspend the device when 1 is written to the
>>>>>>>> device_suspend_ctlr. The device MUST continue to present 1 in
>>>>>>>> device_suspend_ctrl as the suspend operation is ongoing in the device.
>>>>>>>>> The device MUST present 0 in the device_suspend_ctlr register
>>>>>>>>> when
>>>>>>>> device has completely suspended the device.
>>>>>>>>> The device MUST resume the device when 2 is written to the
>>>>>>>> device_suspend_ctrl. The device MUST continue to present 2 in the
>>>>>>>> device_suspend_ctrl as the resume operation may not have yet
>>>> completed.
>>>>>>>>> The device MUST present 0 in the device_suspend_ctrl register
>>>>>>>>> when the
>>>>>>>> device has completely resumed the device.
>>>>>>>>> At this point, the driver may resume notifying the device and
>>>>>>>>> accessing the
>>>>>>>> configuration space.
>>>>>>>>> Can you please enhance this part?
>>>>>>>> Hi Parav
>>>>>>>>
>>>>>>>> Yes they are necessary contents and we will address them in the
>>>>>>>> following patches.
>>>>>>>>
>>>>>>> The register polarity is critical.
>>>>>>> In your reply to Cornelia, you descried that suspend bit cleared
>>>>>>> in the device
>>>>>> after driver sets it (until the device is suspended).
>>>>>>> This is ambiguous behavior of the register. Queue_reset register
>>>>>>> was like that
>>>>>> before it was fixed in 1.2.
>>>>>> once suspended, the driver should not access the configuration
>>>>>> space except for the device status, so it should not reset any vqs
>>>>>> through configuration space.
>>>>> I am not talking about VQ reset at all.
>>>>> I gave an example of how the VQ reset register polarity works (with
>>>>> was
>>>> broken like how the proposed suspend bit is broken).
>>>>> Suspend register needs to work the way the reset register works.
>>>>> Can you please go through my exact text I replied in the previous email?
>>>>>
>>>>> Basically, the bug is:
>>>>> When driver has initiated the suspend (writing 1), the device
>>>>> continue to
>>>> respond 0, while suspend is going on.
>>>>> At this point, just by looking at the device_status register one
>>>>> cannot figure
>>>> out what is going on in the device.
>>>>> Is driver initiated suspend is ongoing, or it was never started.
>>>>> It is ambiguous.
>>>> From the device perspective, there is only one user: the driver, and
>>>> the driver sets or clears SUSPEND, it knows the status of the device.
>>> It does not work elegantly when the hypervisor or other system is looking
>> dealing with this register.
>>> In current proposal, anyone else than the driver looking at the device_status,
>> it is ambiguous.
>>> For example,
>>> suspend_bit: 0x1 suspended.
>>> 	     0x0 not suspended or suspend is ongoing. (ambiguous for
>>> hypervisor debugging this device)
>>>
>>> For device migration, it requires yet another side bit in the device parts.
>>>
>>> An elegant and non-ambiguous interface can be,
>>>
>>> ctrl_register =	0x0 (a usual default)
>>> 		0x1, suspend the device
>>> 		0x2, resume the device.
>>> status_register =	0x0, (still) running.
>>> 			0x1, suspended.
>>>
>>> With this driver and hypervisor (both) has very clear view of what is going on
>> and what is the status.
>>> No more ambiguity for anyone.
>>>
>>> Please consider pros and cons of both approaches.
>> The driver "owns" the device, other components access the device
>> configurations through the driver, I am not sure there are other "owners" than
>> the driver.
> Device migration and debug channel [1] plans to access the device from the hypervisor.
> I don’t see any disadvantage of separate control and status register.
> Do you see any?
>
> [1] https://lore.kernel.org/virtio-comment/20240601145042.2074739-1-parav@nvidia.com/T/#m14b80fd996e8d2522723967d8af844b70e131a90
is the admin vq governed by the driver? There should be only one owner of the device, or you need locks.

I am not sure we want to repeat the discussion on the admin vq for live migration last year, you know it breaks nested.

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Wednesday, June 12, 2024 4:00 PM
> 
> On 6/12/2024 6:07 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Wednesday, June 12, 2024 2:50 PM
> >>
> >> On 6/11/2024 6:37 PM, Parav Pandit wrote:
> >>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>> Sent: Tuesday, June 11, 2024 3:42 PM
> >>>>
> >>>> On 6/11/2024 5:43 PM, Parav Pandit wrote:
> >>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>>> Sent: Tuesday, June 11, 2024 3:03 PM
> >>>>>>
> >>>>>> On 6/11/2024 4:48 PM, Parav Pandit wrote:
> >>>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>>>>> Sent: Tuesday, June 11, 2024 1:57 PM
> >>>>>>>>
> >>>>>>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
> >>>>>>>>> Hi Lingshan, David,
> >>>>>>>>>
> >>>>>>>>>> From: Cornelia Huck <cohuck@redhat.com>
> >>>>>>>>>> Sent: Monday, June 10, 2024 9:34 PM
> >>>>>>>>>>
> >>>>>>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com>
> 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.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>>>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>>>>>>>>>> ---
> >>>>>>>>>>>  content.tex | 69
> >>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++------
> --
> >>>>>>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
> >>>>>>>>>> Can you please add a changelog? Especially as some of the
> >>>>>>>>>> previous discussion has been lost due to the broken old
> >>>>>>>>>> mailing
> >> lists...
> >>>>>>>>>> (...)
> >>>>>>>>>>
> >>>>>>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
> >>>>>>>>>>> +Initialization And Device Operation / Device Suspend}
> >>>>>>>>>>> +
> >>>>>>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set
> >>>>>>>>>>> +or
> >>>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>>>>>>>> +
> >>>>>>>>>>> +Once the driver sets SUSPEND to \field{device status} of
> >>>>>>>>>>> +the
> >> device:
> >>>>>>>>>>> +\begin{itemize}
> >>>>>>>>>>> +\item The driver MUST re-read \field{device status} to
> >>>>>>>>>>> +verify whether the
> >>>>>>>>>> SUSPEND bit is set.
> >>>>>>>>>>> +If not, the device does not support the SUSPEND feature.
> >>>>>>>>>> That sentence is a bit weird: I'd expect the device to not
> >>>>>>>>>> offer VIRTIO_F_SUSPEND in the first place in that case...
> >>>>>>>>>> could this rather happen if the device is not able to handle
> >>>>>>>>>> the request at a specific point in
> >>>>>>>> time?
> >>>>>>>>>>> +\item The driver MUST NOT make any more buffers available
> >>>>>>>>>>> +to the
> >>>>>>>> device.
> >>>>>>>>>>> +\item The driver MUST NOT access any fields of any
> >>>>>>>>>>> +virtqueues or notify
> >>>>>>>>>> any virtqueues.
> >>>>>>>>>>
> >>>>>>>>>> "send notifications for any virtqueues"?
> >>>>>>>>>>
> >>>>>>>>>>> +\item The driver MUST NOT access Device Configuration Space.
> >>>>>>>>>> ...except for the status field, if it is part of the config space?
> >>>>>>>>>>
> >>>>>>>>>>> +\end{itemize}
> >>>>>>>>>>> +
> >>>>>>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
> >>>>>>>>>>> +Initialization And Device Operation / Device Suspend}
> >>>>>>>>>>> +
> >>>>>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> >>>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
> >>>>>>>>>>> +
> >>>>>>>>>>> +The device MUST ignore all access to its Configuration
> >>>>>>>>>>> +Space while suspended, except for \field{device status}.
> >>>>>>>>>> ...if it is part of the configuration space.
> >>>>>>>>>>
> >>>>>>>>>>> +
> >>>>>>>>>>> +A device MUST NOT send any notifications, access any
> >>>>>>>>>>> +virtqueues, or modify any fields in its configuration space
> >>>>>>>>>>> +while
> >>>> suspended.
> >>>>>>>>>>> +
> >>>>>>>>>>> +If SUSPEND is set in \field{device status}, when the driver
> >>>>>>>>>>> +clears SUSPEND,
> >>>>>>>>>> "subsequently clears SUSPEND"?
> >>>>>>>>>>
> >>>>>>>>>>> +the device MUST either resume normal operation or set
> >>>>>>>>>> DEVICE_NEEDS_RESET.
> >>>>>>>>>>> +
> >>>>>>>>>>> +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 processing more buffers of any virtqueues \item
> >>>>>>>>>>> +Wait until all buffers that are being processed have been used.
> >>>>>>>>>>> +\item Send used buffer notifications to the driver.
> >>>>>>>>>>> +\end{itemize}
> >>>>>>>>>> So, is there any opportunity for the device to fail setting
> SUSPEND?
> >>>>>>>>>> I mean, if the driver is supposed to look whether it sticks,
> >>>>>>>>>> there should be conditions for when the device might clear it
> again...
> >>>>>>>>>>
> >>>>>>>>> Additionally, a suspend operation usually involves saving
> >>>>>>>>> things to a slow
> >>>>>>>> memory (or media).
> >>>>>>>>> This is because the device implementation wouldn't know when
> >>>>>>>>> exactly the
> >>>>>>>> device will be resumed.
> >>>>>>>>> Few examples, are:
> >>>>>>>>> a. A gpu device with 128MB of video RAM when suspended,
> QEMU
> >>>> needs
> >>>>>>>> to store this into a (for example) rotating hard disk as 1msec IO
> latency.
> >>>>>>>>> b. a NIC may need to store its RSS, queues, flow filters
> >>>>>>>>> configuration for
> >>>>>>>> several tens of KBs to some slow memory.
> >>>>>>>>> c. A block device may prefer to complete some IOs to a
> >>>>>>>>> threshold level
> >>>>>>>> instead of maintaining large list of outstanding IOs in some
> >>>>>>>> suspended memory.
> >>>>>>>>> d. May be more in future.
> >>>>>>>>>
> >>>>>>>>> Additionally, suspend operation needs to be synchronized with
> >>>>>>>>> certain data
> >>>>>>>> path hardware engines to suspend DMA operations (read and write
> >>>>>>>> both) which are inflight.
> >>>>>>>>> (without causing DMA errors). Same would apply to the sw
> >>>>>>>>> backend
> >>>>>>>> implementations too.
> >>>>>>>>> Therefore, the suspend operation that is initiated by the
> >>>>>>>>> driver, should get
> >>>>>>>> the acknowledgement back from the device that it has been
> suspend.
> >>>>>>>>> Some of the good examples if you prefer to follow, a
> >>>>>>>>> driver<->device
> >>>>>>>> interface needs a suspend register which should behave like
> >>>>>>>> below queue reset register.
> >>>>>>>>> Spec snippet:
> >>>>>>>>> "The device MUST reset the queue when 1 is written to
> queue_reset.
> >>>>>>>>> The device MUST continue to present
> >>>>>>>>> 1 in queue_reset as long as the queue reset is ongoing. The
> >>>>>>>>> device MUST present 0 in both queue_reset and queue_enable
> >>>>>>>>> when queue reset
> >>>>>>>> has completed."
> >>>>>>>>> At minimum, we need, something implementable like,
> >>>>>>>>>
> >>>>>>>>> The device MUST suspend the device when 1 is written to the
> >>>>>>>> device_suspend_ctlr. The device MUST continue to present 1 in
> >>>>>>>> device_suspend_ctrl as the suspend operation is ongoing in the
> device.
> >>>>>>>>> The device MUST present 0 in the device_suspend_ctlr register
> >>>>>>>>> when
> >>>>>>>> device has completely suspended the device.
> >>>>>>>>> The device MUST resume the device when 2 is written to the
> >>>>>>>> device_suspend_ctrl. The device MUST continue to present 2 in
> >>>>>>>> the device_suspend_ctrl as the resume operation may not have
> >>>>>>>> yet
> >>>> completed.
> >>>>>>>>> The device MUST present 0 in the device_suspend_ctrl register
> >>>>>>>>> when the
> >>>>>>>> device has completely resumed the device.
> >>>>>>>>> At this point, the driver may resume notifying the device and
> >>>>>>>>> accessing the
> >>>>>>>> configuration space.
> >>>>>>>>> Can you please enhance this part?
> >>>>>>>> Hi Parav
> >>>>>>>>
> >>>>>>>> Yes they are necessary contents and we will address them in the
> >>>>>>>> following patches.
> >>>>>>>>
> >>>>>>> The register polarity is critical.
> >>>>>>> In your reply to Cornelia, you descried that suspend bit cleared
> >>>>>>> in the device
> >>>>>> after driver sets it (until the device is suspended).
> >>>>>>> This is ambiguous behavior of the register. Queue_reset register
> >>>>>>> was like that
> >>>>>> before it was fixed in 1.2.
> >>>>>> once suspended, the driver should not access the configuration
> >>>>>> space except for the device status, so it should not reset any
> >>>>>> vqs through configuration space.
> >>>>> I am not talking about VQ reset at all.
> >>>>> I gave an example of how the VQ reset register polarity works
> >>>>> (with was
> >>>> broken like how the proposed suspend bit is broken).
> >>>>> Suspend register needs to work the way the reset register works.
> >>>>> Can you please go through my exact text I replied in the previous email?
> >>>>>
> >>>>> Basically, the bug is:
> >>>>> When driver has initiated the suspend (writing 1), the device
> >>>>> continue to
> >>>> respond 0, while suspend is going on.
> >>>>> At this point, just by looking at the device_status register one
> >>>>> cannot figure
> >>>> out what is going on in the device.
> >>>>> Is driver initiated suspend is ongoing, or it was never started.
> >>>>> It is ambiguous.
> >>>> From the device perspective, there is only one user: the driver,
> >>>> and the driver sets or clears SUSPEND, it knows the status of the device.
> >>> It does not work elegantly when the hypervisor or other system is
> >>> looking
> >> dealing with this register.
> >>> In current proposal, anyone else than the driver looking at the
> >>> device_status,
> >> it is ambiguous.
> >>> For example,
> >>> suspend_bit: 0x1 suspended.
> >>> 	     0x0 not suspended or suspend is ongoing. (ambiguous for
> >>> hypervisor debugging this device)
> >>>
> >>> For device migration, it requires yet another side bit in the device parts.
> >>>
> >>> An elegant and non-ambiguous interface can be,
> >>>
> >>> ctrl_register =	0x0 (a usual default)
> >>> 		0x1, suspend the device
> >>> 		0x2, resume the device.
> >>> status_register =	0x0, (still) running.
> >>> 			0x1, suspended.
> >>>
> >>> With this driver and hypervisor (both) has very clear view of what
> >>> is going on
> >> and what is the status.
> >>> No more ambiguity for anyone.
> >>>
> >>> Please consider pros and cons of both approaches.
> >> The driver "owns" the device, other components access the device
> >> configurations through the driver, I am not sure there are other
> >> "owners" than the driver.
> > Device migration and debug channel [1] plans to access the device from the
> hypervisor.
> > I don't see any disadvantage of separate control and status register.
> > Do you see any?
> >
> > [1]
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > .kernel.org%2Fvirtio-comment%2F20240601145042.2074739-1-
> parav%40nvidia
> >
> .com%2FT%2F%23m14b80fd996e8d2522723967d8af844b70e131a90&dat
> a=05%7C02%7
> >
> Cparav%40nvidia.com%7C06c09157b2d74af8b7f608dc8acaab0a%7C43083
> d1572734
> >
> 0c1b7db39efd9ccc17a%7C0%7C0%7C638537850271999791%7CUnknown
> %7CTWFpbGZsb
> >
> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%
> >
> 7C0%7C%7C%7C&sdata=e%2BBDJMpuFzmj47CKqnWU1jXcRZUXATmqhhMZ
> 7WC8zj8%3D&re
> > served=0
> is the admin vq governed by the driver? There should be only one owner of
> the device, or you need locks.
> 
No locks are needed. Hypervisor is accessing the admin vq on the PF.
VF is accessing the suspend ctrl and status register of its own from its own MMIO space.
If for some reason, this registers is trapped and accessed in the hypervisor too, it is upto the hypervisor on lock/no-lock etc.

> I am not sure we want to repeat the discussion on the admin vq for live
> migration last year, you know it breaks nested.
I am showing you the use case where ctrl+status is useful without any negatives.
You are diverging the discussion to a point of no return.
I wish you can be more cooperative by seeing larger picture. :)

You didn't answer my question, do you see any disadvantage of ctrl+status register?
If not, lets converge and progress as it enables suspend functionality + device migration both using non-ambiguous register pair.

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

[Michael S. Tsirkin]

On Tue, Jun 11, 2024 at 05:17:32AM +0000, Parav Pandit wrote:
> Hi Lingshan, David,
> 
> > From: Cornelia Huck <cohuck@redhat.com>
> > Sent: Monday, June 10, 2024 9:34 PM
> > 
> > On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> > >
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > ---
> > >  content.tex | 69
> > > +++++++++++++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 59 insertions(+), 10 deletions(-)
> > 
> > Can you please add a changelog? Especially as some of the previous discussion
> > has been lost due to the broken old mailing lists...
> > 
> > (...)
> > 
> > > +\drivernormative{\subsection}{Device Suspend}{General Initialization
> > > +And Device Operation / Device Suspend}
> > > +
> > > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or
> > VIRTIO_F_SUSPEND is not negotiated.
> > > +
> > > +Once the driver sets SUSPEND to \field{device status} of the device:
> > > +\begin{itemize}
> > > +\item The driver MUST re-read \field{device status} to verify whether the
> > SUSPEND bit is set.
> > > +If not, the device does not support the SUSPEND feature.
> > 
> > That sentence is a bit weird: I'd expect the device to not offer
> > VIRTIO_F_SUSPEND in the first place in that case... could this rather happen if
> > the device is not able to handle the request at a specific point in time?
> > 
> > > +\item The driver MUST NOT make any more buffers available to the device.
> > > +\item The driver MUST NOT access any fields of any virtqueues or notify
> > any virtqueues.
> > 
> > "send notifications for any virtqueues"?
> > 
> > > +\item The driver MUST NOT access Device Configuration Space.
> > 
> > ...except for the status field, if it is part of the config space?
> > 
> > > +\end{itemize}
> > > +
> > > +\devicenormative{\subsection}{Device Suspend}{General Initialization
> > > +And Device Operation / Device Suspend}
> > > +
> > > +The device MUST ignore SUSPEND if FEATURES_OK is not set or
> > VIRTIO_F_SUSPEND is not negotiated.
> > > +
> > > +The device MUST ignore all access to its Configuration Space while
> > > +suspended, except for \field{device status}.
> > 
> > ...if it is part of the configuration space.
> > 
> > > +
> > > +A device MUST NOT send any notifications, access any virtqueues, or
> > > +modify any fields in its configuration space while suspended.
> > > +
> > > +If SUSPEND is set in \field{device status}, when the driver clears
> > > +SUSPEND,
> > 
> > "subsequently clears SUSPEND"?
> > 
> > > +the device MUST either resume normal operation or set
> > DEVICE_NEEDS_RESET.
> > > +
> > > +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 processing more buffers of any virtqueues \item Wait until
> > > +all buffers that are being processed have been used.
> > > +\item Send used buffer notifications to the driver.
> > > +\end{itemize}
> > 
> > So, is there any opportunity for the device to fail setting SUSPEND? I mean, if
> > the driver is supposed to look whether it sticks, there should be conditions for
> > when the device might clear it again...
> > 
> Additionally, a suspend operation usually involves saving things to a slow memory (or media).
> This is because the device implementation wouldn’t know when exactly the device will be resumed.
> Few examples, are:
> a. A gpu device with 128MB of video RAM when suspended, QEMU needs to store this into a (for example) rotating hard disk as 1msec IO latency.
> 
> b. a NIC may need to store its RSS, queues, flow filters configuration for several tens of KBs to some slow memory.
> 
> c. A block device may prefer to complete some IOs to a threshold level instead of maintaining large list of outstanding IOs in some suspended memory.
> 
> d. May be more in future.
> 
> Additionally, suspend operation needs to be synchronized with certain data path hardware engines to suspend DMA operations (read and write both) which are inflight.
> (without causing DMA errors). Same would apply to the sw backend implementations too.
> 
> Therefore, the suspend operation that is initiated by the driver, should get the acknowledgement back from the device that it has been suspend.
> 
> Some of the good examples if you prefer to follow, a driver<->device interface needs a suspend register which should behave like below queue reset register.
> 
> Spec snippet:
> "The device MUST reset the queue when 1 is written to queue_reset. The device MUST continue to present
> 1 in queue_reset as long as the queue reset is ongoing. The device MUST present 0 in both queue_reset
> and queue_enable when queue reset has completed."
> 
> At minimum, we need, something implementable like,
> 
> The device MUST suspend the device when 1 is written to the device_suspend_ctlr. The device MUST continue to present 1 in device_suspend_ctrl as the suspend operation is ongoing in the device.
> The device MUST present 0 in the device_suspend_ctlr register when device has completely suspended the device.
> 
> The device MUST resume the device when 2 is written to the device_suspend_ctrl. The device MUST continue to present 2 in the device_suspend_ctrl as the resume operation may not have yet completed.
> The device MUST present 0 in the device_suspend_ctrl register when the device has completely resumed the device.
> At this point, the driver may resume notifying the device and accessing the configuration space.
> 
> Can you please enhance this part?

Yea, this gives me pause too.


> 
> > > +
> > >  \chapter{Virtio Transport Options}\label{sec:Virtio Transport
> > > Options}
> > >
> > >  Virtio can use various different buses, thus the standard is split @@
> > > -872,6 +917,10 @@ \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
> > > +   set SUSPEND to the device.
> > 
> > "that the driver can trigger suspending the device via the SUSPEND flag"?
> > 
> > > +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> > > +
> > >  \end{description}
> > >
> > >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
> > > Bits}
> > 
> 

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

[Michael S. Tsirkin]

On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
> No locks are needed. Hypervisor is accessing the admin vq on the PF.

I'm not even going to try and get through a jumble of this thread
heavily corrupted by outlook and whatnot, but from the glimpses I got
this discussion looks like just two people talking past each other.
Lingshan, Parav's not the only one bothered by how the device seemingly
has to block a transaction until it completes suspend. Parav, I feel how
to address this is up to the author to resolve really, leave it at that.

Thanks,

-- 
MST

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

[Michael S. Tsirkin]

On Wed, Jun 12, 2024 at 05:53:56PM +0800, Zhu Lingshan wrote:
> 
> 
> On 6/12/2024 12:26 AM, Michael S. Tsirkin wrote:
> > On Tue, Jun 11, 2024 at 04:20:42PM +0800, Zhu Lingshan wrote:
> >>
> >> On 6/11/2024 12:04 AM, Cornelia Huck wrote:
> >>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com> 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.
> >>>>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
> >>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>> Signed-off-by: David Stevens <stevensd@chromium.org>
> >>>> ---
> >>>>  content.tex | 69 +++++++++++++++++++++++++++++++++++++++++++++--------
> >>>>  1 file changed, 59 insertions(+), 10 deletions(-)
> >>> Can you please add a changelog? Especially as some of the previous
> >>> discussion has been lost due to the broken old mailing lists...
> >>>
> >>> (...)
> >> will add change log in next version.
> >>>> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >>>> +
> >>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> >>>> +
> >>>> +Once the driver sets SUSPEND to \field{device status} of the device:
> >>>> +\begin{itemize}
> >>>> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> >>>> +If not, the device does not support the SUSPEND feature.
> >>> That sentence is a bit weird: I'd expect the device to not offer
> >>> VIRTIO_F_SUSPEND in the first place in that case... could this rather
> >>> happen if the device is not able to handle the request at a specific
> >>> point in time?
> >> Yes, this can be improved
> >>
> >> How about:
> >> If not, this indicating the device is not able to handle SUSPEND at the moment.
> >
> > I don't get what this means, agrammatical and vague.
> > Please make it explicit. Do you mean if driver reads
> > status and SUSPEND is clear then devide failed to
> > enter suspend state? Might be problematic: time to 
> > suspend might be quite high.
> I think this is a common problem how the driver handle changes of the device status.
> Even without this SUSPEND feature, spec section 3.1.1 Driver Requirements: Device Initialization says:
> “Re-read device status to ensure the FEATURES_OK bit is still set: otherwise, the device does not support our subset of features and the device is unusable”
> Other status changing may also be costly, so how long should the driver wait for the device responding to status changes?

The driver does not wait with FEATURES_OK. It does a single write
followed by a single read. If it sees 0 there, then these features
do not work.


> This currently depends on the driver's tolerance.
> What should the driver do when overdue? Shall it reset the device or set FAILED?

Actually drivers in the field can and do change features and try again,
that is also an option.

> I think the device should either SUSPEND successfully or set NEEDS_RESET, but maybe not a good idea to set a time-out in the spec,
> the driver can set FAILED or just reset the device if the operation exceeds the tolerance in pulling.
> 
> Any suggestions?

Seems ok to me.


> >
> >
> >>>> +\item The driver MUST NOT make any more buffers available to the device.
> >>>> +\item The driver MUST NOT access any fields of any virtqueues or notify any virtqueues.
> >>> "send notifications for any virtqueues"?
> >> OK
> >>>> +\item The driver MUST NOT access Device Configuration Space.
> >>> ...except for the status field, if it is part of the config space?
> >> Yes, that is the original design, then I am convinced that the status fields is
> >> not a part of the config space because it has a dedicated section in the spec.
> >>
> >> I will add this sentence in the next version.
> >>>> +\end{itemize}
> >>>> +
> >>>> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >>>> +
> >>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> >>>> +
> >>>> +The device MUST ignore all access to its Configuration Space while
> >>>> +suspended, except for \field{device status}.
> >>> ...if it is part of the configuration space.
> >> Yes
> >>>> +
> >>>> +A device MUST NOT send any notifications, access any virtqueues, or modify
> >>>> +any fields in its configuration space while suspended.
> >>>> +
> >>>> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
> >>> "subsequently clears SUSPEND"?
> >> I am a native speaker, but "subsequently" sounds like clearing SUSPEND right after "setting SUSPEND", I guess there can be
> >> some operations during SUSPEND.
> >>
> >>>> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
> >>>> +
> >>>> +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 processing more buffers of any virtqueues
> >>>> +\item Wait until all buffers that are being processed have been used.
> >>>> +\item Send used buffer notifications to the driver.
> >>>> +\end{itemize}
> >>> So, is there any opportunity for the device to fail setting SUSPEND? I
> >>> mean, if the driver is supposed to look whether it sticks, there should
> >>> be conditions for when the device might clear it again...
> >> The device should not present SUSPEND bit in the device status until it is suspended,
> >> so I am afraid there is not a SUSPEND bit to clear if the device fails to suspend itself.
> >>
> >> I think there can be two indications of a failed suspend:
> >> 1) The driver sets SUSPEND, but the device does not present SUSPEND after a while, depends
> >> on the driver tolerance, the driver may assume it is a failed SUSPEND, like how we handle FEATURES_OK
> >
> > This is not how we handle FEATURES_OK.
> setting FEATURES_OK may not always be successful and can take longer time than expect. What should the driver
> do when the pulling re-reads does not return FEATURES_OK?
> >
> >> 2)The device runs into errors, presenting NEEDS_RESET.
> >>
> > More reasonable.
> OK, so the driver should re-read the status, it should see either SUSPEND or NEEDS_RESET, this works for me.
> 
> Thanks

Or neither to mean "suspend in progress"?


> >
> >>>> +
> >>>>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
> >>>>  
> >>>>  Virtio can use various different buses, thus the standard is split
> >>>> @@ -872,6 +917,10 @@ \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
> >>>> +   set SUSPEND to the device.
> >>> "that the driver can trigger suspending the device via the SUSPEND flag"?
> >> Yes
> >>>> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> >>>> +
> >>>>  \end{description}
> >>>>  
> >>>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

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

[David Stevens]

On Wed, Jun 12, 2024 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
> > No locks are needed. Hypervisor is accessing the admin vq on the PF.
>
> I'm not even going to try and get through a jumble of this thread
> heavily corrupted by outlook and whatnot, but from the glimpses I got
> this discussion looks like just two people talking past each other.
> Lingshan, Parav's not the only one bothered by how the device seemingly
> has to block a transaction until it completes suspend. Parav, I feel how
> to address this is up to the author to resolve really, leave it at that.

IIUC, Prava's concern here is that with the current proposal, the
device's state can't be determined unambiguously by reading the device
status field. If the SUSPEND bit is 0, it's ambiguous as to whether
the device is running normally or if it's in the process of
suspending, and there is a corresponding issue for suspended vs
resuming. The device and driver can know based on their internal
information what state the device is in. However, some hypothetical
admin tool in guest userspace wouldn't be able to tell simply by
reading the values in the PCI BAR.

Assuming that's actually a problem that we want to solve, then I think
the most straightforward way to address this would be to use another
bit in the status field as a SUSPEND_TRANSITION bit, with something
like:

"After the driver writes or clears the SUSPEND bit, the device MUST
present the SUSPEND_TRANSITION bit until it completes the requested
suspend or resume transition. If the device is unable to successfully
complete the transition, it MUST present DEVICE_NEEDS_RESET."

Doing that would give distinct values for each of the
running/suspending/suspended/resuming states. That said, although the
device status field doesn't have a defined width, both the PCI and
channel transports assume that it's a single byte, so we'd be using
the last remaining bits those transports have available.

-David

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

[Zhu Lingshan]

On 6/12/2024 7:26 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Wednesday, June 12, 2024 4:00 PM
>>
>> On 6/12/2024 6:07 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>> Sent: Wednesday, June 12, 2024 2:50 PM
>>>>
>>>> On 6/11/2024 6:37 PM, Parav Pandit wrote:
>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>> Sent: Tuesday, June 11, 2024 3:42 PM
>>>>>>
>>>>>> On 6/11/2024 5:43 PM, Parav Pandit wrote:
>>>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>>> Sent: Tuesday, June 11, 2024 3:03 PM
>>>>>>>>
>>>>>>>> On 6/11/2024 4:48 PM, Parav Pandit wrote:
>>>>>>>>>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>>>>> Sent: Tuesday, June 11, 2024 1:57 PM
>>>>>>>>>>
>>>>>>>>>> On 6/11/2024 1:17 PM, Parav Pandit wrote:
>>>>>>>>>>> Hi Lingshan, David,
>>>>>>>>>>>
>>>>>>>>>>>> From: Cornelia Huck <cohuck@redhat.com>
>>>>>>>>>>>> Sent: Monday, June 10, 2024 9:34 PM
>>>>>>>>>>>>
>>>>>>>>>>>> On Fri, Jun 07 2024, Zhu Lingshan <lingshan.zhu@amd.com>
>> 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.
>>>>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@amd.com>
>>>>>>>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>>>>>>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>  content.tex | 69
>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++++++++++++++------
>> --
>>>>>>>>>>>>>  1 file changed, 59 insertions(+), 10 deletions(-)
>>>>>>>>>>>> Can you please add a changelog? Especially as some of the
>>>>>>>>>>>> previous discussion has been lost due to the broken old
>>>>>>>>>>>> mailing
>>>> lists...
>>>>>>>>>>>> (...)
>>>>>>>>>>>>
>>>>>>>>>>>>> +\drivernormative{\subsection}{Device Suspend}{General
>>>>>>>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set
>>>>>>>>>>>>> +or
>>>>>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +Once the driver sets SUSPEND to \field{device status} of
>>>>>>>>>>>>> +the
>>>> device:
>>>>>>>>>>>>> +\begin{itemize}
>>>>>>>>>>>>> +\item The driver MUST re-read \field{device status} to
>>>>>>>>>>>>> +verify whether the
>>>>>>>>>>>> SUSPEND bit is set.
>>>>>>>>>>>>> +If not, the device does not support the SUSPEND feature.
>>>>>>>>>>>> That sentence is a bit weird: I'd expect the device to not
>>>>>>>>>>>> offer VIRTIO_F_SUSPEND in the first place in that case...
>>>>>>>>>>>> could this rather happen if the device is not able to handle
>>>>>>>>>>>> the request at a specific point in
>>>>>>>>>> time?
>>>>>>>>>>>>> +\item The driver MUST NOT make any more buffers available
>>>>>>>>>>>>> +to the
>>>>>>>>>> device.
>>>>>>>>>>>>> +\item The driver MUST NOT access any fields of any
>>>>>>>>>>>>> +virtqueues or notify
>>>>>>>>>>>> any virtqueues.
>>>>>>>>>>>>
>>>>>>>>>>>> "send notifications for any virtqueues"?
>>>>>>>>>>>>
>>>>>>>>>>>>> +\item The driver MUST NOT access Device Configuration Space.
>>>>>>>>>>>> ...except for the status field, if it is part of the config space?
>>>>>>>>>>>>
>>>>>>>>>>>>> +\end{itemize}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +\devicenormative{\subsection}{Device Suspend}{General
>>>>>>>>>>>>> +Initialization And Device Operation / Device Suspend}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +The device MUST ignore SUSPEND if FEATURES_OK is not set or
>>>>>>>>>>>> VIRTIO_F_SUSPEND is not negotiated.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +The device MUST ignore all access to its Configuration
>>>>>>>>>>>>> +Space while suspended, except for \field{device status}.
>>>>>>>>>>>> ...if it is part of the configuration space.
>>>>>>>>>>>>
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +A device MUST NOT send any notifications, access any
>>>>>>>>>>>>> +virtqueues, or modify any fields in its configuration space
>>>>>>>>>>>>> +while
>>>>>> suspended.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +If SUSPEND is set in \field{device status}, when the driver
>>>>>>>>>>>>> +clears SUSPEND,
>>>>>>>>>>>> "subsequently clears SUSPEND"?
>>>>>>>>>>>>
>>>>>>>>>>>>> +the device MUST either resume normal operation or set
>>>>>>>>>>>> DEVICE_NEEDS_RESET.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +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 processing more buffers of any virtqueues \item
>>>>>>>>>>>>> +Wait until all buffers that are being processed have been used.
>>>>>>>>>>>>> +\item Send used buffer notifications to the driver.
>>>>>>>>>>>>> +\end{itemize}
>>>>>>>>>>>> So, is there any opportunity for the device to fail setting
>> SUSPEND?
>>>>>>>>>>>> I mean, if the driver is supposed to look whether it sticks,
>>>>>>>>>>>> there should be conditions for when the device might clear it
>> again...
>>>>>>>>>>> Additionally, a suspend operation usually involves saving
>>>>>>>>>>> things to a slow
>>>>>>>>>> memory (or media).
>>>>>>>>>>> This is because the device implementation wouldn't know when
>>>>>>>>>>> exactly the
>>>>>>>>>> device will be resumed.
>>>>>>>>>>> Few examples, are:
>>>>>>>>>>> a. A gpu device with 128MB of video RAM when suspended,
>> QEMU
>>>>>> needs
>>>>>>>>>> to store this into a (for example) rotating hard disk as 1msec IO
>> latency.
>>>>>>>>>>> b. a NIC may need to store its RSS, queues, flow filters
>>>>>>>>>>> configuration for
>>>>>>>>>> several tens of KBs to some slow memory.
>>>>>>>>>>> c. A block device may prefer to complete some IOs to a
>>>>>>>>>>> threshold level
>>>>>>>>>> instead of maintaining large list of outstanding IOs in some
>>>>>>>>>> suspended memory.
>>>>>>>>>>> d. May be more in future.
>>>>>>>>>>>
>>>>>>>>>>> Additionally, suspend operation needs to be synchronized with
>>>>>>>>>>> certain data
>>>>>>>>>> path hardware engines to suspend DMA operations (read and write
>>>>>>>>>> both) which are inflight.
>>>>>>>>>>> (without causing DMA errors). Same would apply to the sw
>>>>>>>>>>> backend
>>>>>>>>>> implementations too.
>>>>>>>>>>> Therefore, the suspend operation that is initiated by the
>>>>>>>>>>> driver, should get
>>>>>>>>>> the acknowledgement back from the device that it has been
>> suspend.
>>>>>>>>>>> Some of the good examples if you prefer to follow, a
>>>>>>>>>>> driver<->device
>>>>>>>>>> interface needs a suspend register which should behave like
>>>>>>>>>> below queue reset register.
>>>>>>>>>>> Spec snippet:
>>>>>>>>>>> "The device MUST reset the queue when 1 is written to
>> queue_reset.
>>>>>>>>>>> The device MUST continue to present
>>>>>>>>>>> 1 in queue_reset as long as the queue reset is ongoing. The
>>>>>>>>>>> device MUST present 0 in both queue_reset and queue_enable
>>>>>>>>>>> when queue reset
>>>>>>>>>> has completed."
>>>>>>>>>>> At minimum, we need, something implementable like,
>>>>>>>>>>>
>>>>>>>>>>> The device MUST suspend the device when 1 is written to the
>>>>>>>>>> device_suspend_ctlr. The device MUST continue to present 1 in
>>>>>>>>>> device_suspend_ctrl as the suspend operation is ongoing in the
>> device.
>>>>>>>>>>> The device MUST present 0 in the device_suspend_ctlr register
>>>>>>>>>>> when
>>>>>>>>>> device has completely suspended the device.
>>>>>>>>>>> The device MUST resume the device when 2 is written to the
>>>>>>>>>> device_suspend_ctrl. The device MUST continue to present 2 in
>>>>>>>>>> the device_suspend_ctrl as the resume operation may not have
>>>>>>>>>> yet
>>>>>> completed.
>>>>>>>>>>> The device MUST present 0 in the device_suspend_ctrl register
>>>>>>>>>>> when the
>>>>>>>>>> device has completely resumed the device.
>>>>>>>>>>> At this point, the driver may resume notifying the device and
>>>>>>>>>>> accessing the
>>>>>>>>>> configuration space.
>>>>>>>>>>> Can you please enhance this part?
>>>>>>>>>> Hi Parav
>>>>>>>>>>
>>>>>>>>>> Yes they are necessary contents and we will address them in the
>>>>>>>>>> following patches.
>>>>>>>>>>
>>>>>>>>> The register polarity is critical.
>>>>>>>>> In your reply to Cornelia, you descried that suspend bit cleared
>>>>>>>>> in the device
>>>>>>>> after driver sets it (until the device is suspended).
>>>>>>>>> This is ambiguous behavior of the register. Queue_reset register
>>>>>>>>> was like that
>>>>>>>> before it was fixed in 1.2.
>>>>>>>> once suspended, the driver should not access the configuration
>>>>>>>> space except for the device status, so it should not reset any
>>>>>>>> vqs through configuration space.
>>>>>>> I am not talking about VQ reset at all.
>>>>>>> I gave an example of how the VQ reset register polarity works
>>>>>>> (with was
>>>>>> broken like how the proposed suspend bit is broken).
>>>>>>> Suspend register needs to work the way the reset register works.
>>>>>>> Can you please go through my exact text I replied in the previous email?
>>>>>>>
>>>>>>> Basically, the bug is:
>>>>>>> When driver has initiated the suspend (writing 1), the device
>>>>>>> continue to
>>>>>> respond 0, while suspend is going on.
>>>>>>> At this point, just by looking at the device_status register one
>>>>>>> cannot figure
>>>>>> out what is going on in the device.
>>>>>>> Is driver initiated suspend is ongoing, or it was never started.
>>>>>>> It is ambiguous.
>>>>>> From the device perspective, there is only one user: the driver,
>>>>>> and the driver sets or clears SUSPEND, it knows the status of the device.
>>>>> It does not work elegantly when the hypervisor or other system is
>>>>> looking
>>>> dealing with this register.
>>>>> In current proposal, anyone else than the driver looking at the
>>>>> device_status,
>>>> it is ambiguous.
>>>>> For example,
>>>>> suspend_bit: 0x1 suspended.
>>>>> 	     0x0 not suspended or suspend is ongoing. (ambiguous for
>>>>> hypervisor debugging this device)
>>>>>
>>>>> For device migration, it requires yet another side bit in the device parts.
>>>>>
>>>>> An elegant and non-ambiguous interface can be,
>>>>>
>>>>> ctrl_register =	0x0 (a usual default)
>>>>> 		0x1, suspend the device
>>>>> 		0x2, resume the device.
>>>>> status_register =	0x0, (still) running.
>>>>> 			0x1, suspended.
>>>>>
>>>>> With this driver and hypervisor (both) has very clear view of what
>>>>> is going on
>>>> and what is the status.
>>>>> No more ambiguity for anyone.
>>>>>
>>>>> Please consider pros and cons of both approaches.
>>>> The driver "owns" the device, other components access the device
>>>> configurations through the driver, I am not sure there are other
>>>> "owners" than the driver.
>>> Device migration and debug channel [1] plans to access the device from the
>> hypervisor.
>>> I don't see any disadvantage of separate control and status register.
>>> Do you see any?
>>>
>>> [1]
>>>
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
>>> .kernel.org%2Fvirtio-comment%2F20240601145042.2074739-1-
>> parav%40nvidia
>> .com%2FT%2F%23m14b80fd996e8d2522723967d8af844b70e131a90&dat
>> a=05%7C02%7
>> Cparav%40nvidia.com%7C06c09157b2d74af8b7f608dc8acaab0a%7C43083
>> d1572734
>> 0c1b7db39efd9ccc17a%7C0%7C0%7C638537850271999791%7CUnknown
>> %7CTWFpbGZsb
>> 3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
>> 0%3D%
>> 7C0%7C%7C%7C&sdata=e%2BBDJMpuFzmj47CKqnWU1jXcRZUXATmqhhMZ
>> 7WC8zj8%3D&re
>>> served=0
>> is the admin vq governed by the driver? There should be only one owner of
>> the device, or you need locks.
>>
> No locks are needed. Hypervisor is accessing the admin vq on the PF.
> VF is accessing the suspend ctrl and status register of its own from its own MMIO space.
> If for some reason, this registers is trapped and accessed in the hypervisor too, it is upto the hypervisor on lock/no-lock etc.
Even access device status from PF through admin vq, it is still trying to RW the unique vf->dev_status,
so there are too individuals accessing the same copy of resource, there need a lock.
>
>> I am not sure we want to repeat the discussion on the admin vq for live
>> migration last year, you know it breaks nested.
> I am showing you the use case where ctrl+status is useful without any negatives.
> You are diverging the discussion to a point of no return.
> I wish you can be more cooperative by seeing larger picture. :)
>
> You didn't answer my question, do you see any disadvantage of ctrl+status register?
> If not, lets converge and progress as it enables suspend functionality + device migration both using non-ambiguous register pair.
This is not about SUSPEND or ctrl + status. If we take the transition period into consideration, we need a total solution for all status transitions.
For example DRIVER_OK and FEATURES_OK, or the device setting NEEDS_RESET,
they can also take longer time than expect to take effects, depends on the device implementation.

Maybe we need a new bit in the device stauts: TRANSITION.

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

[Zhu Lingshan]

On 6/13/2024 1:58 PM, David Stevens wrote:
> On Wed, Jun 12, 2024 at 9:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
>>> No locks are needed. Hypervisor is accessing the admin vq on the PF.
>> I'm not even going to try and get through a jumble of this thread
>> heavily corrupted by outlook and whatnot, but from the glimpses I got
>> this discussion looks like just two people talking past each other.
>> Lingshan, Parav's not the only one bothered by how the device seemingly
>> has to block a transaction until it completes suspend. Parav, I feel how
>> to address this is up to the author to resolve really, leave it at that.
> IIUC, Prava's concern here is that with the current proposal, the
> device's state can't be determined unambiguously by reading the device
> status field. If the SUSPEND bit is 0, it's ambiguous as to whether
> the device is running normally or if it's in the process of
> suspending, and there is a corresponding issue for suspended vs
> resuming. The device and driver can know based on their internal
> information what state the device is in. However, some hypothetical
> admin tool in guest userspace wouldn't be able to tell simply by
> reading the values in the PCI BAR.
>
> Assuming that's actually a problem that we want to solve, then I think
> the most straightforward way to address this would be to use another
> bit in the status field as a SUSPEND_TRANSITION bit, with something
> like:
>
> "After the driver writes or clears the SUSPEND bit, the device MUST
> present the SUSPEND_TRANSITION bit until it completes the requested
> suspend or resume transition. If the device is unable to successfully
> complete the transition, it MUST present DEVICE_NEEDS_RESET."
>
> Doing that would give distinct values for each of the
> running/suspending/suspended/resuming states. That said, although the
> device status field doesn't have a defined width, both the PCI and
> channel transports assume that it's a single byte, so we'd be using
> the last remaining bits those transports have available.
Current spec does not handle status transitions well, not only for SUSPEND and resuming.
other status transitions may also take longer time than expect depends on how
the device is implemented, it can be sluggish if implemented by SW during high traffic,
even when handling FEATURES_OK or present NEEDS_RESET.

This patch wants to copy the mechanism of handling FEATURES_OK, but if we want to
resolve the problem of device status transition, I agree to add a new status bit: TRANSITION.

The device should present TRANSITION until it complete the requested status transition.
Combining the knowledge of the device, the driver can tell the exact device status.

Any thoughts?

Thanks
Zhu Lingshan
>
> -David

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@amd.com>
> Sent: Thursday, June 13, 2024 3:29 PM
> 
> On 6/13/2024 1:58 PM, David Stevens wrote:
> > On Wed, Jun 12, 2024 at 9:20 PM Michael S. Tsirkin <mst@redhat.com>
> wrote:
> >> On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
> >>> No locks are needed. Hypervisor is accessing the admin vq on the PF.
> >> I'm not even going to try and get through a jumble of this thread
> >> heavily corrupted by outlook and whatnot, but from the glimpses I got
> >> this discussion looks like just two people talking past each other.
> >> Lingshan, Parav's not the only one bothered by how the device
> >> seemingly has to block a transaction until it completes suspend.
> >> Parav, I feel how to address this is up to the author to resolve really, leave
> it at that.
> > IIUC, Prava's concern here is that with the current proposal, the
> > device's state can't be determined unambiguously by reading the device
> > status field. If the SUSPEND bit is 0, it's ambiguous as to whether
> > the device is running normally or if it's in the process of
> > suspending, and there is a corresponding issue for suspended vs
> > resuming. The device and driver can know based on their internal
> > information what state the device is in. However, some hypothetical
> > admin tool in guest userspace wouldn't be able to tell simply by
> > reading the values in the PCI BAR.
> >
> > Assuming that's actually a problem that we want to solve, then I think
> > the most straightforward way to address this would be to use another
> > bit in the status field as a SUSPEND_TRANSITION bit, with something
> > like:
> >
> > "After the driver writes or clears the SUSPEND bit, the device MUST
> > present the SUSPEND_TRANSITION bit until it completes the requested
> > suspend or resume transition. If the device is unable to successfully
> > complete the transition, it MUST present DEVICE_NEEDS_RESET."
> >
> > Doing that would give distinct values for each of the
> > running/suspending/suspended/resuming states. That said, although the
> > device status field doesn't have a defined width, both the PCI and
> > channel transports assume that it's a single byte, so we'd be using
> > the last remaining bits those transports have available.
> Current spec does not handle status transitions well, not only for SUSPEND
> and resuming.
> other status transitions may also take longer time than expect depends on
> how the device is implemented, it can be sluggish if implemented by SW
> during high traffic, even when handling FEATURES_OK or present
> NEEDS_RESET.
> 
> This patch wants to copy the mechanism of handling FEATURES_OK, but if we
> want to resolve the problem of device status transition, I agree to add a new
> status bit: TRANSITION.
> 
> The device should present TRANSITION until it complete the requested status
> transition.
> Combining the knowledge of the device, the driver can tell the exact device
> status.
> 
Yes, this will be good idea.
The TRANSITION bit which must turn very quickly to 1 on an MMIO write requires very special circuitry.
It needs to turn 1 just before the next read arrives from the guest driver.
This is roughly 10nsec of time (write followed by a read).

To simplify this aspect, a better approach can be,
1. device_status to have SUSPENDED bit, which starts with 0.
2. a new ctrl_register is added. When cmd is written there, its status reflects in device_status.

3. So even if the device takes 30nsec or 30usec to respond, things don’t break.

For example,
a. driver writes device_ctrl = 0x1 (cmd=suspend)
b. driver reads device_status = (suspended=0) -> not yet suspended
c. driver loops on #b few more nsecs.
d. finally device turns it suspended.  So #b returns supended=1.

And we can extend it more to better restructure the other operations too such as features_ok, driver_ok etc.
And can also make the current "reset" from SHOULD to MUST.

Its one time price to pay for the extra register but it is far easier to implement for the devices (and works fine for sw backend too).

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

[Jason Wang]

On Sat, Jun 15, 2024 at 12:33 PM Parav Pandit <parav@nvidia.com> wrote:
>
> > From: Zhu Lingshan <lingshan.zhu@amd.com>
> > Sent: Thursday, June 13, 2024 3:29 PM
> >
> > On 6/13/2024 1:58 PM, David Stevens wrote:
> > > On Wed, Jun 12, 2024 at 9:20 PM Michael S. Tsirkin <mst@redhat.com>
> > wrote:
> > >> On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
> > >>> No locks are needed. Hypervisor is accessing the admin vq on the PF.
> > >> I'm not even going to try and get through a jumble of this thread
> > >> heavily corrupted by outlook and whatnot, but from the glimpses I got
> > >> this discussion looks like just two people talking past each other.
> > >> Lingshan, Parav's not the only one bothered by how the device
> > >> seemingly has to block a transaction until it completes suspend.
> > >> Parav, I feel how to address this is up to the author to resolve really, leave
> > it at that.
> > > IIUC, Prava's concern here is that with the current proposal, the
> > > device's state can't be determined unambiguously by reading the device
> > > status field. If the SUSPEND bit is 0, it's ambiguous as to whether
> > > the device is running normally or if it's in the process of
> > > suspending, and there is a corresponding issue for suspended vs
> > > resuming. The device and driver can know based on their internal
> > > information what state the device is in. However, some hypothetical
> > > admin tool in guest userspace wouldn't be able to tell simply by
> > > reading the values in the PCI BAR.
> > >
> > > Assuming that's actually a problem that we want to solve, then I think
> > > the most straightforward way to address this would be to use another
> > > bit in the status field as a SUSPEND_TRANSITION bit, with something
> > > like:
> > >
> > > "After the driver writes or clears the SUSPEND bit, the device MUST
> > > present the SUSPEND_TRANSITION bit until it completes the requested
> > > suspend or resume transition. If the device is unable to successfully
> > > complete the transition, it MUST present DEVICE_NEEDS_RESET."
> > >
> > > Doing that would give distinct values for each of the
> > > running/suspending/suspended/resuming states. That said, although the
> > > device status field doesn't have a defined width, both the PCI and
> > > channel transports assume that it's a single byte, so we'd be using
> > > the last remaining bits those transports have available.
> > Current spec does not handle status transitions well, not only for SUSPEND
> > and resuming.
> > other status transitions may also take longer time than expect depends on
> > how the device is implemented, it can be sluggish if implemented by SW
> > during high traffic, even when handling FEATURES_OK or present
> > NEEDS_RESET.
> >
> > This patch wants to copy the mechanism of handling FEATURES_OK, but if we
> > want to resolve the problem of device status transition, I agree to add a new
> > status bit: TRANSITION.
> >
> > The device should present TRANSITION until it complete the requested status
> > transition.
> > Combining the knowledge of the device, the driver can tell the exact device
> > status.
> >
> Yes, this will be good idea.
> The TRANSITION bit which must turn very quickly to 1 on an MMIO write requires very special circuitry.
> It needs to turn 1 just before the next read arrives from the guest driver.
> This is roughly 10nsec of time (write followed by a read).
>
> To simplify this aspect, a better approach can be,
> 1. device_status to have SUSPENDED bit, which starts with 0.
> 2. a new ctrl_register is added. When cmd is written there, its status reflects in device_status.
>
> 3. So even if the device takes 30nsec or 30usec to respond, things don’t break.
>
> For example,
> a. driver writes device_ctrl = 0x1 (cmd=suspend)
> b. driver reads device_status = (suspended=0) -> not yet suspended
> c. driver loops on #b few more nsecs.
> d. finally device turns it suspended.  So #b returns supended=1.

Is there any real difference? Currently, transitions could be spotted
by the driver easily: read after write doesn't match.

>
> And we can extend it more to better restructure the other operations too such as features_ok, driver_ok etc.
> And can also make the current "reset" from SHOULD to MUST.
>
> Its one time price to pay for the extra register but it is far easier to implement for the devices (and works fine for sw backend too).

Well, I don't know why this needs to be addressed in this series as
it's not specific to suspend.

Thanks

>
>
>
>

RE: [PATCH v5] virtio: introduce SUSPEND bit in device status

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, June 17, 2024 7:52 AM
> 
> On Sat, Jun 15, 2024 at 12:33 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> > > From: Zhu Lingshan <lingshan.zhu@amd.com>
> > > Sent: Thursday, June 13, 2024 3:29 PM
> > >
> > > On 6/13/2024 1:58 PM, David Stevens wrote:
> > > > On Wed, Jun 12, 2024 at 9:20 PM Michael S. Tsirkin
> > > > <mst@redhat.com>
> > > wrote:
> > > >> On Wed, Jun 12, 2024 at 11:26:39AM +0000, Parav Pandit wrote:
> > > >>> No locks are needed. Hypervisor is accessing the admin vq on the PF.
> > > >> I'm not even going to try and get through a jumble of this thread
> > > >> heavily corrupted by outlook and whatnot, but from the glimpses I
> > > >> got this discussion looks like just two people talking past each other.
> > > >> Lingshan, Parav's not the only one bothered by how the device
> > > >> seemingly has to block a transaction until it completes suspend.
> > > >> Parav, I feel how to address this is up to the author to resolve
> > > >> really, leave
> > > it at that.
> > > > IIUC, Prava's concern here is that with the current proposal, the
> > > > device's state can't be determined unambiguously by reading the
> > > > device status field. If the SUSPEND bit is 0, it's ambiguous as to
> > > > whether the device is running normally or if it's in the process
> > > > of suspending, and there is a corresponding issue for suspended vs
> > > > resuming. The device and driver can know based on their internal
> > > > information what state the device is in. However, some
> > > > hypothetical admin tool in guest userspace wouldn't be able to
> > > > tell simply by reading the values in the PCI BAR.
> > > >
> > > > Assuming that's actually a problem that we want to solve, then I
> > > > think the most straightforward way to address this would be to use
> > > > another bit in the status field as a SUSPEND_TRANSITION bit, with
> > > > something
> > > > like:
> > > >
> > > > "After the driver writes or clears the SUSPEND bit, the device
> > > > MUST present the SUSPEND_TRANSITION bit until it completes the
> > > > requested suspend or resume transition. If the device is unable to
> > > > successfully complete the transition, it MUST present
> DEVICE_NEEDS_RESET."
> > > >
> > > > Doing that would give distinct values for each of the
> > > > running/suspending/suspended/resuming states. That said, although
> > > > the device status field doesn't have a defined width, both the PCI
> > > > and channel transports assume that it's a single byte, so we'd be
> > > > using the last remaining bits those transports have available.
> > > Current spec does not handle status transitions well, not only for
> > > SUSPEND and resuming.
> > > other status transitions may also take longer time than expect
> > > depends on how the device is implemented, it can be sluggish if
> > > implemented by SW during high traffic, even when handling
> > > FEATURES_OK or present NEEDS_RESET.
> > >
> > > This patch wants to copy the mechanism of handling FEATURES_OK, but
> > > if we want to resolve the problem of device status transition, I
> > > agree to add a new status bit: TRANSITION.
> > >
> > > The device should present TRANSITION until it complete the requested
> > > status transition.
> > > Combining the knowledge of the device, the driver can tell the exact
> > > device status.
> > >
> > Yes, this will be good idea.
> > The TRANSITION bit which must turn very quickly to 1 on an MMIO write
> requires very special circuitry.
> > It needs to turn 1 just before the next read arrives from the guest driver.
> > This is roughly 10nsec of time (write followed by a read).
> >
> > To simplify this aspect, a better approach can be, 1. device_status to
> > have SUSPENDED bit, which starts with 0.
> > 2. a new ctrl_register is added. When cmd is written there, its status
> reflects in device_status.
> >
> > 3. So even if the device takes 30nsec or 30usec to respond, things don’t
> break.
> >
> > For example,
> > a. driver writes device_ctrl = 0x1 (cmd=suspend) b. driver reads
> > device_status = (suspended=0) -> not yet suspended c. driver loops on
> > #b few more nsecs.
> > d. finally device turns it suspended.  So #b returns supended=1.
> 
> Is there any real difference? Currently, transitions could be spotted by the
> driver easily: read after write doesn't match.
>
Yes, the real difference is: simplified device implementation.
The device does not have the time constraint to reply (transition=1) to the incoming read in next 10nsec.