All of lore.kernel.org
 help / color / mirror / Atom feed
* [virtio-comment] [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
@ 2021-02-18  6:07 Arseny Krasnov
  2021-02-18  6:08 ` [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description Arseny Krasnov
  2021-02-22 10:16   ` [virtio-comment] " Stefano Garzarella
  0 siblings, 2 replies; 22+ messages in thread
From: Arseny Krasnov @ 2021-02-18  6:07 UTC (permalink / raw)
  To: cohuck, virtio-comment, Stefan Hajnoczi, Stefano Garzarella,
	Michael S. Tsirkin, Jason Wang, David S. Miller, Jakub Kicinski,
	Arseny Krasnov, Jorgen Hansen, Norbert Slusarek, Colin Ian King,
	Andra Paraschiv
  Cc: virtualization, oxffffaa

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.

- 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').

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>

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


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-02-18  6:07 [virtio-comment] [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description Arseny Krasnov
@ 2021-02-18  6:08 ` Arseny Krasnov
  2021-02-24  9:32     ` Stefano Garzarella
  2021-03-16 13:49     ` Michael S. Tsirkin
  2021-02-22 10:16   ` [virtio-comment] " Stefano Garzarella
  1 sibling, 2 replies; 22+ messages in thread
From: Arseny Krasnov @ 2021-02-18  6:08 UTC (permalink / raw)
  To: cohuck, virtio-comment, Stefan Hajnoczi, Stefano Garzarella,
	Michael S. Tsirkin, Jason Wang, David S. Miller, Jakub Kicinski,
	Arseny Krasnov, Jorgen Hansen, Andra Paraschiv, Norbert Slusarek,
	Colin Ian King
  Cc: virtualization, oxffffaa

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


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
  2021-02-18  6:07 [virtio-comment] [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description Arseny Krasnov
@ 2021-02-22 10:16   ` Stefano Garzarella
  2021-02-22 10:16   ` [virtio-comment] " Stefano Garzarella
  1 sibling, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-02-22 10:16 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Andra Paraschiv, Michael S. Tsirkin, cohuck, Colin Ian King,
	oxffffaa, Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, virtualization, David S. Miller, Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
@ 2021-02-22 10:16   ` Stefano Garzarella
  0 siblings, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-02-22 10:16 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: cohuck, virtio-comment, Stefan Hajnoczi, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Norbert Slusarek, Colin Ian King, Andra Paraschiv,
	virtualization, oxffffaa

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


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [virtio-comment] Re: [MASSMAIL KLMS] [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
  2021-02-22 10:16   ` [virtio-comment] " Stefano Garzarella
  (?)
@ 2021-02-22 10:38   ` Arseny Krasnov
  -1 siblings, 0 replies; 22+ messages in thread
From: Arseny Krasnov @ 2021-02-22 10:38 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: cohuck, virtio-comment, Stefan Hajnoczi, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Norbert Slusarek, Colin Ian King, Andra Paraschiv,
	virtualization, oxffffaa


On 22.02.2021 13:16, 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.

Well let it be 'msg_id', because it's used for packet drop control, and

counter is one of implementations.

> 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'.
Ack
>
> 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/
>
>

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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-02-18  6:08 ` [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description Arseny Krasnov
@ 2021-02-24  9:32     ` Stefano Garzarella
  2021-03-16 13:49     ` Michael S. Tsirkin
  1 sibling, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-02-24  9:32 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Andra Paraschiv, Michael S. Tsirkin, cohuck, Colin Ian King,
	oxffffaa, Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, virtualization, David S. Miller, Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
@ 2021-02-24  9:32     ` Stefano Garzarella
  0 siblings, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-02-24  9:32 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: cohuck, virtio-comment, Stefan Hajnoczi, Michael S. Tsirkin,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa

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


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-02-24  9:32     ` Stefano Garzarella
@ 2021-03-03 12:08       ` Cornelia Huck
  -1 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2021-03-03 12:08 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Andra Paraschiv, Michael S. Tsirkin, Colin Ian King, oxffffaa,
	Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, Arseny Krasnov, virtualization, David S. Miller,
	Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
@ 2021-03-03 12:08       ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2021-03-03 12:08 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Arseny Krasnov, virtio-comment, Stefan Hajnoczi,
	Michael S. Tsirkin, Jason Wang, David S. Miller, Jakub Kicinski,
	Jorgen Hansen, Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa

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).


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-03-03 12:08       ` Cornelia Huck
@ 2021-03-03 12:35         ` Stefano Garzarella
  -1 siblings, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-03-03 12:35 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Andra Paraschiv, Michael S. Tsirkin, Colin Ian King, oxffffaa,
	Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, Arseny Krasnov, virtualization, David S. Miller,
	Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
@ 2021-03-03 12:35         ` Stefano Garzarella
  0 siblings, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-03-03 12:35 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Arseny Krasnov, virtio-comment, Stefan Hajnoczi,
	Michael S. Tsirkin, Jason Wang, David S. Miller, Jakub Kicinski,
	Jorgen Hansen, Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa

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


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-03-03 12:08       ` Cornelia Huck
@ 2021-03-03 16:52         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-03-03 16:52 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Andra Paraschiv, Colin Ian King, oxffffaa, Norbert Slusarek,
	Stefan Hajnoczi, virtio-comment, Jakub Kicinski, Arseny Krasnov,
	virtualization, David S. Miller, Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
@ 2021-03-03 16:52         ` Michael S. Tsirkin
  0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-03-03 16:52 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Stefano Garzarella, Arseny Krasnov, virtio-comment,
	Stefan Hajnoczi, Jason Wang, David S. Miller, Jakub Kicinski,
	Jorgen Hansen, Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa

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


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-03-03 16:52         ` Michael S. Tsirkin
@ 2021-03-03 17:08           ` Cornelia Huck
  -1 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2021-03-03 17:08 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Andra Paraschiv, Colin Ian King, oxffffaa, Norbert Slusarek,
	Stefan Hajnoczi, virtio-comment, Jakub Kicinski, Arseny Krasnov,
	virtualization, David S. Miller, Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
@ 2021-03-03 17:08           ` Cornelia Huck
  0 siblings, 0 replies; 22+ messages in thread
From: Cornelia Huck @ 2021-03-03 17:08 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Stefano Garzarella, Arseny Krasnov, virtio-comment,
	Stefan Hajnoczi, Jason Wang, David S. Miller, Jakub Kicinski,
	Jorgen Hansen, Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa

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.)


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-02-18  6:08 ` [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description Arseny Krasnov
@ 2021-03-16 13:49     ` Michael S. Tsirkin
  2021-03-16 13:49     ` Michael S. Tsirkin
  1 sibling, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-03-16 13:49 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: Andra Paraschiv, cohuck, Colin Ian King, oxffffaa,
	Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, virtualization, David S. Miller, Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
@ 2021-03-16 13:49     ` Michael S. Tsirkin
  0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-03-16 13:49 UTC (permalink / raw)
  To: Arseny Krasnov
  Cc: cohuck, virtio-comment, Stefan Hajnoczi, Stefano Garzarella,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa

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/


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
  2021-02-22 10:16   ` [virtio-comment] " Stefano Garzarella
@ 2021-03-16 13:50     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-03-16 13:50 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Andra Paraschiv, cohuck, Colin Ian King, oxffffaa,
	Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, Arseny Krasnov, virtualization, David S. Miller,
	Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
@ 2021-03-16 13:50     ` Michael S. Tsirkin
  0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2021-03-16 13:50 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: Arseny Krasnov, cohuck, virtio-comment, Stefan Hajnoczi,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Norbert Slusarek, Colin Ian King, Andra Paraschiv,
	virtualization, oxffffaa

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/


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
  2021-03-16 13:50     ` Michael S. Tsirkin
@ 2021-03-16 14:08       ` Stefano Garzarella
  -1 siblings, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-03-16 14:08 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Andra Paraschiv, cohuck, Colin Ian King, oxffffaa,
	Norbert Slusarek, Stefan Hajnoczi, virtio-comment,
	Jakub Kicinski, Arseny Krasnov, virtualization, David S. Miller,
	Jorgen Hansen

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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] Re: [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description
@ 2021-03-16 14:08       ` Stefano Garzarella
  0 siblings, 0 replies; 22+ messages in thread
From: Stefano Garzarella @ 2021-03-16 14:08 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Arseny Krasnov, cohuck, virtio-comment, Stefan Hajnoczi,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Norbert Slusarek, Colin Ian King, Andra Paraschiv,
	virtualization, oxffffaa

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


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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description
  2021-03-16 13:49     ` Michael S. Tsirkin
  (?)
@ 2021-03-17  9:23     ` Arseny Krasnov
  -1 siblings, 0 replies; 22+ messages in thread
From: Arseny Krasnov @ 2021-03-17  9:23 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: cohuck, virtio-comment, Stefan Hajnoczi, Stefano Garzarella,
	Jason Wang, David S. Miller, Jakub Kicinski, Jorgen Hansen,
	Andra Paraschiv, Norbert Slusarek, Colin Ian King,
	virtualization, oxffffaa


On 16.03.2021 16:49, Michael S. Tsirkin 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
>> +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?

Ok, this can be renamed to "special data structure in paylod". And no

extra payload are allowed beyond or before it.

>
>> +
>> +\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.
Ack
>
> If not then how do we limit host memory use?

Both VIRTIO_VSOCK_OP_SEQ_BEGIN/VIRTIO_VSOCK_OP_SEQ_END are accounted

by flow control logic(e.g. size of 'virtio_vsock_seq_hdr'). I'll add that 'virtio_vsock_seq_hdr'

in payload of such packets must never be fragmented.

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

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/


^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-03-17  9:23 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-18  6:07 [virtio-comment] [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description Arseny Krasnov
2021-02-18  6:08 ` [virtio-comment] [RFC PATCH v1 1/1] virtio-vsock: add SOCK_SEQPACKET description Arseny Krasnov
2021-02-24  9:32   ` Stefano Garzarella
2021-02-24  9:32     ` Stefano Garzarella
2021-03-03 12:08     ` Cornelia Huck
2021-03-03 12:08       ` Cornelia Huck
2021-03-03 12:35       ` Stefano Garzarella
2021-03-03 12:35         ` Stefano Garzarella
2021-03-03 16:52       ` Michael S. Tsirkin
2021-03-03 16:52         ` Michael S. Tsirkin
2021-03-03 17:08         ` Cornelia Huck
2021-03-03 17:08           ` Cornelia Huck
2021-03-16 13:49   ` Michael S. Tsirkin
2021-03-16 13:49     ` Michael S. Tsirkin
2021-03-17  9:23     ` Arseny Krasnov
2021-02-22 10:16 ` [RFC PATCH v1 0/1] virtio-vsock: introduce SEQPACKET description Stefano Garzarella
2021-02-22 10:16   ` [virtio-comment] " Stefano Garzarella
2021-02-22 10:38   ` [virtio-comment] Re: [MASSMAIL KLMS] " Arseny Krasnov
2021-03-16 13:50   ` Michael S. Tsirkin
2021-03-16 13:50     ` Michael S. Tsirkin
2021-03-16 14:08     ` Stefano Garzarella
2021-03-16 14:08       ` Stefano Garzarella

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.