[PATCH net-next 0/4] virtio-net: avoid XDP and _F_GUEST_CSUM

[PATCH net-next 0/4] virtio-net: avoid XDP and _F_GUEST_CSUM

[Heng Qi]

virtio-net needs to clear the VIRTIO_NET_F_GUEST_CSUM feature when
loading XDP. The main reason for doing this is because
VIRTIO_NET_F_GUEST_CSUM allows to receive packets marked as
VIRTIO_NET_HDR_F_NEEDS_CSUM. Such packets are not compatible with
XDP programs, because we cannot guarantee that the csum_{start, offset}
fields are correct after XDP modifies the packets.

There is also an existing problem, in the same host vm-vm (eg
[vm]<->[ovs vhost-user]<->[vm]) scenario, loading XDP will cause packet loss.

To solve the above problems, we have discussed in the [1] proposal, and
now try to solve it through the method of reprobing fields suggested
by Jason.

[1] https://lists.oasis-open.org/archives/virtio-dev/202305/msg00318.html

Heng Qi (4):
  virtio-net: a helper for probing the pseudo-header checksum
  virtio-net: reprobe csum related fields for skb passed by XDP
  virtio-net: virtio-net: support coexistence of XDP and _F_GUEST_CSUM
  virtio-net: remove F_GUEST_CSUM check for XDP loading

 drivers/net/virtio_net.c | 173 +++++++++++++++++++++++++++++++++++----
 1 file changed, 158 insertions(+), 15 deletions(-)

-- 
2.19.1.6.gb485710b

[PATCH net-next 1/4] virtio-net: a helper for probing the pseudo-header checksum

[Heng Qi]

This helper parses UDP/TCP and calculates the pseudo-header checksum
for virtio-net.

virtio-net currently does not support insertion/deletion of VLANs nor
SCTP checksum offloading.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 95 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 5a7f7a76b920..36cae78f6311 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1568,6 +1568,101 @@ static void virtio_skb_set_hash(const struct virtio_net_hdr_v1_hash *hdr_hash,
 	skb_set_hash(skb, __le32_to_cpu(hdr_hash->hash_value), rss_hash_type);
 }
 
+static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff *skb)
+{
+	struct net_device *dev = vi->dev;
+	struct flow_keys_basic keys;
+	struct udphdr *uh;
+	struct tcphdr *th;
+	int len, offset;
+
+	/* The flow dissector needs this information. */
+	skb->dev = dev;
+	skb_reset_mac_header(skb);
+	skb->protocol = dev_parse_header_protocol(skb);
+	/* virtio-net does not need to resolve VLAN. */
+	skb_set_network_header(skb, ETH_HLEN);
+	if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
+					      NULL, 0, 0, 0, 0))
+		return -EINVAL;
+
+	/* 1. Pseudo-header checksum calculation requires:
+	 *    (1) saddr/daddr (2) IP_PROTO (3) length of transport payload
+	 * 2. We don't parse SCTP because virtio-net currently doesn't
+	 *    support CRC offloading for SCTP.
+	 */
+	if (keys.basic.n_proto == htons(ETH_P_IP)) {
+		struct iphdr *iph;
+
+		/* Flow dissector has verified that there is an IP header. */
+		iph = ip_hdr(skb);
+		if (iph->version != 4 || !pskb_may_pull(skb, iph->ihl * 4))
+			return -EINVAL;
+
+		skb->transport_header = skb->mac_header + keys.control.thoff;
+		offset = skb_transport_offset(skb);
+		len = skb->len - offset;
+		if (keys.basic.ip_proto == IPPROTO_UDP) {
+			if (!pskb_may_pull(skb, offset + sizeof(struct udphdr)))
+				return -EINVAL;
+
+			uh = udp_hdr(skb);
+			skb->csum_offset = offsetof(struct udphdr, check);
+			/* Although uh->len is already the 3rd parameter for the calculation
+			 * of the pseudo-header checksum, we have already calculated the
+			 * length of the transport layer, so use 'len' here directly.
+			 */
+			uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, len,
+					IPPROTO_UDP, 0);
+		} else if (keys.basic.ip_proto == IPPROTO_TCP) {
+			if (!pskb_may_pull(skb, offset + sizeof(struct tcphdr)))
+				return -EINVAL;
+
+			th = tcp_hdr(skb);
+			skb->csum_offset = offsetof(struct tcphdr, check);
+			th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, len,
+					IPPROTO_TCP, 0);
+		} /* virtio-net doesn't support checksums for SCTP hw offloading.*/
+	} else if (keys.basic.n_proto == htons(ETH_P_IPV6)) {
+		struct ipv6hdr *ip6h;
+
+		ip6h = ipv6_hdr(skb);
+		if (ip6h->version != 6)
+			return -EINVAL;
+
+		/* We have skipped the possible extension headers for IPv6.
+		 * If there is a Routing Header, the tx's check value is calculated by
+		 * final_dst, and that value is the rx's daddr.
+		 */
+		skb->transport_header = skb->mac_header + keys.control.thoff;
+		offset = skb_transport_offset(skb);
+		len = skb->len - offset;
+		if (keys.basic.ip_proto == IPPROTO_UDP) {
+			if (!pskb_may_pull(skb, offset + sizeof(struct udphdr)))
+				return -EINVAL;
+
+			uh = udp_hdr(skb);
+			skb->csum_offset = offsetof(struct udphdr, check);
+			uh->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
+					(const struct in6_addr *)&ip6h->daddr,
+					len, IPPROTO_UDP, 0);
+		} else if (keys.basic.ip_proto == IPPROTO_TCP) {
+			if (!pskb_may_pull(skb, offset + sizeof(struct tcphdr)))
+				return -EINVAL;
+
+			th = tcp_hdr(skb);
+			skb->csum_offset = offsetof(struct tcphdr, check);
+			th->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
+					(const struct in6_addr *)&ip6h->daddr,
+					len, IPPROTO_TCP, 0);
+		}
+	}
+
+	skb->csum_start = skb->transport_header;
+
+	return 0;
+}
+
 static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 			void *buf, unsigned int len, void **ctx,
 			unsigned int *xdp_xmit,
-- 
2.19.1.6.gb485710b

[PATCH net-next 2/4] virtio-net: reprobe csum related fields for skb passed by XDP

[Heng Qi]

Currently, the VIRTIO_NET_F_GUEST_CSUM (corresponds to NETIF_F_RXCSUM
for netdev) feature of the virtio-net driver conflicts with the loading
of XDP, which is caused by the problem described in [1][2], that is,
XDP may cause errors in partial csumed-related fields which can lead
to packet dropping.

In addition, when communicating between vm and vm on the same host, the
receiving side vm will receive packets marked as
VIRTIO_NET_HDR_F_NEEDS_CSUM, but after these packets are processed by
XDP, the VIRTIO_NET_HDR_F_NEEDS_CSUM and skb CHECKSUM_PARTIAL flags will
be cleared, causing the packet dropping.

This patch introduces a helper function, which will try to solve the
above problems in the subsequent patch.

[1] commit 18ba58e1c234 ("virtio-net: fail XDP set if guest csum is negotiated")
[2] commit e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 36cae78f6311..07b4801d689c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1663,6 +1663,44 @@ static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff
 	return 0;
 }
 
+static int virtnet_set_csum_after_xdp(struct virtnet_info *vi,
+				      struct sk_buff *skb,
+				      __u8 flags)
+{
+	int err;
+
+	/* When XDP program is loaded, for example, the vm-vm scenario
+	 * on the same host, packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM
+	 * will travel. Although these packets are safe from the point of
+	 * view of the vm, to avoid modification by XDP and successful
+	 * forwarding in the upper layer, we re-probe the necessary checksum
+	 * related information: skb->csum_{start, offset}, pseudo-header csum.
+	 *
+	 * This benefits us:
+	 * 1. XDP can be loaded when there's _F_GUEST_CSUM.
+	 * 2. The device verifies the checksum of packets , especially
+	 *    benefiting for large packets.
+	 * 3. In the same-host vm-vm scenario, packets marked as
+	 *    VIRTIO_NET_HDR_F_NEEDS_CSUM are no longer dropped after being
+	 *    processed by XDP.
+	 */
+	if (flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
+		err = virtnet_flow_dissect_udp_tcp(vi, skb);
+		if (err < 0)
+			return -EINVAL;
+
+		skb->ip_summed = CHECKSUM_PARTIAL;
+	} else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
+		/* We want to benefit from this: XDP guarantees that packets marked
+		 * as VIRTIO_NET_HDR_F_DATA_VALID still have correct csum after they
+		 * are processed.
+		 */
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+	}
+
+	return 0;
+}
+
 static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 			void *buf, unsigned int len, void **ctx,
 			unsigned int *xdp_xmit,
