Re: [PATCH v7] virtio-net: add Max MTU configuration field

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

On Fri, Aug 26, 2016 at 01:57:53PM -0400, Aaron Conole wrote:
> Sometimes it is essential for a host to be able to configure MTU
> on guest's NICs to a value other than the assumed 1500 byte value.
> 
> The change adds a new field to configuration area of network
> devices. It will be used to pass a maximum MTU from the device to
> the driver. This will be used by the driver as a maximum value for
> packet sizes during transmission, without transmit segment offloading.
> 
> In addition, in order to support backward and forward compatibility,
> we introduce a new feature bit called VIRTIO_NET_F_MTU.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Cc: Victor Kaplansky <victork@redhat.com>


Thanks!
I agree with the intent, with 1 minor exception, some comments on formatting
and wording.

> ---
> Previous thread:
> https://lists.oasis-open.org/archives/virtio-dev/201603/msg00001.html
> 
> Note: should this proposal be accepted and approved, one or more
>       claims disclosed to the TC admin and listed on the Virtio TC
>       IPR page https://www.oasis-open.org/committees/virtio/ipr.php
>       might become Essential Claims.
> 
>  content.tex | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 4b45678..4dc9beb 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3049,6 +3049,14 @@ features.
>  \item[VIRTIO_NET_F_CTRL_GUEST_OFFLOADS (2)] Control channel offloads
>          reconfiguration support.
>  
> +\item[VIRTIO_NET_F_MTU(3)] Maximum negotiated MTU is supported. If
> +    offered by the device, device advises driver about the value of
> +    MTU to be used. If negotiated, the driver uses \field{mtu} as
> +    the maximum MTU value supplied to the operating system.
> +
> +    Note: many operating systems override the MTU value provided by the
> +    driver.
> +
>  \item[VIRTIO_NET_F_MAC (5)] Device has given MAC address.
>  
>  \item[VIRTIO_NET_F_GUEST_TSO4 (7)] Driver can receive TSOv4.
> @@ -3140,11 +3148,16 @@ of each of transmit and receive virtqueues (receiveq1\ldots receiveqN
>  and transmitq1\ldots transmitqN respectively) that can be configured once VIRTIO_NET_F_MQ
>  is negotiated.
>  
> +The following driver-read-only field, \field{mtu} only exists if
> +VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
> +use.
> +
>  \begin{lstlisting}
>  struct virtio_net_config {
>          u8 mac[6];
>          le16 status;
>          le16 max_virtqueue_pairs;
> +        le16 mtu;
>  };
>  \end{lstlisting}
>  
> @@ -3153,6 +3166,19 @@ struct virtio_net_config {
>  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
>  if it offers VIRTIO_NET_F_MQ.
>  
> +\devicenormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
> +

We already have this header, pls do not add two. If you do add
new conformance sections you must update the conformance
clause list, but here you do not.

> +The device MUST set \field{mtu} to between 68 and 65535 inclusive,
> +if it offers VIRTIO_NET_F_MTU.
> +
> +The device MUST NOT modify \field{mtu} once it has been set.
> +
> +The device MUST NOT pass received packets that exceed \field{mtu} size
> +with gso type NONE or ECN.
> +
> +The device MUST forward transmitted packets of up to MTU size with
> +gso type NONE or ECN,

Pls explicitly limit the clauses above to when VIRTIO_NET_F_MTU has been
offered.

Also pls write \field{gso_type}.

> without fragmentation.
> +

I would say "and SHOULD do so without fragmentation" since
things mostly work if you fragment in some rare cases.

And maybe add: failure to do so will result in performance
degration or service interruptions.

>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>  
>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3165,6 +3191,16 @@ If the driver does not negotiate the VIRTIO_NET_F_STATUS feature, it SHOULD
>  assume the link is active, otherwise it SHOULD read the link status from
>  the bottom bit of \field{status}.
>  
> +A driver SHOULD supply enough receive buffers to be able to receive at least
> +one receive packet with gso type NONE or ECN.

Thinking about it, I think we should go back to MUST -
we do not say when it must happen so it's ok to do this
later when recovered from errors.
Reason is, things just do not work if you do not
supply enough packets.

For now, I would limit this to when VIRTIO_NET_F_MTU
has been negotiated, and say:
one packet of size \field{mtu}.

Should we require something without VIRTIO_NET_F_MTU?
We would have to explore what do existing drivers
do such that there is a match between spec and
implementations.

And maybe add: failure to do so will result in performance
degration or service interruptions.

Also pls write \field{gso_type}.

> +
> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
> +
> +A driver MUST honor \field{mtu} if it negotiates VIRTIO_NET_F_MTU.
>

Isn't the following one enough?

> +A driver MUST NOT transmit packets of size exceeding the value of \field{mtu}
> +with gso type NONE or ECN, if it negotiates VIRTIO_NET_F_MTU.

Please reorder with condition first:
If driver negotiates VIRTIO_NET_F_MTU, it MUST NOT ...

Also pls write \field{gso_type}.

> +
>  \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout / Legacy Interface: Device configuration layout}
>  \label{sec:Device Types / Block Device / Feature bits / Device configuration layout / Legacy Interface: Device configuration layout}
>  When using the legacy interface, transitional devices and drivers
> -- 
> 2.5.5