RE: RE: [virtio-comment] [PATCH 2/2] virtio-net: Clarify the size of the struct virtio_net_hdr for tx

[Parav Pandit <parav@nvidia.com>]

> 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.