RE: [virtio-comment] Re: [PATCH V2 4/6] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Parav Pandit <parav@nvidia.com>]

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Zhu, Lingshan
> Sent: Friday, November 3, 2023 8:27 PM
> 
> On 11/3/2023 7:35 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Friday, November 3, 2023 4:05 PM
> >>
> >> This patch adds two new le16 fields to common configuration structure
> >> to support VIRTIO_F_QUEUE_STATE in PCI transport layer.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   transport-pci.tex | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >>
> >> diff --git a/transport-pci.tex b/transport-pci.tex index
> >> a5c6719..3161519 100644
> >> --- a/transport-pci.tex
> >> +++ b/transport-pci.tex
> >> @@ -325,6 +325,10 @@ \subsubsection{Common configuration structure
> >> layout}\label{sec:Virtio Transport
> >>           /* About the administration virtqueue. */
> >>           le16 admin_queue_index;         /* read-only for driver */
> >>           le16 admin_queue_num;         /* read-only for driver */
> >> +
> >> +	/* Virtqueue state */
> >> +        le16 queue_avail_state;         /* read-write */
> >> +        le16 queue_used_state;          /* read-write */
> > This tiny interface for 128 virtio net queues through register read writes, does
> not work effectively.
> > There are inflight out of order descriptors for block also.
> > Hence toy registers like this do not work.
> Do you know there is a queue_select? Why this does not work? Do you know
> how other queue related fields work?
:)
Yes. If you notice queue_reset related critical spec bug fix was done when it was introduced so that live migration can _actually_ work.

When queue_select is done for 128 queues serially, it take a lot of time to read those slow register interface for this + inflight descriptors + more.

> Like how to set a queue size and enable it?
Those are meant to be used before DRIVER_OK stage as they are init time registers.
Not to keep abusing them..

> >
> > Series [1] is comprehensive that covers it even if you consider non-
> passtrhough device migration model.
> > Where you can suspend individual queues using new admin command and get
> them in the device context state.
> >
> > [1]
> > https://lists.oasis-open.org/archives/virtio-comment/202310/msg00472.h
> > tml
> I suggest you read QEMU migration code. If you don't want to, not my fault.
> >
> >>   };
> >>   \end{lstlisting}
> >>
> >> @@ -428,6 +432,17 @@ \subsubsection{Common configuration structure
> >> layout}\label{sec:Virtio Transport
> >>   	The value 0 indicates no supported administration virtqueues.
> >>   	This field is valid only if VIRTIO_F_ADMIN_VQ has been
> >>   	negotiated.
> >> +
> >> +\item[\field{queue_avail_state}]
> >> +        This field is valid only if VIRTIO_F_QUEUE_STATE has been
> >> +        negotiated. The driver sets and gets the available state of
> >> +        the virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
> >> +
> >> +\item[\field{queue_used_state}]
> >> +        This field is valid only if VIRTIO_F_QUEUE_STATE has been
> >> +        negotiated. The driver sets and gets the used state of the
> >> +        virtqueue here (see \ref{sec:Virtqueues / Virtqueue State}).
> >> +
> >>   \end{description}
> >>
> >>   \devicenormative{\paragraph}{Common configuration structure
> >> layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device
> >> Layout / Common configuration structure layout} @@ -488,6 +503,9 @@
> >> \subsubsection{Common configuration structure
> >> layout}\label{sec:Virtio Transport  present either a value of 0 or a power of 2
> in  \field{queue_size}.
> >>
> >> +If VIRTIO_F_QUEUE_STATE has not been negotiated, the device MUST
> >> +ignore any accesses to \field{queue_avail_state} and
> \field{queue_used_state}.
> >> +
> >>   If VIRTIO_F_ADMIN_VQ has been negotiated, the value
> >> \field{admin_queue_index} MUST be equal to, or bigger than
> >> \field{num_queues}; also, \field{admin_queue_num} MUST be
> >> --
> >> 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/