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

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

[Stefano Garzarella]

v11:
- reworked "Message and record boundaries" paragraph [stefanha]

v10: https://markmail.org/message/aebu4zqp4kxdm4ej

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

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

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

Thanks,
Stefano

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

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

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

-- 
2.31.1

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

[Stefano Garzarella]

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

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

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

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

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

[Stefano Garzarella]

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

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

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

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

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

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

[Stefano Garzarella]

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

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

Signed-off-by: Arseny Krasnov <arseny.krasnov@kaspersky.com>
[reworked "Message and record boundaries" paragraph]
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 virtio-vsock.tex | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

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

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

[Cornelia Huck]

On Wed, Jan 12 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:

> v11:
> - reworked "Message and record boundaries" paragraph [stefanha]
>
> v10: https://markmail.org/message/aebu4zqp4kxdm4ej
>
> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> so I'm resending Arseny's patches to have consistent virtio-spec
> and implementation.

It really should not have been merged without a spec :( But now that it
has happened already, we need to get a proper spec added sooner rather
than later, and it would be nice if it could make virtio 1.2, which
would mean that voting needs to start this week.

(I don't see an issue at https://github.com/oasis-tcs/virtio-spec/issues
yet; creating one would be an obvious pre-req.)

>
> I added patch 2, following the discussion about F_STREAM feature bit:
> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>
> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
> (F_STREAM), so at this point I don't know if it's better to use a negative
> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
> linux-stable (and QEMU?) to solve this issue.

I fear that even with a patch for stable, there can still be old setups
around for which "only F_SEQPACKET set" translates to "supports both
stream and seqpacket"... so I guess it mostly becomes a question of
whether ignoring those broken setups is tolerable. (The old setups not
setting any feature bits are fine with the proposed patch.) Using a
negative feature bit is uglier, but also clearer. I suppose we want to
be able to make stream sockets optional? If so, I think the negative
approach is the better option, unless we can live with some broken
setups.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Stefano Garzarella]

