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

[zhenwei pi <pizhenwei@bytedance.com>]

On 5/31/23 22:23, Stefan Hajnoczi wrote:
> 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?
> 

OK.

>> +\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?
> 

OK

>> +\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

OK, I think reordering the text is needed in the next version.

-- 
zhenwei pi

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/