On 6/12/25 5:53 AM, Jason Wang wrote:

On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni [off-list ref] wrote:

quoted

+static inline int virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
+                                             struct virtio_net_hdr *hdr,
+                                             unsigned int tnl_offset,
+                                             bool little_endian,
+                                             int vlan_hlen)
+{
+       struct virtio_net_hdr_tunnel *tnl;
+       unsigned int inner_nh, outer_th;
+       int tnl_gso_type;
+       int ret;
+
+       tnl_gso_type = skb_shinfo(skb)->gso_type & (SKB_GSO_UDP_TUNNEL |
+

SKB_GSO_UDP_TUNNEL_CSUM);

quoted

+       if (!tnl_gso_type)
+               return virtio_net_hdr_from_skb(skb, hdr,

little_endian, false,

quoted

+                                              vlan_hlen);

So tun_vnet_hdr_from_skb() has

        int vlan_hlen = skb_vlan_tag_present(skb) ? VLAN_HLEN : 0;
        int tnl_offset = tun_vnet_tnl_offset(flags);

        if (virtio_net_hdr_tnl_from_skb(skb, hdr, tnl_offset,
                                        tun_vnet_is_little_endian(flags),
                                        vlan_hlen)) {


It looks like the outer vlan_hlen is used for the inner here?

vlan_hlen always refers to the outer vlan tag (if present), as it moves
the (inner) transport csum offset accordingly.

I can a comment to clarify the parsing.

Note that in the above call there is a single set of headers (no
encapsulation) so the vlan_hlen should be unambigous.

quoted

+
+       /* Tunnel support not negotiated but skb ask for it. */
+       if (!tnl_offset)
+               return -EINVAL;
+
+       /* Let the basic parsing deal with plain GSO features. */
+       skb_shinfo(skb)->gso_type &= ~tnl_gso_type;
+       ret = virtio_net_hdr_from_skb(skb, hdr, true, false, vlan_hlen);

Here I'll add:

	Here vlan_hlen refers to the outer headers set, but still affect
	the inner transport header offset.

quoted

@@ -181,6 +208,22 @@ struct virtio_net_hdr_v1_hash {
        __le16 padding;
 };

+/* This header after hashing information */
+struct virtio_net_hdr_tunnel {
+       __le16 outer_th_offset;
+       __le16 inner_nh_offset;
+};
+
+struct virtio_net_hdr_v1_tunnel {
+       struct virtio_net_hdr_v1 hdr;
+       struct virtio_net_hdr_tunnel tnl;
+};
+
+struct virtio_net_hdr_v1_hash_tunnel {
+       struct virtio_net_hdr_v1_hash hdr;
+       struct virtio_net_hdr_tunnel tnl;
+};

Not a native speaker but I realize there's probably an issue:

        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 or VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
negotiated)
        le16 inner_nh_offset;   (Only if
VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO or VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
negotiated)
        le16 outer_nh_offset;   /* Only if VIRTIO_NET_F_OUT_NET_HEADER
negotiated */
        /* Only if VIRTIO_NET_F_OUT_NET_HEADER or VIRTIO_NET_F_IPSEC
negotiated */
        union {
                u8 padding_reserved_2[6];
                struct ipsec_resource_hdr {
                        le32 resource_id;
                        le16 resource_type;
                } ipsec_resource_hdr;
        };

I thought e.g outer_th_offset should have a fixed offset then
everything is simplified but it looks not the case here. If we decide
to do things like this, we will end up with a very huge uAPI
definition for different features combinations. This doesn't follow
the existing headers for example num_buffers exist no matter if
MRG_RXBUF is negotiated.>> At least, if we decide to go with the

dynamic offset, it seems less

valuable to define those headers with different combinations if both
device and driver process the vnet header piece wisely

I'm a little confused here. AFAICT the dynamic offset is
requested/mandated by the specifications: if the hash related fields are
not present, they are actually non existing and everything below moves
upward.  I think we spent together quite some time to agree on this.

If you want/intend the tunnel header to be at fixed offset inside the
virtio_hdr regardless of the negotiated features? That would yield to
slightly simpler but also slightly less efficient implementation.

Also I guess (fear mostly) some specification clarification would be needed.

/P