On Thu, Jan 13, 2022 at 11:50:09AM +0100, Cornelia Huck wrote:
>On Wed, Jan 12 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
>> v11:
>> - reworked "Message and record boundaries" paragraph [stefanha]
>>
>> v10: https://markmail.org/message/aebu4zqp4kxdm4ej
>>
>> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>> so I'm resending Arseny's patches to have consistent virtio-spec
>> and implementation.
>
>It really should not have been merged without a spec :( But now that it

Yeah, I agree but the linux-net maintainers merged it right away, so we 
rushed to merge QEMU too to use the feature.

>has happened already, we need to get a proper spec added sooner rather
>than later, and it would be nice if it could make virtio 1.2, which
>would mean that voting needs to start this week.

Having it in virtio 1.2 would be great.

>
>(I don't see an issue at https://github.com/oasis-tcs/virtio-spec/issues
>yet; creating one would be an obvious pre-req.)

Do I create it now or when we have all the patches ready?

>
>>
>> I added patch 2, following the discussion about F_STREAM feature bit:
>> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>>
>> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
>> (F_STREAM), so at this point I don't know if it's better to use a negative
>> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
>> linux-stable (and QEMU?) to solve this issue.
>
>I fear that even with a patch for stable, there can still be old setups
>around for which "only F_SEQPACKET set" translates to "supports both
>stream and seqpacket"...

Which is usually true, since seqpacket is pretty much stream with the 
message/record boundaries.

> so I guess it mostly becomes a question of
>whether ignoring those broken setups is tolerable. (The old setups not
>setting any feature bits are fine with the proposed patch.) Using a
>negative feature bit is uglier, but also clearer. I suppose we want to
>be able to make stream sockets optional?

Yes, especially with other types, like datagram (not yet merged). We 
would like to be able to allow just datagram, without stream and 
seqpacket.

> If so, I think the negative
>approach is the better option, unless we can live with some broken
>setups.
>

Yes, it's probably the best thing.

I'm thinking about QEMU and the mess to add a new kernel supported 
feature. (vsock is almost always used with vhost-vsock).
We might be in the case where we migrate from a new QEMU that supports 
F_STREAM to an old one that doesn't set it but supports stream anyway.

So I'd like to change patch 2 to F_NO_STREAM. For the spec, it's not the 
best, but for the current implementation, I think it's the cleanest 
solution.
Otherwise maybe we should write in the spec that if F_SEQPACKET is set 
this means that stream is supported even if F_STREAM is not set.


Michael, Stefan, what do you think?


Thanks,
Stefano

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

[Cornelia Huck]

On Thu, Jan 13 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:

> On Thu, Jan 13, 2022 at 11:50:09AM +0100, Cornelia Huck wrote:
>>On Wed, Jan 12 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>>> v11:
>>> - reworked "Message and record boundaries" paragraph [stefanha]
>>>
>>> v10: https://markmail.org/message/aebu4zqp4kxdm4ej
>>>
>>> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>>> so I'm resending Arseny's patches to have consistent virtio-spec
>>> and implementation.
>>
>>It really should not have been merged without a spec :( But now that it
>
> Yeah, I agree but the linux-net maintainers merged it right away, so we 
> rushed to merge QEMU too to use the feature.
>
>>has happened already, we need to get a proper spec added sooner rather
>>than later, and it would be nice if it could make virtio 1.2, which
>>would mean that voting needs to start this week.
>
> Having it in virtio 1.2 would be great.
>
>>
>>(I don't see an issue at https://github.com/oasis-tcs/virtio-spec/issues
>>yet; creating one would be an obvious pre-req.)
>
> Do I create it now or when we have all the patches ready?

If you already know that there will be a v12, I'd probably hold it off
until you post that; but it is easy to simply edit the issue to update
the link to the archive.

>
>>
>>>
>>> I added patch 2, following the discussion about F_STREAM feature bit:
>>> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>>>
>>> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
>>> (F_STREAM), so at this point I don't know if it's better to use a negative
>>> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
>>> linux-stable (and QEMU?) to solve this issue.
>>
>>I fear that even with a patch for stable, there can still be old setups
>>around for which "only F_SEQPACKET set" translates to "supports both
>>stream and seqpacket"...
>
> Which is usually true, since seqpacket is pretty much stream with the 
> message/record boundaries.

Hm; so, does it make much sense to support seqpacket but not stream? If
not, maybe seqpacket can always imply stream, and we'd eliminate the
uncertainty.


>
>> so I guess it mostly becomes a question of
>>whether ignoring those broken setups is tolerable. (The old setups not
>>setting any feature bits are fine with the proposed patch.) Using a
>>negative feature bit is uglier, but also clearer. I suppose we want to
>>be able to make stream sockets optional?
>
> Yes, especially with other types, like datagram (not yet merged). We 
> would like to be able to allow just datagram, without stream and 
> seqpacket.

This would probably work well even if we have seqpacket imply stream.

>
>> If so, I think the negative
>>approach is the better option, unless we can live with some broken
>>setups.
>>
>
> Yes, it's probably the best thing.
>
> I'm thinking about QEMU and the mess to add a new kernel supported 
> feature. (vsock is almost always used with vhost-vsock).
> We might be in the case where we migrate from a new QEMU that supports 
> F_STREAM to an old one that doesn't set it but supports stream anyway.
>
> So I'd like to change patch 2 to F_NO_STREAM. For the spec, it's not the 
> best, but for the current implementation, I think it's the cleanest 
> solution.
> Otherwise maybe we should write in the spec that if F_SEQPACKET is set 
> this means that stream is supported even if F_STREAM is not set.

Yes, if that works, it would probably be the less ugly option.

>
>
> Michael, Stefan, what do you think?
>
>
> 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/

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

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 11:50:09AM +0100, Cornelia Huck wrote:
> On Wed, Jan 12 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> > v11:
> > - reworked "Message and record boundaries" paragraph [stefanha]
> >
> > v10: https://markmail.org/message/aebu4zqp4kxdm4ej
> >
> > Linux kernel and QEMU already merged SOCK_SEQPACKET support,
> > so I'm resending Arseny's patches to have consistent virtio-spec
> > and implementation.
> 
> It really should not have been merged without a spec :( But now that it
> has happened already, we need to get a proper spec added sooner rather
> than later, and it would be nice if it could make virtio 1.2, which
> would mean that voting needs to start this week.
> 
> (I don't see an issue at https://github.com/oasis-tcs/virtio-spec/issues
> yet; creating one would be an obvious pre-req.)
> 
> >
> > I added patch 2, following the discussion about F_STREAM feature bit:
> > https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
> >
> > About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
> > (F_STREAM), so at this point I don't know if it's better to use a negative
> > feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
> > linux-stable (and QEMU?) to solve this issue.
> 
> I fear that even with a patch for stable, there can still be old setups
> around for which "only F_SEQPACKET set" translates to "supports both
> stream and seqpacket"... so I guess it mostly becomes a question of
> whether ignoring those broken setups is tolerable. (The old setups not
> setting any feature bits are fine with the proposed patch.)

It's only been there since 5.14. If we fix it in 5.17 then I think
it's tolerable. And the broken part just would mean creating
sockets does not work, so easy to debug.

> Using a
> negative feature bit is uglier, but also clearer. I suppose we want to
> be able to make stream sockets optional? If so, I think the negative
> approach is the better option, unless we can live with some broken
> setups.

I think we can live with broken setups personally. new features
are often broken in linux, c'est la vie.

-- 
MST

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

[Stefano Garzarella]

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

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/132

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

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

[Stefano Garzarella]

On Thu, Jan 13, 2022 at 12:34:20PM +0100, Cornelia Huck wrote:
>On Thu, Jan 13 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
>> On Thu, Jan 13, 2022 at 11:50:09AM +0100, Cornelia Huck wrote:
>>>On Wed, Jan 12 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
>>>
>>>> v11:
>>>> - reworked "Message and record boundaries" paragraph [stefanha]
>>>>
>>>> v10: https://markmail.org/message/aebu4zqp4kxdm4ej
>>>>
>>>> Linux kernel and QEMU already merged SOCK_SEQPACKET support,
>>>> so I'm resending Arseny's patches to have consistent virtio-spec
>>>> and implementation.
>>>
>>>It really should not have been merged without a spec :( But now that it
>>
>> Yeah, I agree but the linux-net maintainers merged it right away, so we
>> rushed to merge QEMU too to use the feature.
>>
>>>has happened already, we need to get a proper spec added sooner rather
>>>than later, and it would be nice if it could make virtio 1.2, which
>>>would mean that voting needs to start this week.
>>
>> Having it in virtio 1.2 would be great.
>>
>>>
>>>(I don't see an issue at https://github.com/oasis-tcs/virtio-spec/issues
>>>yet; creating one would be an obvious pre-req.)
>>
>> Do I create it now or when we have all the patches ready?
>
>If you already know that there will be a v12, I'd probably hold it off
>until you post that; but it is easy to simply edit the issue to update
>the link to the archive.

Issue created and I also sent an email to the cover letter with the 
follwing tag:

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/132

>
>>
>>>
>>>>
>>>> I added patch 2, following the discussion about F_STREAM feature bit:
>>>> https://markmail.org/message/aoaspjy2jhidwbuo#query:+page:1+mid:obw54zzikgqimhjk+state:results
>>>>
>>>> About patch 2, the vhost-vsock device in the Linux kernel doesn't set bit 0
>>>> (F_STREAM), so at this point I don't know if it's better to use a negative
>>>> feature flag (e.g. F_NO_STREAM) or we go for F_STREAM and send a patch to
>>>> linux-stable (and QEMU?) to solve this issue.
>>>
>>>I fear that even with a patch for stable, there can still be old setups
>>>around for which "only F_SEQPACKET set" translates to "supports both
>>>stream and seqpacket"...
>>
>> Which is usually true, since seqpacket is pretty much stream with the
>> message/record boundaries.
>
>Hm; so, does it make much sense to support seqpacket but not stream? If
>not, maybe seqpacket can always imply stream, and we'd eliminate the
>uncertainty.

No, it doesn't make much sense, so I think the implication always makes 
sense.

>
>
>>
>>> so I guess it mostly becomes a question of
>>>whether ignoring those broken setups is tolerable. (The old setups 
>>>not
>>>setting any feature bits are fine with the proposed patch.) Using a
>>>negative feature bit is uglier, but also clearer. I suppose we want to
>>>be able to make stream sockets optional?
>>
>> Yes, especially with other types, like datagram (not yet merged). We
>> would like to be able to allow just datagram, without stream and
>> seqpacket.
>
>This would probably work well even if we have seqpacket imply stream.

Yep.

>
>>
>>> If so, I think the negative
>>>approach is the better option, unless we can live with some broken
>>>setups.
>>>
>>
>> Yes, it's probably the best thing.
>>
>> I'm thinking about QEMU and the mess to add a new kernel supported
>> feature. (vsock is almost always used with vhost-vsock).
>> We might be in the case where we migrate from a new QEMU that supports
>> F_STREAM to an old one that doesn't set it but supports stream anyway.
>>
>> So I'd like to change patch 2 to F_NO_STREAM. For the spec, it's not the
>> best, but for the current implementation, I think it's the cleanest
>> solution.
>> Otherwise maybe we should write in the spec that if F_SEQPACKET is set
>> this means that stream is supported even if F_STREAM is not set.
>
>Yes, if that works, it would probably be the less ugly option.

Okay, let's go for F_STREAM (even Michael's comment seems to agree with 
that).

Do you think we need to write this implication into the specification, 
or do we leave it to the implementation to solve this transient problem?

Thanks,
Stefano

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

[Cornelia Huck]

On Thu, Jan 13 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:

> On Thu, Jan 13, 2022 at 12:34:20PM +0100, Cornelia Huck wrote:
>>> Otherwise maybe we should write in the spec that if F_SEQPACKET is set
>>> this means that stream is supported even if F_STREAM is not set.
>>
>>Yes, if that works, it would probably be the less ugly option.
>
> Okay, let's go for F_STREAM (even Michael's comment seems to agree with 
> that).
>
> Do you think we need to write this implication into the specification, 
> or do we leave it to the implementation to solve this transient problem?

I would add something like "if F_SEQPACKET has been negotiated, the
[device|driver] MUST act as if F_STREAM has also been negotiated".


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

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

[Stefano Garzarella]

On Thu, Jan 13, 2022 at 03:06:54PM +0100, Cornelia Huck wrote:
>On Thu, Jan 13 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
>
>> On Thu, Jan 13, 2022 at 12:34:20PM +0100, Cornelia Huck wrote:
>>>> Otherwise maybe we should write in the spec that if F_SEQPACKET is set
>>>> this means that stream is supported even if F_STREAM is not set.
>>>
>>>Yes, if that works, it would probably be the less ugly option.
>>
>> Okay, let's go for F_STREAM (even Michael's comment seems to agree with
>> that).
>>
>> Do you think we need to write this implication into the specification,
>> or do we leave it to the implementation to solve this transient problem?
>
>I would add something like "if F_SEQPACKET has been negotiated, the
>[device|driver] MUST act as if F_STREAM has also been negotiated".
>

Ack, I'll send v12 with this change.

Thanks,
Stefano

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

[Michael S. Tsirkin]

On Thu, Jan 13, 2022 at 03:06:54PM +0100, Cornelia Huck wrote:
> On Thu, Jan 13 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
> 
> > On Thu, Jan 13, 2022 at 12:34:20PM +0100, Cornelia Huck wrote:
> >>> Otherwise maybe we should write in the spec that if F_SEQPACKET is set
> >>> this means that stream is supported even if F_STREAM is not set.
> >>
> >>Yes, if that works, it would probably be the less ugly option.
> >
> > Okay, let's go for F_STREAM (even Michael's comment seems to agree with 
> > that).
> >
> > Do you think we need to write this implication into the specification, 
> > or do we leave it to the implementation to solve this transient problem?
> 
> I would add something like "if F_SEQPACKET has been negotiated, the
> [device|driver] MUST act as if F_STREAM has also been negotiated".

