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

[Cornelia Huck <cohuck@redhat.com>]

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?

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. If virtio_net_hdr has a fixed size, we shouldn't need
the second patch, either.