Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description

Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description

[Stefano Garzarella]

On Thu, Feb 18, 2021 at 09:07:12AM +0300, Arseny Krasnov wrote:
>This patchset adds description of SOCK_SEQPACKET socket's type
>of virtio vsock.
>
>Here is implementation:
>https://lkml.org/lkml/2021/2/18/24
>
> Arseny Krasnov(1):
>  virtio-vsock: add SOCK_SEQPACKET description
>
> virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
> 1 files changed, 37 insertions(+), 3 deletions(-)
>
>TODO:
>- for messages identification I use header's field called 'msg_cnt'.
>  May be it is better to call it 'msg_id' or 'msg_num', because it
>  is used as ID, but implemented as counter.

If we use it only as an identifier, I think 'msg_id' is preferable and 
we shouldn't mention in the specs that it's a counter, since it's just a 
possible implementation.
Instead if we use the counter for some control, for example if we lost a 
packet, then maybe msg_cnt is better.
But since the channel shouldn't lose packets, I don't think this is the 
case.

>
>- in current version of specification, some values of headers' fields
>  still unnamed, for example type of socket (stream or seqpacket), then
>  shutdown flags. Spec says that for stream socket value of 'type'
>  must be 1. For receive shutdown bit 0 is set in 'flags', for send
>  shutdown bit 1 is set in 'flags'. But in Linux these unnamed ones and
>  zeroes are implemented as enums, so may be it will be ok to add such
>  enums in specification (as 'enum virtio_vsock_event_id').

Since we have an enumerate for VIRTIO_VSOCK_OP_* values, I think we can 
add enums for socket type and maybe 'flags'.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description

[Stefano Garzarella]

On Thu, Feb 18, 2021 at 09:08:23AM +0300, Arseny Krasnov wrote:
>Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>---
> virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 37 insertions(+), 3 deletions(-)
>
>diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>index da7e641..1ee8f99 100644
>--- a/virtio-vsock.tex
>+++ b/virtio-vsock.tex
>@@ -102,6 +102,11 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
> 	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
> 	/* Request the peer to send the credit info to us */
> 	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
>+
>+	/* Message begin for SOCK_SEQPACKET */
>+	VIRTIO_VSOCK_OP_SEQ_BEGIN = 8,
>+	/* Message end for SOCK_SEQPACKET */
>+	VIRTIO_VSOCK_OP_SEQ_END = 9,
> };
> \end{lstlisting}
>
>@@ -140,11 +145,11 @@ \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 stream and seqpacket sockets are supported. \field{type} is 1 for
>+stream socket types. \field{type} is 2 for seqpacket socket types.
>
> Stream sockets provide in-order, guaranteed, connection-oriented delivery
>-without message boundaries.
>+without message boundaries. Seqpacket sockets also provide message 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
>@@ -240,6 +245,35 @@ \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}
>+
>+Seqpacket sockets differ from stream sockets only in data transmission way: in
>+stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
>+seqpacket sockets, to provide message boundaries, every sequence of
>+VIRTIO_VSOCK_OP_RW packets of each message is headed with
                                              ^
Since this is a spec, I think we should use MUST when something must be 
respected by the peer, for example here we can say "MUST be headed"

>+VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
>+Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets carry
                                                                      ^
Same here "MUST carry" and in the rest of the patch.

>+special header in payload:
>+
>+\begin{lstlisting}
>+struct virtio_vsock_seq_hdr {
>+	__le32  msg_cnt;
>+	__le32  msg_len;
>+};
>+\end{lstlisting}
>+
>+\field{msg_cnt} is per socket and incremented by 1 on every send of
>+VIRTIO_VSOCK_OP_SEQ_BEGIN or VIRTIO_VSOCK_OP_SEQ_END. \field{msg_len} contains
>+message length. This header is used to check integrity of each message: message
>+is valid if length of data in VIRTIO_VSOCK_OP_RW packets is equal to
>+\field{msg_len} of prepending VIRTIO_VSOCK_OP_SEQ_BEGIN and \field{msg_cnt} of
>+VIRTIO_VSOCK_OP_SEQ_END differs from \field{msg_cnt} of
>+VIRTIO_VSOCK_OP_SEQ_BEGIN by 1.

