RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size calculation text

[Parav Pandit <parav@nvidia.com>]

> From: Cornelia Huck <cohuck@redhat.com>
> Sent: Monday, January 15, 2024 10:14 PM

> On Mon, Jan 15 2024, 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>
> >
> > ---
> > changelog:
> > v1->v2:
> > - addressed comments from Cornelia
> > - rephrase buffer size wording without explicit length of virtio_net_hdr
> >   at 3 places.
> > - replaced otherwise to ', otherwise'
> > - rephrase non normative to write as driver normative
> > ---
> >  device-types/net/description.tex | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> >
> > diff --git a/device-types/net/description.tex
> > b/device-types/net/description.tex
> > index aff5e08..53a56ee 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -657,10 +657,13 @@ \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 be 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.
> 
> Hm, I wonder if we should still mention that
> 
> "\field{struct virtio_net_header} will be prepended to this, which is either 20
> or 12 bytes long, depending on whether VIRTIO_NET_F_HASH_REPORT is
> negotiated or not."
>
Ah I lost that in this change. I will add it. Good point.
 
> > +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}
> >
> > @@ -669,18 +672,24 @@ \subsubsection{Setting Up Receive
> Buffers}\label{sec:Device Types / Network Devi
> >    \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 65609 bytes if
> > +      VIRTIO_NET_F_HASH_REPORT is negotiated, and of at least 65601
> bytes if not.
> >      \item Otherwise, the driver SHOULD populate the receive queue(s)
> > -      with buffers of at least 1526 bytes.
> > +      with buffers of at least 1534 bytes if VIRTIO_NET_F_HASH_REPORT
> > +      is negotiated, and of at least 1526 bytes if not.
> >    \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 size of \field{struct virtio_net_hdr}, i.e. 20 bytes if
> > +VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if not.
> >  \end{itemize}
> >
> >  \begin{note}
> >  Obviously each buffer can be split across multiple descriptor elements.
> >  \end{note}
> >
> > +The driver MUST consider size of field \field{struct virtio_net_hdr}
> > +20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated, and 12 bytes if
> not.
> > +
> 
> Requiring the driver to consider the size of something to be its actual size
> seems a bit odd :) I don't think we need this, as the length can be derived
> from looking at the definitions, and is already spelled out explicitly, if you
> consider my suggestion above.
We need this because tx side also needs to refer to the virtio_net_hdr in patch 2 to be same as that of the rx side.
And hence, this normative sets base line for tx side too. Relying on rest of the receive packet normative is not enough.