virtio-comment

There are few small issues around size of receive buffer and size of the
struct virtio_net_hdr as following.

1. VIRTIO_NET_F_HASH_REPORT is only applicable on the rx side processing.
However it changes the size of the struct virtio_net_hdr for the tx side
as well. This was not clarified when VIRTIO_NET_F_HASH_REPORT feature was
introduced.

2. Receive buffer size calculation description and requirements did not
reflect negotiation of VIRTIO_NET_F_HASH_REPORT.

3. Receive buffer size calculation was incorrect for ipv6 guest gso feature.

Above issue 2 was reported in [1].
Since issue 2 and 3 both touches the same set of requirements and description,
they are updated under a new issue along with tx side fix as well touching the
clarification needed for VIRTIO_NET_F_HASH_REPORT.

Hence above three issues are fixed under issue of [2].

Patch summary:
patch-1 fixes receive side description and requirement
patch-2 adds transmit side clarification for VIRTIO_NET_F_HASH_REPORT

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183

Please review.

Parav Pandit (2):
virtio-net: Fix receive buffer size calculation text
virtio-net: Clarify the size of the struct virtio_net_hdr for tx

device-types/net/description.tex | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)

--
2.34.1

Receive buffer size calculation is based on the following
negotiated features.

The text has wrong calculation for IPv6 and also it has missed
VIRTIO_NET_F_HASH_REPORT.

The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
in [1], however fix for ipv6 payload length must also be
considered.

Since for the both the fixes touching same requirements, a
new issue is created as [2].

This patch brings following fixes.

1. Fix annotating struct virtio_net_hdr as field
2. Fix receive buffer calculation for guest GSO cases to consider
ipv6 payload length
3. small grammar corrections for article
4. reword the requirement to consider the virtio_ndr_hdr which is
depends on the negotiated feature, hence first clarify the
struct virtio_net_hdr size

[1] https://github.com/oasis-tcs/virtio-spec/issues/170
[2] https://github.com/oasis-tcs/virtio-spec/issues/183

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
device-types/net/description.tex | 25 ++++++++++++++++++-------
1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 96f7dc5..f5647d4 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -660,24 +660,35 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6
features are used, the maximum incoming packet
-will be to 65550 bytes long (the maximum size of a
-TCP or UDP packet, plus the 14 byte ethernet header), otherwise
-1514 bytes. The 12-byte struct virtio_net_hdr is prepended to this,
-making for 65562 or 1526 bytes.
+will of size 65589 bytes long (14 bytes of Ethernet header, plus 40 bytes of
+the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
+extension header) otherwise 1514 bytes.
+When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive buffer
+size is either 65601 or 1526 bytes.
+When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
+size is either 65609 or 1534 bytes.

\drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}

+If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
+\field{struct virtio_net_hdr} is 12 bytes.
+
+If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
+\field{struct virtio_net_hdr} is 20 bytes.
+
\begin{itemize}
\item If VIRTIO_NET_F_MRG_RXBUF is not negotiated:
\begin{itemize}
\item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO,
VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated, the driver SHOULD populate
- the receive queue(s) with buffers of at least 65562 bytes.
+ the receive queue(s) with buffers of at least 65589 bytes plus the size
+ of the \field{struct virtio_net_hdr}.
\item Otherwise, the driver SHOULD populate the receive queue(s)
- with buffers of at least 1526 bytes.
+ with buffers of at least 1514 bytes plus the size of the
+ \field{struct virtio_net_hdr}.
\end{itemize}
\item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
-least the size of the struct virtio_net_hdr.
+least the size of the \field{struct virtio_net_hdr}.
\end{itemize}

\begin{note}
--
2.34.1

