On 5/23/25 1:35 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? >> > > virtio is also an interface, and we can reuse it for QEMU-internal > interfaces too if it is appropriate. > > That said, the feature bitmask defined by virtio is inappropriate for > for qemu_set_offload() because it also contains other features not > related to guest offloading. We need an alternative interface, and the > current qemu_set_offload() just passes each flag separately. > > Now, "[PATCH RFC 12/16] virtio-net: implement extended features > support." is adding another format that derives from virtio for guest > offloading. This format only contains bits related to guest offloading > by definition and suits well with qemu_set_offload(). > > Bit names like VIRTIO_NET_O_GUEST_UDP_TUNNEL_GSO will imply that it > derives from the virtio spec I think this is actually an improvement; > the virtio spec have been the definitive document of the offloading > features of tuntap, and some features even used the virtio header (so > e1000e and igb parse and build virtio headers). These bit names make > this relationship between tuntap and the virtio spec explicit.
Let me check we are on the same page. You are suggesting the following: - change set_offload() signature to: typedef void (SetOffload)(NetClientState *, uint64_t); - define VIRTIO_NET_O_GUEST_<offload> masks for known/supported offload in include/net/net.h (including TSO, USO, etc...) - adapt the drivers to the above interface. - move this patch as series pre-req. Am I correct? Thanks, Paolo