-- 
2.19.1.6.gb485710b

[PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Heng Qi]

We are now re-probing the csum related fields and  trying
to have XDP and RX hw checksum capabilities coexist on the
XDP path. For the benefit of:
1. RX hw checksum capability can be used if XDP is loaded.
2. Avoid packet loss when loading XDP in the vm-vm scenario.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 07b4801d689c..25b486ab74db 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 	struct net_device *dev = vi->dev;
 	struct sk_buff *skb;
 	struct virtio_net_hdr_mrg_rxbuf *hdr;
+	__u8 flags;
 
 	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
 		pr_debug("%s: short packet %i\n", dev->name, len);
@@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 		return;
 	}
 
+	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
+
 	if (vi->mergeable_rx_bufs)
 		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
 					stats);
@@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
 	if (unlikely(!skb))
 		return;
 
-	hdr = skb_vnet_hdr(skb);
-	if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
-		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
-
-	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
-		skb->ip_summed = CHECKSUM_UNNECESSARY;
+	if (unlikely(vi->xdp_enabled)) {
+		if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
+			pr_debug("%s: errors occurred in flow dissector setting csum",
+				 dev->name);
+			goto frame_err;
+		}
 
-	if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
-				  virtio_is_little_endian(vi->vdev))) {
-		net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
-				     dev->name, hdr->hdr.gso_type,
-				     hdr->hdr.gso_size);
-		goto frame_err;
+	} else {
+		hdr = skb_vnet_hdr(skb);
+		if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
+			virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
+
+		if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+		if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
+					  virtio_is_little_endian(vi->vdev))) {
+			net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
+					     dev->name, hdr->hdr.gso_type,
+					     hdr->hdr.gso_size);
+			goto frame_err;
+		}
 	}
 
 	skb_record_rx_queue(skb, vq2rxq(rq->vq));
-- 
2.19.1.6.gb485710b

[PATCH net-next 4/4] virtio-net: remove F_GUEST_CSUM check for XDP loading

[Heng Qi]

Lay the foundation for the subsequent patch to complete the coexistence
of XDP and virtio-net guest csum.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 25b486ab74db..79471de64b56 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -60,7 +60,6 @@ static const unsigned long guest_offloads[] = {
 	VIRTIO_NET_F_GUEST_TSO6,
 	VIRTIO_NET_F_GUEST_ECN,
 	VIRTIO_NET_F_GUEST_UFO,
-	VIRTIO_NET_F_GUEST_CSUM,
 	VIRTIO_NET_F_GUEST_USO4,
 	VIRTIO_NET_F_GUEST_USO6,
 	VIRTIO_NET_F_GUEST_HDRLEN
@@ -3522,10 +3521,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
 	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
 		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
-		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM) ||
 		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO4) ||
 		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO6))) {
-		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first");
+		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW, disable GRO_HW first");
 		return -EOPNOTSUPP;
 	}
 
-- 
2.19.1.6.gb485710b

Re: [PATCH net-next 4/4] virtio-net: remove F_GUEST_CSUM check for XDP loading

[Michael S. Tsirkin]

On Mon, Jun 19, 2023 at 06:57:38PM +0800, Heng Qi wrote:
> Lay the foundation for the subsequent patch

which subsequent patch? this is the last one in series.

> to complete the coexistence
> of XDP and virtio-net guest csum.
> 
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 25b486ab74db..79471de64b56 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -60,7 +60,6 @@ static const unsigned long guest_offloads[] = {
>  	VIRTIO_NET_F_GUEST_TSO6,
>  	VIRTIO_NET_F_GUEST_ECN,
>  	VIRTIO_NET_F_GUEST_UFO,
> -	VIRTIO_NET_F_GUEST_CSUM,
>  	VIRTIO_NET_F_GUEST_USO4,
>  	VIRTIO_NET_F_GUEST_USO6,
>  	VIRTIO_NET_F_GUEST_HDRLEN

What is this doing? Drop support for VIRTIO_NET_F_GUEST_CSUM? Why?
This will disable all of guest offloads I think ..


> @@ -3522,10 +3521,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
>  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
> -		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM) ||
>  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO4) ||
>  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO6))) {
> -		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first");
> +		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW, disable GRO_HW first");
>  		return -EOPNOTSUPP;
>  	}
>  
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Michael S. Tsirkin]

On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> We are now re-probing the csum related fields and  trying
> to have XDP and RX hw checksum capabilities coexist on the
> XDP path. For the benefit of:
> 1. RX hw checksum capability can be used if XDP is loaded.
> 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> 
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
>  1 file changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 07b4801d689c..25b486ab74db 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  	struct net_device *dev = vi->dev;
>  	struct sk_buff *skb;
>  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> +	__u8 flags;
>  
>  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
>  		pr_debug("%s: short packet %i\n", dev->name, len);
> @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  		return;
>  	}
>  
> +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> +
>  	if (vi->mergeable_rx_bufs)
>  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
>  					stats);

what's going on here?

> @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  	if (unlikely(!skb))
>  		return;
>  
> -	hdr = skb_vnet_hdr(skb);
> -	if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> -		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> -
> -	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> -		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	if (unlikely(vi->xdp_enabled)) {
> +		if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
> +			pr_debug("%s: errors occurred in flow dissector setting csum",
> +				 dev->name);
> +			goto frame_err;
> +		}
>  
> -	if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> -				  virtio_is_little_endian(vi->vdev))) {
> -		net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> -				     dev->name, hdr->hdr.gso_type,
> -				     hdr->hdr.gso_size);
> -		goto frame_err;
> +	} else {
> +		hdr = skb_vnet_hdr(skb);
> +		if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> +			virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> +
> +		if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> +
> +		if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> +					  virtio_is_little_endian(vi->vdev))) {
> +			net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> +					     dev->name, hdr->hdr.gso_type,
> +					     hdr->hdr.gso_size);
> +			goto frame_err;
> +		}
>  	}
>  
>  	skb_record_rx_queue(skb, vq2rxq(rq->vq));
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH net-next 2/4] virtio-net: reprobe csum related fields for skb passed by XDP

[Michael S. Tsirkin]

On Mon, Jun 19, 2023 at 06:57:36PM +0800, Heng Qi wrote:
> Currently, the VIRTIO_NET_F_GUEST_CSUM (corresponds to NETIF_F_RXCSUM
> for netdev) feature of the virtio-net driver conflicts with the loading
> of XDP, which is caused by the problem described in [1][2], that is,
> XDP may cause errors in partial csumed-related fields which can lead
> to packet dropping.
> 
> In addition, when communicating between vm and vm on the same host, the
> receiving side vm will receive packets marked as
> VIRTIO_NET_HDR_F_NEEDS_CSUM, but after these packets are processed by
> XDP, the VIRTIO_NET_HDR_F_NEEDS_CSUM and skb CHECKSUM_PARTIAL flags will
> be cleared, causing the packet dropping.
> 
> This patch introduces a helper function, which will try to solve the
> above problems in the subsequent patch.
> 
> [1] commit 18ba58e1c234 ("virtio-net: fail XDP set if guest csum is negotiated")
> [2] commit e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> 
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>


squash this and patch 1 into where the helpers are used.

in particular so we don't get warnings with bisect.

