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

[Michael S. Tsirkin]

On Wed, Sep 07, 2016 at 10:20:24AM -0400, Aaron Conole wrote:
> > Again, sorry for being late with comments, but I'll vote with no for
> > now. I can still change the vote if you can convince me, though.
> 
> No problem, thanks so much for the review.  I'm hoping you would be
> convinced to change your vote to yes with the adjusted wording - I'll
> publish a v10 next week (to give folks enough time to respond).
> 
> -Aaron

In that case I'll withdraw the ballot for now. Please let me know
when you want me to restart it.

-- 
MST

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

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

[Aaron Conole]

"Michael S. Tsirkin"  writes:

> On Wed, Sep 07, 2016 at 11:18:22AM +0200, Cornelia Huck wrote:
>> On Tue,  6 Sep 2016 10:31:01 -0400
>> Aaron Conole  wrote:
>> 
>> > this as this is up for vote>
>> 
>> > diff --git a/content.tex b/content.tex
>> > index 4b45678..b90cbad 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.
>> 
>> I'm wondering: Do we need to distinguish between what the _driver_ does
>> and what the _guest_ does, generally speaking?
>> 
>> > +
>> >  \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,18 @@ 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.
>> > 
>> > +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.
>> 
>> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
>> successfully negotiated"? I don't think the driver should assume
>> anything until after feature negotiation is complete.
>
> I don't think this matters much, but generally it's best
> if the config space can be read at any time.
> For example, this can allow validating MTU and not negotiating
> it if it's e.g. too small.
> Do you see value in tweaking the MTU after negotiation?
>
>
>> > +
>> > +The device MUST NOT pass received packets that exceed \field{mtu} size
>> > +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
>> 
>> I don't think it should be the offer that is the deciding factor, but
>> rather the final negotiation.
>
> Makes sense.
>
>> But maybe we can make this "SHOULD NOT"?
>
> It's important to make this a MUST because otherwise device
> needs to do something if it gets a bigger packet -
> e.g. fragment it, or drop it.
> With this wording, it does not have to.
>
>
>> > +
>> > +The device MUST forward transmitted packets of up to MTU size with
>> > +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
>> > +offers VIRTIO_NET_F_MTU.
>> > +
>> >  \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 +3190,15 @@ 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}.
>> > 
>> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
>> > +buffers of size \field{mtu} to be able to receive at least one receive
>> > +packet with \field{gso_type} NONE or ECN.
>> 
>> Again, this should be the final negotiation instead of the offer,
>> especially as...
>
> Agree here - we also can't declare all existing drivers nonconforming.
>
>
>> > +
>> > +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
>> 
>> ...this is SHOULD and not MUST. The driver should be able to fail
>> negotiating the feature if it cannot fulfill the condition above.
>
> For that to work, MTU must not change before negotiation though.
>
>> > +
>> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit
>> > packets of
>> > +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
>> > +
>> >  \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 

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

[Aaron Conole]

Cornelia Huck  writes:

> On Tue,  6 Sep 2016 10:31:01 -0400
> Aaron Conole  wrote:
>
>  this as this is up for vote>
>
>> diff --git a/content.tex b/content.tex
>> index 4b45678..b90cbad 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.
>
> I'm wondering: Do we need to distinguish between what the _driver_ does
> and what the _guest_ does, generally speaking?
>
>> +
>>  \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,18 @@ 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.
>> 
>> +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.
>
> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
> successfully negotiated"? I don't think the driver should assume
> anything until after feature negotiation is complete.

I agree; that was always the intent, but the wording does not convey
that.  Apologies.

>> +
>> +The device MUST NOT pass received packets that exceed \field{mtu} size
>> +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
>
> I don't think it should be the offer that is the deciding factor, but
> rather the final negotiation. But maybe we can make this "SHOULD NOT"?

It really needs to be MUST NOT, but as you note above this needs to be
based on the successful negotiation of the VIRTIO_NET_F_MTU feature,
rather than just the offer.

>> +
>> +The device MUST forward transmitted packets of up to MTU size with
>> +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
>> +offers VIRTIO_NET_F_MTU.
>> +
>>  \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 +3190,15 @@ 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}.
>> 
>> +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
>> +buffers of size \field{mtu} to be able to receive at least one receive
>> +packet with \field{gso_type} NONE or ECN.
>
> Again, this should be the final negotiation instead of the offer,
> especially as...
>
>> +
>> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
>
> ...this is SHOULD and not MUST. The driver should be able to fail
> negotiating the feature if it cannot fulfill the condition above.