On Fri, 10 Nov 2023 14:40:39 +0200, Parav Pandit <parav@nvidia.com> wrote:
> Receive buffer size calculation is based on the following
> negotiated features.
>
> The text has wrong calculation for IPv6 and also it has missed
> VIRTIO_NET_F_HASH_REPORT.
>
> The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
> in [1], however fix for ipv6 payload length must also be
> considered.
>
> Since for the both the fixes touching same requirements, a
> new issue is created as [2].
>
> This patch brings following fixes.
>
> 1. Fix annotating struct virtio_net_hdr as field
> 2. Fix receive buffer calculation for guest GSO cases to consider
> ipv6 payload length
> 3. small grammar corrections for article
> 4. reword the requirement to consider the virtio_ndr_hdr which is
> depends on the negotiated feature, hence first clarify the
> struct virtio_net_hdr size
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> [2] https://github.com/oasis-tcs/virtio-spec/issues/183
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
> device-types/net/description.tex | 25 ++++++++++++++++++-------
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 96f7dc5..f5647d4 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -660,24 +660,35 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
> If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6
> features are used, the maximum incoming packet
> -will be to 65550 bytes long (the maximum size of a
> -TCP or UDP packet, plus the 14 byte ethernet header), otherwise
> -1514 bytes. The 12-byte struct virtio_net_hdr is prepended to this,
> -making for 65562 or 1526 bytes.
> +will of size 65589 bytes long (14 bytes of Ethernet header, plus 40 bytes of
> +the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
> +extension header) otherwise 1514 bytes.
> +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive buffer
> +size is either 65601 or 1526 bytes.
> +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
> +size is either 65609 or 1534 bytes.
>
> \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}
>
> +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 12 bytes.
> +
> +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 20 bytes.

For the receive side, is these needed?

Other is ok to me.

Thanks