> ---
>  drivers/net/virtio_net.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 36cae78f6311..07b4801d689c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1663,6 +1663,44 @@ static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff
>  	return 0;
>  }
>  
> +static int virtnet_set_csum_after_xdp(struct virtnet_info *vi,
> +				      struct sk_buff *skb,
> +				      __u8 flags)
> +{
> +	int err;
> +
> +	/* When XDP program is loaded, for example, the vm-vm scenario
> +	 * on the same host, packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM
> +	 * will travel. Although these packets are safe from the point of
> +	 * view of the vm, to avoid modification by XDP and successful
> +	 * forwarding in the upper layer, we re-probe the necessary checksum
> +	 * related information: skb->csum_{start, offset}, pseudo-header csum.
> +	 *
> +	 * This benefits us:
> +	 * 1. XDP can be loaded when there's _F_GUEST_CSUM.
> +	 * 2. The device verifies the checksum of packets , especially
> +	 *    benefiting for large packets.
> +	 * 3. In the same-host vm-vm scenario, packets marked as
> +	 *    VIRTIO_NET_HDR_F_NEEDS_CSUM are no longer dropped after being
> +	 *    processed by XDP.
> +	 */
> +	if (flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> +		err = virtnet_flow_dissect_udp_tcp(vi, skb);
> +		if (err < 0)
> +			return -EINVAL;
> +
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +	} else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
> +		/* We want to benefit from this: XDP guarantees that packets marked
> +		 * as VIRTIO_NET_HDR_F_DATA_VALID still have correct csum after they
> +		 * are processed.
> +		 */
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	}
> +
> +	return 0;
> +}
> +
>  static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>  			void *buf, unsigned int len, void **ctx,
>  			unsigned int *xdp_xmit,
> -- 
> 2.19.1.6.gb485710b

Re: [PATCH net-next 2/4] virtio-net: reprobe csum related fields for skb passed by XDP

[Heng Qi]

On Mon, Jun 19, 2023 at 07:27:26AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 19, 2023 at 06:57:36PM +0800, Heng Qi wrote:
> > Currently, the VIRTIO_NET_F_GUEST_CSUM (corresponds to NETIF_F_RXCSUM
> > for netdev) feature of the virtio-net driver conflicts with the loading
> > of XDP, which is caused by the problem described in [1][2], that is,
> > XDP may cause errors in partial csumed-related fields which can lead
> > to packet dropping.
> > 
> > In addition, when communicating between vm and vm on the same host, the
> > receiving side vm will receive packets marked as
> > VIRTIO_NET_HDR_F_NEEDS_CSUM, but after these packets are processed by
> > XDP, the VIRTIO_NET_HDR_F_NEEDS_CSUM and skb CHECKSUM_PARTIAL flags will
> > be cleared, causing the packet dropping.
> > 
> > This patch introduces a helper function, which will try to solve the
> > above problems in the subsequent patch.
> > 
> > [1] commit 18ba58e1c234 ("virtio-net: fail XDP set if guest csum is negotiated")
> > [2] commit e59ff2c49ae1 ("virtio-net: disable guest csum during XDP set")
> > 
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> 
> 
> squash this and patch 1 into where the helpers are used.
> 
> in particular so we don't get warnings with bisect.

Ok. Will modify.

Thanks.

> 
> > ---
> >  drivers/net/virtio_net.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 36cae78f6311..07b4801d689c 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1663,6 +1663,44 @@ static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff
> >  	return 0;
> >  }
> >  
> > +static int virtnet_set_csum_after_xdp(struct virtnet_info *vi,
> > +				      struct sk_buff *skb,
> > +				      __u8 flags)
> > +{
> > +	int err;
> > +
> > +	/* When XDP program is loaded, for example, the vm-vm scenario
> > +	 * on the same host, packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM
> > +	 * will travel. Although these packets are safe from the point of
> > +	 * view of the vm, to avoid modification by XDP and successful
> > +	 * forwarding in the upper layer, we re-probe the necessary checksum
> > +	 * related information: skb->csum_{start, offset}, pseudo-header csum.
> > +	 *
> > +	 * This benefits us:
> > +	 * 1. XDP can be loaded when there's _F_GUEST_CSUM.
> > +	 * 2. The device verifies the checksum of packets , especially
> > +	 *    benefiting for large packets.
> > +	 * 3. In the same-host vm-vm scenario, packets marked as
> > +	 *    VIRTIO_NET_HDR_F_NEEDS_CSUM are no longer dropped after being
> > +	 *    processed by XDP.
> > +	 */
> > +	if (flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> > +		err = virtnet_flow_dissect_udp_tcp(vi, skb);
> > +		if (err < 0)
> > +			return -EINVAL;
> > +
> > +		skb->ip_summed = CHECKSUM_PARTIAL;
> > +	} else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
> > +		/* We want to benefit from this: XDP guarantees that packets marked
> > +		 * as VIRTIO_NET_HDR_F_DATA_VALID still have correct csum after they
> > +		 * are processed.
> > +		 */
> > +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  			void *buf, unsigned int len, void **ctx,
> >  			unsigned int *xdp_xmit,
> > -- 
> > 2.19.1.6.gb485710b

Re: [PATCH net-next 1/4] virtio-net: a helper for probing the pseudo-header checksum

[kernel test robot]

Hi Heng,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Qi/virtio-net-a-helper-for-probing-the-pseudo-header-checksum/20230619-190212
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230619105738.117733-2-hengqi%40linux.alibaba.com
patch subject: [PATCH net-next 1/4] virtio-net: a helper for probing the pseudo-header checksum
config: x86_64-randconfig-r014-20230619 (https://download.01.org/0day-ci/archive/20230619/202306192049.8y7DR5F1-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230619/202306192049.8y7DR5F1-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306192049.8y7DR5F1-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/net/virtio_net.c:1648:17: error: call to undeclared function 'csum_ipv6_magic'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
                           uh->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
                                        ^
   drivers/net/virtio_net.c:1648:17: note: did you mean 'csum_tcpudp_magic'?
   include/asm-generic/checksum.h:52:1: note: 'csum_tcpudp_magic' declared here
   csum_tcpudp_magic(__be32 saddr, __be32 daddr, __u32 len,
   ^
   drivers/net/virtio_net.c:1657:17: error: call to undeclared function 'csum_ipv6_magic'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
                           th->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
                                        ^
   2 errors generated.


vim +/csum_ipv6_magic +1648 drivers/net/virtio_net.c

  1572	
  1573	static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff *skb)
  1574	{
  1575		struct net_device *dev = vi->dev;
  1576		struct flow_keys_basic keys;
  1577		struct udphdr *uh;
  1578		struct tcphdr *th;
  1579		int len, offset;
  1580	
  1581		/* The flow dissector needs this information. */
  1582		skb->dev = dev;
  1583		skb_reset_mac_header(skb);
  1584		skb->protocol = dev_parse_header_protocol(skb);
  1585		/* virtio-net does not need to resolve VLAN. */
  1586		skb_set_network_header(skb, ETH_HLEN);
  1587		if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
  1588						      NULL, 0, 0, 0, 0))
  1589			return -EINVAL;
  1590	
  1591		/* 1. Pseudo-header checksum calculation requires:
  1592		 *    (1) saddr/daddr (2) IP_PROTO (3) length of transport payload
  1593		 * 2. We don't parse SCTP because virtio-net currently doesn't
  1594		 *    support CRC offloading for SCTP.
  1595		 */
  1596		if (keys.basic.n_proto == htons(ETH_P_IP)) {
  1597			struct iphdr *iph;
  1598	
  1599			/* Flow dissector has verified that there is an IP header. */
  1600			iph = ip_hdr(skb);
  1601			if (iph->version != 4 || !pskb_may_pull(skb, iph->ihl * 4))
  1602				return -EINVAL;
  1603	
  1604			skb->transport_header = skb->mac_header + keys.control.thoff;
  1605			offset = skb_transport_offset(skb);
  1606			len = skb->len - offset;
  1607			if (keys.basic.ip_proto == IPPROTO_UDP) {
  1608				if (!pskb_may_pull(skb, offset + sizeof(struct udphdr)))
  1609					return -EINVAL;
  1610	
  1611				uh = udp_hdr(skb);
  1612				skb->csum_offset = offsetof(struct udphdr, check);
  1613				/* Although uh->len is already the 3rd parameter for the calculation
  1614				 * of the pseudo-header checksum, we have already calculated the
  1615				 * length of the transport layer, so use 'len' here directly.
  1616				 */
  1617				uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, len,
  1618						IPPROTO_UDP, 0);
  1619			} else if (keys.basic.ip_proto == IPPROTO_TCP) {
  1620				if (!pskb_may_pull(skb, offset + sizeof(struct tcphdr)))
  1621					return -EINVAL;
  1622	
  1623				th = tcp_hdr(skb);
  1624				skb->csum_offset = offsetof(struct tcphdr, check);
  1625				th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, len,
  1626						IPPROTO_TCP, 0);
  1627			} /* virtio-net doesn't support checksums for SCTP hw offloading.*/
  1628		} else if (keys.basic.n_proto == htons(ETH_P_IPV6)) {
  1629			struct ipv6hdr *ip6h;
  1630	
  1631			ip6h = ipv6_hdr(skb);
  1632			if (ip6h->version != 6)
  1633				return -EINVAL;
  1634	
  1635			/* We have skipped the possible extension headers for IPv6.
  1636			 * If there is a Routing Header, the tx's check value is calculated by
  1637			 * final_dst, and that value is the rx's daddr.
  1638			 */
  1639			skb->transport_header = skb->mac_header + keys.control.thoff;
  1640			offset = skb_transport_offset(skb);
  1641			len = skb->len - offset;
  1642			if (keys.basic.ip_proto == IPPROTO_UDP) {
  1643				if (!pskb_may_pull(skb, offset + sizeof(struct udphdr)))
  1644					return -EINVAL;
  1645	
  1646				uh = udp_hdr(skb);
  1647				skb->csum_offset = offsetof(struct udphdr, check);
> 1648				uh->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
  1649						(const struct in6_addr *)&ip6h->daddr,
  1650						len, IPPROTO_UDP, 0);
  1651			} else if (keys.basic.ip_proto == IPPROTO_TCP) {
  1652				if (!pskb_may_pull(skb, offset + sizeof(struct tcphdr)))
  1653					return -EINVAL;
  1654	
  1655				th = tcp_hdr(skb);
  1656				skb->csum_offset = offsetof(struct tcphdr, check);
  1657				th->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
  1658						(const struct in6_addr *)&ip6h->daddr,
  1659						len, IPPROTO_TCP, 0);
  1660			}
  1661		}
  1662	
  1663		skb->csum_start = skb->transport_header;
  1664	
  1665		return 0;
  1666	}
  1667	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 1/4] virtio-net: a helper for probing the pseudo-header checksum