If we replace msg_cnt with msg_id, I think we should have the same 
'msg_id' for VIRTIO_VSOCK_OP_SEQ_BEGIN or VIRTIO_VSOCK_OP_SEQ_END. If 
you want to use 'msg_cnt' and you increment it between BEGIN and END, we 
should consider the overflow case.

>+
>+POSIX MSG_EOR flag is supported by special value of \field{flags} in packet's
>+header. If this flag is set for message, then all VIRTIO_VSOCK_OP_RW packets
>+of this message have bit 0 is set to 1 in \field{flags}.

Maybe we need to define what MSG_EOR means.

Thanks,
Stefano

>+
> \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.25.1
>
>
>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/
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description

[Cornelia Huck]

On Wed, 24 Feb 2021 10:32:00 +0100
Stefano Garzarella <sgarzare@redhat.com> wrote:

> On Thu, Feb 18, 2021 at 09:08:23AM +0300, Arseny Krasnov wrote:
> >Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> >---
> > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 37 insertions(+), 3 deletions(-)
> >
> >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> >index da7e641..1ee8f99 100644
> >--- a/virtio-vsock.tex
> >+++ b/virtio-vsock.tex
> >@@ -102,6 +102,11 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
> > 	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
> > 	/* Request the peer to send the credit info to us */
> > 	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
> >+
> >+	/* Message begin for SOCK_SEQPACKET */
> >+	VIRTIO_VSOCK_OP_SEQ_BEGIN = 8,
> >+	/* Message end for SOCK_SEQPACKET */
> >+	VIRTIO_VSOCK_OP_SEQ_END = 9,
> > };
> > \end{lstlisting}
> >
> >@@ -140,11 +145,11 @@ \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 stream and seqpacket sockets are supported. \field{type} is 1 for
> >+stream socket types. \field{type} is 2 for seqpacket socket types.
> >
> > Stream sockets provide in-order, guaranteed, connection-oriented delivery
> >-without message boundaries.
> >+without message boundaries. Seqpacket sockets also provide message 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
> >@@ -240,6 +245,35 @@ \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}
> >+
> >+Seqpacket sockets differ from stream sockets only in data transmission way: in
> >+stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
> >+seqpacket sockets, to provide message boundaries, every sequence of
> >+VIRTIO_VSOCK_OP_RW packets of each message is headed with  
>                                               ^
> Since this is a spec, I think we should use MUST when something must be 
> respected by the peer, for example here we can say "MUST be headed"
> 
> >+VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
> >+Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets carry  
>                                                                       ^
> Same here "MUST carry" and in the rest of the patch.

