Re: [PATCH v6 1/2] virtio-net: define UDP tunnel segmentation offload feature

[Paolo Abeni <pabeni@redhat.com>]

On 7/31/24 06:10, Jason Wang wrote:
> On Tue, Jul 30, 2024 at 3:33 PM Paolo Abeni <pabeni@redhat.com> wrote:
>> On 7/29/24 23:04, Willem de Bruijn wrote:
>>>> @@ -423,6 +441,9 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
>>>>            le32 hash_value;        (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>>>>            le16 hash_report;       (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>>>>            le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
>>>> +        le16 outer_th_offset    (Only if VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO negotiated)
>>>> +        le16 inner_protocol_type;(Only if VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO negotiated)
>>>> +        le16 inner_nh_offset;   (Only if VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO negotiated)
>>>>    };

[...]

>> If we would have VIRTIO_NET_HDR_GSO_UDPv4_L4 and
>> VIRTIO_NET_HDR_GSO_UDPv6_L4 variants, we could avoid entirely
>> 'inner_protocol_type', as the network protocol will be implied by the
>> GSO type itself.
> 
> I may miss something, but can it be done by just introducing a new
> gso_type without vnet header extension?

Yep, that is what I mean above: we could replace the
'inner_protocol_type' field - which carries a single bit information, 
inner ipv4 vs ipv6 - with some (inner) GSO-type related info.

Unfortunately that last step does not look 110% 'clean' to me, as the 
inner GSO type is already specified by VIRTIO_NET_HDR_GSO_TCPV4, 
VIRTIO_NET_HDR_GSO_TCPV6 or
VIRTIO_NET_HDR_GSO_UDP_L4. In case of inner TCP, the inner network 
protocol is already specified, but it's not in case of UDP.

I'm wondering: why the single VIRTIO_NET_HDR_GSO_UDP_L4 value has been 
preferred to a pair of ipv4/ipv6 variants? Could we add such variants 
now? Note that I would like such option very much, as it will make this 
change even more complex, but looks IMHO quite clean.

Otherwise another option would be to allocate another bit in the GSO 
type field, set such field when VIRTIO_NET_HDR_GSO_UDP_TUNNEL is set, 
too, and use such field specify the inner header network protocol. I 
don't like much even this option because the new bit will carry 
duplicate information in case of TCP, and it's implementation looks bug 
prone.

I don't see other options, any suggestions/preferences welcome!

Thanks,

Paolo





> 
>>
>>>> +\item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature,
>>>> +  the VIRTIO_NET_HDR_GSO_UDP_TUNNEL bit in \field{gso_type} indicates that
>>>> +  the GSO protocol is encapsulated in a UDP tunnel.
>>>> +  The outer UDP header must not require checksumming, e.g. the outer UDP checksum
>>>> +  must be zero.
>>>
>>> nit: "e.g." should be "i.e.," here
>>>
>>> and maybe must be a positive zero (0x0) checksum as defined in UDP RFC 768.
>>>
>>>> +If the bit VIRTIO_NET_HDR_GSO_UDP_TUNNEL in \field{gso_type} is set, the outer
>>>> +UDP header MUST NOT require checksumming. That is the, the outer UDP checksum
>>>> +value must be 0, or the valid complete checksum for such header.
>>>
>>> MUST NOT require checksum validation. [..] or the validated complete checksum
>>
>> Ack to all the above.
>>
>> Thanks!
>>
>> Paolo
>>
> 
> Thanks
>