Re: [PATCH v7 2/2] virtio-net: define UDP tunnel checksum offload feature

[Paolo Abeni <pabeni@redhat.com>]

On 8/21/24 21:42, Willem de Bruijn wrote:
> On Wed, Aug 21, 2024 at 6:03 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> @@ -425,6 +446,7 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
>>   #define VIRTIO_NET_HDR_F_NEEDS_CSUM    1
>>   #define VIRTIO_NET_HDR_F_DATA_VALID    2
>>   #define VIRTIO_NET_HDR_F_RSC_INFO      4
>> +#define VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM 8
> 
> Only now notice: since this is only valid with GSO, should this be a
> gso_type bit, like VIRTIO_NET_HDR_GSO_ECN?

I originally placed the flag here for consistency with NEEDS_CSUM, 
before making the TUNNEL_CSUM offload depending on TUNNEL_GSO offload.

I think it's still a valid choice: the bit space in 'gso_type' is quite 
low; will be significantly lower then what will have in 'flags' with 
such change, I *think* it's more future proof this way. Do you have 
strong opinion otherwise?

>> +If the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM feature has been negotiated,
>> +and the VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or
>> +VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit in \field{gso_type} are set,
>> +the driver MAY set the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in
>> +\field{flags}, if so:
>> +\begin{enumerate}
>> +\item the driver MUST validate the packet checksum at
>> +       offset 6 from \field{outer_th_offset} as well as all
>> +       preceding offsets;
> 
> I don't follow this comment: when programming checksum offload, why
> does the driver have to validate any checksums?

I must admit this is plain cargo cult programming, from the original 
csum fields specification. I naively (and forcefully) interpreted the 
original statement as 'the driver must validate the csums related 
offset' (yup, it requires quite a bit of good interpretation will ;).

I now think we are better off dropping this sentence (and similar ones) 
completely.

Thanks,

Paolo

> 
> Same in two other locations.
>