OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.

 


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-comment message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-comment] [PATCH] virtio-vsock: add max payload size config field

On 4/12/22 17:13, Stefano Garzarella wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. 



On Fri, Apr 08, 2022 at 12:57:19PM +0300, Laura Loghin wrote:

On 4/7/22 15:03, Stefano Garzarella wrote:

CAUTION: This email originated from outside of the organization. Do
not click links or open attachments unless you can confirm the
sender and know the content is safe.



On Mon, Apr 04, 2022 at 04:49:31PM +0300, Laura Loghin wrote:

Added a new field to the vsock device config space that
is limiting the size of the packet payload. This way
the driver is not allowed to allocate huge buffers, and
potentially fill up the entire memory.
Also defined a new feature bit for this, VIRTIO_VSOCK_F_SIZE_MAX,
in order to keep backwards compatibility.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
---
virtio-vsock.tex | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)

diff --git a/virtio-vsock.tex b/virtio-vsock.tex
index d79984d..a6950cc 100644
--- a/virtio-vsock.tex
+++ b/virtio-vsock.tex
@@ -23,6 +23,10 @@ \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. 
+\item[VIRTIO_VSOCK_F_SIZE_MAX (2)] Maximum size of the packet
payload is in
+ÂÂÂÂÂ \field{data_max_size}. If offered by the device, device
advises driver
+ÂÂÂÂÂ about the value of its maximum payload size. If negotiated,
the driver uses
+ÂÂÂÂÂ \field{data_max_size} as the maximum packet payload size value.
\end{description}

\subsection{Device configuration layout}\label{sec:Device Types /
Socket Device / Device configuration layout}
@@ -32,6 +36,7 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Socket Device
\begin{lstlisting}
struct virtio_vsock_config {
ÂÂÂÂÂ le64 guest_cid;
+ÂÂÂÂÂ le32 data_max_size;
};
\end{lstlisting}

@@ -57,6 +62,35 @@ \subsection{Device configuration
layout}\label{sec:Device Types / Socket Device
\hline
\end{tabular}

+The following driver-read-only field, \field{data_max_size} only
exists if
+VIRTIO_VSOCK_F_SIZE_MAX is set. This field specifies the maximum
packet payload
+size for the driver to use.


Should we also define the maximum payload size when the feature is not
negotiated?
We never defined it, but the current implementation does not use
payloads larger than 64k.


Hmm I don't know exactly how the spec applies to the vsock. So do we
consider the linux implementation as the "standard" one? In case the


For the driver, I suppose the linux could be the "standard".


driver/device
implementation accepted payloads larger than 64k before, then they will be 

I'm not sure, IIRC we were limited to 4K, then we increased to 64K for
the TX:
0038ff357f05 vsock/virtio: change the maximum packet size allowed
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0038ff357f05fee242aa4e5ec5e75f83fa1ed28c
forced now to negotiate `data_max_size` because of the fixed default value? Or that implementation was considered as not being compliant to the spec? 
I'm not exactly sure how backwards compatibility works here.


Yes, I think we can avoid that. When the flag is not negotiated, the
driver sets the buffer it wants.




+
+\devicenormative{\subsubsection}{Device configuration
layout}{Device Types / Socket Device / Device configuration
layout}
+
+The device MUST set \field{data_max_size} to between 0 and 65536


IIUC, this value is the maximum the device can manage, but then the
driver can always allocate smaller buffers both for TX and RX, so maybe
we could not put a limit, at the end the device can say what it wants,
the driver must only avoid to exceed that value.

True, I was thinking that a hard limit might be useful here, but I
don't have
a strong preference. I can remove this.



inclusive, if
+it offers VIRTIO_VSOCK_F_SIZE_MAX.
+
+The device MUST NOT modify \field{data_max_size} once it has been set. 
+
+The device MUST NOT pass received packets that exceed
\field{data_max_size}
+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been
successfully
+negotiated.


For the device the limit is anyway the buffer (allocated by the driver)
it finds in RX, so I don't know if this sentence is useful.


Yes, I don't think this sentence is needed, I'll remove it.

+
+The device MUST forward transmitted packets of up to
\field{data_max_size}
+(plus header length) size after VIRTIO_VSOCK_F_SIZE_MAX has been
successfully
+negotiated.
+
+\drivernormative{\subsubsection}{Device configuration
layout}{Device Types / Socket Device / Device configuration
layout}
+
+A driver SHOULD negotiate VIRTIO_VSOCK_F_SIZE_MAX if the device
offers it.
+
+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST supply
receive
+buffers that don't exceed the size \field{data_max_size} (plus
header length).
+
+If the driver negotiates VIRTIO_VSOCK_F_SIZE_MAX, it MUST NOT
transmit packets
+of size exceeding the value of \field{data_max_size} (plus header
length).
+
\subsection{Device Initialization}\label{sec:Device Types / Socket
Device / Device Initialization}

\begin{enumerate}
-- 2.17.1


I don't know if it is useful to define also a minimum size for the RX
buffers.


The reason for that would be to not work with a large number of
descriptor chains for a packet?


Yes, exactly. I don't know if it makes sense to have it in the
configuration, though.



Right now I don't see it there as well.



Thanks,
Stefano


I addressed your comments in v2:
https://lists.oasis-open.org/archives/virtio-comment/202204/msg00062.html.
Please have a look!





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/






Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]