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

[Paolo Abeni <pabeni@redhat.com>]

On 10/25/24 10:28, Jason Wang wrote:
> On Wed, Oct 23, 2024 at 12:56 AM Paolo Abeni <pabeni@redhat.com> wrote:
>> I proposed the following:
>>
>> For GSO over UDP tunnel VIRTIO_NET_HDR_F_DATA_VALID is allowed if and
>> only even  VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM is set in flags.
>>
>> That means that the device supports csum offload for both the inner and
>> the outer and both csums are valid. The corresponding skb will have
>> CHECKSUM_UNNECESSARY and csum_level == 1.
> 
> Nit: csum_level seems to be Linux specific.

Note: the above was an informal description of the proposal, not the
actual wording to be used in the spec.

>> There is actually another point which I'm quite scared to mention
>> because it caused v9 and implicitly all this discussion.
>>
>> I want to prevent the driver receiving from the device GSO over UDP
>> tunnel packets without csum_start/csum_offset, because the header
>> probing code looks fragile and bug-prone and will be even more complex
>> in case of tunnels.
> 
> This needs more thought. For example, it seems not specific to GSO.
> Should we have a new feature for this? (And build a GSO UDP tunnel on
> top?)

Double checking I read the above correctly.

Do you mean something alike the following to negotiated another feature
controlling a new bit in flag telling 'the csum_start field is set even
if NEEDS_CSUM is not set'?

What about using the flag already defined in patch 2 instead (currently
named VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM, possibly a different better name
can be used [suggestion more than welcome])?

The overall schema will be:

- DATA_VALID retains its current semantic when GSO over UDP features are
not negotiated and when VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM is not set

- when VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM is set that means the following:
  - 2 checksums are offloaded - the 2 outer-most one in case the packet
carries more nested headers.
  and
  - 'csum_start' points to the innermost offloaded header

- DATA_VALID can be set together with VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM,
  meaning:
  - CHECKSUM_UNNECESSARY with csum_level == 1,
  and
  - csum_start -> inner transport offset. If the packet carries many
nested headers csum_start points to the 2nd outermost one.

- VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV{4,6} can be set/used if and only if:
  - NEEDS_CSUM is set (csum_start/csum_offset avail)
    or
  - DATA_VALID and VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM are both set
    (meaning 2 csums are offloaded and csum_start is avail)

Note that even the following will be also allowed:

VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV{4,6} is set, NEEDS_CSUM is set,
VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM is set.

meaning it's GSO over UDP tunnel packets, with both checksum offloaded
and CSUM_PARTIAL.

WDYT?

Thanks,

Paolo