I don't think it's necessary really. A couple of months of drivers
do not constitute a legacy that we have to maintain for ever if
the failure mode is graceful enough.
Certainly not for the driver,
and even for device I'd make it MAY, i.e. device can allow
a driver to create stream sockets without negotiating properly.

-- 
MST

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

[Stefano Garzarella]

On Thu, Jan 13, 2022 at 4:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jan 13, 2022 at 03:06:54PM +0100, Cornelia Huck wrote:
> > On Thu, Jan 13 2022, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >
> > > On Thu, Jan 13, 2022 at 12:34:20PM +0100, Cornelia Huck wrote:
> > >>> Otherwise maybe we should write in the spec that if F_SEQPACKET is set
> > >>> this means that stream is supported even if F_STREAM is not set.
> > >>
> > >>Yes, if that works, it would probably be the less ugly option.
> > >
> > > Okay, let's go for F_STREAM (even Michael's comment seems to agree with
> > > that).
> > >
> > > Do you think we need to write this implication into the specification,
> > > or do we leave it to the implementation to solve this transient problem?
> >
> > I would add something like "if F_SEQPACKET has been negotiated, the
> > [device|driver] MUST act as if F_STREAM has also been negotiated".
>
> I don't think it's necessary really. A couple of months of drivers
> do not constitute a legacy that we have to maintain for ever if
> the failure mode is graceful enough.
> Certainly not for the driver,
> and even for device I'd make it MAY, i.e. device can allow
> a driver to create stream sockets without negotiating properly.

Ack, I'll use MAY and only for the device in v12.

Thanks,
Stefano