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

["Michael S. Tsirkin" <mst@redhat.com>]

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}