[virtio-dev] Re: [PATCH] virtio-vsock: add VIRTIO_VSOCK_F_DGRAM feature bit

2023-09-01 Thread Bobby Eshleman
On Fri, Sep 01, 2023 at 02:45:14PM +0200, Stefano Garzarella wrote:
> On Tue, Aug 29, 2023 at 09:29:45PM +, Bobby Eshleman wrote:
> > This adds support for datagrams to the virtio-vsock device.
> > 
> > virtio-vsock already supports stream and seqpacket types. The existing
> > message types and header fields are extended to support datagrams.
> > Semantic differences between the flow types are stated, as well as any
> > additional requirements for devices and drivers implementing this
> > feature.
> > 
> > Signed-off-by: Bobby Eshleman 
> > ---
> > device-types/vsock/description.tex | 95 +++---
> > 1 file changed, 88 insertions(+), 7 deletions(-)
> > 
> > diff --git a/device-types/vsock/description.tex 
> > b/device-types/vsock/description.tex
> > index 7d91d159872f..638dca8e5da1 100644
> > --- a/device-types/vsock/description.tex
> > +++ b/device-types/vsock/description.tex
> > @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket 
> > Device / Feature bits}
> > \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> > \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
> > \item[VIRTIO_VSOCK_F_NO_IMPLIED_STREAM (2)] stream socket type is not 
> > implied.
> > +\item[VIRTIO_VSOCK_F_DGRAM (3)] datagram socket type is supported.
> > \end{description}
> > 
> > \drivernormative{\subsubsection}{Feature bits}{Device Types / Socket Device 
> > / Feature bits}
> > @@ -167,17 +168,22 @@ \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 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.
> > +
> > +Currently stream, seqpacket, and datagram sockets are supported. 
> > \field{type} is
> > +1 (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, 2 
> > (VIRTIO_VSOCK_TYPE_SEQPACKET) for
> > +seqpacket socket types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for datagram 
> > socket types.
> > 
> > \begin{lstlisting}
> > #define VIRTIO_VSOCK_TYPE_STREAM1
> > #define VIRTIO_VSOCK_TYPE_SEQPACKET 2
> > +#define VIRTIO_VSOCK_TYPE_DGRAM 3
> > \end{lstlisting}
> > 
> > Stream sockets provide in-order, guaranteed, connection-oriented delivery
> > without message boundaries. Seqpacket sockets provide in-order, guaranteed,
> > -connection-oriented delivery with message and record boundaries.
> > +connection-oriented delivery with message and record boundaries. Datagram
> > +sockets provide connection-less, best-effort delivery of messages, with no
> > +order or reliability guarantees.
> > 
> > \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
> > @@ -203,16 +209,19 @@ \subsubsection{Buffer Space 
> > Management}\label{sec:Device Types / Socket Device /
> > previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
> > communicating updates any time a change in buffer space occurs.
> > 
> > +\field{buf_alloc} and \field{fwd_cnt} are reserved for future use by 
> > datagram
> > +sockets. These fields are not used for datagram buffer space management.
> > +
> > \drivernormative{\paragraph}{Device Operation: Buffer Space 
> > Management}{Device Types / Socket Device / Device Operation / Buffer Space 
> > Management}
> > -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > -sufficient free buffer space for the payload.
> > +For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only 
> > be
> > +transmitted when the peer has sufficient free buffer space for the payload.
> > 
> > All packets associated with a stream flow MUST contain valid information in
> > \field{buf_alloc} and \field{fwd_cnt} fields.
> > 
> > \devicenormative{\paragraph}{Device Operation: Buffer Space
> > Management}{Device Types / Socket Device / Device Operation / Buffer
> > Space Management}
> > -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
> > -sufficient free buffer space for the payload.
> > +For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only 
> > be
> > +transmitted when the peer has sufficient free buffer space for the payload.
> > 
> > All packets associated with a stream flow MUST contain valid information in
> > \field{buf_alloc} and \field{fwd_cnt} fields.
> > @@ -299,6 +308,78 @@ \subsubsection{Seqpacket Sockets}\label{sec:Device 
> > Types / Socket Device / Devic
> > #define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
> > \end{lstlisting}
> > 
> > +\subsubsection{Datagram Sockets}\label{sec:Device Types / Socket Device / 
> > Device Operation / Datagram Sockets}
> > +
> 

[virtio-dev] Re: [PATCH] virtio-vsock: add VIRTIO_VSOCK_F_DGRAM feature bit

2023-09-01 Thread Bobby Eshleman
On Fri, Sep 01, 2023 at 09:31:03AM -0400, Michael S. Tsirkin wrote:
> On Wed, Aug 30, 2023 at 12:43:03AM +, Bobby Eshleman wrote:
> > On Tue, Aug 29, 2023 at 06:21:35PM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Aug 29, 2023 at 09:29:45PM +, Bobby Eshleman wrote:
> > > > This adds support for datagrams to the virtio-vsock device.
> > > > 
> > > > virtio-vsock already supports stream and seqpacket types. The existing
> > > > message types and header fields are extended to support datagrams.
> > > > Semantic differences between the flow types are stated, as well as any
> > > > additional requirements for devices and drivers implementing this
> > > > feature.
> > > > 
> > > > Signed-off-by: Bobby Eshleman 
> > > > ---
> > > >  device-types/vsock/description.tex | 95 +++---
> > > >  1 file changed, 88 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/device-types/vsock/description.tex 
> > > > b/device-types/vsock/description.tex
> > > > index 7d91d159872f..638dca8e5da1 100644
> > > > --- a/device-types/vsock/description.tex
> > > > +++ b/device-types/vsock/description.tex
> > > > @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > > Socket Device / Feature bits}
> > > >  \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> > > >  \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
> > > >  \item[VIRTIO_VSOCK_F_NO_IMPLIED_STREAM (2)] stream socket type is not 
> > > > implied.
> > > > +\item[VIRTIO_VSOCK_F_DGRAM (3)] datagram socket type is supported.
> > > >  \end{description}
> > > >  
> > > >  \drivernormative{\subsubsection}{Feature bits}{Device Types / Socket 
> > > > Device / Feature bits}
> > > > @@ -167,17 +168,22 @@ \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 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.
> > > > +
> > > > +Currently stream, seqpacket, and datagram sockets are supported. 
> > > > \field{type} is
> > > > +1 (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, 2 
> > > > (VIRTIO_VSOCK_TYPE_SEQPACKET) for
> > > > +seqpacket socket types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for datagram 
> > > > socket types.
> > > >  
> > > >  \begin{lstlisting}
> > > >  #define VIRTIO_VSOCK_TYPE_STREAM1
> > > >  #define VIRTIO_VSOCK_TYPE_SEQPACKET 2
> > > > +#define VIRTIO_VSOCK_TYPE_DGRAM 3
> > > >  \end{lstlisting}
> > > >  
> > > >  Stream sockets provide in-order, guaranteed, connection-oriented 
> > > > delivery
> > > >  without message boundaries. Seqpacket sockets provide in-order, 
> > > > guaranteed,
> > > > -connection-oriented delivery with message and record boundaries.
> > > > +connection-oriented delivery with message and record boundaries. 
> > > > Datagram
> > > > +sockets provide connection-less, best-effort delivery of messages, 
> > > > with no
> > > > +order or reliability guarantees.
> > > >  
> > > >  \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
> > > > @@ -203,16 +209,19 @@ \subsubsection{Buffer Space 
> > > > Management}\label{sec:Device Types / Socket Device /
> > > >  previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This 
> > > > allows
> > > >  communicating updates any time a change in buffer space occurs.
> > > >  
> > > > +\field{buf_alloc} and \field{fwd_cnt} are reserved for future use by 
> > > > datagram
> > > > +sockets. These fields are not used for datagram buffer space 
> > > > management.
> > > 
> > > no limits on datagram size?
> > > 
> > 
> > In the Linux proof-of-concept, it is 64KB. I can add that here too.
> 
> or device driven maybe ...
> 

Ah yes, I think Stefano was suggesting something like Laura's proposal:
https://lists.oasis-open.org/archives/virtio-comment/202206/msg00093.html

> > > also with no flow control at all there's a performance problem:
> > > imagine queue gets full. now buffers begin to be dropped.
> > > problem is, dropping is faster than delivering to application.
> > > so now application sees its packets consumed super quickly
> > > and starts sending even faster.
> > > not good for performance.
> > > 
> > > yes datagram expects some way to drop packets but just disabling flow
> > > control completely is not the right thing to do I think.
> > > 
> > 
> > On the LKML I discussed using congestion notification as a way to handle
> > this situation, but deferred it to a future feature bit. I can build
> > it in from the beginning t

[virtio-dev] Re: [PATCH v3] virtio-spi: add the device specification

2023-09-01 Thread Harald Mommer

Hello,

I'm currently integrating your V3 update in our proprietary virtio SPI 
device and in our virtio SPI Linux driver.


The virtio SPI Linux driver is currently not yet in a shape to be shown 
and the paperwork with the company lawyer is not yet done.


Some short first remarks, I'll have to look again into the updated V3 
specification next week.


On 01.09.23 05:29, Haixu Cui wrote:

+
+\begin{lstlisting}
+struct virtio_spi_transfer_head {
+u8 slave_id;
+u8 bits_per_word;
+u8 cs_change;
+u8 tx_nbits;
+u8 rx_nbits;
The "u8 reserved[3];" which was previously here must remain for 
alignment reasons. The "le32 mode" needs to be cleanly and visibly 
aligned to a 4 byte boundary.

+le32 mode;
+le32 freq;


Any hint what to do with the next 3 values in Linux? Currently on the 
device side I've a check that all values are zero. If this is not the case:


if ([any of those values is != zero]) {
    pr_warn_once("word_delay_ns, cs_setup_ns or cs_delay_hold_ns 
not supported\n");

    /* Pray that the SPI device can still cope with the timing */
}

No idea how to cope with those values using /dev/spidev.0 as backend 
device. Looked into some data sheets: If being not on Linux but on some 
small embedded controllers I would probably also have difficulties in 
some cases.


I guess printing a warning and hoping that the SPI device still can cope 
with the timing is the only thing which can be done in some 
environments. However having those values in the message is probably a 
good idea, this is not the point.



+le32 word_delay_ns;
+le32 cs_setup_ns;
+le32 cs_delay_hold_ns;
+le32 cs_change_delay_inactive_ns;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_result {
+u8 result;
+};
+\end{lstlisting}
+
+\begin{lstlisting}
+struct virtio_spi_transfer_req {
+struct virtio_spi_transfer_head head;
+u8 tx_buf[];
+u8 rx_buf[];
+struct virtio_spi_transfer_result result;
Exactly the order which is needed. Having the result as last element 
there are no alignment problems with word sizes > 8 bits. My former 
proposal to move the result field to a different place was simply wrong. 
Perfect now.

+};
+\end{lstlisting}





-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] Re: [PATCH v16] virtio-net: support device stats

2023-09-01 Thread Michael S. Tsirkin
On Fri, Sep 01, 2023 at 01:58:32PM +, Parav Pandit wrote:
> 
> 
> > From: Xuan Zhuo 
> > Sent: Wednesday, August 30, 2023 11:23 AM
> > To: virtio-dev@lists.oasis-open.org
> > Cc: jasow...@redhat.com; Michael S. Tsirkin ; Parav Pandit
> > ; David Edmondson ;
> > virtio-comm...@lists.oasis-open.org
> 
> We should be following [1].
> Virtio list for conducting technical committee work, which is this patch.
> And to get feedback from community virtio-comment list, which you already 
> CCed.
> 
> So virtio-dev should be replaced with virtio list because this patch is about 
> developing the virtio spec itself, (not implementing the spec in 
> device/driver etc).
> 
> [1] 
> https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback


No, virtio-comment was already CC'd, and I think it's the best list for
comments (this is how we classify patches, technically) since anyone can 
subscribe
to that one.

We use virtio mostly for discussion about release schedules, voting,
bylaws and other issues not of interest to TC non members.


Removing virtio-dev is a good idea, copying both is really not necessary.

...

> > +\item [rx_drop_busy]
> > +This is the number of packets dropped by the device when the 
> > device is
> > +busy.
> > +
> I think the busy counter does not belong to busy category as it is slightly 
> difficult to define.
> Lets please move to different category.

Or better define it better. What does "busy" mean? I suspect basically
our of internal resources?
> > +\item [rx_csum_bad]
> > +The number of packets with abnormal csum.
> s/abnormal/wrong
> 
> no strong opinion though.

checsum mismatch.
avoid abbreviation


> > +
> > +\end{description}
> > +
> > +\subparagraph{Transmitq CSUM Statistic}\label{sec:Device Types /
> > +Network Device / Device Operation / Control Virtqueue / Device
> > +Statistic / Transmitq CSUM Statistic}
> > +
> > +The structure corresponding to the transmitq csum statistics is
> > virtio_net_stats_tx_csum.
> > +The corresponding type is VIRTIO_NET_STATS_TYPE_TX_CSUM. This is for the
> > transmitq.
> > +
> > +Only after the VIRTIO_NET_F_CSUM is negotiated, the transmitq csum
> > +statistics can be obtained.
> > +
> > +The following are the transmitq csum statistics:
> > +
> > +\begin{lstlisting}
> > +struct virtio_net_stats_tx_csum {
> > +struct virtio_net_stats_reply_hdr hdr;
> > +
> > +le64 tx_csum_none;
> > +le64 tx_needs_csum;
> > +};
> > +\end{lstlisting}
> > +
> > +The packets described below are all from a specific virtqueue.
> > +\begin{description}
> > +\item [tx_csum_none]
> > +The number of packets which didn't require hardware csum.
> > +
> The number of packets which requires checksum calculation by the device.

which require - it's not the number that requires checksum it's the
packets that require it.

...

> > +The device has the allowance for the speed. If
> > +VIRTIO_NET_F_SPEED_DUPLEX has been negotiated, the driver can get this by
> > \field{speed}.
> > +When the real speed exceeds the speed allowance, some packets will be
> > +dropped by the device.
> > +
> Above description regarding "real speed" is confusing.
> I think you wanted to say,
> 
> When the received packets rate exceeds the negotiated speed, some packets may 
> be dropped by the device.

negotiated how?

yes, I don't understand what this does, either.



-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org



[virtio-dev] RE: [PATCH v16] virtio-net: support device stats

2023-09-01 Thread Parav Pandit



> From: Xuan Zhuo 
> Sent: Wednesday, August 30, 2023 11:23 AM
> To: virtio-dev@lists.oasis-open.org
> Cc: jasow...@redhat.com; Michael S. Tsirkin ; Parav Pandit
> ; David Edmondson ;
> virtio-comm...@lists.oasis-open.org

We should be following [1].
Virtio list for conducting technical committee work, which is this patch.
And to get feedback from community virtio-comment list, which you already CCed.

So virtio-dev should be replaced with virtio list because this patch is about 
developing the virtio spec itself, (not implementing the spec in device/driver 
etc).

[1] https://www.oasis-open.org/committees/tc_home.php?wg_abbrev=virtio#feedback

> Subject: [PATCH v16] virtio-net: support device stats
> 
> This patch allows the driver to obtain some statistics from the device.
> 
> In the device implementation, we can count a lot of such information, which
> can be used for debugging and judging the running status of the device. We
> hope to directly display it to the user through ethtool.
> 
> To get stats atomically, try to get stats for all/multiple queue pairs in one
> command.
> 
> Signed-off-by: Xuan Zhuo 
> Suggested-by: Michael S. Tsirkin 

> +
> +If the VIRTIO_NET_F_DEVICE_STATS feature is negotiated, the driver can
> +obtain device statistics from the device by using the following command.
> +
> +Different types of virtqueues have different statistics. The statistics
> +of the receiveq are different from those of the transmitq.
> +
> +The statistics of a certain type of virtqueue are also divided into
> +multiple types because different types require different features. This
> +enables the expansion of new statistics.
> +
> +In one command, the driver can obtain the statistics of one or multiple
> virtqueues.
> +Additionally, the driver can obtain multiple type statistics of each 
> virtqueue.
> +
> +\subparagraph{Query Statistic Capabilities}\label{sec:Device Types /
> +Network Device / Device Operation / Control Virtqueue / Device
> +Statistic / Query Statistic Capabilities}
> +
> +\begin{lstlisting}
> +#define VIRTIO_NET_CTRL_STATS 7
> +#define VIRTIO_NET_CTRL_STATS_QUERY   0
> +#define VIRTIO_NET_CTRL_STATS_GET 1
> +
> +struct virtio_net_stats_capabilities {
> +
> +#define VIRTIO_NET_STATS_TYPE_CVQ   (1 << 32)
> +
> +#define VIRTIO_NET_STATS_TYPE_RX_BASIC  (1 << 0)
> +#define VIRTIO_NET_STATS_TYPE_RX_CSUM   (1 << 1)
> +#define VIRTIO_NET_STATS_TYPE_RX_GSO(1 << 2)
> +#define VIRTIO_NET_STATS_TYPE_RX_SPEED  (1 << 3)
> +
> +#define VIRTIO_NET_STATS_TYPE_TX_BASIC  (1 << 16)
> +#define VIRTIO_NET_STATS_TYPE_TX_CSUM   (1 << 17)
> +#define VIRTIO_NET_STATS_TYPE_TX_GSO(1 << 18)
> +#define VIRTIO_NET_STATS_TYPE_TX_SPEED  (1 << 19)
> +
> +le64 supported_stats_types[1];
> +}
> +\end{lstlisting}
> +
> +To obtain device statistic capability, use the
> +VIRTIO_NET_CTRL_STATS_QUERY command. When the command completes
> +successfully, \field{command-specific-result} is in the format of 
> \field{struct
> virtio_net_stats_capabilities}.
> +
> +\subparagraph{Get Statistics}\label{sec:Device Types / Network Device /
> +Device Operation / Control Virtqueue / Device Statistic / Get
> +Statistic}
> +
s/Device Statistic/Device Statistics

s/Get Statistic/Get Statistics

> +\begin{lstlisting}
> +struct virtio_net_ctrl_queue_stats {
> +   struct {
> +   le16 vq_index;
> +   le16 padding[3];
> +   le64 types_bitmap[1];
> +   } stats[];
> +};
> +
> +struct virtio_net_stats_reply_hdr {
> +le32 size;
> +le16 vq_index;
> +
> +#define VIRTIO_NET_STATS_TYPE_REPLY_CVQ   32
> +
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_BASIC  0
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_CSUM   1
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_GSO2
> +#define VIRTIO_NET_STATS_TYPE_REPLY_RX_SPEED  3
> +
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_BASIC  16
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_CSUM   17
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_GSO18
> +#define VIRTIO_NET_STATS_TYPE_REPLY_TX_SPEED  19
> +u8 type;
> +u8 padding;
s/padding/reserved so that we can use the reserved for something else in future.

I think arranging it as,
u8 type
u8 reserved
le16 vq_index
le32 size
reads better, to look at the type first and demultiplex from it.
Schema as TLV (type length value) reads obvious.

> +}
> +\end{lstlisting}
> +
> +To obtain device statistics, use the VIRTIO_NET_CTRL_STATS_GET command
> +with the \field{command-specific-data} which is in the format of 
> \field{struct
> virtio_net_ctrl_queue_stats}.
> +When the command completes successfully,
> +\field{command-specific-result} contains multiple statistic results,
> +each statistic result has the \field{struct virtio_net_stats_reply_hdr} as 
> the
> header.
> +
> +The fields of the \field{struct virtio_net_ctrl_queue_stats}:
> +\begin{description}
> +\item [vq_index]
> +The index of the virtqueue to obtain the statistics.
> +
> +\item [types_bitmap]
> +This is a bitmask of the types o

[virtio-dev] Re: [PATCH] virtio-vsock: add VIRTIO_VSOCK_F_DGRAM feature bit

2023-09-01 Thread Michael S. Tsirkin
On Wed, Aug 30, 2023 at 12:43:03AM +, Bobby Eshleman wrote:
> On Tue, Aug 29, 2023 at 06:21:35PM -0400, Michael S. Tsirkin wrote:
> > On Tue, Aug 29, 2023 at 09:29:45PM +, Bobby Eshleman wrote:
> > > This adds support for datagrams to the virtio-vsock device.
> > > 
> > > virtio-vsock already supports stream and seqpacket types. The existing
> > > message types and header fields are extended to support datagrams.
> > > Semantic differences between the flow types are stated, as well as any
> > > additional requirements for devices and drivers implementing this
> > > feature.
> > > 
> > > Signed-off-by: Bobby Eshleman 
> > > ---
> > >  device-types/vsock/description.tex | 95 +++---
> > >  1 file changed, 88 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/device-types/vsock/description.tex 
> > > b/device-types/vsock/description.tex
> > > index 7d91d159872f..638dca8e5da1 100644
> > > --- a/device-types/vsock/description.tex
> > > +++ b/device-types/vsock/description.tex
> > > @@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > Socket Device / Feature bits}
> > >  \item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
> > >  \item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
> > >  \item[VIRTIO_VSOCK_F_NO_IMPLIED_STREAM (2)] stream socket type is not 
> > > implied.
> > > +\item[VIRTIO_VSOCK_F_DGRAM (3)] datagram socket type is supported.
> > >  \end{description}
> > >  
> > >  \drivernormative{\subsubsection}{Feature bits}{Device Types / Socket 
> > > Device / Feature bits}
> > > @@ -167,17 +168,22 @@ \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 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.
> > > +
> > > +Currently stream, seqpacket, and datagram sockets are supported. 
> > > \field{type} is
> > > +1 (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, 2 
> > > (VIRTIO_VSOCK_TYPE_SEQPACKET) for
> > > +seqpacket socket types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for datagram 
> > > socket types.
> > >  
> > >  \begin{lstlisting}
> > >  #define VIRTIO_VSOCK_TYPE_STREAM1
> > >  #define VIRTIO_VSOCK_TYPE_SEQPACKET 2
> > > +#define VIRTIO_VSOCK_TYPE_DGRAM 3
> > >  \end{lstlisting}
> > >  
> > >  Stream sockets provide in-order, guaranteed, connection-oriented delivery
> > >  without message boundaries. Seqpacket sockets provide in-order, 
> > > guaranteed,
> > > -connection-oriented delivery with message and record boundaries.
> > > +connection-oriented delivery with message and record boundaries. Datagram
> > > +sockets provide connection-less, best-effort delivery of messages, with 
> > > no
> > > +order or reliability guarantees.
> > >  
> > >  \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
> > > @@ -203,16 +209,19 @@ \subsubsection{Buffer Space 
> > > Management}\label{sec:Device Types / Socket Device /
> > >  previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
> > >  communicating updates any time a change in buffer space occurs.
> > >  
> > > +\field{buf_alloc} and \field{fwd_cnt} are reserved for future use by 
> > > datagram
> > > +sockets. These fields are not used for datagram buffer space management.
> > 
> > no limits on datagram size?
> > 
> 
> In the Linux proof-of-concept, it is 64KB. I can add that here too.

or device driven maybe ...

> > also with no flow control at all there's a performance problem:
> > imagine queue gets full. now buffers begin to be dropped.
> > problem is, dropping is faster than delivering to application.
> > so now application sees its packets consumed super quickly
> > and starts sending even faster.
> > not good for performance.
> > 
> > yes datagram expects some way to drop packets but just disabling flow
> > control completely is not the right thing to do I think.
> > 
> 
> On the LKML I discussed using congestion notification as a way to handle
> this situation, but deferred it to a future feature bit. I can build
> it in from the beginning though.

as in messages to stop/start transmission? might work.

> > 
> > > +
> > >  \drivernormative{\paragraph}{Device Operation: Buffer Space 
> > > Management}{Device Types / Socket Device / Device Operation / Buffer 
> > > Space Management}
> > > -VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer 
> > > has
> > > -sufficient free buffer space for the payload.
> > > +For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets M

[virtio-dev] Re: [PATCH] virtio-vsock: add VIRTIO_VSOCK_F_DGRAM feature bit

2023-09-01 Thread Stefano Garzarella

On Tue, Aug 29, 2023 at 09:29:45PM +, Bobby Eshleman wrote:

This adds support for datagrams to the virtio-vsock device.

virtio-vsock already supports stream and seqpacket types. The existing
message types and header fields are extended to support datagrams.
Semantic differences between the flow types are stated, as well as any
additional requirements for devices and drivers implementing this
feature.

Signed-off-by: Bobby Eshleman 
---
device-types/vsock/description.tex | 95 +++---
1 file changed, 88 insertions(+), 7 deletions(-)

diff --git a/device-types/vsock/description.tex 
b/device-types/vsock/description.tex
index 7d91d159872f..638dca8e5da1 100644
--- a/device-types/vsock/description.tex
+++ b/device-types/vsock/description.tex
@@ -20,6 +20,7 @@ \subsection{Feature bits}\label{sec:Device Types / Socket 
Device / Feature bits}
\item[VIRTIO_VSOCK_F_STREAM (0)] stream socket type is supported.
\item[VIRTIO_VSOCK_F_SEQPACKET (1)] seqpacket socket type is supported.
\item[VIRTIO_VSOCK_F_NO_IMPLIED_STREAM (2)] stream socket type is not implied.
+\item[VIRTIO_VSOCK_F_DGRAM (3)] datagram socket type is supported.
\end{description}

\drivernormative{\subsubsection}{Feature bits}{Device Types / Socket Device / 
Feature bits}
@@ -167,17 +168,22 @@ \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 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.
+
+Currently stream, seqpacket, and datagram sockets are supported. \field{type} 
is
+1 (VIRTIO_VSOCK_TYPE_STREAM) for stream socket types, 2 
(VIRTIO_VSOCK_TYPE_SEQPACKET) for
+seqpacket socket types, and 3 (VIRTIO_VSOCK_TYPE_DGRAM) for datagram socket 
types.

\begin{lstlisting}
#define VIRTIO_VSOCK_TYPE_STREAM1
#define VIRTIO_VSOCK_TYPE_SEQPACKET 2
+#define VIRTIO_VSOCK_TYPE_DGRAM 3
\end{lstlisting}

Stream sockets provide in-order, guaranteed, connection-oriented delivery
without message boundaries. Seqpacket sockets provide in-order, guaranteed,
-connection-oriented delivery with message and record boundaries.
+connection-oriented delivery with message and record boundaries. Datagram
+sockets provide connection-less, best-effort delivery of messages, with no
+order or reliability guarantees.

\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
@@ -203,16 +209,19 @@ \subsubsection{Buffer Space Management}\label{sec:Device 
Types / Socket Device /
previously receiving a VIRTIO_VSOCK_OP_CREDIT_REQUEST packet. This allows
communicating updates any time a change in buffer space occurs.

+\field{buf_alloc} and \field{fwd_cnt} are reserved for future use by datagram
+sockets. These fields are not used for datagram buffer space management.
+
\drivernormative{\paragraph}{Device Operation: Buffer Space Management}{Device 
Types / Socket Device / Device Operation / Buffer Space Management}
-VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
-sufficient free buffer space for the payload.
+For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only be
+transmitted when the peer has sufficient free buffer space for the payload.

All packets associated with a stream flow MUST contain valid information in
\field{buf_alloc} and \field{fwd_cnt} fields.

\devicenormative{\paragraph}{Device Operation: Buffer Space 
Management}{Device Types / Socket Device / Device Operation / Buffer 
Space Management}

-VIRTIO_VSOCK_OP_RW data packets MUST only be transmitted when the peer has
-sufficient free buffer space for the payload.
+For stream and seqpacket flows, VIRTIO_VSOCK_OP_RW data packets MUST only be
+transmitted when the peer has sufficient free buffer space for the payload.

All packets associated with a stream flow MUST contain valid information in
\field{buf_alloc} and \field{fwd_cnt} fields.
@@ -299,6 +308,78 @@ \subsubsection{Seqpacket Sockets}\label{sec:Device Types / 
Socket Device / Devic
#define VIRTIO_VSOCK_SEQ_EOR (1 << 1)
\end{lstlisting}

+\subsubsection{Datagram Sockets}\label{sec:Device Types / Socket Device / 
Device Operation / Datagram Sockets}
+
+\drivernormative{\paragraph}{Device Operation: Packet Fragmentation}{Device 
Types / Socket Device / Datagram Sockets / Fragmentation}
+
+Drivers MAY disassemble packets into smaller fragments. If drivers fragment a
+packet, they MUST follow the fragmentation rules described in section
+\ref{sec:Device Types / Socket Device / Device Operation / Datagram Sockets / 
Fragmentation}.
+
+Drivers MUST support assembly of received packet fragments according to the
+fragmentation rules d

Re: [virtio-dev] [RFC PATCH v2] docs/interop: define PROBE feature for vhost-user VirtIO devices

2023-09-01 Thread Albert Esteve
This looks great! Thanks for this proposal.

On Fri, Sep 1, 2023 at 1:00 PM Alex Bennée  wrote:

> Currently QEMU has to know some details about the VirtIO device
> supported by a vhost-user daemon to be able to setup the guest. This
> makes it hard for QEMU to add support for additional vhost-user
> daemons without adding specific stubs for each additional VirtIO
> device.
>
> This patch suggests a new feature flag (VHOST_USER_PROTOCOL_F_PROBE)
> which the back-end can advertise which allows a probe message to be
> sent to get all the details QEMU needs to know in one message.
>
> Together with the existing features VHOST_USER_PROTOCOL_F_STATUS and
> VHOST_USER_PROTOCOL_F_CONFIG we can create "standalone" vhost-user
> daemons which are capable of handling all aspects of the VirtIO
> transactions with only a generic stub on the QEMU side. These daemons
> can also be used without QEMU in situations where there isn't a full
> VMM managing their setup.
>
> Signed-off-by: Alex Bennée 
>
> ---
> v2
>   - dropped F_STANDALONE in favour of F_PROBE
>   - split probe details across several messages
>   - probe messages don't automatically imply a standalone daemon
>   - add wording where probe details interact (F_MQ/F_CONFIG)
>   - define VMM and make clear QEMU is only one of many potential VMMs
>   - reword commit message
> ---
>  docs/interop/vhost-user.rst | 90 -
>  hw/virtio/vhost-user.c  |  8 
>  2 files changed, 88 insertions(+), 10 deletions(-)
>
> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> index 5a070adbc1..ba3b5e07b7 100644
> --- a/docs/interop/vhost-user.rst
> +++ b/docs/interop/vhost-user.rst
> @@ -7,6 +7,7 @@ Vhost-user Protocol
>  ..
>Copyright 2014 Virtual Open Systems Sarl.
>Copyright 2019 Intel Corporation
> +  Copyright 2023 Linaro Ltd
>Licence: This work is licensed under the terms of the GNU GPL,
> version 2 or later. See the COPYING file in the top-level
> directory.
> @@ -27,17 +28,31 @@ The protocol defines 2 sides of the communication,
> *front-end* and
>  *back-end*. The *front-end* is the application that shares its
> virtqueues, in
>  our case QEMU. The *back-end* is the consumer of the virtqueues.
>
> -In the current implementation QEMU is the *front-end*, and the *back-end*
> -is the external process consuming the virtio queues, for example a
> -software Ethernet switch running in user space, such as Snabbswitch,
> -or a block device back-end processing read & write to a virtual
> -disk. In order to facilitate interoperability between various back-end
> -implementations, it is recommended to follow the :ref:`Backend program
> -conventions `.
> +In the current implementation a Virtual Machine Manager (VMM) such as
> +QEMU is the *front-end*, and the *back-end* is the external process
> +consuming the virtio queues, for example a software Ethernet switch
> +running in user space, such as Snabbswitch, or a block device back-end
> +processing read & write to a virtual disk. In order to facilitate
> +interoperability between various back-end implementations, it is
> +recommended to follow the :ref:`Backend program conventions
> +`.
>
>  The *front-end* and *back-end* can be either a client (i.e. connecting) or
>  server (listening) in the socket communication.
>
> +Probing device details
> +--
> +
> +Traditionally the vhost-user daemon *back-end* shares configuration
> +responsibilities with the VMM *front-end* which needs to know certain
> +key bits of information about the device. This means the VMM needs to
> +define at least a minimal stub for each VirtIO device it wants to
> +support. If the daemon supports the right set of protocol features the
> +VMM can probe the daemon for the information it needs to setup the
> +device. See :ref:`Probing features for standalone daemons
> +` for more details.
> +
> +
>  Support for platforms other than Linux
>  --
>
> @@ -316,6 +331,7 @@ replies. Here is a list of the ones that do:
>  * ``VHOST_USER_GET_VRING_BASE``
>  * ``VHOST_USER_SET_LOG_BASE`` (if ``VHOST_USER_PROTOCOL_F_LOG_SHMFD``)
>  * ``VHOST_USER_GET_INFLIGHT_FD`` (if
> ``VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD``)
> +* ``VHOST_USER_GET_BACKEND_SPECS`` (if
> ``VHOST_USER_PROTOCOL_F_STANDALONE``)
>
>  .. seealso::
>
> @@ -396,9 +412,10 @@ must support changing some configuration aspects on
> the fly.
>  Multiple queue support
>  --
>
> -Many devices have a fixed number of virtqueues.  In this case the
> front-end
> -already knows the number of available virtqueues without communicating
> with the
> -back-end.
> +Many devices have a fixed number of virtqueues. In this case the
> +*front-end* usually already knows the number of available virtqueues
> +without communicating with the back-end. For standalone daemons this
> +number can be can be probed with the ``VHOST_USER_GET_MIN_VQ`` message.
>
>  Some devices do 

[virtio-dev] [RFC PATCH v2] docs/interop: define PROBE feature for vhost-user VirtIO devices

2023-09-01 Thread Alex Bennée
Currently QEMU has to know some details about the VirtIO device
supported by a vhost-user daemon to be able to setup the guest. This
makes it hard for QEMU to add support for additional vhost-user
daemons without adding specific stubs for each additional VirtIO
device.

This patch suggests a new feature flag (VHOST_USER_PROTOCOL_F_PROBE)
which the back-end can advertise which allows a probe message to be
sent to get all the details QEMU needs to know in one message.

Together with the existing features VHOST_USER_PROTOCOL_F_STATUS and
VHOST_USER_PROTOCOL_F_CONFIG we can create "standalone" vhost-user
daemons which are capable of handling all aspects of the VirtIO
transactions with only a generic stub on the QEMU side. These daemons
can also be used without QEMU in situations where there isn't a full
VMM managing their setup.

Signed-off-by: Alex Bennée 

---
v2
  - dropped F_STANDALONE in favour of F_PROBE
  - split probe details across several messages
  - probe messages don't automatically imply a standalone daemon
  - add wording where probe details interact (F_MQ/F_CONFIG)
  - define VMM and make clear QEMU is only one of many potential VMMs
  - reword commit message
---
 docs/interop/vhost-user.rst | 90 -
 hw/virtio/vhost-user.c  |  8 
 2 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
index 5a070adbc1..ba3b5e07b7 100644
--- a/docs/interop/vhost-user.rst
+++ b/docs/interop/vhost-user.rst
@@ -7,6 +7,7 @@ Vhost-user Protocol
 ..
   Copyright 2014 Virtual Open Systems Sarl.
   Copyright 2019 Intel Corporation
+  Copyright 2023 Linaro Ltd
   Licence: This work is licensed under the terms of the GNU GPL,
version 2 or later. See the COPYING file in the top-level
directory.
@@ -27,17 +28,31 @@ The protocol defines 2 sides of the communication, 
*front-end* and
 *back-end*. The *front-end* is the application that shares its virtqueues, in
 our case QEMU. The *back-end* is the consumer of the virtqueues.
 
-In the current implementation QEMU is the *front-end*, and the *back-end*
-is the external process consuming the virtio queues, for example a
-software Ethernet switch running in user space, such as Snabbswitch,
-or a block device back-end processing read & write to a virtual
-disk. In order to facilitate interoperability between various back-end
-implementations, it is recommended to follow the :ref:`Backend program
-conventions `.
+In the current implementation a Virtual Machine Manager (VMM) such as
+QEMU is the *front-end*, and the *back-end* is the external process
+consuming the virtio queues, for example a software Ethernet switch
+running in user space, such as Snabbswitch, or a block device back-end
+processing read & write to a virtual disk. In order to facilitate
+interoperability between various back-end implementations, it is
+recommended to follow the :ref:`Backend program conventions
+`.
 
 The *front-end* and *back-end* can be either a client (i.e. connecting) or
 server (listening) in the socket communication.
 
+Probing device details
+--
+
+Traditionally the vhost-user daemon *back-end* shares configuration
+responsibilities with the VMM *front-end* which needs to know certain
+key bits of information about the device. This means the VMM needs to
+define at least a minimal stub for each VirtIO device it wants to
+support. If the daemon supports the right set of protocol features the
+VMM can probe the daemon for the information it needs to setup the
+device. See :ref:`Probing features for standalone daemons
+` for more details.
+
+
 Support for platforms other than Linux
 --
 
@@ -316,6 +331,7 @@ replies. Here is a list of the ones that do:
 * ``VHOST_USER_GET_VRING_BASE``
 * ``VHOST_USER_SET_LOG_BASE`` (if ``VHOST_USER_PROTOCOL_F_LOG_SHMFD``)
 * ``VHOST_USER_GET_INFLIGHT_FD`` (if ``VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD``)
+* ``VHOST_USER_GET_BACKEND_SPECS`` (if ``VHOST_USER_PROTOCOL_F_STANDALONE``)
 
 .. seealso::
 
@@ -396,9 +412,10 @@ must support changing some configuration aspects on the 
fly.
 Multiple queue support
 --
 
-Many devices have a fixed number of virtqueues.  In this case the front-end
-already knows the number of available virtqueues without communicating with the
-back-end.
+Many devices have a fixed number of virtqueues. In this case the
+*front-end* usually already knows the number of available virtqueues
+without communicating with the back-end. For standalone daemons this
+number can be can be probed with the ``VHOST_USER_GET_MIN_VQ`` message.
 
 Some devices do not have a fixed number of virtqueues.  Instead the maximum
 number of virtqueues is chosen by the back-end.  The number can depend on host
@@ -885,6 +902,23 @@ Protocol features
   #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS  15
   #define VHOST_USER_PROTOCOL_F_STATUS   16