Actually, MUST and friends are really for normative sections; I'd
advise to have a description of how this feature works and then some
device/driver normative clauses with MUST statements (like "the device
MUST reject <malformed packets>" or so).

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description

[Stefano Garzarella]

On Wed, Mar 03, 2021 at 01:08:43PM +0100, Cornelia Huck wrote:
>On Wed, 24 Feb 2021 10:32:00 +0100
>Stefano Garzarella <sgarzare@redhat.com> wrote:
>
>> On Thu, Feb 18, 2021 at 09:08:23AM +0300, Arseny Krasnov wrote:
>> >Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
>> >---
>> > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
>> > 1 file changed, 37 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
>> >index da7e641..1ee8f99 100644
>> >--- a/virtio-vsock.tex
>> >+++ b/virtio-vsock.tex
>> >@@ -102,6 +102,11 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
>> > 	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
>> > 	/* Request the peer to send the credit info to us */
>> > 	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
>> >+
>> >+	/* Message begin for SOCK_SEQPACKET */
>> >+	VIRTIO_VSOCK_OP_SEQ_BEGIN = 8,
>> >+	/* Message end for SOCK_SEQPACKET */
>> >+	VIRTIO_VSOCK_OP_SEQ_END = 9,
>> > };
>> > \end{lstlisting}
>> >
>> >@@ -140,11 +145,11 @@ \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 stream and seqpacket sockets are supported. \field{type} is 1 for
>> >+stream socket types. \field{type} is 2 for seqpacket socket types.
>> >
>> > Stream sockets provide in-order, guaranteed, connection-oriented delivery
>> >-without message boundaries.
>> >+without message boundaries. Seqpacket sockets also provide message 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
>> >@@ -240,6 +245,35 @@ \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}
>> >+
>> >+Seqpacket sockets differ from stream sockets only in data transmission way: in
>> >+stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
>> >+seqpacket sockets, to provide message boundaries, every sequence of
>> >+VIRTIO_VSOCK_OP_RW packets of each message is headed with
>>                                               ^
>> Since this is a spec, I think we should use MUST when something must be
>> respected by the peer, for example here we can say "MUST be headed"
>>
>> >+VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
>> >+Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets carry
>>                                                                       ^
>> Same here "MUST carry" and in the rest of the patch.
>
>Actually, MUST and friends are really for normative sections; I'd
>advise to have a description of how this feature works and then some
>device/driver normative clauses with MUST statements (like "the device
>MUST reject <malformed packets>" or so).
>

Okay, got it.

Thanks for the clarification,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description

[Michael S. Tsirkin]

On Wed, Mar 03, 2021 at 01:08:43PM +0100, Cornelia Huck wrote:
> On Wed, 24 Feb 2021 10:32:00 +0100
> Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> > On Thu, Feb 18, 2021 at 09:08:23AM +0300, Arseny Krasnov wrote:
> > >Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> > >---
> > > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
> > > 1 file changed, 37 insertions(+), 3 deletions(-)
> > >
> > >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > >index da7e641..1ee8f99 100644
> > >--- a/virtio-vsock.tex
> > >+++ b/virtio-vsock.tex
> > >@@ -102,6 +102,11 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
> > > 	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
> > > 	/* Request the peer to send the credit info to us */
> > > 	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
> > >+
> > >+	/* Message begin for SOCK_SEQPACKET */
> > >+	VIRTIO_VSOCK_OP_SEQ_BEGIN = 8,
> > >+	/* Message end for SOCK_SEQPACKET */
> > >+	VIRTIO_VSOCK_OP_SEQ_END = 9,
> > > };
> > > \end{lstlisting}
> > >
> > >@@ -140,11 +145,11 @@ \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 stream and seqpacket sockets are supported. \field{type} is 1 for
> > >+stream socket types. \field{type} is 2 for seqpacket socket types.
> > >
> > > Stream sockets provide in-order, guaranteed, connection-oriented delivery
> > >-without message boundaries.
> > >+without message boundaries. Seqpacket sockets also provide message 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
> > >@@ -240,6 +245,35 @@ \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}
> > >+
> > >+Seqpacket sockets differ from stream sockets only in data transmission way: in
> > >+stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
> > >+seqpacket sockets, to provide message boundaries, every sequence of
> > >+VIRTIO_VSOCK_OP_RW packets of each message is headed with  
> >                                               ^
> > Since this is a spec, I think we should use MUST when something must be 
> > respected by the peer, for example here we can say "MUST be headed"
> > 
> > >+VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
> > >+Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets carry  
> >                                                                       ^
> > Same here "MUST carry" and in the rest of the patch.
> 
> Actually, MUST and friends are really for normative sections; I'd
> advise to have a description of how this feature works and then some
> device/driver normative clauses with MUST statements (like "the device
> MUST reject <malformed packets>" or so).

I agree we do want normative sections but please don't add MUST etc elsewhere.
Also vague text saying "malformed" isn't all that helpful if it's a
MUST. How does driver know for sure it's malformed? easy to miss
some requirement.
Therefore easiest thing it just to do some copy-pasting.

E.g. You start with above and add a normative section saying:
Driver MUST use XYZ in VIRTIO_VSOCK_OP_SEQ_END packets.

We typically don't specify behaviour when out of spec,
if we should here then please make a separate chapter
for this explaining the how and the why.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description

[Cornelia Huck]

On Wed, 3 Mar 2021 11:52:43 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Mar 03, 2021 at 01:08:43PM +0100, Cornelia Huck wrote:
> > On Wed, 24 Feb 2021 10:32:00 +0100
> > Stefano Garzarella <sgarzare@redhat.com> wrote:
> >   
> > > On Thu, Feb 18, 2021 at 09:08:23AM +0300, Arseny Krasnov wrote:  
> > > >Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> > > >---
> > > > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
> > > > 1 file changed, 37 insertions(+), 3 deletions(-)
> > > >
> > > >diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> > > >index da7e641..1ee8f99 100644
> > > >--- a/virtio-vsock.tex
> > > >+++ b/virtio-vsock.tex
> > > >@@ -102,6 +102,11 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
> > > > 	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
> > > > 	/* Request the peer to send the credit info to us */
> > > > 	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
> > > >+
> > > >+	/* Message begin for SOCK_SEQPACKET */
> > > >+	VIRTIO_VSOCK_OP_SEQ_BEGIN = 8,
> > > >+	/* Message end for SOCK_SEQPACKET */
> > > >+	VIRTIO_VSOCK_OP_SEQ_END = 9,
> > > > };
> > > > \end{lstlisting}
> > > >
> > > >@@ -140,11 +145,11 @@ \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 stream and seqpacket sockets are supported. \field{type} is 1 for
> > > >+stream socket types. \field{type} is 2 for seqpacket socket types.
> > > >
> > > > Stream sockets provide in-order, guaranteed, connection-oriented delivery
> > > >-without message boundaries.
> > > >+without message boundaries. Seqpacket sockets also provide message 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
> > > >@@ -240,6 +245,35 @@ \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}
> > > >+
> > > >+Seqpacket sockets differ from stream sockets only in data transmission way: in
> > > >+stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
> > > >+seqpacket sockets, to provide message boundaries, every sequence of
> > > >+VIRTIO_VSOCK_OP_RW packets of each message is headed with    
> > >                                               ^
> > > Since this is a spec, I think we should use MUST when something must be 
> > > respected by the peer, for example here we can say "MUST be headed"
> > >   
> > > >+VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
> > > >+Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets carry    
> > >                                                                       ^
> > > Same here "MUST carry" and in the rest of the patch.  
> > 
> > Actually, MUST and friends are really for normative sections; I'd
> > advise to have a description of how this feature works and then some
> > device/driver normative clauses with MUST statements (like "the device
> > MUST reject <malformed packets>" or so).  
> 
> I agree we do want normative sections but please don't add MUST etc elsewhere.
> Also vague text saying "malformed" isn't all that helpful if it's a
> MUST. How does driver know for sure it's malformed? easy to miss
> some requirement.

Actually, I intended "<malformed packet>" to be a placeholder for a
precise description...

> Therefore easiest thing it just to do some copy-pasting.
> 
> E.g. You start with above and add a normative section saying:
> Driver MUST use XYZ in VIRTIO_VSOCK_OP_SEQ_END packets.
> 
> We typically don't specify behaviour when out of spec,
> if we should here then please make a separate chapter
> for this explaining the how and the why.
> 

I think it makes sense if we want to be concrete on what should happen
for out-of-spec operation (e.g. in cases where ignoring it is
preferable to actively rejecting it.)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description

[Michael S. Tsirkin]

On Thu, Feb 18, 2021 at 09:08:23AM +0300, Arseny Krasnov wrote:
> Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
> ---
>  virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/virtio-vsock.tex b/virtio-vsock.tex
> index da7e641..1ee8f99 100644
> --- a/virtio-vsock.tex
> +++ b/virtio-vsock.tex
> @@ -102,6 +102,11 @@ \subsection{Device Operation}\label{sec:Device Types / Socket Device / Device Op
>  	VIRTIO_VSOCK_OP_CREDIT_UPDATE = 6,
>  	/* Request the peer to send the credit info to us */
>  	VIRTIO_VSOCK_OP_CREDIT_REQUEST = 7,
> +
> +	/* Message begin for SOCK_SEQPACKET */
> +	VIRTIO_VSOCK_OP_SEQ_BEGIN = 8,
> +	/* Message end for SOCK_SEQPACKET */
> +	VIRTIO_VSOCK_OP_SEQ_END = 9,
>  };
>  \end{lstlisting}
>  
> @@ -140,11 +145,11 @@ \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 stream and seqpacket sockets are supported. \field{type} is 1 for
> +stream socket types. \field{type} is 2 for seqpacket socket types.
>  
>  Stream sockets provide in-order, guaranteed, connection-oriented delivery
> -without message boundaries.
> +without message boundaries. Seqpacket sockets also provide message 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
> @@ -240,6 +245,35 @@ \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}
> +
> +Seqpacket sockets differ from stream sockets only in data transmission way: in
> +stream sockets all data is sent using only VIRTIO_VSOCK_OP_RW packets. In
> +seqpacket sockets, to provide message boundaries, every sequence of
> +VIRTIO_VSOCK_OP_RW packets of each message is headed with
> +VIRTIO_VSOCK_OP_SEQ_BEGIN and tailed with VIRTIO_VSOCK_OP_SEQ_END packets.
> +Both VIRTIO_VSOCK_OP_SEQ_BEGIN and VIRTIO_VSOCK_OP_SEQ_END packets carry
> +special header in payload:
> +
> +\begin{lstlisting}
> +struct virtio_vsock_seq_hdr {
> +	__le32  msg_cnt;
> +	__le32  msg_len;
> +};
> +\end{lstlisting}

