[virtio-dev] Re: [virtio-comment] [RFC PATCH 3/5] virtio: The actions by the device upon SUSPEND

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5403 bytes --]

On Tue, Aug 15, 2023 at 07:07:42PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 8/14/2023 11:00 PM, Stefan Hajnoczi wrote:
> > On Tue, Aug 15, 2023 at 03:29:02AM +0800, Zhu Lingshan wrote:
> > > This commit specifies the actions to be taken by the device upon
> > > SUSPEND.
> > > 
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > >   content.tex | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 074f43e..43bd5de 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -96,6 +96,15 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > >   and resumes operation upon DRIVER_OK.
> > > +If VIRTIO_F_SUSPEND is negotiated, when SUSPEND is set, the device MUST perform the following operations:
> > "when SUSPEND is set" is ambigious. It could mean when the driver writes
> > the Device Status Field, when the device transitions to SUSPEND, or
> > something else. Maybe "before the device reports the SUSPEND bit set in
> > the Device Status Field"?
> Nice catch! will fix: when the driver sets SUSPEND, before the device
> reports the
> SUSPEND bit set in the \field{device status}, the device MUST perform the
> following actions.
> > 
> > > +\begin{itemize}
> > > +\item Stop comsuming any descriptors
> > "consuming"
> yes
> > 
> > It may be more consistent to talk about virtqueue buffers (i.e.
> > requests) rather than descriptors here.
> yes
> > 
> > > +\item Mark all finished descriptors as used and send used buffer notification to the driver
> > The device has to complete everything that is in flight, just completing
> > "finished" stuff is not enough. Does "finished descriptors" really mean
> > "in-flight virtqueue buffers"?
> A patch of tracking in-flight descriptors is planned. Here I expect
> "finished" mean "done" buffers, transmitted and received ACK.

Yes, I don't mean tracking in-flight virtqueue buffers, I mean waiting
until they are marked as used. I think "finished" is unclear and the
text should explain that the device waits for in-flight virtqueue
buffers (in the future this could be relaxed with in-flight tracking
functionality).

> > 
> > "send a used buffer notification" or "send used buffer notifications"
> Yes will fix
> > 
> > > +\item Record Virtqueue State of each enabled virtqueue, see section \ref{sec:Virtqueues / Virtqueue State}
> > Does "Record" mean that Virtqueue State fields can only be accessed by
> > the driver while device is paused (they may be outdated or invalid while
> > the device is unpaused)?
> Yes, to avoid race with the device and make device implementation easier,
> this is also mentioned in Patch 4.
> > 
> > > +\item Pause its operation and preserve all configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > What does "preserve" mean? Does it mean the Device Configuration Space
> > is not allowed to change during SUSPEND?
> Yes, once the device presents SUSPEND in its device status, it should not
> change any
> fields in its Device Configuration Space.
> 
> Thanks for your review
> > 
> > > +\item Present SUSPEND in \field{device status}
> > > +\end{itemize}
> > > +
> > >   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> > >   Each virtio device offers all the features it understands.  During
> > > -- 
> > > 2.35.3
> > > 
> > > 
> > > This publicly archived list offers a means to provide input to the
> > > OASIS Virtual I/O Device (VIRTIO) TC.
> > > 
> > > In order to verify user consent to the Feedback License terms and
> > > to minimize spam in the list archive, subscription is required
> > > before posting.
> > > 
> > > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > > List help: virtio-comment-help@lists.oasis-open.org
> > > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > > Committee: https://www.oasis-open.org/committees/virtio/
> > > Join OASIS: https://www.oasis-open.org/join/
> > > 
> 
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]