> +
> \begin{itemize}
> \item If VIRTIO_NET_F_MRG_RXBUF is not negotiated:
> \begin{itemize}
> \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO,
> VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated, the driver SHOULD populate
> - the receive queue(s) with buffers of at least 65562 bytes.
> + the receive queue(s) with buffers of at least 65589 bytes plus the size
> + of the \field{struct virtio_net_hdr}.
> \item Otherwise, the driver SHOULD populate the receive queue(s)
> - with buffers of at least 1526 bytes.
> + with buffers of at least 1514 bytes plus the size of the
> + \field{struct virtio_net_hdr}.
> \end{itemize}
> \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
> -least the size of the struct virtio_net_hdr.
> +least the size of the \field{struct virtio_net_hdr}.
> \end{itemize}
>
> \begin{note}
> --
> 2.34.1
>
>
> 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/
>

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Wednesday, December 13, 2023 12:13 PM
>
> On Fri, 10 Nov 2023 14:40:39 +0200, Parav Pandit <parav@nvidia.com>
> wrote:
> > Receive buffer size calculation is based on the following negotiated
> > features.
> >
> > The text has wrong calculation for IPv6 and also it has missed
> > VIRTIO_NET_F_HASH_REPORT.
> >
> > The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported in
> > [1], however fix for ipv6 payload length must also be considered.
> >
> > Since for the both the fixes touching same requirements, a new issue
> > is created as [2].
> >
> > This patch brings following fixes.
> >
> > 1. Fix annotating struct virtio_net_hdr as field 2. Fix receive buffer
> > calculation for guest GSO cases to consider
> > ipv6 payload length
> > 3. small grammar corrections for article 4. reword the requirement to
> > consider the virtio_ndr_hdr which is
> > depends on the negotiated feature, hence first clarify the
> > struct virtio_net_hdr size
> >
> > [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> > [2] https://github.com/oasis-tcs/virtio-spec/issues/183
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> > device-types/net/description.tex | 25 ++++++++++++++++++-------
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index 96f7dc5..f5647d4 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -660,24 +660,35 @@ \subsubsection{Setting Up Receive
> > Buffers}\label{sec:Device Types / Network Devi If the
> > VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> > VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or
> > VIRTIO_NET_F_GUEST_USO6 features are used, the maximum incoming
> > packet -will be to 65550 bytes long (the maximum size of a -TCP or UDP
> > packet, plus the 14 byte ethernet header), otherwise
> > -1514 bytes. The 12-byte struct virtio_net_hdr is prepended to this,
> > -making for 65562 or 1526 bytes.
> > +will of size 65589 bytes long (14 bytes of Ethernet header, plus 40
> > +bytes of the IPv6 header, plus 65535 bytes of maximum IPv6 payload
> > +including any extension header) otherwise 1514 bytes.
> > +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive
> > +buffer size is either 65601 or 1526 bytes.
> > +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive
> > +buffer size is either 65609 or 1534 bytes.
> >
> > \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types
> > / Network Device / Device Operation / Setting Up Receive Buffers}
> >
> > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 12 bytes.
> > +
I realized just now, that above normative is incorrect due to optional VIRTIO_NET_F_MRG_RXBUF.
I will remove/reword it.

> > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 20 bytes.
>
> For the receive side, is these needed?
>
For uniformity between tx and rx I thought it is useful as it is not covered anywhere else in the normative.

> Other is ok to me.
>
> Thanks

On Wed, Dec 13, 2023 at 8:44?AM Xuan Zhuo <xuanzhuo@linux.alibaba.com>
wrote:

> On Fri, 10 Nov 2023 14:40:39 +0200, Parav Pandit <parav@nvidia.com> wrote:
> > Receive buffer size calculation is based on the following
> > negotiated features.
> >
> > The text has wrong calculation for IPv6 and also it has missed
> > VIRTIO_NET_F_HASH_REPORT.
> >
> > The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
> > in [1], however fix for ipv6 payload length must also be
> > considered.
> >
> > Since for the both the fixes touching same requirements, a
> > new issue is created as [2].
> >
> > This patch brings following fixes.
> >
> > 1. Fix annotating struct virtio_net_hdr as field
> > 2. Fix receive buffer calculation for guest GSO cases to consider
> > ipv6 payload length
> > 3. small grammar corrections for article
> > 4. reword the requirement to consider the virtio_ndr_hdr which is
> > depends on the negotiated feature, hence first clarify the
> > struct virtio_net_hdr size
> >
> > [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> > [2] https://github.com/oasis-tcs/virtio-spec/issues/183
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> > device-types/net/description.tex | 25 ++++++++++++++++++-------
> > 1 file changed, 18 insertions(+), 7 deletions(-)
> >
> > diff --git a/device-types/net/description.tex
> b/device-types/net/description.tex
> > index 96f7dc5..f5647d4 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -660,24 +660,35 @@ \subsubsection{Setting Up Receive
> Buffers}\label{sec:Device Types / Network Devi
> > If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> > VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or
> VIRTIO_NET_F_GUEST_USO6
> > features are used, the maximum incoming packet
> > -will be to 65550 bytes long (the maximum size of a
> > -TCP or UDP packet, plus the 14 byte ethernet header), otherwise
> > -1514 bytes. The 12-byte struct virtio_net_hdr is prepended to this,
> > -making for 65562 or 1526 bytes.
> > +will of size 65589 bytes long (14 bytes of Ethernet header, plus 40
> bytes of
> > +the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
> > +extension header) otherwise 1514 bytes.
> > +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive
> buffer
> > +size is either 65601 or 1526 bytes.
> > +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
> > +size is either 65609 or 1534 bytes.
> >
> > \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types /
> Network Device / Device Operation / Setting Up Receive Buffers}
> >
> > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 12 bytes.
> > +
> > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 20 bytes.
>
> For the receive side, is these needed?
>
> Other is ok to me.
>
> Thanks
>
>
At the moment the virtio_net header size is equal for RX and TX
So, if the VIRTIO_NET_F_HASH_REPORT is negotiated the header size will be
20 bytes for both directions..
In TX direction the extra space is the header is currently not used (if I'm
not mistaken).


>
> > +
> > \begin{itemize}
> > \item If VIRTIO_NET_F_MRG_RXBUF is not negotiated:
> > \begin{itemize}
> > \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> VIRTIO_NET_F_GUEST_UFO,
> > VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated,
> the driver SHOULD populate
> > - the receive queue(s) with buffers of at least 65562 bytes.
> > + the receive queue(s) with buffers of at least 65589 bytes plus
> the size
> > + of the \field{struct virtio_net_hdr}.
> > \item Otherwise, the driver SHOULD populate the receive queue(s)
> > - with buffers of at least 1526 bytes.
> > + with buffers of at least 1514 bytes plus the size of the
> > + \field{struct virtio_net_hdr}.
> > \end{itemize}
> > \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
> > -least the size of the struct virtio_net_hdr.
> > +least the size of the \field{struct virtio_net_hdr}.
> > \end{itemize}
> >
> > \begin{note}
> > --
> > 2.34.1
> >
> >
> > 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/
> >
>
> 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/
>
>

On Wed, 13 Dec 2023 12:28:48 +0200, Yuri Benditovich <yuri.benditovich@daynix.com> wrote:
> On Wed, Dec 13, 2023 at 8:44?AM Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> wrote:
>
> > On Fri, 10 Nov 2023 14:40:39 +0200, Parav Pandit <parav@nvidia.com> wrote:
> > > Receive buffer size calculation is based on the following
> > > negotiated features.
> > >
> > > The text has wrong calculation for IPv6 and also it has missed
> > > VIRTIO_NET_F_HASH_REPORT.
> > >
> > > The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
> > > in [1], however fix for ipv6 payload length must also be
> > > considered.
> > >
> > > Since for the both the fixes touching same requirements, a
> > > new issue is created as [2].
> > >
> > > This patch brings following fixes.
> > >
> > > 1. Fix annotating struct virtio_net_hdr as field
> > > 2. Fix receive buffer calculation for guest GSO cases to consider
> > > ipv6 payload length
> > > 3. small grammar corrections for article
> > > 4. reword the requirement to consider the virtio_ndr_hdr which is
> > > depends on the negotiated feature, hence first clarify the
> > > struct virtio_net_hdr size
> > >
> > > [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> > > [2] https://github.com/oasis-tcs/virtio-spec/issues/183
> > >
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > ---
> > > device-types/net/description.tex | 25 ++++++++++++++++++-------
> > > 1 file changed, 18 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > > index 96f7dc5..f5647d4 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -660,24 +660,35 @@ \subsubsection{Setting Up Receive
> > Buffers}\label{sec:Device Types / Network Devi
> > > If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> > > VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or
> > VIRTIO_NET_F_GUEST_USO6
> > > features are used, the maximum incoming packet
> > > -will be to 65550 bytes long (the maximum size of a
> > > -TCP or UDP packet, plus the 14 byte ethernet header), otherwise
> > > -1514 bytes. The 12-byte struct virtio_net_hdr is prepended to this,
> > > -making for 65562 or 1526 bytes.
> > > +will of size 65589 bytes long (14 bytes of Ethernet header, plus 40
> > bytes of
> > > +the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
> > > +extension header) otherwise 1514 bytes.
> > > +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive
> > buffer
> > > +size is either 65601 or 1526 bytes.
> > > +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
> > > +size is either 65609 or 1534 bytes.
> > >
> > > \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types /
> > Network Device / Device Operation / Setting Up Receive Buffers}
> > >
> > > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> > > +\field{struct virtio_net_hdr} is 12 bytes.
> > > +
> > > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > > +\field{struct virtio_net_hdr} is 20 bytes.
> >
> > For the receive side, is these needed?
> >
> > Other is ok to me.
> >
> > Thanks
> >
> >
> At the moment the virtio_net header size is equal for RX and TX
> So, if the VIRTIO_NET_F_HASH_REPORT is negotiated the header size will be
> 20 bytes for both directions..
> In TX direction the extra space is the header is currently not used (if I'm
> not mistaken).

YES.


>
>
> >
> > > +
> > > \begin{itemize}
> > > \item If VIRTIO_NET_F_MRG_RXBUF is not negotiated:
> > > \begin{itemize}
> > > \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> > VIRTIO_NET_F_GUEST_UFO,
> > > VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated,
> > the driver SHOULD populate
> > > - the receive queue(s) with buffers of at least 65562 bytes.
> > > + the receive queue(s) with buffers of at least 65589 bytes plus
> > the size
> > > + of the \field{struct virtio_net_hdr}.
> > > \item Otherwise, the driver SHOULD populate the receive queue(s)
> > > - with buffers of at least 1526 bytes.
> > > + with buffers of at least 1514 bytes plus the size of the
> > > + \field{struct virtio_net_hdr}.
> > > \end{itemize}
> > > \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
> > > -least the size of the struct virtio_net_hdr.
> > > +least the size of the \field{struct virtio_net_hdr}.
> > > \end{itemize}
> > >
> > > \begin{note}
> > > --
> > > 2.34.1
> > >
> > >
> > > 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/
> > >
> >
> > 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/
> >
> >
>

The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it was
not clarified that the size of the struct virtio_net_hdr on the packet
transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
negotiated.

Explicitly clarify this.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
device-types/net/description.tex | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index f5647d4..00ea58d 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -531,6 +531,12 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De

\drivernormative{\paragraph}{Packet Transmission}{Device Types / Network Device / Device Operation / Packet Transmission}

+If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
+\field{struct virtio_net_hdr} is 12 bytes.
+
+If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
+\field{struct virtio_net_hdr} is 20 bytes.
+
The driver MUST set \field{num_buffers} to zero.

If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
--
2.34.1

On Fri, 10 Nov 2023 14:40:40 +0200, Parav Pandit <parav@nvidia.com> wrote:
> The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it was
> not clarified that the size of the struct virtio_net_hdr on the packet
> transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> negotiated.
>
> Explicitly clarify this.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
> device-types/net/description.tex | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index f5647d4..00ea58d 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -531,6 +531,12 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>
> \drivernormative{\paragraph}{Packet Transmission}{Device Types / Network Device / Device Operation / Packet Transmission}
>
> +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 12 bytes.
> +
> +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 20 bytes.
> +

If we need a new feature, because that the implement of the current linux kernel
has the same size on the tx patch.

Thanks.


> The driver MUST set \field{num_buffers} to zero.
>
> If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
> --
> 2.34.1
>
>
> 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/
>

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Wednesday, December 13, 2023 11:33 AM
>
> On Fri, 10 Nov 2023 14:40:40 +0200, Parav Pandit <parav@nvidia.com>
> wrote:
> > The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> > However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it
> was
> > not clarified that the size of the struct virtio_net_hdr on the packet
> > transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> > negotiated.
> >
> > Explicitly clarify this.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> > device-types/net/description.tex | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index f5647d4..00ea58d 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -531,6 +531,12 @@ \subsubsection{Packet
> > Transmission}\label{sec:Device Types / Network Device / De
> >
> > \drivernormative{\paragraph}{Packet Transmission}{Device Types /
> > Network Device / Device Operation / Packet Transmission}
> >
> > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 12 bytes.
> > +
> > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 20 bytes.
> > +
>
> If we need a new feature, because that the implement of the current linux
> kernel has the same size on the tx patch.

I didn't understand your comment - "if we need a new feature" part.

Current Linux kernel has same size for tx and rx, snippet below.

if (vi->has_rss_hash_report)
vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);

So if you are asking if we need a new feature bit? I believe, we don't need a new feature bit for current implementations and behavior.

We will new feature bit in future to have shorter virtio_net_hdr on tx, however we better consume it as part of new inline descriptor format.

On Wed, 13 Dec 2023 06:23:35 +0000, Parav Pandit <parav@nvidia.com> wrote:
>
> > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > Sent: Wednesday, December 13, 2023 11:33 AM
> >
> > On Fri, 10 Nov 2023 14:40:40 +0200, Parav Pandit <parav@nvidia.com>
> > wrote:
> > > The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> > > However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it
> > was
> > > not clarified that the size of the struct virtio_net_hdr on the packet
> > > transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> > > negotiated.
> > >
> > > Explicitly clarify this.
> > >
> > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > ---
> > > device-types/net/description.tex | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/device-types/net/description.tex
> > > b/device-types/net/description.tex
> > > index f5647d4..00ea58d 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -531,6 +531,12 @@ \subsubsection{Packet
> > > Transmission}\label{sec:Device Types / Network Device / De
> > >
> > > \drivernormative{\paragraph}{Packet Transmission}{Device Types /
> > > Network Device / Device Operation / Packet Transmission}
> > >
> > > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> > > +\field{struct virtio_net_hdr} is 12 bytes.
> > > +
> > > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > > +\field{struct virtio_net_hdr} is 20 bytes.
> > > +
> >
> > If we need a new feature, because that the implement of the current linux
> > kernel has the same size on the tx patch.
>
> I didn't understand your comment - "if we need a new feature" part.
>
> Current Linux kernel has same size for tx and rx, snippet below.
>
> if (vi->has_rss_hash_report)
> vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
>
> So if you are asking if we need a new feature bit? I believe, we don't need a new feature bit for current implementations and behavior.
>
> We will new feature bit in future to have shorter virtio_net_hdr on tx, however we better consume it as part of new inline descriptor format.

Sorry, I missed something.

One question, for "If VIRTIO_NET_F_HASH_REPORT is not negotiated", should we
consider the legacy mode.

Thanks.

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Wednesday, December 13, 2023 12:00 PM
>
> On Wed, 13 Dec 2023 06:23:35 +0000, Parav Pandit <parav@nvidia.com>
> wrote:
> >
> > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > Sent: Wednesday, December 13, 2023 11:33 AM
> > >
> > > On Fri, 10 Nov 2023 14:40:40 +0200, Parav Pandit <parav@nvidia.com>
> > > wrote:
> > > > The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive
> side.
> > > > However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it
> > > was
> > > > not clarified that the size of the struct virtio_net_hdr on the
> > > > packet transmission also uses higher size when
> > > > VIRTIO_NET_F_HASH_REPORT is negotiated.
> > > >
> > > > Explicitly clarify this.
> > > >
> > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > > ---
> > > > device-types/net/description.tex | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/device-types/net/description.tex
> > > > b/device-types/net/description.tex
> > > > index f5647d4..00ea58d 100644
> > > > --- a/device-types/net/description.tex
> > > > +++ b/device-types/net/description.tex
> > > > @@ -531,6 +531,12 @@ \subsubsection{Packet
> > > > Transmission}\label{sec:Device Types / Network Device / De
> > > >
> > > > \drivernormative{\paragraph}{Packet Transmission}{Device Types /
> > > > Network Device / Device Operation / Packet Transmission}
> > > >
> > > > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the
> > > > +field \field{struct virtio_net_hdr} is 12 bytes.
> > > > +
> > > > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > > > +\field{struct virtio_net_hdr} is 20 bytes.
> > > > +
> > >
> > > If we need a new feature, because that the implement of the current
> > > linux kernel has the same size on the tx patch.
> >
> > I didn't understand your comment - "if we need a new feature" part.
> >
> > Current Linux kernel has same size for tx and rx, snippet below.
> >
> > if (vi->has_rss_hash_report)
> > vi->hdr_len = sizeof(struct virtio_net_hdr_v1_hash);
> >
> > So if you are asking if we need a new feature bit? I believe, we don't need a
> new feature bit for current implementations and behavior.
> >
> > We will new feature bit in future to have shorter virtio_net_hdr on tx,
> however we better consume it as part of new inline descriptor format.
>
> Sorry, I missed something.
>
> One question, for "If VIRTIO_NET_F_HASH_REPORT is not negotiated", should
> we consider the legacy mode.

Most of the legacy text is not mixed with rest of the spec, so I think we don't need to mention legacy in this area of the text.

I realized that 12B line is not accurate due to optional VIRTIO_NET_F_MRG_RXBUF feature.
I will send v1 to remove 12B wording, and only keep for HASH_REPORT negotiated.

On Fri, 10 Nov 2023 14:40:40 +0200, Parav Pandit <parav@nvidia.com> wrote:
> The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it was
> not clarified that the size of the struct virtio_net_hdr on the packet
> transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> negotiated.
>
> Explicitly clarify this.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
> device-types/net/description.tex | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index f5647d4..00ea58d 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -531,6 +531,12 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>
> \drivernormative{\paragraph}{Packet Transmission}{Device Types / Network Device / Device Operation / Packet Transmission}
>
> +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 12 bytes.
> +
> +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 20 bytes.
> +


How about just say the tx header is equal to the rx hdr?

Thanks.


> The driver MUST set \field{num_buffers} to zero.
>
> If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
> --
> 2.34.1
>
>
> 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/
>

> From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> Sent: Thursday, December 14, 2023 7:57 AM
> To: Parav Pandit <parav@nvidia.com>
> Cc: Shahaf Shuler <shahafs@nvidia.com>; Parav Pandit <parav@nvidia.com>;
> virtio-comment@lists.oasis-open.org; mst@redhat.com;
> cohuck@redhat.com
> Subject: Re: [virtio-comment] [PATCH 2/2] virtio-net: Clarify the size of the
> struct virtio_net_hdr for tx
>
> On Fri, 10 Nov 2023 14:40:40 +0200, Parav Pandit <parav@nvidia.com>
> wrote:
> > The feature VIRTIO_NET_F_HASH_REPORT only applies to the receive side.
> > However, when VIRTIO_NET_F_HASH_REPORT feature was introduced, it
> was
> > not clarified that the size of the struct virtio_net_hdr on the packet
> > transmission also uses higher size when VIRTIO_NET_F_HASH_REPORT is
> > negotiated.
> >
> > Explicitly clarify this.
> >
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > ---
> > device-types/net/description.tex | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index f5647d4..00ea58d 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -531,6 +531,12 @@ \subsubsection{Packet
> > Transmission}\label{sec:Device Types / Network Device / De
> >
> > \drivernormative{\paragraph}{Packet Transmission}{Device Types /
> > Network Device / Device Operation / Packet Transmission}
> >
> > +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 12 bytes.
> > +
> > +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> > +\field{struct virtio_net_hdr} is 20 bytes.
> > +
>
>
> How about just say the tx header is equal to the rx hdr?
>
Yep. Working on rewording as acked yday.


> Thanks.
>
>
> > The driver MUST set \field{num_buffers} to zero.
> >
> > If VIRTIO_NET_F_CSUM is not negotiated, the driver MUST set
> > --
> > 2.34.1
> >
> >
> > 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/
> >