[kernel test robot]

Hi Heng,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Qi/virtio-net-a-helper-for-probing-the-pseudo-header-checksum/20230619-190212
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230619105738.117733-2-hengqi%40linux.alibaba.com
patch subject: [PATCH net-next 1/4] virtio-net: a helper for probing the pseudo-header checksum
config: m68k-randconfig-r021-20230619 (https://download.01.org/0day-ci/archive/20230619/202306192049.l2eaZ7od-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230619/202306192049.l2eaZ7od-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306192049.l2eaZ7od-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/net/virtio_net.c: In function 'virtnet_flow_dissect_udp_tcp':
>> drivers/net/virtio_net.c:1648:38: error: implicit declaration of function 'csum_ipv6_magic'; did you mean 'csum_tcpudp_magic'? [-Werror=implicit-function-declaration]
    1648 |                         uh->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
         |                                      ^~~~~~~~~~~~~~~
         |                                      csum_tcpudp_magic
   drivers/net/virtio_net.c: At top level:
   drivers/net/virtio_net.c:1573:12: warning: 'virtnet_flow_dissect_udp_tcp' defined but not used [-Wunused-function]
    1573 | static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff *skb)
         |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +1648 drivers/net/virtio_net.c

  1572	
  1573	static int virtnet_flow_dissect_udp_tcp(struct virtnet_info *vi, struct sk_buff *skb)
  1574	{
  1575		struct net_device *dev = vi->dev;
  1576		struct flow_keys_basic keys;
  1577		struct udphdr *uh;
  1578		struct tcphdr *th;
  1579		int len, offset;
  1580	
  1581		/* The flow dissector needs this information. */
  1582		skb->dev = dev;
  1583		skb_reset_mac_header(skb);
  1584		skb->protocol = dev_parse_header_protocol(skb);
  1585		/* virtio-net does not need to resolve VLAN. */
  1586		skb_set_network_header(skb, ETH_HLEN);
  1587		if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
  1588						      NULL, 0, 0, 0, 0))
  1589			return -EINVAL;
  1590	
  1591		/* 1. Pseudo-header checksum calculation requires:
  1592		 *    (1) saddr/daddr (2) IP_PROTO (3) length of transport payload
  1593		 * 2. We don't parse SCTP because virtio-net currently doesn't
  1594		 *    support CRC offloading for SCTP.
  1595		 */
  1596		if (keys.basic.n_proto == htons(ETH_P_IP)) {
  1597			struct iphdr *iph;
  1598	
  1599			/* Flow dissector has verified that there is an IP header. */
  1600			iph = ip_hdr(skb);
  1601			if (iph->version != 4 || !pskb_may_pull(skb, iph->ihl * 4))
  1602				return -EINVAL;
  1603	
  1604			skb->transport_header = skb->mac_header + keys.control.thoff;
  1605			offset = skb_transport_offset(skb);
  1606			len = skb->len - offset;
  1607			if (keys.basic.ip_proto == IPPROTO_UDP) {
  1608				if (!pskb_may_pull(skb, offset + sizeof(struct udphdr)))
  1609					return -EINVAL;
  1610	
  1611				uh = udp_hdr(skb);
  1612				skb->csum_offset = offsetof(struct udphdr, check);
  1613				/* Although uh->len is already the 3rd parameter for the calculation
  1614				 * of the pseudo-header checksum, we have already calculated the
  1615				 * length of the transport layer, so use 'len' here directly.
  1616				 */
  1617				uh->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, len,
  1618						IPPROTO_UDP, 0);
  1619			} else if (keys.basic.ip_proto == IPPROTO_TCP) {
  1620				if (!pskb_may_pull(skb, offset + sizeof(struct tcphdr)))
  1621					return -EINVAL;
  1622	
  1623				th = tcp_hdr(skb);
  1624				skb->csum_offset = offsetof(struct tcphdr, check);
  1625				th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr, len,
  1626						IPPROTO_TCP, 0);
  1627			} /* virtio-net doesn't support checksums for SCTP hw offloading.*/
  1628		} else if (keys.basic.n_proto == htons(ETH_P_IPV6)) {
  1629			struct ipv6hdr *ip6h;
  1630	
  1631			ip6h = ipv6_hdr(skb);
  1632			if (ip6h->version != 6)
  1633				return -EINVAL;
  1634	
  1635			/* We have skipped the possible extension headers for IPv6.
  1636			 * If there is a Routing Header, the tx's check value is calculated by
  1637			 * final_dst, and that value is the rx's daddr.
  1638			 */
  1639			skb->transport_header = skb->mac_header + keys.control.thoff;
  1640			offset = skb_transport_offset(skb);
  1641			len = skb->len - offset;
  1642			if (keys.basic.ip_proto == IPPROTO_UDP) {
  1643				if (!pskb_may_pull(skb, offset + sizeof(struct udphdr)))
  1644					return -EINVAL;
  1645	
  1646				uh = udp_hdr(skb);
  1647				skb->csum_offset = offsetof(struct udphdr, check);
> 1648				uh->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
  1649						(const struct in6_addr *)&ip6h->daddr,
  1650						len, IPPROTO_UDP, 0);
  1651			} else if (keys.basic.ip_proto == IPPROTO_TCP) {
  1652				if (!pskb_may_pull(skb, offset + sizeof(struct tcphdr)))
  1653					return -EINVAL;
  1654	
  1655				th = tcp_hdr(skb);
  1656				skb->csum_offset = offsetof(struct tcphdr, check);
  1657				th->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
  1658						(const struct in6_addr *)&ip6h->daddr,
  1659						len, IPPROTO_TCP, 0);
  1660			}
  1661		}
  1662	
  1663		skb->csum_start = skb->transport_header;
  1664	
  1665		return 0;
  1666	}
  1667	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Heng Qi]

On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > We are now re-probing the csum related fields and  trying
> > to have XDP and RX hw checksum capabilities coexist on the
> > XDP path. For the benefit of:
> > 1. RX hw checksum capability can be used if XDP is loaded.
> > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > 
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 07b4801d689c..25b486ab74db 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  	struct net_device *dev = vi->dev;
> >  	struct sk_buff *skb;
> >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > +	__u8 flags;
> >  
> >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  		return;
> >  	}
> >  
> > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > +
> >  	if (vi->mergeable_rx_bufs)
> >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> >  					stats);
> 
> what's going on here?

Thanks for pointing this out. Will insert into mergeable and small modes
respectively.

