[virtio-dev] Re: [PATCH v10 04/10] admin: introduce virtio admin virtqueues

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Thu, Mar 02, 2023 at 07:05:14PM -0500, Michael S. Tsirkin wrote:
> On Thu, Mar 02, 2023 at 03:40:07PM -0500, Stefan Hajnoczi wrote:
> > On Thu, Mar 02, 2023 at 08:05:06AM -0500, Michael S. Tsirkin wrote:
> > > The admin virtqueues will be the first interface to issue admin commands.
> > > 
> > > Currently virtio specification defines control virtqueue to manipulate
> > > features and configuration of the device it operates on. However,
> > > control virtqueue commands are device type specific, which makes it very
> > > difficult to extend for device agnostic commands.
> > > 
> > > To support this requirement in a more generic way, this patch introduces
> > > a new admin virtqueue interface.
> > 
> > Is this referring to the existing virtio-net, virtio-scsi, etc control
> > virtqueues?
> > 
> > I see the admin virtqueue as the virtqueue equivalent to transport
> > feature bits. The admin queue does nothing device type-specific (net,
> > scsi, etc) and instead focusses on transport and core virtio tasks.
> > 
> > Keeping the device-specific virtqueue separate from the admin virtqueue
> > is simpler and has fewer potential problems. I don't think creating
> > common infrastructure for device-specific control virtqueues across
> > device types worthwhile or within the scope of this patch series.
> 
> yes this commit log is outdated. referred to original
> proposal by Max which also planned to replace cvq.
> will update.
> 
> 
> > > We also support more than one admin virtqueue, for QoS and
> > > scalability requirements.
> > > 
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  admin.tex   | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  content.tex |  7 +++--
> > >  2 files changed, 79 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > index 7e28b77..3ffac2e 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > >  \field{command_specific_data} and \field{command_specific_result}
> > >  depends on these structures and is described separately or is
> > >  implicit in the structure description.
> > > +
> > > +\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
> > > +
> > > +An administration virtqueue of an owner device is used to submit
> > > +group administration commands. An owner device can have more
> > > +than one administration virtqueue.
> > > +
> > > +If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
> > > +or more adminstration virtqueues. The number and locations of the
> > > +administration virtqueues are exposed by the owner device in a transport
> > > +specific manner.
> > > +
> > > +The driver submits commands by queueing them to an arbitrary
> > > +administration virtqueue, and they are processed by the
> > > +device in the order in which they are queued. It is the
> > > +responsibility of the driver to ensure ordering for commands
> > > +placed on different administration virtqueues, because they will
> > > +be executed with no order constraints.
> > 
> > Does "they are processed by the device in the order in which they are
> > queued" mean only 1 admin command can be running at any given time and
> > therefore they will complete in order? This would allow pipelining
> > dependent commands but prevent long-running commands because the
> > virtqueue is blocked while executing a command.
> 
> we have multiple vqs for that.

This reminds me of the async challenges with QEMU's QMP monitor.

Will it ever be possible to abort an in-flight command? I guess the
abort command would need to be submitted on another virtqueue, but how
do you identify the in-flight command that you wish to cancel?

Please clarify the blocking semantics in the spec because it wasn't
clear to me.

Thanks,
Stefan

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