Re: [PATCH v7] virtio-vsock: Add support for multi devices

["Michael S. Tsirkin" <mst@redhat.com>]

On Sat, Apr 12, 2025 at 10:28:25PM +0800, Xuewei Niu wrote:
> This patch brings a new feature, called "multi devices", to the virtio
> vsock. It introduces a "VIRTIO_VSOCK_F_MULTI_DEVICES" feature bit, and a
> "device_order" field to the config for the virtio vsock.
> 
> == Motivition ==
> 
> Vsock is a lightweight and widely used data exchange mechanism between host
> and guest. Currently, the virtio-vsock only supports one device, resulting
> in the inability to enable more than one backend. For instance, two devices
> are required: one to transfer data to the VMM via virtio-vsock, and another
> to a user process via vhost-user-vsock.
> 
> Apart from that, a side gain is that theoretically the performance might be
> improved since each device has its own queue. But it varies depending on
> the implementation.
> 
> == Typical Usages ==
> 
> Assuming there are two virtio-vsock devices on the guest, with CIDs 3 and 4
> respectively. And the device with CID 3 is default.
> 
> Connect to the host using the device with CID 3.
> 
> ```c
> // use default one (no bind)
> fd = socket(AF_VSOCK);
> connect(fd, 2, 1234);
> n = write(fd, buffer);
> 
> // or bind explicitly
> fd = socket(AF_VSOCK);
> bind(fd, 3, -1);
> connect(fd, 2, 1234);
> n = write(fd, buffer);
> ```
> 
> Connect to the host using the device with CID 4.
> 
> ```c
> // must bind explicitly as the device with CID 4 is not default.
> fd = socket(AF_VSOCK);
> bind(fd, 4, -1);
> connect(fd, 2, 1234);
> n = write(fd, buffer);
> ```
> 
> The first version of multi-devices implementation is available at [1].
> 
> v6 -> v7:
> - Addresses minor review comments from Stefano.
> 
> [1] https://lore.kernel.org/virtualization/20240517144607.2595798-1-niuxuewei.nxw@antgroup.com
> 
> Signed-off-by: Xuewei Niu <niuxuewei.nxw@antgroup.com>
> ---
>  device-types/vsock/description.tex | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/device-types/vsock/description.tex b/device-types/vsock/description.tex
> index 7d91d15..392dc76 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_MULTI_DEVICES (3)] multiple devices feature is supported.
>  \end{description}
>  
>  \drivernormative{\subsubsection}{Feature bits}{Device Types / Socket Device / Feature bits}
> @@ -34,6 +35,12 @@ \subsection{Feature bits}\label{sec:Device Types / Socket Device / Feature bits}
>  VIRTIO_VSOCK_F_NO_IMPLIED_STREAM, the driver MAY act as if
>  VIRTIO_VSOCK_F_STREAM has also been negotiated.
>  
> +The driver SHOULD ignore devices that do not have
> +VIRTIO_VSOCK_F_MULTI_DEVICES if the feature has been negotiated.
> +
> +The driver SHOULD ignore all subsequent devices if a device without
> +VIRTIO_VSOCK_F_MULTI_DEVICES is present.
> +

all this is really vague. any better way to put it?

what are subsequent devices? if the feature has been negotiated where?
what does ignore mean? you can not know features without interacting
with the device.



>  \devicenormative{\subsubsection}{Feature bits}{Device Types / Socket Device / Feature bits}
>  
>  The device SHOULD offer the VIRTIO_VSOCK_F_NO_IMPLIED_STREAM feature.
> @@ -52,6 +59,7 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
>  \begin{lstlisting}
>  struct virtio_vsock_config {
>  	le64 guest_cid;
> +	le16 device_order;
>  };
>  \end{lstlisting}
>  
> @@ -77,11 +85,27 @@ \subsection{Device configuration layout}\label{sec:Device Types / Socket Device
>  \hline
>  \end{tabular}
>  
> +The \field{device_order} is used to identify the default device.

no explanation what is the default device.
is it just for the cid?

> Up to
> +65,535 devices can be supported due to the size.

can be -> are
drop "due to the size".

> +
> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
> +
> +The device MUST provide a distinct \field{device_order} if
> +VIRTIO_VSOCK_F_MULTI_DEVICES feature has been negotiated.

distinct to what?

> +
> +\drivernormative{\subsubsection}{Device configuration layout}{Device Types / Socket Device / Device configuration layout}
> +
> +The driver MUST treat the device with the lowest \field{device_order} as
> +the default device.
> +
>  \subsection{Device Initialization}\label{sec:Device Types / Socket Device / Device Initialization}
>  
>  \begin{enumerate}
>  \item The guest's cid is read from \field{guest_cid}.
>  
> +\item If VIRTIO_VSOCK_F_MULTI_DEVICES has been negotiated, the device's
> +order will be read from \field{device_order}.
> +
>  \item Buffers are added to the event virtqueue to receive events from the device.
>  
>  \item Buffers are added to the rx virtqueue to start receiving packets.
> @@ -233,8 +257,10 @@ \subsubsection{Receive and Transmit}\label{sec:Device Types / Socket Device / De
>  
>  \drivernormative{\paragraph}{Device Operation: Receive and Transmit}{Device Types / Socket Device / Device Operation / Receive and Transmit}
>  
> -The \field{guest_cid} configuration field MUST be used as the source CID when
> -sending outgoing packets.
> +If the source socket is not bound to any source CID, the driver MUST assign
> +one. If more than one device is present, the driver SHOULD use the default
> +device's \field{guest_cid} configuration. Otherwise, the driver SHOULD use
> +the \field{guest_cid} of the only available device.

why did you drop requirement about outgoing packets?

>  
>  A VIRTIO_VSOCK_OP_RST reply MUST be sent if a packet is received with an
>  unknown \field{type} value.
> -- 
> 2.34.1