Re: [PATCH V2 1/5] Introduce virito transport virtqueue

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 8/2/2022 4:25 AM, Michael S. Tsirkin wrote:
> On Mon, Aug 01, 2022 at 05:32:12PM +0800, Zhu Lingshan wrote:
> > This commit introduces transport virtqueue as a new transport
> > layer for virtio devices. And the format of the commands
> > through the transport virtqueue is defined as well.
> >
> > We also give examples for the management devices and the
> > managed devices.
> >
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > ---
> >   content.tex      | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
> >   introduction.tex |  3 ++
> >   2 files changed, 76 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index e863709..969ca46 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -2895,6 +2895,79 @@ \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov
> >   MAY also choose to verify reset completion by reading \field{device status} via
> >   CCW_CMD_READ_STATUS and checking whether it is 0 afterwards.
> >   
> > +\section{Virtio Over Transport Virtqueue}\label{sec:Virtio Transport Options Virtio Over Transport Virtqueue}
> > +
> > +In some cases, it is challenging to implement a virtio device in a transport specific method.
> > +
> > +One example is that a physical device may try to present multiple managed (usually virtual) devices
> > +with limited transport specific resources.
> > +
> > +Another example is to implement managed devices which are transport independent.
> > +
> > +In those cases, a transport virtqueue could be used as the transport layer to
> > +implement virtio managed device.
> > +
> > +Feature bit VIRTIO_F_TRANSPT_VQ indicates that a device offers a transport virtqueue.
> > +
> > +\subsection{Basic Concepts}\label{sec:Virtio Transport Options / Virtio over Transport Virtqueue / Basic Concepts}
> > +
> > +All transport virtqueue commands are of the following form:
> > +
> > +\begin{lstlisting}
> > +struct virtio_transportq_ctrl {
> > +        u64 device_id;
> > +        u16 class;
> > +        u16 command;
> > +        u8 command-out-data[];
> > +        u32 ack;
> > +        u8 command-in-data[];
> > +};
> > +
> > +/* ack values */
> > +#define VIRTIO_TRANSPTQ_OK     0
> > +#define VIRTIO_TRANSPTQ_ERR    255
> > +\end{lstlisting}
> > +
> > +The \field{device_id}, \field{class}, \field{command} and
>
> class seems to not be defined.
I will add a description here, a class is a set of commands with similar 
purpose.
> > +\field{command-out-data} are set by the management driver,
> > +and the device sets \field{ack} and \field{command-in-data}.
> > +
> > +\field{device_id} is an UUID to identify a virtio managed device,
> Using UUIDs is very iffy IMHO. Do we really need them unique and in
> what context?
We need an identifier to address a managed device, and the ID should be
allocated by the management device(not the system wide), and be unique 
in the management device context.
So I think UUID can meet the requirement.
> > +The \field{device_id} value 0 is used for identify the management device itself
>
> I don't really understand when is device itself applicable.
Sometimes we may need to send commands against the management device itself.
ike querying the reaming available resource (e.g., how many available 
queues),
to determine what kind and how many managed device(like SIOV devices) 
can be created on the management device.

This also leave flexibility for further extensions.
> > +
> > +\subsubsection{The Management Devices}\label{sec:Virtio Transport Options / Virtio over Transport Virtqueue / Basic Concepts / The Management Devices}
> > +
> > +A device that offers a transport virtqueue (via feature VIRTIO_F_TRANSPT_VQ)
> > +is a management device. It processes the commands through the transport virtqueue commands.
> > +
> > +For example, a PCIe PF with a transport virtqueue is a management device.
> Why a pf specifically?
It is just an example, it can be a VF or other devices, even nested
> > +
> > +\subsubsection{The Managed Devices}\label{sec:Virtio Transport Options / Virtio over Transport Virtqueue / Basic Concepts / The Managed Devices}
> > +
> > +A managed device is a kind of device that is created through a transport virtqueue,
> > +and utilizes the transport virtqueue as its transport layer.
> > +
> > +An example of managed devices is a Scalable I/O Virtualization \hyperref[intro:SIOV]{[SIOV]} device which is created and managed
> > +through a transport virtqueue of a management device.
> > +
> > +\devicenormative{\subsubsection}{Basic Concepts}{Virtio Transport Options / Virtio Over Transport Virtqueue / Basic Concepts}
> > +
> > +The management device MUST offer feature bit VIRTIO_F_TRANSPT_VQ and a transport virtqueue as a device specific virtqueue.
>
> I think I like the idea of exposing the vq index in a register better.
Is it add a new filed "transport_vq_idx" in the device config space? I 
am not sure, if we expose the queue_index of the transport virtqueue in 
the config sapce,
it looks we have limited the management device transport interface to be 
PCI, then the transport vq can not be nested and self-contained.
I see the control vq is just offered through the CVQ feature bit, and in 
the chapter "Device Types", we have defined the queue index of cvq.
> > +
> > +The management device MUST fail all commands against the transport virtqueue if VIRTIO_F_TRANSPT_VQ is not negotiated.
> Fail how? The vq can't be configured even. Are you saying is MUST
> process the vq just to fail commands even though the feature bit is not there?
> That's .... unusual.
you are right, I should remove this section since if VIRTIO_F_TRANSPT_VQ 
is not negotiated, the transport vq can not be initialized,
then no commands through the transport vq.
> > +
> > +\drivernormative{\subsubsection}{Basic Concepts}{Virtio Transport Options / Virtio Over Transport Virtqueue / Basic Concepts}
> > +
> > +The management driver MUST not send any commands against the transport virtqueue if VIRTIO_F_TRANSPT_VQ is not negotiated.
>
> what is the management driver? Do we need this term even?
I can add a description here, the management driver is the driver of the 
management device.
> > +
> > +\subsection{Management Devices Dscovery}\label{sec:Virtio Transport Options /Virtio Over Transport Virtqueue / Management Devices Discovery}
> > +
> > +Management devices are discovered through their own transport and device-specific method.
> this does not seem to mean anything reader won't already know.
Yes, I can remove this.
> > +
> > +\subsection{Managed Devices Dscovery}\label{sec:Virtio Transport Options /Virtio Over Transport Virtqueue / Managed Devices Discovery}
> > +
> > +Managed devices are created and discovered through a transport virtqueue.
> > +
> >   \chapter{Device Types}\label{sec:Device Types}
> >   
> >   On top of the queues, config space and feature negotiation facilities
> > diff --git a/introduction.tex b/introduction.tex
> > index a9491cf..94a1b51 100644
> > --- a/introduction.tex
> > +++ b/introduction.tex
> > @@ -80,6 +80,9 @@ \section{Normative References}\label{sec:Normative References}
> >   	\phantomsection\label{intro:SCMI}\textbf{[SCMI]} &
> >   	Arm System Control and Management Interface, DEN0056,
> >   	\newline\url{https://developer.arm.com/docs/den0056/c}, version C and any future revisions\\
> > +	\phantomsection\label{intro:SIOV}\textbf{[SIOV]} &
> > +	Scalable I/O Virtualization,
> > +	\newline\url{https://www.opencompute.org/documents/ocp-scalable-io-virtualization-technical-specification-revision-1-v1-2-pdf}\\
> >   
> >   \end{longtable}
> >   
> > --
> > 2.35.3