> 
> > @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  	if (unlikely(!skb))
> >  		return;
> >  
> > -	hdr = skb_vnet_hdr(skb);
> > -	if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > -		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> > -
> > -	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > -		skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	if (unlikely(vi->xdp_enabled)) {
> > +		if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
> > +			pr_debug("%s: errors occurred in flow dissector setting csum",
> > +				 dev->name);
> > +			goto frame_err;
> > +		}
> >  
> > -	if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > -				  virtio_is_little_endian(vi->vdev))) {
> > -		net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> > -				     dev->name, hdr->hdr.gso_type,
> > -				     hdr->hdr.gso_size);
> > -		goto frame_err;
> > +	} else {
> > +		hdr = skb_vnet_hdr(skb);
> > +		if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > +			virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> > +
> > +		if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +
> > +		if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > +					  virtio_is_little_endian(vi->vdev))) {
> > +			net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> > +					     dev->name, hdr->hdr.gso_type,
> > +					     hdr->hdr.gso_size);
> > +			goto frame_err;
> > +		}
> >  	}
> >  
> >  	skb_record_rx_queue(skb, vq2rxq(rq->vq));
> > -- 
> > 2.19.1.6.gb485710b

Re: [PATCH net-next 4/4] virtio-net: remove F_GUEST_CSUM check for XDP loading

[Heng Qi]

On Mon, Jun 19, 2023 at 07:16:20AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 19, 2023 at 06:57:38PM +0800, Heng Qi wrote:
> > Lay the foundation for the subsequent patch
> 
> which subsequent patch? this is the last one in series.
> 
> > to complete the coexistence
> > of XDP and virtio-net guest csum.
> > 
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 25b486ab74db..79471de64b56 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -60,7 +60,6 @@ static const unsigned long guest_offloads[] = {
> >  	VIRTIO_NET_F_GUEST_TSO6,
> >  	VIRTIO_NET_F_GUEST_ECN,
> >  	VIRTIO_NET_F_GUEST_UFO,
> > -	VIRTIO_NET_F_GUEST_CSUM,
> >  	VIRTIO_NET_F_GUEST_USO4,
> >  	VIRTIO_NET_F_GUEST_USO6,
> >  	VIRTIO_NET_F_GUEST_HDRLEN
> 
> What is this doing? Drop support for VIRTIO_NET_F_GUEST_CSUM? Why?

guest_offloads[] is used by the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
command to switch features when XDP is loaded/unloaded.

If the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated:
1. When XDP is loaded, virtnet_xdp_set() uses virtnet_clear_guest_offloads()
to automatically turn off the features in guest_offloads[].

2. when XDP is unloaded, virtnet_xdp_set() uses virtnet_restore_guest_offloads()
to automatically restore the features in guest_offloads[].

Now, this work no longer makes XDP and _F_GUEST_CSUM mutually
exclusive, so this patch removed the _F_GUEST_CSUM from guest_offloads[].

> This will disable all of guest offloads I think ..

No. This doesn't change the dependencies of other features on
_F_GUEST_CSUM. Removing _F_GUEST_CSUM here does not mean that other
features that depend on it will be turned off at the same time, such as
_F_GUEST_TSO{4,6}, F_GUEST_USO{4,6}, etc.

Thanks.

> 
> 
> > @@ -3522,10 +3521,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
> > -		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM) ||
> >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO4) ||
> >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO6))) {
> > -		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first");
> > +		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW, disable GRO_HW first");
> >  		return -EOPNOTSUPP;
> >  	}
> >  
> > -- 
> > 2.19.1.6.gb485710b

Re: [PATCH net-next 2/4] virtio-net: reprobe csum related fields for skb passed by XDP

[kernel test robot]

Hi Heng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Qi/virtio-net-a-helper-for-probing-the-pseudo-header-checksum/20230619-190212
base:   net-next/main
patch link:    https://lore.kernel.org/r/20230619105738.117733-3-hengqi%40linux.alibaba.com
patch subject: [PATCH net-next 2/4] virtio-net: reprobe csum related fields for skb passed by XDP
config: x86_64-randconfig-r014-20230619 (https://download.01.org/0day-ci/archive/20230619/202306192151.YMz3NiKw-lkp@intel.com/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230619/202306192151.YMz3NiKw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306192151.YMz3NiKw-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/virtio_net.c:1648:17: error: call to undeclared function 'csum_ipv6_magic'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
                           uh->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
                                        ^
   drivers/net/virtio_net.c:1648:17: note: did you mean 'csum_tcpudp_magic'?
   include/asm-generic/checksum.h:52:1: note: 'csum_tcpudp_magic' declared here
   csum_tcpudp_magic(__be32 saddr, __be32 daddr, __u32 len,
   ^
   drivers/net/virtio_net.c:1657:17: error: call to undeclared function 'csum_ipv6_magic'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
                           th->check = ~csum_ipv6_magic((const struct in6_addr *)&ip6h->saddr,
                                        ^
>> drivers/net/virtio_net.c:1695:19: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand]
           } else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
                            ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/virtio_net.c:1695:19: note: use '&' for a bitwise operation
           } else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
                            ^~
                            &
   drivers/net/virtio_net.c:1695:19: note: remove constant to silence this warning
           } else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
                           ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 2 errors generated.


vim +1695 drivers/net/virtio_net.c

  1667	
  1668	static int virtnet_set_csum_after_xdp(struct virtnet_info *vi,
  1669					      struct sk_buff *skb,
  1670					      __u8 flags)
  1671	{
  1672		int err;
  1673	
  1674		/* When XDP program is loaded, for example, the vm-vm scenario
  1675		 * on the same host, packets marked as VIRTIO_NET_HDR_F_NEEDS_CSUM
  1676		 * will travel. Although these packets are safe from the point of
  1677		 * view of the vm, to avoid modification by XDP and successful
  1678		 * forwarding in the upper layer, we re-probe the necessary checksum
  1679		 * related information: skb->csum_{start, offset}, pseudo-header csum.
  1680		 *
  1681		 * This benefits us:
  1682		 * 1. XDP can be loaded when there's _F_GUEST_CSUM.
  1683		 * 2. The device verifies the checksum of packets , especially
  1684		 *    benefiting for large packets.
  1685		 * 3. In the same-host vm-vm scenario, packets marked as
  1686		 *    VIRTIO_NET_HDR_F_NEEDS_CSUM are no longer dropped after being
  1687		 *    processed by XDP.
  1688		 */
  1689		if (flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
  1690			err = virtnet_flow_dissect_udp_tcp(vi, skb);
  1691			if (err < 0)
  1692				return -EINVAL;
  1693	
  1694			skb->ip_summed = CHECKSUM_PARTIAL;
> 1695		} else if (flags && VIRTIO_NET_HDR_F_DATA_VALID) {
  1696			/* We want to benefit from this: XDP guarantees that packets marked
  1697			 * as VIRTIO_NET_HDR_F_DATA_VALID still have correct csum after they
  1698			 * are processed.
  1699			 */
  1700			skb->ip_summed = CHECKSUM_UNNECESSARY;
  1701		}
  1702	
  1703		return 0;
  1704	}
  1705	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next 4/4] virtio-net: remove F_GUEST_CSUM check for XDP loading

[Michael S. Tsirkin]