header in what sense? is there more payload beyond that? is header
part of the payload or not? does this replace virtio_vsock_hdr?

> +
> +\field{msg_cnt} is per socket and incremented by 1 on every send of
> +VIRTIO_VSOCK_OP_SEQ_BEGIN or VIRTIO_VSOCK_OP_SEQ_END. \field{msg_len} contains
> +message length. This header is used to check integrity of each message: message
> +is valid if length of data in VIRTIO_VSOCK_OP_RW packets is equal to
> +\field{msg_len} of prepending VIRTIO_VSOCK_OP_SEQ_BEGIN and \field{msg_cnt} of
> +VIRTIO_VSOCK_OP_SEQ_END differs from \field{msg_cnt} of
> +VIRTIO_VSOCK_OP_SEQ_BEGIN by 1.
> +
> +POSIX MSG_EOR flag is supported by special value of \field{flags} in packet's
> +header. If this flag is set for message, then all VIRTIO_VSOCK_OP_RW packets
> +of this message have bit 0 is set to 1 in \field{flags}.
> +
>  \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

So if I understand it correctly, receiver must maintain
VIRTIO_VSOCK_OP_SEQ_BEGIN/VIRTIO_VSOCK_OP_SEQ_END in its buffer,
correct? If so these need to be accounted for as part of
the flow control math. This calls for more changes in the spec,
e.g.

VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
sufficient free buffer space for the payload.

would be

VIRTIO_VSOCK_OP_RW data packets as well as VIRTIO_VSOCK_OP_SEQ_BEGIN and
VIRTIO_VSOCK_OP_SEQ_END message boundary packets MUST only be
transmitted when the peer has sufficient free buffer space for the
payload.

If not then how do we limit host memory use?


Also, what is considered payload here size? the header? are we
worried about wasting buffer space?

the definition of payload and header is important for this:

\field{buf_alloc} and \field{fwd_cnt} are used for buffer space management of
stream sockets. The guest and the device publish how much buffer space is
available per socket. Only payload bytes are counted and header bytes are not
included. This facilitates flow control so data is never dropped.





> -- 
> 2.25.1
> 
> 
> 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/

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description

[Michael S. Tsirkin]

On Mon, Feb 22, 2021 at 11:16:54AM +0100, Stefano Garzarella wrote:
> On Thu, Feb 18, 2021 at 09:07:12AM +0300, Arseny Krasnov wrote:
> > This patchset adds description of SOCK_SEQPACKET socket's type
> > of virtio vsock.
> > 
> > Here is implementation:
> > https://lkml.org/lkml/2021/2/18/24
> > 
> > Arseny Krasnov(1):
> >  virtio-vsock: add SOCK_SEQPACKET description
> > 
> > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
> > 1 files changed, 37 insertions(+), 3 deletions(-)
> > 
> > TODO:
> > - for messages identification I use header's field called 'msg_cnt'.
> >  May be it is better to call it 'msg_id' or 'msg_num', because it
> >  is used as ID, but implemented as counter.
> 
> If we use it only as an identifier, I think 'msg_id' is preferable and we
> shouldn't mention in the specs that it's a counter, since it's just a
> possible implementation.
> Instead if we use the counter for some control, for example if we lost a
> packet, then maybe msg_cnt is better.
> But since the channel shouldn't lose packets, I don't think this is the
> case.
> 
> > 
> > - in current version of specification, some values of headers' fields
> >  still unnamed, for example type of socket (stream or seqpacket), then
> >  shutdown flags. Spec says that for stream socket value of 'type'
> >  must be 1. For receive shutdown bit 0 is set in 'flags', for send
> >  shutdown bit 1 is set in 'flags'. But in Linux these unnamed ones and
> >  zeroes are implemented as enums, so may be it will be ok to add such
> >  enums in specification (as 'enum virtio_vsock_event_id').
> 
> Since we have an enumerate for VIRTIO_VSOCK_OP_* values, I think we can add
> enums for socket type and maybe 'flags'.

