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.