On Mon, Jun 19, 2023 at 08:41:54PM +0800, Heng Qi wrote:
> On Mon, Jun 19, 2023 at 07:16:20AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 19, 2023 at 06:57:38PM +0800, Heng Qi wrote:
> > > Lay the foundation for the subsequent patch
> > 
> > which subsequent patch? this is the last one in series.
> > 
> > > to complete the coexistence
> > > of XDP and virtio-net guest csum.
> > > 
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  drivers/net/virtio_net.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 25b486ab74db..79471de64b56 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -60,7 +60,6 @@ static const unsigned long guest_offloads[] = {
> > >  	VIRTIO_NET_F_GUEST_TSO6,
> > >  	VIRTIO_NET_F_GUEST_ECN,
> > >  	VIRTIO_NET_F_GUEST_UFO,
> > > -	VIRTIO_NET_F_GUEST_CSUM,
> > >  	VIRTIO_NET_F_GUEST_USO4,
> > >  	VIRTIO_NET_F_GUEST_USO6,
> > >  	VIRTIO_NET_F_GUEST_HDRLEN
> > 
> > What is this doing? Drop support for VIRTIO_NET_F_GUEST_CSUM? Why?
> 
> guest_offloads[] is used by the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> command to switch features when XDP is loaded/unloaded.
> 
> If the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated:
> 1. When XDP is loaded, virtnet_xdp_set() uses virtnet_clear_guest_offloads()
> to automatically turn off the features in guest_offloads[].
> 
> 2. when XDP is unloaded, virtnet_xdp_set() uses virtnet_restore_guest_offloads()
> to automatically restore the features in guest_offloads[].
> 
> Now, this work no longer makes XDP and _F_GUEST_CSUM mutually
> exclusive, so this patch removed the _F_GUEST_CSUM from guest_offloads[].
> 
> > This will disable all of guest offloads I think ..
> 
> No. This doesn't change the dependencies of other features on
> _F_GUEST_CSUM. Removing _F_GUEST_CSUM here does not mean that other
> features that depend on it will be turned off at the same time, such as
> _F_GUEST_TSO{4,6}, F_GUEST_USO{4,6}, etc.
> 
> Thanks.

Hmm I don't get it.

static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
{               
        u64 offloads = vi->guest_offloads;
                        
        if (!vi->guest_offloads)
                return 0;
        
        return virtnet_set_guest_offloads(vi, offloads); 
}               
                        
is the bit _F_GUEST_CSUM set in vi->guest_offloads?

Because if it isn't then we'll try to set _F_GUEST_TSO
without setting _F_GUEST_CSUM and that's a spec
violation I think.


> > 
> > 
> > > @@ -3522,10 +3521,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> > >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
> > > -		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM) ||
> > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO4) ||
> > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO6))) {
> > > -		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first");
> > > +		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW, disable GRO_HW first");
> > >  		return -EOPNOTSUPP;
> > >  	}
> > >  
> > > -- 
> > > 2.19.1.6.gb485710b

Re: [PATCH net-next 4/4] virtio-net: remove F_GUEST_CSUM check for XDP loading

[Heng Qi]

On Mon, Jun 19, 2023 at 10:33:44AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 19, 2023 at 08:41:54PM +0800, Heng Qi wrote:
> > On Mon, Jun 19, 2023 at 07:16:20AM -0400, Michael S. Tsirkin wrote:
> > > On Mon, Jun 19, 2023 at 06:57:38PM +0800, Heng Qi wrote:
> > > > Lay the foundation for the subsequent patch
> > > 
> > > which subsequent patch? this is the last one in series.
> > > 
> > > > to complete the coexistence
> > > > of XDP and virtio-net guest csum.
> > > > 
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 4 +---
> > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 25b486ab74db..79471de64b56 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -60,7 +60,6 @@ static const unsigned long guest_offloads[] = {
> > > >  	VIRTIO_NET_F_GUEST_TSO6,
> > > >  	VIRTIO_NET_F_GUEST_ECN,
> > > >  	VIRTIO_NET_F_GUEST_UFO,
> > > > -	VIRTIO_NET_F_GUEST_CSUM,
> > > >  	VIRTIO_NET_F_GUEST_USO4,
> > > >  	VIRTIO_NET_F_GUEST_USO6,
> > > >  	VIRTIO_NET_F_GUEST_HDRLEN
> > > 
> > > What is this doing? Drop support for VIRTIO_NET_F_GUEST_CSUM? Why?
> > 
> > guest_offloads[] is used by the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > command to switch features when XDP is loaded/unloaded.
> > 
> > If the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated:
> > 1. When XDP is loaded, virtnet_xdp_set() uses virtnet_clear_guest_offloads()
> > to automatically turn off the features in guest_offloads[].
> > 
> > 2. when XDP is unloaded, virtnet_xdp_set() uses virtnet_restore_guest_offloads()
> > to automatically restore the features in guest_offloads[].
> > 
> > Now, this work no longer makes XDP and _F_GUEST_CSUM mutually
> > exclusive, so this patch removed the _F_GUEST_CSUM from guest_offloads[].
> > 
> > > This will disable all of guest offloads I think ..
> > 
> > No. This doesn't change the dependencies of other features on
> > _F_GUEST_CSUM. Removing _F_GUEST_CSUM here does not mean that other
> > features that depend on it will be turned off at the same time, such as
> > _F_GUEST_TSO{4,6}, F_GUEST_USO{4,6}, etc.
> > 
> > Thanks.
> 
> Hmm I don't get it.
> 
> static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
> {               
>         u64 offloads = vi->guest_offloads;
>                         
>         if (!vi->guest_offloads)
>                 return 0;
>         
>         return virtnet_set_guest_offloads(vi, offloads); 
> }               
>                         
> is the bit _F_GUEST_CSUM set in vi->guest_offloads?

No, but first we doesn't clear _F_GUEST_CSUM in virtnet_clear_guest_offloads().

If VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is negotiated, features that can
be dynamically controlled by the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
command must also be negotiated. Therefore, if GRO_HW_MASK features such
as _F_GUEST_TSO exist, then _F_GUEST_CSUM must exist (according to the
dependencies defined by the spec).

Now, we only dynamically turn off/on the features contained in
guest_offloads[] through XDP loading/unloading (with this patch,
_F_GUEST_CSUM will not be controlled), and _F_GUEST_CSUM is always on.

Another point is that the virtio-net NETIF_F_RXCSUM corresponding to
_F_GUEST_CSUM is only included in dev->features, not in dev->hw_features,
which means that users cannot manually control the switch of
_F_GUEST_CSUM.

> 
> Because if it isn't then we'll try to set _F_GUEST_TSO
> without setting _F_GUEST_CSUM and that's a spec
> violation I think.

As explained above, we did not cause a specification violation.

Thanks.

> 
> 
> > > 
> > > 
> > > > @@ -3522,10 +3521,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> > > >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> > > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
> > > > -		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM) ||
> > > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO4) ||
> > > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO6))) {
> > > > -		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first");
> > > > +		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW, disable GRO_HW first");
> > > >  		return -EOPNOTSUPP;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.19.1.6.gb485710b

Re: [PATCH net-next 4/4] virtio-net: remove F_GUEST_CSUM check for XDP loading

[Michael S. Tsirkin]

On Mon, Jun 19, 2023 at 11:43:29PM +0800, Heng Qi wrote:
> On Mon, Jun 19, 2023 at 10:33:44AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 19, 2023 at 08:41:54PM +0800, Heng Qi wrote:
> > > On Mon, Jun 19, 2023 at 07:16:20AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Jun 19, 2023 at 06:57:38PM +0800, Heng Qi wrote:
> > > > > Lay the foundation for the subsequent patch
> > > > 
> > > > which subsequent patch? this is the last one in series.
> > > > 
> > > > > to complete the coexistence
> > > > > of XDP and virtio-net guest csum.
> > > > > 
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > >  drivers/net/virtio_net.c | 4 +---
> > > > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 25b486ab74db..79471de64b56 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -60,7 +60,6 @@ static const unsigned long guest_offloads[] = {
> > > > >  	VIRTIO_NET_F_GUEST_TSO6,
> > > > >  	VIRTIO_NET_F_GUEST_ECN,
> > > > >  	VIRTIO_NET_F_GUEST_UFO,
> > > > > -	VIRTIO_NET_F_GUEST_CSUM,
> > > > >  	VIRTIO_NET_F_GUEST_USO4,
> > > > >  	VIRTIO_NET_F_GUEST_USO6,
> > > > >  	VIRTIO_NET_F_GUEST_HDRLEN
> > > > 
> > > > What is this doing? Drop support for VIRTIO_NET_F_GUEST_CSUM? Why?
> > > 
> > > guest_offloads[] is used by the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> > > command to switch features when XDP is loaded/unloaded.
> > > 
> > > If the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated:
> > > 1. When XDP is loaded, virtnet_xdp_set() uses virtnet_clear_guest_offloads()
> > > to automatically turn off the features in guest_offloads[].
> > > 
> > > 2. when XDP is unloaded, virtnet_xdp_set() uses virtnet_restore_guest_offloads()
> > > to automatically restore the features in guest_offloads[].
> > > 
> > > Now, this work no longer makes XDP and _F_GUEST_CSUM mutually
> > > exclusive, so this patch removed the _F_GUEST_CSUM from guest_offloads[].
> > > 
> > > > This will disable all of guest offloads I think ..
> > > 
> > > No. This doesn't change the dependencies of other features on
> > > _F_GUEST_CSUM. Removing _F_GUEST_CSUM here does not mean that other
> > > features that depend on it will be turned off at the same time, such as
> > > _F_GUEST_TSO{4,6}, F_GUEST_USO{4,6}, etc.
> > > 
> > > Thanks.
> > 
> > Hmm I don't get it.
> > 
> > static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
> > {               
> >         u64 offloads = vi->guest_offloads;
> >                         
> >         if (!vi->guest_offloads)
> >                 return 0;
> >         
> >         return virtnet_set_guest_offloads(vi, offloads); 
> > }               
> >                         
> > is the bit _F_GUEST_CSUM set in vi->guest_offloads?
> 
> No, but first we doesn't clear _F_GUEST_CSUM in virtnet_clear_guest_offloads().
> 
> If VIRTIO_NET_F_CTRL_GUEST_OFFLOADS is negotiated, features that can
> be dynamically controlled by the VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> command must also be negotiated. Therefore, if GRO_HW_MASK features such
> as _F_GUEST_TSO exist, then _F_GUEST_CSUM must exist (according to the
> dependencies defined by the spec).
> 
> Now, we only dynamically turn off/on the features contained in
> guest_offloads[] through XDP loading/unloading (with this patch,
> _F_GUEST_CSUM will not be controlled), and _F_GUEST_CSUM is always on.
> 
> Another point is that the virtio-net NETIF_F_RXCSUM corresponding to
> _F_GUEST_CSUM is only included in dev->features, not in dev->hw_features,
> which means that users cannot manually control the switch of
> _F_GUEST_CSUM.

I think I get it now.

> > 
> > Because if it isn't then we'll try to set _F_GUEST_TSO
> > without setting _F_GUEST_CSUM and that's a spec
> > violation I think.
> 
> As explained above, we did not cause a specification violation.
> 
> Thanks.

Right.

> > 
> > 
> > > > 
> > > > 
> > > > > @@ -3522,10 +3521,9 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > > >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> > > > >  	        virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> > > > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO) ||
> > > > > -		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM) ||
> > > > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO4) ||
> > > > >  		virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_USO6))) {
> > > > > -		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW/CSUM, disable GRO_HW/CSUM first");
> > > > > +		NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing GRO_HW, disable GRO_HW first");
> > > > >  		return -EOPNOTSUPP;
> > > > >  	}
> > > > >  
> > > > > -- 
> > > > > 2.19.1.6.gb485710b

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Heng Qi]