We really need to switch enums to defines, for consistency.



> Thanks,
> Stefano
> 
> 
> 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/

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description

[Stefano Garzarella]

On Tue, Mar 16, 2021 at 09:50:48AM -0400, Michael S. Tsirkin wrote:
>On Mon, Feb 22, 2021 at 11:16:54AM +0100, Stefano Garzarella wrote:
>> On Thu, Feb 18, 2021 at 09:07:12AM +0300, Arseny Krasnov wrote:
>> > This patchset adds description of SOCK_SEQPACKET socket's type
>> > of virtio vsock.
>> >
>> > Here is implementation:
>> > https://lkml.org/lkml/2021/2/18/24
>> >
>> > Arseny Krasnov(1):
>> >  virtio-vsock: add SOCK_SEQPACKET description
>> >
>> > virtio-vsock.tex | 40 +++++++++++++++++++++++++++++++++++++---
>> > 1 files changed, 37 insertions(+), 3 deletions(-)
>> >
>> > TODO:
>> > - for messages identification I use header's field called 'msg_cnt'.
>> >  May be it is better to call it 'msg_id' or 'msg_num', because it
>> >  is used as ID, but implemented as counter.
>>
>> If we use it only as an identifier, I think 'msg_id' is preferable and we
>> shouldn't mention in the specs that it's a counter, since it's just a
>> possible implementation.
>> Instead if we use the counter for some control, for example if we lost a
>> packet, then maybe msg_cnt is better.
>> But since the channel shouldn't lose packets, I don't think this is the
>> case.
>>
>> >
>> > - in current version of specification, some values of headers' fields
>> >  still unnamed, for example type of socket (stream or seqpacket), then
>> >  shutdown flags. Spec says that for stream socket value of 'type'
>> >  must be 1. For receive shutdown bit 0 is set in 'flags', for send
>> >  shutdown bit 1 is set in 'flags'. But in Linux these unnamed ones and
>> >  zeroes are implemented as enums, so may be it will be ok to add such
>> >  enums in specification (as 'enum virtio_vsock_event_id').
>>
>> Since we have an enumerate for VIRTIO_VSOCK_OP_* values, I think we can add
>> enums for socket type and maybe 'flags'.
>
>We really need to switch enums to defines, for consistency.

I fully agree, indeed at least in the virtio-vsock part, we use the 
enumerate as they were many defines, always redefining the assigned 
value.
Using defines, we are forced to always define the value and I think 
that's fair!

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization