Re: [RFC PATCH] Introduce admin virtqueue as a new transport

[Stefan Hajnoczi <stefanha@redhat.com>]

On Wed, Aug 04, 2021 at 04:39:24PM +0800, Jason Wang wrote:
> å 2021/8/4 äå2:39, Stefan Hajnoczi åé:
> > On Wed, Aug 04, 2021 at 11:01:39AM +0800, Jason Wang wrote:
> > > å 2021/8/3 äå10:51, Stefan Hajnoczi åé:
> > > > On Tue, Aug 03, 2021 at 11:20:06AM +0800, Jason Wang wrote:
> > > > > +\subsection{Admin Virtqueue Capabilities}\label{sec:Virtio Transport Options / Virtio Over Admin Virtqueue / Admin Virtqueue Capabilities}
> > > > > +
> > > > > +The capabilites that are supported by the admin virtqueue could be
> > > > > +fetched through the following commands:
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +#define VIRTIO_ADMIN_CTRL_CAP    0
> > > > > + #define VIRTIO_ADMIN_CTRL_CAP_GET        0
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +The VIRTIO_ADMIN_CTRL_CAP_GET is used to get the capabilites that are
> > > > > +supported by the admin virtqueue through a u64 which is a bit mask of
> > > > > +the capabilies in command-in-data. There's no command-out-data.
> > > > I'm not sure what this paragraph is describing. Is VIRTIO_ADMIN_CTRL_CAP
> > > > a struct virtio_admin_ctrl::command value?
> > > 
> > > VIRTIO_ADMIN_CTRL_CAP is the class, VIRTIO_ADMIN_CTRL_CAP_GET is the
> > > command.
> > Okay. I found the admin virtqueue command descriptions hard to read, not
> > just because the class/command definitions weren't obvious to me, but
> > also because the command/response layout is described in English instead
> > of a table or C-like notation.
> > 
> > I think something like this would make the commands easier to
> > understand:
> > 
> >    Capabilities supported by the admin virtqueue are fetched as follows:
> > 
> >    Driver->Device:
> >    Field       Value                          Type
> >    -----------------------------------------------
> >    device_id   0                              u64
> >    class       VIRTIO_ADMIN_CTRL_CAP (0)      u16
> >    command     VIRTIO_ADMIN_CTRL_CAP_GET (0)  u16
> > 
> >    Device->Driver:
> >    Field         Value                        Type
> >    -----------------------------------------------
> >    ack           VIRTIO_ADMIN_OK (0)          u8
> >    capabilities  <supported capability bits>  u64
> 
> 
> I just follows styles that is used in virtio-net control vq:
> 
> 
> \begin{lstlisting}
> #define VIRTIO_NET_CTRL_MQÂÂÂ 4
> Â#define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SETÂÂÂÂÂÂÂ 0 (for automatic receive
> steering)
> Â#define VIRTIO_NET_CTRL_MQ_RSS_CONFIGÂÂÂÂÂÂÂÂÂ 1 (for configurable receive
> steering)
> Â#define VIRTIO_NET_CTRL_MQ_HASH_CONFIGÂÂÂÂÂÂÂÂ 2 (for configurable hash
> calculation)
> \end{lstlisting}
> 
> ...
> 
> Do you suggest to change that as well?

My preference is to change it because I think describing inputs/outputs
in English makes it hard to understand the exact binary layout.

> 
> 
> > 
> > > > Another question: I wonder why there is an admin virtqueue feature bit
> > > > instead of a new VIRTIO device ID for the management device? Does this
> > > > mean regular virtio-net, virtio-blk, etc devices can have an admin queue
> > > > in addition to their normal role?
> > > 
> > > I think so, I just follow the normal networking PF role which is usually a
> > > network device which allows some kind of remote management.
> > > 
> > > But it doesn't forbid us to create the management device.
> > > 
> > > I think it's better to not mandate the management device for now, or is
> > > there any reason for doing that?
> > Since the admin virtqueue is generic infrastructure (not specific to
> > vdev management) I think it makes sense to use a feature bit and not a
> > separate management device. This wasn't obvious to me from this
> > document, maybe the text can be tweaked to clearly separate the
> > (generic) admin virtqueue from vdev management.
> 
> 
> Maybe adding a device normative like
> 
> "
> 
> The management device MUST offer the admin virtqueue is a device specific
> virtqueue.
> 
> "

Yes, with
s/is/as/

Attachment: signature.asc
Description: PGP signature