[PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

v9: https://markmail.org/message/4s6kfbeblxw4vzk4

Linux kernel and QEMU already merged SOCK_SEQPACKET support,
so I'm resending Arseny's patches to have consistent virtio-spec
and implementation.

I added patch 2, following the discussion about F_STREAM feature bit:
https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results

About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
(F_STREAM), so at this point I don't know if it's better to use a negative
feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
linux-stable (and QEMU?) to solve this issue.

Thanks,
Stefano

Arseny Krasnov (2):
  virtio-vsock: use C style defines for constants
  virtio-vsock: SOCK_SEQPACKET description

Stefano Garzarella (1):
  virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit

 virtio-vsock.tex | 93 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH v10 1/3] virtio-vsock: use C style defines for constants

[Stefano Garzarella]

From: Arseny Krasnov <arseny.krasnov@kaspersky.com>

This:
1) Replaces enums with C style "defines", because
   use of enums is not documented, while "defines"
   are widely used in spec.
2) Adds defines for some constants.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 virtio-vsock.tex | 54 +++++++++++++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 26 deletions(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index da7e641..ad57f9d 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -86,23 +86,18 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
 operation constants:
 
 \begin{lstlisting}
-enum {
-	VIRTIO_VSOCK_OP_INVALID = 0,
-
-	/* Connect operations */
-	VIRTIO_VSOCK_OP_REQUEST = 1,
-	VIRTIO_VSOCK_OP_RESPONSE = 2,
-	VIRTIO_VSOCK_OP_RST = 3,
-	VIRTIO_VSOCK_OP_SHUTDOWN = 4,
-
-	/* To send payload */
-	VIRTIO_VSOCK_OP_RW = 5,
-
-	/* Tell the peer our credit info */
-	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
-	/* Request the peer to send the credit info to us */
-	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
-};
+#define VIRTIO_VSOCK_OP_INVALID        0
+/* Connect operations */
+#define VIRTIO_VSOCK_OP_REQUEST        1
+#define VIRTIO_VSOCK_OP_RESPONSE       2
+#define VIRTIO_VSOCK_OP_RST            3
+#define VIRTIO_VSOCK_OP_SHUTDOWN       4
+/* To send payload */
+#define VIRTIO_VSOCK_OP_RW             5
+/* Tell the peer our credit info */
+#define VIRTIO_VSOCK_OP_CREDIT_UPDATE  6
+/* Request the peer to send the credit info to us */
+#define VIRTIO_VSOCK_OP_CREDIT_REQUEST 7
 \end{lstlisting}
 
 \subsubsection{Virtqueue Flow Control}\label{sec:Device Types / Socket Device / Device Operation / Virtqueue Flow Control}
@@ -140,8 +135,12 @@ \subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
 consists of a (cid, port number) tuple. The header fields used for this are
 \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
 
-Currently only stream sockets are supported. \field{type} is 1 for stream
-socket types.
+Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
+for stream socket types.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_TYPE_STREAM 1
+\end{lstlisting}
 
 Stream sockets provide in-order, guaranteed, connection-oriented delivery
 without message boundaries.
@@ -222,10 +221,15 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
 insufficient resources to establish the connection.
 
 When a connected socket receives VIRTIO_VSOCK_OP_SHUTDOWN the header
-\field{flags} field bit 0 indicates that the peer will not receive any more
-data and bit 1 indicates that the peer will not send any more data.  These
-hints are permanent once sent and successive packets with bits clear do not
-reset them.
+\field{flags} field bit VIRTIO_VSOCK_SHUTDOWN_F_RECEIVE (bit 0) set indicates
+that the peer will not receive any more data and bit VIRTIO_VSOCK_SHUTDOWN_F_SEND
+(bit 1) set indicates that the peer will not send any more data.  These hints are
+permanent once sent and successive packets with bits clear do not reset them.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_SHUTDOWN_F_RECEIVE 0
+#define VIRTIO_VSOCK_SHUTDOWN_F_SEND    1
+\end{lstlisting}
 
 The VIRTIO_VSOCK_OP_RST packet aborts the connection process or forcibly
 disconnects a connected socket.
@@ -248,9 +252,7 @@ \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Op
 The event buffer is as follows:
 
 \begin{lstlisting}
-enum virtio_vsock_event_id {
-        VIRTIO_VSOCK_EVENT_TRANSPORT_RESET = 0,
-};
+#define VIRTIO_VSOCK_EVENT_TRANSPORT_RESET 0
 
 struct virtio_vsock_event {
         le32 id;
-- 
2.31.1

[PATCH v10 2/3] virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit

[Stefano Garzarella]

Initially vsock devices only supported stream sockets, but now
we are adding support for new types (i.e. SEQPACKET, DGRAM).

Since some devices may not want to support stream sockets, we add
a feature bit for this type.

For backward compatibility, if no feature bit is set, only stream
socket type is supported.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 virtio-vsock.tex | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index ad57f9d..bf015ac 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -16,7 +16,11 @@ \subsection{Virtqueues}\label{sec:Device Types / Socket Device / Virtqueues}
 
 \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
 
-There are currently no feature bits defined for this device.
+If no feature bit is set, only stream socket type is supported.
+
+\begin{description}
+\item VIRTIO_VSOCK_F_STREAM (0) stream socket type is supported.
+\end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
 
-- 
2.31.1

[PATCH v10 3/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

From: Arseny Krasnov <arseny.krasnov@kaspersky.com>

This adds description of SOCK_SEQPACKET socket type
support for virtio-vsock.

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 virtio-vsock.tex | 39 +++++++++++++++++++++++++++++++++++----
 1 file changed, 35 insertions(+), 4 deletions(-)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index bf015ac..9fa93f2 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
 
 \begin{description}
 \item VIRTIO_VSOCK_F_STREAM (0) stream socket type is supported.
+\item VIRTIO_VSOCK_F_SEQPACKET (1) seqpacket socket type is supported.
 \end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
@@ -139,15 +140,17 @@ \subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
 consists of a (cid, port number) tuple. The header fields used for this are
 \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
 
-Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
-for stream socket types.
+Currently stream and seqpacket sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
+for stream socket types, and 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket socket types.
 
 \begin{lstlisting}
-#define VIRTIO_VSOCK_TYPE_STREAM 1
+#define VIRTIO_VSOCK_TYPE_STREAM    1
+#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
 \end{lstlisting}
 
 Stream sockets provide in-order, guaranteed, connection-oriented delivery
-without message boundaries.
+without message boundaries. Seqpacket sockets provide in-order, guaranteed,
+connection-oriented delivery with message and record boundaries.
 
 \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket Device / Device Operation / Buffer Space Management}
 \field{buf_alloc} and \field{fwd_cnt} are used for buffer space management of
@@ -248,6 +251,34 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
 destination) address tuple for a new connection while the other peer is still
 processing the old connection.
 
+\subsubsection{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
+
+\paragraph{Message and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
+Two types of boundaries supported: message and record boundaries.
+
+A message contains data sent by a single system call. Message boundary means
+that data of single send system call is guaranteed to be read wholly by single
+receive system call. If receive buffer is not enough, then out of size data
+will be dropped.
+
+Record is any number of subsequent messages, where last message is sent with POSIX
+MSG_EOR flag set. Record boundary means that receiver gets MSG_EOR flag set
+in the corresponding message where sender set it.
+
+To provide message boundaries, last RW packet of each message has VIRTIO_VSOCK_SEQ_EOM
+bit (bit 0) set in the \field{flags} of packet's header.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_SEQ_EOM (1 << 0)
+\end{lstlisting}
+
+To provide record boundaries, last RW packet of each record has VIRTIO_VSOCK_SEQ_EOR
+bit (bit 1) set in the \field{flags} of packet's header.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
+\end{lstlisting}
+
 \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Operation / Device Events}
 
 Certain events are communicated by the device to the driver using the event
-- 
2.31.1

[virtio-comment] Re: [!!Mass Mail KSE][MASSMAIL KLMS] [virtio-comment] [PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[Arseny Krasnov]

Thank You for this resend!

On 05.01.2022 19:35, Stefano Garzarella wrote:
> v9: https://markmail.org/message/4s6kfbeblxw4vzk4
>
> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> so I'm resending Arseny's patches to have consistent virtio-spec
> and implementation.
>
> I added patch 2, following the discussion about F_STREAM feature bit:
> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>
> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
> (F_STREAM), so at this point I don't know if it's better to use a negative
> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
> linux-stable (and QEMU?) to solve this issue.
>
> Thanks,
> Stefano
>
> Arseny Krasnov (2):
>   virtio-vsock: use C style defines for constants
>   virtio-vsock: SOCK_SEQPACKET description
>
> Stefano Garzarella (1):
>   virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
>
>  virtio-vsock.tex | 93 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 65 insertions(+), 28 deletions(-)
>

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/

Re: [PATCH v10 2/3] virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit

[Stefan Hajnoczi]

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

On Wed, Jan 05, 2022 at 05:35:04PM +0100, Stefano Garzarella wrote:
> Initially vsock devices only supported stream sockets, but now
> we are adding support for new types (i.e. SEQPACKET, DGRAM).
> 
> Since some devices may not want to support stream sockets, we add
> a feature bit for this type.
> 
> For backward compatibility, if no feature bit is set, only stream
> socket type is supported.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  virtio-vsock.tex | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [PATCH v10 3/3] virtio-vsock: SOCK_SEQPACKET description

[Stefan Hajnoczi]

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

On Wed, Jan 05, 2022 at 05:35:05PM +0100, Stefano Garzarella wrote:
> From: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> 
> This adds description of SOCK_SEQPACKET socket type
> support for virtio-vsock.
> 
> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  virtio-vsock.tex | 39 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index bf015ac..9fa93f2 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>  
>  \begin{description}
>  \item VIRTIO_VSOCK_F_STREAM (0) stream socket type is supported.
> +\item VIRTIO_VSOCK_F_SEQPACKET (1) seqpacket socket type is supported.
>  \end{description}
>  
>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
> @@ -139,15 +140,17 @@ \subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
>  consists of a (cid, port number) tuple. The header fields used for this are
>  \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
>  
> -Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
> -for stream socket types.
> +Currently stream and seqpacket sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
> +for stream socket types, and 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket socket types.
>  
>  \begin{lstlisting}
> -#define VIRTIO_VSOCK_TYPE_STREAM 1
> +#define VIRTIO_VSOCK_TYPE_STREAM    1
> +#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
>  \end{lstlisting}
>  
>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
> -without message boundaries.
> +without message boundaries. Seqpacket sockets provide in-order, guaranteed,
> +connection-oriented delivery with message and record boundaries.
>  
>  \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket Device / Device Operation / Buffer Space Management}
>  \field{buf_alloc} and \field{fwd_cnt} are used for buffer space management of
> @@ -248,6 +251,34 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
>  destination) address tuple for a new connection while the other peer is still
>  processing the old connection.
>  
> +\subsubsection{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
> +
> +\paragraph{Message and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
> +Two types of boundaries supported: message and record boundaries.

s/boundaries supported/boundaries are supported/

> +
> +A message contains data sent by a single system call. Message boundary means
> +that data of single send system call is guaranteed to be read wholly by single
> +receive system call. If receive buffer is not enough, then out of size data
> +will be dropped.

The VIRTIO spec tends to focus on the hardware interface and avoids
implementation details and application APIs like system calls. Maybe
system calls are an important concept here but I think this could be
rephrased in terms of "operations":

  A message contains data sent in a single operation. ...

The "Message boundary" sentence isn't clear to me. I guess it's saying
that messages are never split into smaller pieces from the perspective
of read operations? This seems like an implementation detail and the
device would work fine if a different implementation supported read
operations that split messages into multiple read buffers - it doesn't
make a different from the hardware interface perspective.

"If receive buffer is not enough, then out of size data ..." -> "If the
receive buffer is not large enough, then data exceeding the buffer is
dropped."

I think this receive buffer refers to the read(2)/recv(2) syscall data
buffer and not the virtio-vsock credit limit? So this statement relates
to the behavior of SOCK_SEQPACKET, which is outside the scope of the
VIRTIO spec (it's not part of the hardware interface and it's a
Linux/POSIX-specific Sockets API feature). I think it can be dropped
from the spec.

> +
> +Record is any number of subsequent messages, where last message is sent with POSIX
> +MSG_EOR flag set. Record boundary means that receiver gets MSG_EOR flag set
> +in the corresponding message where sender set it.
> +
> +To provide message boundaries, last RW packet of each message has VIRTIO_VSOCK_SEQ_EOM
> +bit (bit 0) set in the \field{flags} of packet's header.
> +
> +\begin{lstlisting}
> +#define VIRTIO_VSOCK_SEQ_EOM (1 << 0)
> +\end{lstlisting}
> +
> +To provide record boundaries, last RW packet of each record has VIRTIO_VSOCK_SEQ_EOR
> +bit (bit 1) set in the \field{flags} of packet's header.
> +
> +\begin{lstlisting}
> +#define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
> +\end{lstlisting}
> +
>  \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Operation / Device Events}
>  
>  Certain events are communicated by the device to the driver using the event
> -- 
> 2.31.1
> 

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

Re: [PATCH v10 3/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

On Mon, Jan 10, 2022 at 11:19:27AM +0000, Stefan Hajnoczi wrote:
>On Wed, Jan 05, 2022 at 05:35:05PM +0100, Stefano Garzarella wrote:
>> From: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>>
>> This adds description of SOCK_SEQPACKET socket type
>> support for virtio-vsock.
>>
>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>> ---
>>  virtio-vsock.tex | 39 +++++++++++++++++++++++++++++++++++----
>>  1 file changed, 35 insertions(+), 4 deletions(-)
>>
>> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>> index bf015ac..9fa93f2 100644
>> --- a/virtio-vsock.tex
>> +++ b/virtio-vsock.tex
>> @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>>
>>  \begin{description}
>>  \item VIRTIO_VSOCK_F_STREAM (0) stream socket type is supported.
>> +\item VIRTIO_VSOCK_F_SEQPACKET (1) seqpacket socket type is supported.
>>  \end{description}
>>
>>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
>> @@ -139,15 +140,17 @@ \subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
>>  consists of a (cid, port number) tuple. The header fields used for this are
>>  \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
>>
>> -Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
>> -for stream socket types.
>> +Currently stream and seqpacket sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
>> +for stream socket types, and 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket socket types.
>>
>>  \begin{lstlisting}
>> -#define VIRTIO_VSOCK_TYPE_STREAM 1
>> +#define VIRTIO_VSOCK_TYPE_STREAM    1
>> +#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
>>  \end{lstlisting}
>>
>>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
>> -without message boundaries.
>> +without message boundaries. Seqpacket sockets provide in-order, guaranteed,
>> +connection-oriented delivery with message and record boundaries.
>>
>>  \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket Device / Device Operation / Buffer Space Management}
>>  \field{buf_alloc} and \field{fwd_cnt} are used for buffer space management of
>> @@ -248,6 +251,34 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
>>  destination) address tuple for a new connection while the other peer is still
>>  processing the old connection.
>>
>> +\subsubsection{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
>> +
>> +\paragraph{Message and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
>> +Two types of boundaries supported: message and record boundaries.
>
>s/boundaries supported/boundaries are supported/

Will fix.

>
>> +
>> +A message contains data sent by a single system call. Message boundary means
>> +that data of single send system call is guaranteed to be read wholly by single
>> +receive system call. If receive buffer is not enough, then out of size data
>> +will be dropped.
>
>The VIRTIO spec tends to focus on the hardware interface and avoids
>implementation details and application APIs like system calls. Maybe
>system calls are an important concept here but I think this could be
>rephrased in terms of "operations":
>
>  A message contains data sent in a single operation. ...

Okay, I agree to use "operations" instead of "system calls".

>
>The "Message boundary" sentence isn't clear to me. I guess it's saying
>that messages are never split into smaller pieces from the perspective
>of read operations? This seems like an implementation detail and the
>device would work fine if a different implementation supported read
>operations that split messages into multiple read buffers - it doesn't
>make a different from the hardware interface perspective.

Yes, I would like to remove this part and simply say that the message
can be split into multiple RW packets and rearrange the paragraph to
say right away that the last packet will have the EOM bit set.

>
>"If receive buffer is not enough, then out of size data ..." -> "If the
>receive buffer is not large enough, then data exceeding the buffer is
>dropped."
>
>I think this receive buffer refers to the read(2)/recv(2) syscall data
>buffer and not the virtio-vsock credit limit? So this statement relates
>to the behavior of SOCK_SEQPACKET, which is outside the scope of the
>VIRTIO spec (it's not part of the hardware interface and it's a
>Linux/POSIX-specific Sockets API feature). I think it can be dropped
>from the spec.

Yep, I'll drop it.


So the paragraph will be like this:

@@ -248,6 +251,27 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
  destination) address tuple for a new connection while the other peer is still
  processing the old connection.

+\subsubsection{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
+
+\paragraph{Message and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
+Two types of boundaries are supported: message and record boundaries.
+
+A message contains data sent in a single operation. A single message can be
+split into multiple RW packets.
+To provide message boundaries, last RW packet of each message has
+VIRTIO_VSOCK_SEQ_EOM bit (bit 0) set in the \field{flags} of packet's header.
+
+Record is any number of subsequent messages, where last message is sent with POSIX
+MSG_EOR flag set. Record boundary means that receiver gets MSG_EOR flag set
+in the corresponding message where sender set it.
+To provide record boundaries, last RW packet of each record has VIRTIO_VSOCK_SEQ_EOR
+bit (bit 1) set in the \field{flags} of packet's header.
+
+\begin{lstlisting}
+#define VIRTIO_VSOCK_SEQ_EOM (1 << 0)
+#define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
+\end{lstlisting}
+
  \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Operation / Device Events}

  Certain events are communicated by the device to the driver using the event

Thanks,
Stefano

[virtio-comment] Re: [PATCH v10 3/3] virtio-vsock: SOCK_SEQPACKET description

[Arseny Krasnov]

On 11.01.2022 15:12, Stefano Garzarella wrote:
> On Mon, Jan 10, 2022 at 11:19:27AM +0000, Stefan Hajnoczi wrote:
>> On Wed, Jan 05, 2022 at 05:35:05PM +0100, Stefano Garzarella wrote:
>>> From: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>>>
>>> This adds description of SOCK_SEQPACKET socket type
>>> support for virtio-vsock.
>>>
>>> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> ---
>>>  virtio-vsock.tex | 39 +++++++++++++++++++++++++++++++++++----
>>>  1 file changed, 35 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>>> index bf015ac..9fa93f2 100644
>>> --- a/virtio-vsock.tex
>>> +++ b/virtio-vsock.tex
>>> @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>>>
>>>  \begin{description}
>>>  \item VIRTIO_VSOCK_F_STREAM (0) stream socket type is supported.
>>> +\item VIRTIO_VSOCK_F_SEQPACKET (1) seqpacket socket type is supported.
>>>  \end{description}
>>>
>>>  \subsection{Device configuration layout}\label{sec:Device Types / Socket Device / Device configuration layout}
>>> @@ -139,15 +140,17 @@ \subsubsection{Addressing}\label{sec:Device Types / Socket Device / Device Opera
>>>  consists of a (cid, port number) tuple. The header fields used for this are
>>>  \field{src_cid}, \field{src_port}, \field{dst_cid}, and \field{dst_port}.
>>>
>>> -Currently only stream sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
>>> -for stream socket types.
>>> +Currently stream and seqpacket sockets are supported. \field{type} is 1 (VIRTIO_VSOCK_TYPE_STREAM)
>>> +for stream socket types, and 2 (VIRTIO_VSOCK_TYPE_SEQPACKET) for seqpacket socket types.
>>>
>>>  \begin{lstlisting}
>>> -#define VIRTIO_VSOCK_TYPE_STREAM 1
>>> +#define VIRTIO_VSOCK_TYPE_STREAM    1
>>> +#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
>>>  \end{lstlisting}
>>>
>>>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
>>> -without message boundaries.
>>> +without message boundaries. Seqpacket sockets provide in-order, guaranteed,
>>> +connection-oriented delivery with message and record boundaries.
>>>
>>>  \subsubsection{Buffer Space Management}\label{sec:Device Types / Socket Device / Device Operation / Buffer Space Management}
>>>  \field{buf_alloc} and \field{fwd_cnt} are used for buffer space management of
>>> @@ -248,6 +251,34 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
>>>  destination) address tuple for a new connection while the other peer is still
>>>  processing the old connection.
>>>
>>> +\subsubsection{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
>>> +
>>> +\paragraph{Message and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
>>> +Two types of boundaries supported: message and record boundaries.
>> s/boundaries supported/boundaries are supported/
> Will fix.
>
>>> +
>>> +A message contains data sent by a single system call. Message boundary means
>>> +that data of single send system call is guaranteed to be read wholly by single
>>> +receive system call. If receive buffer is not enough, then out of size data
>>> +will be dropped.
>> The VIRTIO spec tends to focus on the hardware interface and avoids
>> implementation details and application APIs like system calls. Maybe
>> system calls are an important concept here but I think this could be
>> rephrased in terms of "operations":
>>
>>  A message contains data sent in a single operation. ...
> Okay, I agree to use "operations" instead of "system calls".
>
>> The "Message boundary" sentence isn't clear to me. I guess it's saying
>> that messages are never split into smaller pieces from the perspective
>> of read operations? This seems like an implementation detail and the
>> device would work fine if a different implementation supported read
>> operations that split messages into multiple read buffers - it doesn't
>> make a different from the hardware interface perspective.
> Yes, I would like to remove this part and simply say that the message
> can be split into multiple RW packets and rearrange the paragraph to
> say right away that the last packet will have the EOM bit set.
>
>> "If receive buffer is not enough, then out of size data ..." -> "If the
>> receive buffer is not large enough, then data exceeding the buffer is
>> dropped."
>>
>> I think this receive buffer refers to the read(2)/recv(2) syscall data
>> buffer and not the virtio-vsock credit limit? So this statement relates
>> to the behavior of SOCK_SEQPACKET, which is outside the scope of the
>> VIRTIO spec (it's not part of the hardware interface and it's a
>> Linux/POSIX-specific Sockets API feature). I think it can be dropped
> >from the spec.
>
> Yep, I'll drop it.
>
>
> So the paragraph will be like this:
>
> @@ -248,6 +251,27 @@ \subsubsection{Stream Sockets}\label{sec:Device Types / Socket Device / Device O
>   destination) address tuple for a new connection while the other peer is still
>   processing the old connection.
>
> +\subsubsection{Seqpacket Sockets}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets}
> +
> +\paragraph{Message and record boundaries}\label{sec:Device Types / Socket Device / Device Operation / Seqpacket Sockets / Boundaries}
> +Two types of boundaries are supported: message and record boundaries.
> +
> +A message contains data sent in a single operation. A single message can be
> +split into multiple RW packets.
> +To provide message boundaries, last RW packet of each message has
> +VIRTIO_VSOCK_SEQ_EOM bit (bit 0) set in the \field{flags} of packet's header.
> +
> +Record is any number of subsequent messages, where last message is sent with POSIX
> +MSG_EOR flag set. Record boundary means that receiver gets MSG_EOR flag set
> +in the corresponding message where sender set it.
> +To provide record boundaries, last RW packet of each record has VIRTIO_VSOCK_SEQ_EOR
> +bit (bit 1) set in the \field{flags} of packet's header.
> +
> +\begin{lstlisting}
> +#define VIRTIO_VSOCK_SEQ_EOM (1 << 0)
> +#define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
> +\end{lstlisting}
> +
>   \subsubsection{Device Events}\label{sec:Device Types / Socket Device / Device Operation / Device Events}
>
>   Certain events are communicated by the device to the driver using the event
>
> Thanks,
> Stefano

Thanks for fixes of this patch, seems it was really a little bit strange to

use system call specific things in this spec.

>
>

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/

Re: [PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[Michael S. Tsirkin]

On Wed, Jan 05, 2022 at 05:35:02PM +0100, Stefano Garzarella wrote:
> v9: https://markmail.org/message/4s6kfbeblxw4vzk4
> 
> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> so I'm resending Arseny's patches to have consistent virtio-spec
> and implementation.
> 
> I added patch 2, following the discussion about F_STREAM feature bit:
> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
> 
> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
> (F_STREAM), so at this point I don't know if it's better to use a negative
> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
> linux-stable (and QEMU?) to solve this issue.
> 
> Thanks,
> Stefano

I thought a lot about this.  Given things are in the field, I think
fundamentally F_NO_STREAM is a good idea, however I think maybe
we should split it:
VIRTIO_VSOCK_F_NO_IMPLIED_STREAM meaning "stream is not implied"
VIRTIO_VSOCK_F_STREAM meaning "stream is supported"

and then we say everyone SHOULD set VIRTIO_VSOCK_F_NO_IMPLIED_STREAM.

Hmm?

> Arseny Krasnov (2):
>   virtio-vsock: use C style defines for constants
>   virtio-vsock: SOCK_SEQPACKET description
> 
> Stefano Garzarella (1):
>   virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
> 
>  virtio-vsock.tex | 93 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 65 insertions(+), 28 deletions(-)
> 
> -- 
> 2.31.1

Re: [PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

On Fri, Feb 04, 2022 at 06:57:10AM -0500, Michael S. Tsirkin wrote:
>On Wed, Jan 05, 2022 at 05:35:02PM +0100, Stefano Garzarella wrote:
>> v9: https://markmail.org/message/4s6kfbeblxw4vzk4
>>
>> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>> so I'm resending Arseny's patches to have consistent virtio-spec
>> and implementation.
>>
>> I added patch 2, following the discussion about F_STREAM feature bit:
>> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>>
>> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
>> (F_STREAM), so at this point I don't know if it's better to use a negative
>> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
>> linux-stable (and QEMU?) to solve this issue.
>>
>> Thanks,
>> Stefano
>
>I thought a lot about this.  Given things are in the field, I think
>fundamentally F_NO_STREAM is a good idea,

Yep, last weeks, I've been trying to write patches for QEMU and Linux to 
align with the spec, but actually using just F_STREAM or a 
F_SEQPACKET/F_STREAM combination is a little confusing.

> however I think maybe
>we should split it:
>VIRTIO_VSOCK_F_NO_IMPLIED_STREAM meaning "stream is not implied"
>VIRTIO_VSOCK_F_STREAM meaning "stream is supported"

Yes, that's probably the best thing to do.

What do you think if we use a more generic 
VIRTIO_VSOCK_F_NO_IMPLIED_TYPES meaning "support of any socket types is 
not implied" ?

Maybe it's superfluous since we only supported STREAM, but from the 
point of view of the final result of the specifications, it should be 
clearer... But if it is not set we imply that STREAM is supported, so 
no, maybe better VIRTIO_VSOCK_F_NO_IMPLIED_STREAM.

Can we make this change before the release of 1.2?

Stefano

>
>and then we say everyone SHOULD set VIRTIO_VSOCK_F_NO_IMPLIED_STREAM.
>
>Hmm?
>
>> Arseny Krasnov (2):
>>   virtio-vsock: use C style defines for constants
>>   virtio-vsock: SOCK_SEQPACKET description
>>
>> Stefano Garzarella (1):
>>   virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
>>
>>  virtio-vsock.tex | 93 +++++++++++++++++++++++++++++++++---------------
>>  1 file changed, 65 insertions(+), 28 deletions(-)
>>
>> --
>> 2.31.1
>

Re: [PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[Michael S. Tsirkin]

On Mon, Feb 07, 2022 at 12:35:59PM +0100, Stefano Garzarella wrote:
> On Fri, Feb 04, 2022 at 06:57:10AM -0500, Michael S. Tsirkin wrote:
> > On Wed, Jan 05, 2022 at 05:35:02PM +0100, Stefano Garzarella wrote:
> > > v9: https://markmail.org/message/4s6kfbeblxw4vzk4
> > > 
> > > Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> > > so I'm resending Arseny's patches to have consistent virtio-spec
> > > and implementation.
> > > 
> > > I added patch 2, following the discussion about F_STREAM feature bit:
> > > https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
> > > 
> > > About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
> > > (F_STREAM), so at this point I don't know if it's better to use a negative
> > > feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
> > > linux-stable (and QEMU?) to solve this issue.
> > > 
> > > Thanks,
> > > Stefano
> > 
> > I thought a lot about this.  Given things are in the field, I think
> > fundamentally F_NO_STREAM is a good idea,
> 
> Yep, last weeks, I've been trying to write patches for QEMU and Linux to
> align with the spec, but actually using just F_STREAM or a
> F_SEQPACKET/F_STREAM combination is a little confusing.
> 
> > however I think maybe
> > we should split it:
> > VIRTIO_VSOCK_F_NO_IMPLIED_STREAM meaning "stream is not implied"
> > VIRTIO_VSOCK_F_STREAM meaning "stream is supported"
> 
> Yes, that's probably the best thing to do.
> 
> What do you think if we use a more generic VIRTIO_VSOCK_F_NO_IMPLIED_TYPES
> meaning "support of any socket types is not implied" ?

Fine too.

> Maybe it's superfluous since we only supported STREAM, but from the point of
> view of the final result of the specifications, it should be clearer... But
> if it is not set we imply that STREAM is supported, so no, maybe better
> VIRTIO_VSOCK_F_NO_IMPLIED_STREAM.

my thinking too.

> Can we make this change before the release of 1.2?
> 
> Stefano

I don't think we should rush it into 1.2, but we can just
fork 1.next and merge it for 1.3

> > 
> > and then we say everyone SHOULD set VIRTIO_VSOCK_F_NO_IMPLIED_STREAM.
> > 
> > Hmm?
> > 
> > > Arseny Krasnov (2):
> > >   virtio-vsock: use C style defines for constants
> > >   virtio-vsock: SOCK_SEQPACKET description
> > > 
> > > Stefano Garzarella (1):
> > >   virtio-vsock: add VIRTIO_VSOCK_F_STREAM feature bit
> > > 
> > >  virtio-vsock.tex | 93 +++++++++++++++++++++++++++++++++---------------
> > >  1 file changed, 65 insertions(+), 28 deletions(-)
> > > 
> > > --
> > > 2.31.1
> > 

Re: [PATCH v10 0/3] virtio-vsock: SOCK_SEQPACKET description

[Stefano Garzarella]

On Mon, Feb 07, 2022 at 08:45:43AM -0500, Michael S. Tsirkin wrote:
>On Mon, Feb 07, 2022 at 12:35:59PM +0100, Stefano Garzarella wrote:
>> On Fri, Feb 04, 2022 at 06:57:10AM -0500, Michael S. Tsirkin wrote:
>> > On Wed, Jan 05, 2022 at 05:35:02PM +0100, Stefano Garzarella wrote:
>> > > v9: https://markmail.org/message/4s6kfbeblxw4vzk4
>> > >
>> > > Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>> > > so I'm resending Arseny's patches to have consistent virtio-spec
>> > > and implementation.
>> > >
>> > > I added patch 2, following the discussion about F_STREAM feature bit:
>> > > https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>> > >
>> > > About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
>> > > (F_STREAM), so at this point I don't know if it's better to use a negative
>> > > feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
>> > > linux-stable (and QEMU?) to solve this issue.
>> > >
>> > > Thanks,
>> > > Stefano
>> >
>> > I thought a lot about this.  Given things are in the field, I think
>> > fundamentally F_NO_STREAM is a good idea,
>>
>> Yep, last weeks, I've been trying to write patches for QEMU and Linux to
>> align with the spec, but actually using just F_STREAM or a
>> F_SEQPACKET/F_STREAM combination is a little confusing.
>>
>> > however I think maybe
>> > we should split it:
>> > VIRTIO_VSOCK_F_NO_IMPLIED_STREAM meaning "stream is not implied"
>> > VIRTIO_VSOCK_F_STREAM meaning "stream is supported"
>>
>> Yes, that's probably the best thing to do.
>>
>> What do you think if we use a more generic VIRTIO_VSOCK_F_NO_IMPLIED_TYPES
>> meaning "support of any socket types is not implied" ?
>
>Fine too.
>
>> Maybe it's superfluous since we only supported STREAM, but from the point of
>> view of the final result of the specifications, it should be clearer... But
>> if it is not set we imply that STREAM is supported, so no, maybe better
>> VIRTIO_VSOCK_F_NO_IMPLIED_STREAM.
>
>my thinking too.
>
>> Can we make this change before the release of 1.2?
>>
>> Stefano
>
>I don't think we should rush it into 1.2, but we can just
>fork 1.next and merge it for 1.3
>

Okay, so if it's ok with you, I can send a followup patch later.

Thanks,
Stefano