Re: [virtio-comment] [PATCH v2 03/11] transport-fabircs: introduce Segment Descriptor Definition

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, May 04, 2023 at 04:19:02PM +0800, zhenwei pi wrote:
> Introduce segment descriptor to describe the Virtio device buffer
> segments.
> 
> Signed-off-by: zhenwei pi <pizhenwei@bytedance.com>
> ---
>  transport-fabrics.tex | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/transport-fabrics.tex b/transport-fabrics.tex
> index 26b0192..b88acfd 100644
> --- a/transport-fabrics.tex
> +++ b/transport-fabrics.tex
> @@ -45,3 +45,46 @@ \subsection{Virtio Qualified Name}\label{sec:Virtio Transport Options / Virtio O
>  \item The string is null terminated.
>  \item There is no strict style limitation.
>  \end{itemize}
> +
> +\subsection{Transmission Protocol}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol}
> +This section defines transmission protocol for Virtio Over Fabrics. All the

What does "transmission protocol" mean? I guess this is what is often
called a network protocol or a wire protocol or just a protocol, but it
wasn't clear to me maybe whether the "transmission protocol" is one
protocol out of a set of protocols that make up Virtio Over Fabrics.

This paragraph should describe which connections use this protocol. For
example:

  This protocol is used for both control and virtqueue connections.

> +fields use little endian format.
> +
> +\subsubsection{Segment Descriptor Definition}\label{sec:Virtio Transport Options / Virtio Over Fabrics / Transmission Protocol / Segment Descriptor Definition}
> +Virtio Over Fabrics uses the following structure to describe data segment:

What is a data segment? I guess it's a message/command/request?

There should be an explanation of how data segments are used. For
example:

  The initiator sends a data segment containing the command to the
  target. The target sends a data segment containing the response to the
  command back to the initiator.

> +
> +\begin{lstlisting}
> +struct virtio_of_vring_desc {

I think the name "vring" should be avoided. The vring is an in-memory
layout for implementing virtqueues where shared memory is available.
Calling it virtio_of_vq_desc makes it clear that Virtio Over Fabrics
does not use vrings to implement virtqueues.

> +        le64 addr;
> +        le32 length;
> +        /* This marks the unique ID within a command, no limitation among inflight commands */

What is a command?

> +        le16 id;
> +        /* This marks a buffer as keyed transmission (otherwise stream transmission) */
> +#define VIRTIO_OF_DESC_F_KEYED     1
> +        /* This marks a buffer as device write-only (otherwise device read-only). */
> +#define VIRTIO_OF_DESC_F_WRITE     2
> +        le16 flags;
> +        le32 key;
> +};
> +\end{lstlisting}
> +
> +The structure virtio_of_vring_desc is used for both keyed transmission
> +(i.e. RDMA) and stream transmission(i.e. TCP). The fields is described as follows:
> +
> +\begin{tabular}{ |l|l|l| }
> +\hline
> +Field & keyed transmission & stream transmission \\
> +\hline \hline
> +addr & Start address of remote memory buffer & Start address within the stream buffer \\

What is a stream buffer?

> +\hline
> +length & The length of remote memory buffer & The length of buffer within the stream \\

I'm not sure what buffer means here. I guess it's not the same as a
virtqueue buffer, it's probably a virtqueue descriptor (element)?

Can you avoid using buffer here since it usually means something else in
Virtio?

> +\hline
> +id & The ID of this descriptor & The ID of this descriptor \\
> +\hline
> +flags & both keyed transmission and stream transmission supported & stream transmission only \\

I'm not sure what this means.

> +\hline
> +key & Key of the remote Memory Region & Ignore \\

Should "Ignore" be "Reserved" so that stream transmission can use this
field for something else in the future?

> +\hline
> +\end{tabular}
> +
> +Depending on the opcode, a Command contains zero or more structure virtio_of_vring_desc.

opcode hasn't been defined yet. I guess that's because the first
virtio_of_vring_desc contains a Command and that has an opcode field?
Please make sure the text is ordered so that terms are defined before
they are used.

Stefan

Attachment: signature.asc
Description: PGP signature