On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > We are now re-probing the csum related fields and  trying
> > to have XDP and RX hw checksum capabilities coexist on the
> > XDP path. For the benefit of:
> > 1. RX hw checksum capability can be used if XDP is loaded.
> > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > 
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> >  1 file changed, 24 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 07b4801d689c..25b486ab74db 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  	struct net_device *dev = vi->dev;
> >  	struct sk_buff *skb;
> >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > +	__u8 flags;
> >  
> >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  		return;
> >  	}
> >  
> > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > +
> >  	if (vi->mergeable_rx_bufs)
> >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> >  					stats);
> 
> what's going on here?

Hi, Michael.

Is your question about the function of this code?
1. If yes,
this sentence saves the flags value in virtio-net-hdr in advance
before entering the XDP processing logic, so that it can be used to
judge further logic after XDP processing.

If _NEEDS_CSUM is included in flags before XDP processing, then after
XDP processing we need to re-probe the csum fields and calculate the
pseudo-header checksum.

2. If not,
do you mean that mergeable and small modes should be distinguished for
save actions?
The answer is that we don't need it. The information in virtio-net-hdr in
the current small mode is also forcibly converted into the
virtio_net_hdr_mrg_rxbuf structure, which is consistent with the
virtio_net_hdr structure in terms of code layout, and the results are
consistent.

If you think this is semantically wrong, then we need a bugfix patch.
The simplest example is in receive_small_xdp():
"
unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
...
struct virtio_net_hdr_mrg_rxbuf *hdr = buf + header_offset; "


Thanks.

> 
> > @@ -1728,19 +1731,28 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> >  	if (unlikely(!skb))
> >  		return;
> >  
> > -	hdr = skb_vnet_hdr(skb);
> > -	if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > -		virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> > -
> > -	if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > -		skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	if (unlikely(vi->xdp_enabled)) {
> > +		if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
> > +			pr_debug("%s: errors occurred in flow dissector setting csum",
> > +				 dev->name);
> > +			goto frame_err;
> > +		}
> >  
> > -	if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > -				  virtio_is_little_endian(vi->vdev))) {
> > -		net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> > -				     dev->name, hdr->hdr.gso_type,
> > -				     hdr->hdr.gso_size);
> > -		goto frame_err;
> > +	} else {
> > +		hdr = skb_vnet_hdr(skb);
> > +		if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > +			virtio_skb_set_hash((const struct virtio_net_hdr_v1_hash *)hdr, skb);
> > +
> > +		if (hdr->hdr.flags & VIRTIO_NET_HDR_F_DATA_VALID)
> > +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +
> > +		if (virtio_net_hdr_to_skb(skb, &hdr->hdr,
> > +					  virtio_is_little_endian(vi->vdev))) {
> > +			net_warn_ratelimited("%s: bad gso: type: %u, size: %u\n",
> > +					     dev->name, hdr->hdr.gso_type,
> > +					     hdr->hdr.gso_size);
> > +			goto frame_err;
> > +		}
> >  	}
> >  
> >  	skb_record_rx_queue(skb, vq2rxq(rq->vq));
> > -- 
> > 2.19.1.6.gb485710b

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Michael S. Tsirkin]

On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote:
> On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > > We are now re-probing the csum related fields and  trying
> > > to have XDP and RX hw checksum capabilities coexist on the
> > > XDP path. For the benefit of:
> > > 1. RX hw checksum capability can be used if XDP is loaded.
> > > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > > 
> > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 07b4801d689c..25b486ab74db 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > >  	struct net_device *dev = vi->dev;
> > >  	struct sk_buff *skb;
> > >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > > +	__u8 flags;
> > >  
> > >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > >  		return;
> > >  	}
> > >  
> > > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > > +
> > >  	if (vi->mergeable_rx_bufs)
> > >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> > >  					stats);
> > 
> > what's going on here?
> 
> Hi, Michael.
> 
> Is your question about the function of this code?
> 1. If yes,
> this sentence saves the flags value in virtio-net-hdr in advance
> before entering the XDP processing logic, so that it can be used to
> judge further logic after XDP processing.
> 
> If _NEEDS_CSUM is included in flags before XDP processing, then after
> XDP processing we need to re-probe the csum fields and calculate the
> pseudo-header checksum.

Yes but we previously used this:
-       hdr = skb_vnet_hdr(skb);
which pokes at the copy in skb cb.

Is anything wrong with this?

It seems preferable not to poke at the header an extra time.

-- 
MST

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Heng Qi]

On Tue, Jun 20, 2023 at 06:50:34AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote:
> > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > > > We are now re-probing the csum related fields and  trying
> > > > to have XDP and RX hw checksum capabilities coexist on the
> > > > XDP path. For the benefit of:
> > > > 1. RX hw checksum capability can be used if XDP is loaded.
> > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > > > 
> > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > index 07b4801d689c..25b486ab74db 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > >  	struct net_device *dev = vi->dev;
> > > >  	struct sk_buff *skb;
> > > >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > > > +	__u8 flags;
> > > >  
> > > >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > > >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > >  		return;
> > > >  	}
> > > >  
> > > > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > > > +
> > > >  	if (vi->mergeable_rx_bufs)
> > > >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> > > >  					stats);
> > > 
> > > what's going on here?
> > 
> > Hi, Michael.
> > 
> > Is your question about the function of this code?
> > 1. If yes,
> > this sentence saves the flags value in virtio-net-hdr in advance
> > before entering the XDP processing logic, so that it can be used to
> > judge further logic after XDP processing.
> > 
> > If _NEEDS_CSUM is included in flags before XDP processing, then after
> > XDP processing we need to re-probe the csum fields and calculate the
> > pseudo-header checksum.
> 
> Yes but we previously used this:
> -       hdr = skb_vnet_hdr(skb);
> which pokes at the copy in skb cb.
> 
> Is anything wrong with this?
> 

This is where we save the hdr when there is no XDP loaded (note that
this is the complete hdr, including flags, and also including GSO and
other information). When XDP is loaded, because hdr is invalid, we will
not save it into skb->cb.

But the above situation is not what we want. Now our purpose is to save
the hdr information before XDP processing, that is, when the driver has
just received the packet and has not built the skb (in fact, we only
need flags). Therefore, only flags are saved here.

