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