On 5/23/25 12:54 PM, Akihiko Odaki wrote:
> On 2025/05/23 19:40, Paolo Abeni wrote:
>> On 5/23/25 10:16 AM, Akihiko Odaki wrote:
>>> On 2025/05/21 20:34, Paolo Abeni wrote:
>>>> @@ -890,6 +915,12 @@ static void virtio_net_apply_guest_offloads(VirtIONet
>>>> *n)
>>>> .ufo = !!(n->curr_guest_offloads & (1ULL <<
>>>> VIRTIO_NET_F_GUEST_UFO)),
>>>> .uso4 = !!(n->curr_guest_offloads & (1ULL <<
>>>> VIRTIO_NET_F_GUEST_USO4)),
>>>> .uso6 = !!(n->curr_guest_offloads & (1ULL <<
>>>> VIRTIO_NET_F_GUEST_USO6)),
>>>> +#ifdef CONFIG_INT128
>>>> + .tnl = !!(n->curr_guest_offloads &
>>>> + (1ULL << VIRTIO_NET_O_GUEST_UDP_TUNNEL_GSO)),
>>>> + .tnl_csum = !!(n->curr_guest_offloads &
>>>> + (1ULL << VIRTIO_NET_O_GUEST_UDP_TUNNEL_GSO_CSUM)),
>>>
>>> "[PATCH RFC 14/16] net: bundle all offloads in a single struct" added a
>>> struct for offloading, but how about passing n->curr_guest_offloads as
>>> is instead?
>>>
>>> It loses some type safety and makes it prone to have unknown bits, but
>>> omitting duplicate these bit operations may outweigh the downside.
>>
>> I *think* that one of the relevant point about the current interface is
>> that qemu_set_offload() abstracts from the virtio specifics, as it's
>> also used by other drivers. Forcing them to covert the to-be-configured
>> offloads to a virtio specific bitmask sound incorrect to me. Possibly I
>> misread your suggestion?
>>
>> [...]
>>>> diff --git a/net/tap-linux.c b/net/tap-linux.c
>>>> index aa5f3a6e22..b7662ece63 100644
>>>> --- a/net/tap-linux.c
>>>> +++ b/net/tap-linux.c
>>>> @@ -287,6 +287,12 @@ void tap_fd_set_offload(int fd, const NetOffloads *ol)
>>>> if (ol->uso6) {
>>>> offload |= TUN_F_USO6;
>>>> }
>>>> + if ((ol->tso4 || ol->tso6 || ol->uso4 || ol->uso6) && ol->tnl) {
>>>
>>> Is it possible to have ol->tnl without TSO or USO? If so, is ignoring
>>> ol->tnl really what you want?
>>
>> The virtio specifications actually prevent setting UDP-tunnel offload
>> without any other "inner" offload (TSO or USO), as it makes little to no
>> sense (the stack can't GSO/GRO the outer header without doing the same
>> for the inner).
>>
>> Does the above makes sense/answer your questions?
>
> The code implies the following:
> 1a. ol->tnl may be true while TSO and USO are disabled.
> 2a. It is defined as no-op in such a case.
>
> But the reality is as follows:
> 1b. ol->tnl being true while TSO and USO are disabled is an error.
> 2b. The consequence is undefined in such a case.
>
> In that case, virtio_net_get_features() should report the error for 1b,
> which will prevent the error condition from reaching to
> tap_fd_set_offload().
>
> Making the error condition no-op in tap_fd_set_offload() does not make
> it (more) correct as the consequence is undefined anyway (2b). It may
> simply ignore the condition under the assumption that it will never
> happen or assert that assumption.
I see. I'll add the sanity check in virtio_net_get_features() and will
add an assert in the tap code.
Thanks!
/P
