Re: [virtio-dev] [PATCH 2/2] virtio-net: add default_mtu configuration field

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

On Wed, Aug 12, 2015 at 06:37:24PM +0300, Victor Kaplansky wrote:
> Sometimes it is essential for libvirt to be able to configure MTU
> on guest's NICs to a value different from 1500.
> 
> The change adds a new field to configuration area of network
> devices. It will be used to pass initial MTU from the device to
> the driver, and to pass modified MTU from driver to the device
> when a new MTU is assigned by the guest OS.
> 
> In addition, in order to support backward and forward
> compatibility, we introduce a new feature bit called
> VIRTIO_NET_F_DEFAULT_MTU.
> 
> Signed-off-by: Victor Kaplansky <victork@redhat.com>
> ---
>  content.tex | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 37e6a18..5aba1de 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -3078,6 +3078,13 @@ features.
>  
>  \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
>      channel.
> +
> +\item[VIRTIO_NET_F_DEFAULT_MTU(24)] Default MTU is supported.  If
> +    set by the device, device is able to advise driver about initial
> +    MTU to be used. If it remains set after negotiation, the driver
> +    can use \field{default_mtu} as initial value, and will report MTU
> +    changes to the device.
> +
>  \end{description}
>  
>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -3128,6 +3135,7 @@ struct virtio_net_config {
>          u8 mac[6];
>          le16 status;
>          le16 max_virtqueue_pairs;
> +        le16 default_mtu;
>  };
>  \end{lstlisting}
>  
> @@ -3148,6 +3156,16 @@ struct virtio_net_config {
>      \field{max_virtqueue_pairs} is valid only if VIRTIO_NET_F_MQ is
>      set and should not be modified by the driver.
>  
> +\item [\field{default_mtu}] is a hint to the driver set by
> +    the device. It is valid during feature negotiation only if
> +    VIRTIO_NET_F_DEFAULT_MTU is set



simply say "if VIRTIO_NET_F_DEFAULT_MTU is offered"

>  and holds the initial value of
> +    MTU to be used by the driver.


> If VIRTIO_NET_F_DEFAULT_MTU remains
> +    set after negotiation,

simply "is negotiated".

> the driver should use the
> +    \field{default_mtu} as initial value, and also should report MTU

Looks like we need a conformance statement.

Also, pls don't write should/must/may outside conformance
statements. You can say e.g. "uses", "reports".

Applies to previous patch as well.

> +    changes to the device by \field{default_mtu} writes.  Such
> +    reporting will be used for debugging purposes.

Can it be useful for tuning?

> +
> +


Pls avoid double empty lines, here and elsewhere.

>  \end{description}
>  
>  
> -- 
> --Victor
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org