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

[Parav Pandit <parav@nvidia.com>]

Hi Cornelia,

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Parav Pandit
> Sent: Friday, January 19, 2024 5:56 PM
> 
> Hi Cornelia,
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, January 16, 2024 6:59 PM
> >
> > On Tue, Jan 16, 2024 at 01:18:59PM +0000, Parav Pandit wrote:
> > >
> > > > From: Cornelia Huck <cohuck@redhat.com>
> > > > Sent: Tuesday, January 16, 2024 6:08 PM
> > > > To: Parav Pandit <parav@nvidia.com>;
> > > > virtio-comment@lists.oasis-open.org;
> > > > mst@redhat.com
> > > > Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > > xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > > Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > > calculation text
> > > >
> > > > On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > > >> From: Cornelia Huck <cohuck@redhat.com>
> > > > >> Sent: Tuesday, January 16, 2024 4:33 PM
> > > > >> To: Parav Pandit <parav@nvidia.com>;
> > > > >> virtio-comment@lists.oasis-open.org;
> > > > >> mst@redhat.com
> > > > >> Cc: Shahaf Shuler <shahafs@nvidia.com>;
> > > > >> xuanzhuo@linux.alibaba.com; yuri.benditovich@daynix.com
> > > > >> Subject: RE: [PATCH v2 1/2] virtio-net: Fix receive buffer size
> > > > >> calculation text
> > > > >>
> > > > >> On Tue, Jan 16 2024, Parav Pandit <parav@nvidia.com> wrote:
> > > > >>
> > > > >> >> 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:
> > > > >> >> > +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.
> > > > >>
> > > > >> Hm, why? If struct virtio_net_hdr is well-defined, its size is
> > > > >> well-defined as well, and we do not need to state it explictly?
> > > > > Because,
> > > > > the size of virtio_net_hdr is derived from the rx side features.
> > > > > Today there is no normative line that says that even though you
> > > > > are using A,
> > > > B, C Rx features, due to which your tx side virtio_net_hdr also changes.
> > > > > The 2nd patch in this series adds this explicit normative as
> > > > > explained in the
> > > > cover letter.
> > > >
> > > > Let's step back a bit.
> > > >
> > > > struct virtio_net_hdr is defined at the beginning of the "Device
> Operation"
> > > > section; the definition clearly says that the last three fields
> > > > depend on VIRTIO_NET_F_HASH_REPORT being negotiated. The device
> > and
> > > > the driver agree on whether HASH_REPORT is negotiated, and
> > > > therefore should also agree on the size of virtio_net_hdr?
> > > >
> > > Do you imply that device operation description is enough to not add
> > normative?
> > > If so, for this case and possibly new things if we write as device
> > > operation,
> > would it be enough?
> > >
> > > > Or is the problem that we did not state explicitly that the last
> > > > three fields of virtio_net_hdr do not exist without HASH_REPORT
> > > > (and are not merely invalid)? If yes, we should spell this out,
> > > > instead of adding normative statements about what the size of
> > > > virtio_net_hdr
> > should be considered to be.
> > > This suggested normative is added in this patch.
> > >
> > > > If virtio_net_hdr has a fixed size, we shouldn't need the second
> > > > patch,
> > either.
> > > The fact that HASH_REPORT is only for the rx, if we have to go back
> > > in time,
> > there is no need for the tx to force also to follow the rx virtio_net_hdr.
> > > There is no explicit normative indicating the virtio_net_hdr for the
> > > TX is
> > forced by the RX even though it has no relation to hash report.
> > >
> > > If you say device operation is enough, than I am sort of lost of
> > > when
> > normative is needed, and when device operation is enough.
> >
> >
> > Generally we start adding normatives when we see that something is
> unclear.
> > But I think generally I agree with Parav, if someone has the time to
> > write the normative, it's all good.
> 
> Are you ok with the v3?
> I kept the clarification for tx and addressed rest of your and Xuan's comments
> in it.
> https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.html
> 

Its been nearly two weeks now.
I assume v3 is ok.
Can you please create the voting ballot?
I updated the github issue to refer to the v3 as well.
https://github.com/oasis-tcs/virtio-spec/issues/183
https://lists.oasis-open.org/archives/virtio-comment/202401/msg00070.html


> 
> This publicly archived list offers a means to provide input to theOASIS Virtual
> I/O Device (VIRTIO) TC.In order to verify user consent to the Feedback
> License terms andto minimize spam in the list archive, subscription is
> requiredbefore posting.Subscribe: virtio-comment-subscribe@lists.oasis-
> open.orgUnsubscribe: virtio-comment-unsubscribe@lists.oasis-open.orgList
> help: virtio-comment-help@lists.oasis-open.orgList archive:
> https://lists.oasis-open.org/archives/virtio-comment/Feedback License:
> https://www.oasis-open.org/who/ipr/feedback_license.pdfList Guidelines:
> https://www.oasis-open.org/policies-guidelines/mailing-listsCommittee:
> https://www.oasis-open.org/committees/virtio/Join OASIS:
> https://www.oasis-open.org/join/