On Thu, Jun 12, 2025 at 6:10 PM Paolo Abeni [off-list ref] wrote:

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

quoted

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

quoted

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

little_endian, false,

quoted

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.

I see.

quoted

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.

Thanks, then I want to know if we need to care about the inner vlan or
it is something that is not supported by the kernel right now.

quoted

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)

quoted

        le16 hash_report;       (Only if VIRTIO_NET_F_HASH_REPORT

negotiated)

quoted

        le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT

negotiated)

quoted

        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

quoted

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.

I'm sorry if I lose some context there.

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.

Yes. I feel it's probably too late to fix the spec. But I meant if the
header offset of tunnel gso stuff is dynamic, it's probably not need
to define:

virtio_net_hdr_v1_tunnel and virtio_net_hdr_v1_hash_tunnel

in the uAPI.

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

/P

Thanks