Agreed with both points.

>> +
>> +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets of
>> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
>> +
>>  \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
>
> Again, sorry for being late with comments, but I'll vote with no for
> now. I can still change the vote if you can convince me, though.

No problem, thanks so much for the review.  I'm hoping you would be
convinced to change your vote to yes with the adjusted wording - I'll
publish a v10 next week (to give folks enough time to respond).

-Aaron

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

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

[Michael S. Tsirkin]

On Wed, Sep 07, 2016 at 11:18:22AM +0200, Cornelia Huck wrote:
> On Tue,  6 Sep 2016 10:31:01 -0400
> Aaron Conole  wrote:
> 
>  this as this is up for vote>
> 
> > diff --git a/content.tex b/content.tex
> > index 4b45678..b90cbad 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.
> 
> I'm wondering: Do we need to distinguish between what the _driver_ does
> and what the _guest_ does, generally speaking?
> 
> > +
> >  \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,18 @@ 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.
> > 
> > +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.
> 
> Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
> successfully negotiated"? I don't think the driver should assume
> anything until after feature negotiation is complete.

I don't think this matters much, but generally it's best
if the config space can be read at any time.
For example, this can allow validating MTU and not negotiating
it if it's e.g. too small.
Do you see value in tweaking the MTU after negotiation?


> > +
> > +The device MUST NOT pass received packets that exceed \field{mtu} size
> > +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.
> 
> I don't think it should be the offer that is the deciding factor, but
> rather the final negotiation.

Makes sense.

> But maybe we can make this "SHOULD NOT"?

It's important to make this a MUST because otherwise device
needs to do something if it gets a bigger packet -
e.g. fragment it, or drop it.
With this wording, it does not have to.


> > +
> > +The device MUST forward transmitted packets of up to MTU size with
> > +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
> > +offers VIRTIO_NET_F_MTU.
> > +
> >  \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 +3190,15 @@ 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}.
> > 
> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
> > +buffers of size \field{mtu} to be able to receive at least one receive
> > +packet with \field{gso_type} NONE or ECN.
> 
> Again, this should be the final negotiation instead of the offer,
> especially as...

Agree here - we also can't declare all existing drivers nonconforming.


> > +
> > +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.
> 
> ...this is SHOULD and not MUST. The driver should be able to fail
> negotiating the feature if it cannot fulfill the condition above.

For that to work, MTU must not change before negotiation though.

> > +
> > +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets 
> > of
> > +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> > +
> >  \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
> 
> Again, sorry for being late with comments, but I'll vote with no for
> now. I can still change the 

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

[Cornelia Huck]

On Tue,  6 Sep 2016 10:31:01 -0400
Aaron Conole  wrote:



> diff --git a/content.tex b/content.tex
> index 4b45678..b90cbad 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.

I'm wondering: Do we need to distinguish between what the _driver_ does
and what the _guest_ does, generally speaking?

> +
>  \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,18 @@ 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.
> 
> +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.

Should this rather be relaxed to "after VIRTIO_NET_F_MTU has been
successfully negotiated"? I don't think the driver should assume
anything until after feature negotiation is complete.

> +
> +The device MUST NOT pass received packets that exceed \field{mtu} size
> +with \field{gso_type} NONE or ECN if it offers VIRTIO_NET_F_MTU.

I don't think it should be the offer that is the deciding factor, but
rather the final negotiation. But maybe we can make this "SHOULD NOT"?

> +
> +The device MUST forward transmitted packets of up to MTU size with
> +\field{gso_type} NONE or ECN, and do so without fragmentation, if it
> +offers VIRTIO_NET_F_MTU.
> +
>  \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 +3190,15 @@ 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}.
> 
> +If the device offers VIRTIO_NET_F_MTU, a driver MUST supply enough receive
> +buffers of size \field{mtu} to be able to receive at least one receive
> +packet with \field{gso_type} NONE or ECN.

Again, this should be the final negotiation instead of the offer,
especially as...

> +
> +A driver SHOULD negotiate VIRTIO_NET_F_MTU if the device offers it.

...this is SHOULD and not MUST. The driver should be able to fail
negotiating the feature if it cannot fulfill the condition above.

> +
> +If the device offers VIRTIO_NET_F_MTU, a driver MUST NOT transmit packets of
> +size exceeding the value of \field{mtu} with \field{gso_type} NONE or ECN
> +
>  \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

Again, sorry for being late with comments, but I'll vote with no for
now. I can still change the vote if you can convince me, though.


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