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

[Paolo Abeni <pabeni@redhat.com>]

On 7/31/24 11:02, Paolo Abeni wrote:
> 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.

After more pondering, I think implementing this option would be quite a 
mess. E.g. we would need a feature bit for 
VIRTIO_NET_HDR_GSO_UDP{4,6}_L4 different from 
VIRTIO_NET_F_{GUEST,HOST}_USO{4,6,}...

I guess we are better off excluding such option.

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

In practice it would replace 'inner_protocol_type' with:

#define VIRTIO_NET_HDR_GSO_INNER_IPv6 0x20

And set/clearing such bit depending on the inner network protocol type, 
for VIRTIO_NET_HDR_GSO_UDP_TUNNEL packets.

Processing the virtio net hdr will require validating such bit value 
with WRT the carried GSO type, i.e. must be set when

gso_type & (VIRTIO_NET_HDR_GSO_TCPV6 | VIRTIO_NET_HDR_GSO_UDP_TUNNEL) ==
VIRTIO_NET_HDR_GSO_TCPV6 | VIRTIO_NET_HDR_GSO_UDP_TUNNEL

must be cleared in all the other cases

Cheers,

Paolo