Thanks.

> It seems preferable not to poke at the header an extra time.
> 
> -- 
> MST

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Michael S. Tsirkin]

On Tue, Jun 20, 2023 at 07:01:48PM +0800, Heng Qi wrote:
> On Tue, Jun 20, 2023 at 06:50:34AM -0400, Michael S. Tsirkin wrote:
> > On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote:
> > > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > > > > We are now re-probing the csum related fields and  trying
> > > > > to have XDP and RX hw checksum capabilities coexist on the
> > > > > XDP path. For the benefit of:
> > > > > 1. RX hw checksum capability can be used if XDP is loaded.
> > > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > > > > 
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> > > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > index 07b4801d689c..25b486ab74db 100644
> > > > > --- a/drivers/net/virtio_net.c
> > > > > +++ b/drivers/net/virtio_net.c
> > > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > > >  	struct net_device *dev = vi->dev;
> > > > >  	struct sk_buff *skb;
> > > > >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > > > > +	__u8 flags;
> > > > >  
> > > > >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > > > >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > > >  		return;
> > > > >  	}
> > > > >  
> > > > > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > > > > +
> > > > >  	if (vi->mergeable_rx_bufs)
> > > > >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> > > > >  					stats);
> > > > 
> > > > what's going on here?
> > > 
> > > Hi, Michael.
> > > 
> > > Is your question about the function of this code?
> > > 1. If yes,
> > > this sentence saves the flags value in virtio-net-hdr in advance
> > > before entering the XDP processing logic, so that it can be used to
> > > judge further logic after XDP processing.
> > > 
> > > If _NEEDS_CSUM is included in flags before XDP processing, then after
> > > XDP processing we need to re-probe the csum fields and calculate the
> > > pseudo-header checksum.
> > 
> > Yes but we previously used this:
> > -       hdr = skb_vnet_hdr(skb);
> > which pokes at the copy in skb cb.
> > 
> > Is anything wrong with this?
> > 
> 
> This is where we save the hdr when there is no XDP loaded (note that
> this is the complete hdr, including flags, and also including GSO and
> other information). When XDP is loaded, because hdr is invalid, we will
> not save it into skb->cb.
> 
> But the above situation is not what we want. Now our purpose is to save
> the hdr information before XDP processing, that is, when the driver has
> just received the packet and has not built the skb (in fact, we only
> need flags). Therefore, only flags are saved here.
> 
> Thanks.

I don't get it. this seems to be the only use of flags:


+       if (unlikely(vi->xdp_enabled)) {
+               if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
+                       pr_debug("%s: errors occurred in flow dissector setting csum",
+                                dev->name);
+                       goto frame_err;
+               }

looks like skb has already been created here.

is there another use of flags that I missed?



> > It seems preferable not to poke at the header an extra time.
> > 
> > -- 
> > MST

Re: [PATCH net-next 3/4] virtio-net: support coexistence of XDP and _F_GUEST_CSUM

[Heng Qi]

On Tue, Jun 20, 2023 at 08:10:38AM -0400, Michael S. Tsirkin wrote:
> On Tue, Jun 20, 2023 at 07:01:48PM +0800, Heng Qi wrote:
> > On Tue, Jun 20, 2023 at 06:50:34AM -0400, Michael S. Tsirkin wrote:
> > > On Tue, Jun 20, 2023 at 11:24:30AM +0800, Heng Qi wrote:
> > > > On Mon, Jun 19, 2023 at 07:26:44AM -0400, Michael S. Tsirkin wrote:
> > > > > On Mon, Jun 19, 2023 at 06:57:37PM +0800, Heng Qi wrote:
> > > > > > We are now re-probing the csum related fields and  trying
> > > > > > to have XDP and RX hw checksum capabilities coexist on the
> > > > > > XDP path. For the benefit of:
> > > > > > 1. RX hw checksum capability can be used if XDP is loaded.
> > > > > > 2. Avoid packet loss when loading XDP in the vm-vm scenario.
> > > > > > 
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > ---
> > > > > >  drivers/net/virtio_net.c | 36 ++++++++++++++++++++++++------------
> > > > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > > > > index 07b4801d689c..25b486ab74db 100644
> > > > > > --- a/drivers/net/virtio_net.c
> > > > > > +++ b/drivers/net/virtio_net.c
> > > > > > @@ -1709,6 +1709,7 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > >  	struct net_device *dev = vi->dev;
> > > > > >  	struct sk_buff *skb;
> > > > > >  	struct virtio_net_hdr_mrg_rxbuf *hdr;
> > > > > > +	__u8 flags;
> > > > > >  
> > > > > >  	if (unlikely(len < vi->hdr_len + ETH_HLEN)) {
> > > > > >  		pr_debug("%s: short packet %i\n", dev->name, len);
> > > > > > @@ -1717,6 +1718,8 @@ static void receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> > > > > >  		return;
> > > > > >  	}
> > > > > >  
> > > > > > +	flags = ((struct virtio_net_hdr_mrg_rxbuf *)buf)->hdr.flags;
> > > > > > +
> > > > > >  	if (vi->mergeable_rx_bufs)
> > > > > >  		skb = receive_mergeable(dev, vi, rq, buf, ctx, len, xdp_xmit,
> > > > > >  					stats);
> > > > > 
> > > > > what's going on here?
> > > > 
> > > > Hi, Michael.
> > > > 
> > > > Is your question about the function of this code?
> > > > 1. If yes,
> > > > this sentence saves the flags value in virtio-net-hdr in advance
> > > > before entering the XDP processing logic, so that it can be used to
> > > > judge further logic after XDP processing.
> > > > 
> > > > If _NEEDS_CSUM is included in flags before XDP processing, then after
> > > > XDP processing we need to re-probe the csum fields and calculate the
> > > > pseudo-header checksum.
> > > 
> > > Yes but we previously used this:
> > > -       hdr = skb_vnet_hdr(skb);
> > > which pokes at the copy in skb cb.
> > > 
> > > Is anything wrong with this?
> > > 
> > 
> > This is where we save the hdr when there is no XDP loaded (note that
> > this is the complete hdr, including flags, and also including GSO and
> > other information). When XDP is loaded, because hdr is invalid, we will
> > not save it into skb->cb.
> > 
> > But the above situation is not what we want. Now our purpose is to save
> > the hdr information before XDP processing, that is, when the driver has
> > just received the packet and has not built the skb (in fact, we only
> > need flags). Therefore, only flags are saved here.
> > 
> > Thanks.
> 
> I don't get it. this seems to be the only use of flags:
> 
> 
> +       if (unlikely(vi->xdp_enabled)) {
> +               if (virtnet_set_csum_after_xdp(vi, skb, flags) < 0) {
> +                       pr_debug("%s: errors occurred in flow dissector setting csum",
> +                                dev->name);
> +                       goto frame_err;
> +               }
> 
> looks like skb has already been created here.

I explain more:

First, this patchset only focuses on XDP loaded scenes.

Then in the same-host vm-vm scenario, when the receiver loads XDP, this
is the packet receiving process:
1. The driver receives a packet, represented by *buf.

2. The XDP program builds xdp_buff based on *buf.
   virtio_net_hdr_mrg_rxbuf is located in the headroom of xdp_buff.

3. The XDP program returns XDP_PASS and modifies the headroom, that is,
the information in virtio_net_hdr_mrg_rxbuf becomes invalid (including
flags). So information like csum_{start, offset} is no longer correct.
Therefore, the skb converted from xdp_buff does not save information in
skb cb. And, skb->ip_summed = CHECKSUM_NONE, there is a high probability of
packet loss at this time because the incorrect check value.

So, in order to solve this problem (not only), we save the flags in
virtio_net_hdr_mrg_rxbuf before the #2 step above, which is the
original information, just to know whether the packet is _NEEDS_CSUM or
_DATA_VALID.

If the saved flags contains _NEEDS_CSUM, then we use
virtnet_set_csum_after_xdp() to recalculate csum_{start, offset},
pseudo-header checksum for the skb converted from xdp_buff. That is, the
saved flags are only used to identify whether we want to re-probe for
the packet to avoiding the packet dropping.

Thanks.

> 
> is there another use of flags that I missed?
> 
> 
> 
> > > It seems preferable not to poke at the header an extra time.
> > > 
> > > -- 
> > > MST