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

[Cornelia Huck <cohuck@redhat.com>]

On Tue, Jan 16 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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

My comment here still holds.

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

So if we really want to have normatives, can we come up with something
that reads less weirdly? I currently lack the capacity to suggest
something.