[PATCH net-next 0/6] eth: mpnic: checksum and segmentation offloads

[PATCH net-next 0/6] eth: mpnic: checksum and segmentation offloads

[Daniel Zahka]

Add checksum and segmentation offload support. Tx checksum offload
follows the generic csum_start/csum_offset model. Rx checksum also
follows the normal unnecessary/complete model. The device can validate l4
checksums for tcp or udp, when the l3 type is one of: ipv4, ipv6,
ipv4-in-ipv6, or ipv6-in-ipv6. The full l2 checksum of a frame is always
passed to driver via a Rx completion descriptor field, which can be used
for checksum complete on other packet types.

Tx segmentation supports tcp or udp in ipv4, ipv6, 4-in-6, or 6-in-6
natively, and can support most other tunnels with GSO_PARTIAL.

Counters for CSO/LSO are exported via the netdev qstats API.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
Daniel Zahka (6):
      eth: mpnic: Tx checksum offload
      eth: mpnic: Rx checksum offload
      eth: mpnic: TCP segmentation offload
      eth: mpnic: UDP segmentation offload
      eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation
      eth: mpnic: add GSO_PARTIAL for other tunnels

 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    |  41 +++++
 drivers/net/ethernet/meta/mpnic/mpnic_init.c   |  14 ++
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c |  43 ++++-
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.h |   6 +
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 235 ++++++++++++++++++++++++-
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.h   |   9 +
 6 files changed, 346 insertions(+), 2 deletions(-)
---
base-commit: 6584acd42820478910a771efb28a355a707e9cb0
change-id: 20261102-mpnic-lso-19aeb38e5648

Best regards,
-- 
Daniel Zahka <daniel.zahka@gmail.com>

[PATCH net-next 1/6] eth: mpnic: Tx checksum offload

[Daniel Zahka]

Support generic Tx checksum offload per NETIF_F_HW_CSUM.

Add mpnic_features_check() to ensure that skb metadata can be mapped
into Tx descriptor fields. The headers and offsets used have units of
2B. The device also requires any headers modified to be in the first
256B of the packet.

Zero checksums are inverted by HW, matching the behavior of
skb_checksum_help().

Count CSO frames and report them via netdev qstats API.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    |  6 +++
 drivers/net/ethernet/meta/mpnic/mpnic_init.c   |  2 +
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c | 10 ++++-
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.h |  1 +
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 60 ++++++++++++++++++++++++++
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.h   |  4 ++
 6 files changed, 82 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index d6ab9df58168..62304be203e6 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -13,6 +13,10 @@
 #define DESC_GENMASK(h, l)	GENMASK_ULL(h, l)
 
 /* Transmit Work Descriptor Format */
+#define MPNIC_TWD_L2_HLEN		DESC_GENMASK(5, 0)
+#define MPNIC_TWD_L3_IHLEN		DESC_GENMASK(23, 16)
+#define MPNIC_TWD_CSUM_OFFSET		DESC_GENMASK(27, 24)
+#define MPNIC_TWD_FLAG_REQ_CSO		DESC_BIT(36)
 #define MPNIC_TWD_FLAG_REQ_COMPLETION	DESC_BIT(37)
 #define MPNIC_TWD_FLAG_DEST_MAC		DESC_BIT(43)
 #define MPNIC_TWD_TYPE			DESC_GENMASK(47, 46)
@@ -305,6 +309,8 @@ enum {
 
 /* NIC_CORE_TOF */
 #define MPNIC_TOF_TCAM_DEST_REMAP	0x7e3022		/* 0x1f8c088 */
+#define MPNIC_TOF_MODE			0x7e302e		/* 0x1f8c0b8 */
+#define MPNIC_TOF_MODE_L4_ZERO_CSUM		CSR_BIT(4)
 
 /* PEMO_WRAPPER */
 #define MPNIC_OB_ATTR_RO			CSR_BIT(1)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
index ace4124158fb..71d13bec7ec4 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_init.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
@@ -298,6 +298,8 @@ static void mpnic_txglb_init(struct mpnic_dev *mpd)
 	 * programmed with its addresses.
 	 */
 	mpnic_wr64(mpd, MPNIC_TOF_TCAM_DEST_REMAP, 0);
+	mpnic_wr64(mpd, MPNIC_TOF_MODE,
+		   FIELD_PREP(MPNIC_TOF_MODE_L4_ZERO_CSUM, 0));
 
 	mpnic_wr64(mpd, MPNIC_TCM_MEM_INIT_REQ, MPNIC_MEM_INIT_TCM_VAL);
 	mpnic_wr64(mpd, MPNIC_TDE_MEM_INIT_REQ, MPNIC_MEM_INIT_TDE_VAL);
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index 7fdf57f70151..3de5abc20f9a 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -144,6 +144,7 @@ static const struct net_device_ops mpnic_netdev_ops = {
 	.ndo_stop		= mpnic_stop,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_start_xmit		= mpnic_xmit_frame,
+	.ndo_features_check	= mpnic_features_check,
 	.ndo_get_stats64	= mpnic_get_stats64,
 };
 
@@ -200,6 +201,7 @@ static void mpnic_get_queue_stats_tx(struct net_device *dev, int idx,
 	u64 bytes, packets, stop, wake;
 	struct mpnic_q_triad *qt;
 	unsigned int start;
+	u64 csum_partial;
 
 	if (!txr)
 		return;
@@ -217,11 +219,13 @@ static void mpnic_get_queue_stats_tx(struct net_device *dev, int idx,
 	stats = &txr->stats;
 	do {
 		start = u64_stats_fetch_begin(&stats->syncp);
+		csum_partial = u64_stats_read(&stats->twq.csum_partial);
 		stop = u64_stats_read(&stats->twq.stop);
 	} while (u64_stats_fetch_retry(&stats->syncp, start));
 
 	tx->bytes = bytes;
 	tx->packets = packets;
+	tx->needs_csum = csum_partial;
 	tx->stop = stop;
 	tx->wake = wake;
 }
@@ -237,6 +241,7 @@ static void mpnic_get_base_stats(struct net_device *dev,
 
 	tx->bytes = base->tx.bytes;
 	tx->packets = base->tx.packets;
+	tx->needs_csum = base->tx.csum_partial;
 	tx->stop = base->tx.stop;
 	tx->wake = base->tx.wake;
 
@@ -303,7 +308,10 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 	mpn->num_rx_queues = queues;
 	mpn->num_napi = queues;
 
-	netdev->features |= NETIF_F_SG;
+	netdev->features |=
+		NETIF_F_SG |
+		NETIF_F_HW_CSUM;
+
 	netdev->hw_features |= netdev->features;
 	netdev->vlan_features |= netdev->features;
 
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
index 6711766ebc3c..94f7a3355c5c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
@@ -16,6 +16,7 @@ struct mpnic_base_stats {
 		u64 packets;
 		u64 bytes;
 		u64 dropped;
+		u64 csum_partial;
 		u64 stop;
 		u64 wake;
 	} tx;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index 93abdec44642..edb087b12a1e 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -8,6 +8,7 @@
 #include <linux/slab.h>
 #include <net/netdev_lock.h>
 #include <net/page_pool/helpers.h>
+#include <net/tcp.h>
 
 #include "mpnic.h"
 #include "mpnic_netdev.h"
@@ -18,6 +19,10 @@ struct mpnic_xmit_cb {
 	u8 desc_count;
 };
 
+struct mpnic_tx_hdrs {
+	unsigned int i3len;
+};
+
 #define MPNIC_XMIT_CB(__skb) ((struct mpnic_xmit_cb *)((__skb)->cb))
 #define MPNIC_TWD_TYPE_MASK(_type) \
 	cpu_to_le64(FIELD_PREP(MPNIC_TWD_TYPE, MPNIC_TWD_TYPE_##_type))
@@ -208,6 +213,34 @@ mpnic_tx_map(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
 	return true;
 }
 
+static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
+{
+	hdrs->i3len = skb_checksum_start(skb) - skb_network_header(skb);
+}
+
+static void
+mpnic_tx_offloads(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
+{
+	struct mpnic_tx_hdrs hdrs = {};
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return;
+
+	mpnic_tx_fill_hdrs(skb, &hdrs);
+
+	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_CSUM_OFFSET,
+					skb->csum_offset / 2) |
+			     MPNIC_TWD_FLAG_REQ_CSO);
+
+	u64_stats_update_begin(&ring->stats.syncp);
+	u64_stats_inc(&ring->stats.twq.csum_partial);
+	u64_stats_update_end(&ring->stats.syncp);
+
+	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L2_HLEN,
+					skb_mac_header_len(skb) / 2) |
+			     FIELD_PREP(MPNIC_TWD_L3_IHLEN, hdrs.i3len / 2));
+}
+
 #define MPNIC_MIN_FRAME_LEN	60
 
 static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
@@ -230,6 +263,8 @@ static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
 	MPNIC_XMIT_CB(skb)->bytecount = skb->len;
 	MPNIC_XMIT_CB(skb)->desc_count = 0;
 
+	mpnic_tx_offloads(ring, skb, meta);
+
 	if (mpnic_tx_map(ring, skb, meta))
 		goto err_free;
 
@@ -256,6 +291,30 @@ netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
 	return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
 }
 
+netdev_features_t mpnic_features_check(struct sk_buff *skb,
+				       struct net_device *dev,
+				       netdev_features_t features)
+{
+	unsigned int l2len, i3len;
+
+	if (skb->ip_summed != CHECKSUM_PARTIAL)
+		return features;
+
+	l2len = skb_mac_header_len(skb);
+	if (skb_vlan_tag_present(skb))
+		l2len += VLAN_HLEN;
+	i3len = skb_checksum_start(skb) - skb_network_header(skb);
+
+	if ((l2len | i3len | skb->csum_offset) % 2 ||
+	    !FIELD_FIT(MPNIC_TWD_L2_HLEN, l2len / 2) ||
+	    !FIELD_FIT(MPNIC_TWD_L3_IHLEN, i3len / 2) ||
+	    !FIELD_FIT(MPNIC_TWD_CSUM_OFFSET, skb->csum_offset / 2) ||
+	    l2len + i3len + sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE > 256)
+		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
+
+	return features;
+}
+
 static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
 			     struct mpnic_q_triad *qt, bool discard,
 			     unsigned int hw_head)
@@ -721,6 +780,7 @@ static void mpnic_aggregate_ring_twq_counters(struct mpnic_base_stats *base,
 					      struct mpnic_ring *twq)
 {
 	base->tx.dropped += u64_stats_read(&twq->stats.twq.dropped);
+	base->tx.csum_partial += u64_stats_read(&twq->stats.twq.csum_partial);
 	base->tx.stop += u64_stats_read(&twq->stats.twq.stop);
 }
 
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index 52492e3f484f..a6dff9dc32d6 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -89,6 +89,7 @@ struct mpnic_queue_stats {
 	union {
 		struct {
 			u64_stats_t dropped;
+			u64_stats_t csum_partial;
 			u64_stats_t stop;
 		} twq;
 		struct {
@@ -168,6 +169,9 @@ struct mpnic_napi_vector {
 };
 
 netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev);
+netdev_features_t mpnic_features_check(struct sk_buff *skb,
+				       struct net_device *dev,
+				       netdev_features_t features);
 int mpnic_alloc_napi_vectors(struct mpnic_net *mpn);
 void mpnic_free_napi_vectors(struct mpnic_net *mpn);
 void mpnic_stats_attach_rings(struct mpnic_net *mpn);

-- 
2.52.0

Re: [PATCH net-next 1/6] eth: mpnic: Tx checksum offload

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] mpnic_tx_offloads()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c) increments…
- [Low] The new mpnic_features_check() calls skb_mac_header_len(skb) for
  every CHECKSUM_PARTIAL skb without checking that a MAC header has been…

--

eth: mpnic: Tx checksum offload

This commit adds generic Tx checksum offload (NETIF_F_HW_CSUM) to the
mpnic driver. It also adds an ndo_features_check callback that checks
the skb header lengths and checksum offset fit into the Tx descriptor
fields and the 256 byte header limit. CSO frames are counted and
reported through the netdev qstats API.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 93abdec44642f..edb087b12a1e2 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> +static void
> +mpnic_tx_offloads(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
> +{
> +	struct mpnic_tx_hdrs hdrs = {};
> +
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return;
> +
> +	mpnic_tx_fill_hdrs(skb, &hdrs);
> +
> +	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_CSUM_OFFSET,
> +					skb->csum_offset / 2) |
> +			     MPNIC_TWD_FLAG_REQ_CSO);
> +
> +	u64_stats_update_begin(&ring->stats.syncp);
> +	u64_stats_inc(&ring->stats.twq.csum_partial);
> +	u64_stats_update_end(&ring->stats.syncp);

[ ... ]

> @@ -230,6 +263,8 @@ static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
>  	MPNIC_XMIT_CB(skb)->bytecount = skb->len;
>  	MPNIC_XMIT_CB(skb)->desc_count = 0;
>  
> +	mpnic_tx_offloads(ring, skb, meta);
> +
>  	if (mpnic_tx_map(ring, skb, meta))
>  		goto err_free;
>  

[Severity: Low]
By this point mpnic_tx_offloads() has already incremented
twq.csum_partial. mpnic_tx_map() can then fail if skb_headlen() or a
frag size is above FIELD_MAX(MPNIC_TWD_LEN), or on dma_mapping_error().
In that case the err_free path frees the skb and counts it as dropped:

err_free:
	dev_kfree_skb_any(skb);
	ring->tx_buf[tail] = NULL;
	ring->tail = tail;
err_drop:
	...
	u64_stats_inc(&ring->stats.twq.dropped);

Does the csum_partial increment stay in place for a frame that was
never sent?

mpnic_get_queue_stats_tx() reports this counter as needs_csum, and
mpnic_aggregate_ring_twq_counters() adds it into base->tx.csum_partial.
So dropped frames would also be counted as checksum offloaded frames.

Could the counter be bumped only after mpnic_tx_map() succeeds?

The same ordering is still there at the end of the series (in "eth:
mpnic: add GSO_PARTIAL for other tunnels"). There, mpnic_tx_lso() also
increments hw_gso and hw_gso_wire before mpnic_tx_map() runs.

> @@ -256,6 +291,30 @@ netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
>  	return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
>  }
>  
> +netdev_features_t mpnic_features_check(struct sk_buff *skb,
> +				       struct net_device *dev,
> +				       netdev_features_t features)
> +{
> +	unsigned int l2len, i3len;
> +
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return features;
> +
> +	l2len = skb_mac_header_len(skb);

[Severity: Low]
Can this run before the MAC header has been set? skb_mac_header_len()
starts with:

	DEBUG_NET_WARN_ON_ONCE(!skb_mac_header_was_set(skb));

ndo_features_check is called via netif_skb_features(). The IP GSO slow
paths call that before the L2 header exists, and before
__dev_queue_xmit() calls skb_reset_mac_header():

ip_finish_output_gso()
  netif_skb_features()
    mpnic_features_check()
      skb_mac_header_len()

ip6_finish_output_gso_slowpath_drop() does the same on the IPv6 side.

At that point a locally generated TCP or UDP GSO skb is
CHECKSUM_PARTIAL, and its mac_header is still ~0 from allocation. TCP
builds software GSO skbs even without NETIF_F_TSO, because
sk_setup_caps() adds NETIF_F_GSO_SOFTWARE. So this path can be hit,
for example, when the PMTU drops after the MSS was chosen.

With CONFIG_DEBUG_NET the warning fires. Without it, l2len becomes
network_header - 0xffff as an unsigned int, which fails FIELD_FIT().
The CSUM and GSO features are then masked and segmentation falls back
to software checksums, so the traffic itself stays correct.

Would skb_network_offset() avoid depending on mac_header here? At
ndo_start_xmit time skb->data points at the L2 header, and the slow
path only needs a conservative answer.

fbnic_features_check() in drivers/net/ethernet/meta/fbnic has the same
pattern. This code is also unchanged at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com

[PATCH net-next 2/6] eth: mpnic: Rx checksum offload

[Daniel Zahka]

The device reports two Rx completion fields to the driver related to
checksums: l2_csum and l4_csum_unnecessary. l2_csum is the ones'
complement sum of the frame starting from the end of the ethernet
header. l4_csum_unnecessary means the device validated the l4 checksum of
the packet. This is only supported for tcp or udp in
ipv4/ipv6/4-in-6/6-in-6.

Report checksum none/complete/unnecessary counts via netdev qstats API.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    |  2 ++
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c | 14 ++++++++++-
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.h |  3 +++
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 34 ++++++++++++++++++++++++++
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.h   |  3 +++
 5 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 62304be203e6..1c86f114ab0c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -60,6 +60,8 @@ enum {
 #define MPNIC_RCD_AL_PAGE_FIN		DESC_BIT(53)
 
 /* Metadata Completion Descriptors */
+#define MPNIC_RCD_META_L2_CSUM			DESC_GENMASK(47, 32)
+#define MPNIC_RCD_META_L4_CSUM_UNNECESSARY	DESC_BIT(52)
 #define MPNIC_RCD_META_ERR_MAC_EOP		DESC_BIT(53)
 #define MPNIC_RCD_META_ERR_TRUNCATED_FRAME	DESC_BIT(54)
 #define MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK	\
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index 3de5abc20f9a..91b70fe8483c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -167,10 +167,12 @@ static void mpnic_get_queue_stats_rx(struct net_device *dev, int idx,
 {
 	struct mpnic_net *mpn = netdev_priv(dev);
 	struct mpnic_ring *rxr = mpn->rx[idx];
+	u64 csum_unnecessary, csum_complete;
 	u64 bytes, packets, alloc_failed;
 	struct mpnic_queue_stats *stats;
 	struct mpnic_q_triad *qt;
 	unsigned int start;
+	u64 csum_none;
 
 	if (!rxr)
 		return;
@@ -181,6 +183,9 @@ static void mpnic_get_queue_stats_rx(struct net_device *dev, int idx,
 		bytes = u64_stats_read(&stats->rcq.bytes);
 		packets = u64_stats_read(&stats->rcq.packets);
 		alloc_failed = u64_stats_read(&stats->rcq.alloc_failed);
+		csum_complete = u64_stats_read(&stats->rcq.csum_complete);
+		csum_unnecessary = u64_stats_read(&stats->rcq.csum_unnecessary);
+		csum_none = u64_stats_read(&stats->rcq.csum_none);
 	} while (u64_stats_fetch_retry(&stats->syncp, start));
 
 	qt = container_of(rxr, struct mpnic_q_triad, cmpl);
@@ -190,6 +195,9 @@ static void mpnic_get_queue_stats_rx(struct net_device *dev, int idx,
 	rx->bytes = bytes;
 	rx->packets = packets;
 	rx->alloc_fail = alloc_failed;
+	rx->csum_complete = csum_complete;
+	rx->csum_unnecessary = csum_unnecessary;
+	rx->csum_none = csum_none;
 }
 
 static void mpnic_get_queue_stats_tx(struct net_device *dev, int idx,
@@ -248,6 +256,9 @@ static void mpnic_get_base_stats(struct net_device *dev,
 	rx->bytes = base->rx.bytes;
 	rx->packets = base->rx.packets;
 	rx->alloc_fail = base->rx.alloc_failed;
+	rx->csum_complete = base->rx.csum_complete;
+	rx->csum_unnecessary = base->rx.csum_unnecessary;
+	rx->csum_none = base->rx.csum_none;
 }
 
 static const struct netdev_stat_ops mpnic_stat_ops = {
@@ -310,7 +321,8 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 
 	netdev->features |=
 		NETIF_F_SG |
-		NETIF_F_HW_CSUM;
+		NETIF_F_HW_CSUM |
+		NETIF_F_RXCSUM;
 
 	netdev->hw_features |= netdev->features;
 	netdev->vlan_features |= netdev->features;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
index 94f7a3355c5c..76ec9811e79e 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
@@ -26,6 +26,9 @@ struct mpnic_base_stats {
 		u64 dropped;
 		u64 alloc_failed;
 		u64 errors;
+		u64 csum_complete;
+		u64 csum_unnecessary;
+		u64 csum_none;
 	} rx;
 
 	unsigned int num_tx_queues;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index edb087b12a1e..680625810ec0 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -655,9 +655,34 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
 	page_pool_put_full_page(page->pp, page, napi);
 }
 
+static void mpnic_rx_csum(u64 rcd, struct sk_buff *skb,
+			  unsigned int *csum_complete,
+			  unsigned int *csum_unnecessary,
+			  unsigned int *csum_none)
+{
+	skb_checksum_none_assert(skb);
+
+	if (!(skb->dev->features & NETIF_F_RXCSUM)) {
+		(*csum_none)++;
+		return;
+	}
+
+	if (FIELD_GET(MPNIC_RCD_META_L4_CSUM_UNNECESSARY, rcd)) {
+		skb->ip_summed = CHECKSUM_UNNECESSARY;
+		(*csum_unnecessary)++;
+	} else {
+		u16 csum = FIELD_GET(MPNIC_RCD_META_L2_CSUM, rcd);
+
+		skb->ip_summed = CHECKSUM_COMPLETE;
+		skb->csum = (__force __wsum)csum;
+		(*csum_complete)++;
+	}
+}
+
 static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
 			   struct mpnic_q_triad *qt, int budget)
 {
+	unsigned int csum_complete = 0, csum_unnecessary = 0, csum_none = 0;
 	unsigned int packets = 0, bytes = 0, dropped = 0, errors = 0;
 	struct mpnic_ring *rcq = &qt->cmpl;
 	struct mpnic_rcq_state *state;
@@ -700,6 +725,8 @@ static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
 			}
 
 			if (likely(skb)) {
+				mpnic_rx_csum(rcd, skb, &csum_complete,
+					      &csum_unnecessary, &csum_none);
 				napi_gro_receive(&nv->napi, skb);
 			} else {
 				mpnic_put_pkt_buff(&state->pkt, true);
@@ -732,6 +759,9 @@ static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
 	u64_stats_add(&rcq->stats.rcq.dropped, dropped);
 	u64_stats_add(&rcq->stats.rcq.alloc_failed, alloc_failed);
 	u64_stats_add(&rcq->stats.rcq.errors, errors);
+	u64_stats_add(&rcq->stats.rcq.csum_complete, csum_complete);
+	u64_stats_add(&rcq->stats.rcq.csum_unnecessary, csum_unnecessary);
+	u64_stats_add(&rcq->stats.rcq.csum_none, csum_none);
 	u64_stats_update_end(&rcq->stats.syncp);
 
 	/* Allocate buffers, force dma_wmb(), and then start writing tails */
@@ -806,6 +836,10 @@ static void mpnic_aggregate_ring_rcq_counters(struct mpnic_base_stats *base,
 	base->rx.dropped += u64_stats_read(&rcq->stats.rcq.dropped);
 	base->rx.alloc_failed += u64_stats_read(&rcq->stats.rcq.alloc_failed);
 	base->rx.errors += u64_stats_read(&rcq->stats.rcq.errors);
+	base->rx.csum_complete += u64_stats_read(&rcq->stats.rcq.csum_complete);
+	base->rx.csum_unnecessary +=
+		u64_stats_read(&rcq->stats.rcq.csum_unnecessary);
+	base->rx.csum_none += u64_stats_read(&rcq->stats.rcq.csum_none);
 }
 
 static struct mpnic_base_stats *mpnic_base_stats_prepare(struct mpnic_net *mpn)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index a6dff9dc32d6..714f09f4160e 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -103,6 +103,9 @@ struct mpnic_queue_stats {
 			u64_stats_t dropped;
 			u64_stats_t alloc_failed;
 			u64_stats_t errors;
+			u64_stats_t csum_complete;
+			u64_stats_t csum_unnecessary;
+			u64_stats_t csum_none;
 		} rcq;
 		struct {
 			u64_stats_t alloc_failed;

-- 
2.52.0

Re: [PATCH net-next 2/6] eth: mpnic: Rx checksum offload

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The CHECKSUM_COMPLETE value in mpnic_rx_csum() is only right
  on hosts of one byte order.
- [Low] mpnic_rx_csum() (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c)
  sets skb->ip_summed = CHECKSUM_UNNECESSARY and ignores the full-frame…

--

eth: mpnic: Rx checksum offload

This commit adds Rx checksum offload to the mpnic driver. When the
completion descriptor's l4_csum_unnecessary bit is set, the driver reports
CHECKSUM_UNNECESSARY; otherwise it reports the l2_csum field as
CHECKSUM_COMPLETE. It also exports csum none/complete/unnecessary counters
through the netdev qstats API.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index edb087b12a1e2..680625810ec03 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> @@ -655,9 +655,34 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
>  	page_pool_put_full_page(page->pp, page, napi);
>  }
>  
> +static void mpnic_rx_csum(u64 rcd, struct sk_buff *skb,
> +			  unsigned int *csum_complete,
> +			  unsigned int *csum_unnecessary,
> +			  unsigned int *csum_none)
> +{
> +	skb_checksum_none_assert(skb);
> +
> +	if (!(skb->dev->features & NETIF_F_RXCSUM)) {
> +		(*csum_none)++;
> +		return;
> +	}
> +
> +	if (FIELD_GET(MPNIC_RCD_META_L4_CSUM_UNNECESSARY, rcd)) {
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +		(*csum_unnecessary)++;

[Severity: Low]
This isn't a bug, but should mpnic_rx_csum() prefer CHECKSUM_COMPLETE here,
since the same descriptor also carries MPNIC_RCD_META_L2_CSUM?

The commit message says l2_csum is reported for every frame, and
mpnic_csr.h doesn't define a validity bit for it. The comment in
include/linux/skbuff.h says:

 *   - Even if device supports only some protocols, but is able to produce
 *     skb->csum, it MUST use CHECKSUM_COMPLETE, not CHECKSUM_UNNECESSARY.

With the current ordering, every TCP/UDP packet that the device validates
goes up the stack without skb->csum. Anything the hardware L4 engine did
not check then needs a software checksum. One example is the inner
payload of a UDP tunnel when the checksum conversion path doesn't apply.

The in-tree fbnic driver uses the same pattern in fbnic_rx_csum(), so
this may be intentional. It still doesn't match the documented contract.

> +	} else {
> +		u16 csum = FIELD_GET(MPNIC_RCD_META_L2_CSUM, rcd);
> +
> +		skb->ip_summed = CHECKSUM_COMPLETE;
> +		skb->csum = (__force __wsum)csum;

[Severity: Medium]
Is this skb->csum value correct on big-endian hosts?

mpnic_clean_rcq() converts the descriptor to host order first:

	rcd = le64_to_cpu(*raw_rcd);

So the u16 that FIELD_GET() extracts from bits 47:32 is the same number on
every host. A __wsum, however, is computed by csum_partial() using
host-order 16-bit loads. The correct value therefore differs by a byte
swap between little-endian and big-endian hosts, and storing the raw
field can only be right on one of them.

The MPNIC Kconfig entry only has:

	depends on 64BIT || COMPILE_TEST
	depends on !S390

That still allows ppc64 BE, sparc64 and mips64 BE builds with PCI_MSI.

On such a host, would every good packet reported as CHECKSUM_COMPLETE fail
validation in __skb_checksum_complete()? That would end in
netdev_rx_csum_fault() and "hw csum failure" messages. Decap paths such as
skb_postpull_rcsum() and pskb_trim_rcsum() would also adjust from a wrong
base.

Would a conversion that keeps the device's byte layout help here, for
example (__force __wsum)(__force u16)cpu_to_le16(csum)? Alternatively, the
driver could be restricted to little-endian hosts in Kconfig.

> +		(*csum_complete)++;
> +	}
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com

[PATCH net-next 3/6] eth: mpnic: TCP segmentation offload

[Daniel Zahka]

Support segmentation of non-encapsulated tcp4/tcp6 packets. When doing
segmentation the device expects the l4 checksum to be seeded with 0
length.

IPv4 header and TCP checksums that compute to zero are sent as 0 rather
than 0xffff when the HW segments packets. IPv4 IDs are incrementing
starting from the value that is in the skb's IPv4 header.

Count gso super and wire packets and report via the netdev qstats API.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    | 27 ++++++++
 drivers/net/ethernet/meta/mpnic/mpnic_init.c   | 11 ++++
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c | 17 ++++--
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.h |  2 +
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 85 ++++++++++++++++++++++----
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.h   |  2 +
 6 files changed, 129 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 1c86f114ab0c..672ce0a98c22 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -16,8 +16,21 @@
 #define MPNIC_TWD_L2_HLEN		DESC_GENMASK(5, 0)
 #define MPNIC_TWD_L3_IHLEN		DESC_GENMASK(23, 16)
 #define MPNIC_TWD_CSUM_OFFSET		DESC_GENMASK(27, 24)
+#define MPNIC_TWD_L4_HLEN		DESC_GENMASK(31, 28)
+#define MPNIC_TWD_L4_TYPE		DESC_GENMASK(33, 32)
+enum {
+	MPNIC_TWD_L4_TYPE_TCP	= 1,
+};
+
+#define MPNIC_TWD_FLAG_REQ_LSO		DESC_BIT(35)
 #define MPNIC_TWD_FLAG_REQ_CSO		DESC_BIT(36)
 #define MPNIC_TWD_FLAG_REQ_COMPLETION	DESC_BIT(37)
+#define MPNIC_TWD_L3_TYPE		DESC_GENMASK(42, 40)
+enum {
+	MPNIC_TWD_L3_TYPE_IPV4	= 1,
+	MPNIC_TWD_L3_TYPE_IPV6	= 2,
+};
+
 #define MPNIC_TWD_FLAG_DEST_MAC		DESC_BIT(43)
 #define MPNIC_TWD_TYPE			DESC_GENMASK(47, 46)
 enum {
@@ -26,6 +39,8 @@ enum {
 	MPNIC_TWD_TYPE_LAST_AL	= 3,
 };
 
+#define MPNIC_TWD_MSS			DESC_GENMASK(61, 48)
+
 #define MPNIC_TWD_ADDR			DESC_GENMASK(45, 0)
 #define MPNIC_TWD_LEN			DESC_GENMASK(63, 48)
 
@@ -312,7 +327,19 @@ enum {
 /* NIC_CORE_TOF */
 #define MPNIC_TOF_TCAM_DEST_REMAP	0x7e3022		/* 0x1f8c088 */
 #define MPNIC_TOF_MODE			0x7e302e		/* 0x1f8c0b8 */
+#define MPNIC_TOF_MODE_IPV4_ID_MODE		CSR_GENMASK(0, 0)
+enum {
+	MPNIC_IPV4_ID_MODE_FIXED	= 0,
+	MPNIC_IPV4_ID_MODE_INCREMENT	= 1,
+};
+
+#define MPNIC_TOF_MODE_IPV4_ZERO_CSUM		CSR_BIT(1)
+#define MPNIC_TOF_MODE_TCP_ZERO_CSUM		CSR_BIT(2)
 #define MPNIC_TOF_MODE_L4_ZERO_CSUM		CSR_BIT(4)
+#define MPNIC_TOF_TCP_FLAG_CLR		0x7e3030		/* 0x1f8c0c0 */
+#define MPNIC_TOF_TCP_FLAG_CLR_FIRST_SEGMENT	CSR_GENMASK(8, 0)
+#define MPNIC_TOF_TCP_FLAG_CLR_MIDDLE_SEGMENT	CSR_GENMASK(24, 16)
+#define MPNIC_TOF_TCP_FLAG_CLR_LAST_SEGMENT	CSR_GENMASK(40, 32)
 
 /* PEMO_WRAPPER */
 #define MPNIC_OB_ATTR_RO			CSR_BIT(1)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
index 71d13bec7ec4..b9a41656f8f7 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_init.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
@@ -8,6 +8,7 @@
 #include <linux/iopoll.h>
 #include <linux/log2.h>
 #include <linux/sizes.h>
+#include <net/tcp.h>
 
 #include "mpnic.h"
 #include "mpnic_txrx.h"
@@ -299,7 +300,17 @@ static void mpnic_txglb_init(struct mpnic_dev *mpd)
 	 */
 	mpnic_wr64(mpd, MPNIC_TOF_TCAM_DEST_REMAP, 0);
 	mpnic_wr64(mpd, MPNIC_TOF_MODE,
+		   FIELD_PREP(MPNIC_TOF_MODE_IPV4_ID_MODE,
+			      MPNIC_IPV4_ID_MODE_INCREMENT) |
+		   FIELD_PREP(MPNIC_TOF_MODE_IPV4_ZERO_CSUM, 1) |
+		   FIELD_PREP(MPNIC_TOF_MODE_TCP_ZERO_CSUM, 1) |
 		   FIELD_PREP(MPNIC_TOF_MODE_L4_ZERO_CSUM, 0));
+	mpnic_wr64(mpd, MPNIC_TOF_TCP_FLAG_CLR,
+		   FIELD_PREP(MPNIC_TOF_TCP_FLAG_CLR_FIRST_SEGMENT,
+			      TCPHDR_PSH | TCPHDR_FIN) |
+		   FIELD_PREP(MPNIC_TOF_TCP_FLAG_CLR_MIDDLE_SEGMENT,
+			      TCPHDR_PSH | TCPHDR_CWR | TCPHDR_FIN) |
+		   FIELD_PREP(MPNIC_TOF_TCP_FLAG_CLR_LAST_SEGMENT, TCPHDR_CWR));
 
 	mpnic_wr64(mpd, MPNIC_TCM_MEM_INIT_REQ, MPNIC_MEM_INIT_TCM_VAL);
 	mpnic_wr64(mpd, MPNIC_TDE_MEM_INIT_REQ, MPNIC_MEM_INIT_TDE_VAL);
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index 91b70fe8483c..3a4aa5be57a1 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -205,11 +205,11 @@ static void mpnic_get_queue_stats_tx(struct net_device *dev, int idx,
 {
 	struct mpnic_net *mpn = netdev_priv(dev);
 	struct mpnic_ring *txr = mpn->tx[idx];
+	u64 csum_partial, hw_gso, hw_gso_wire;
 	struct mpnic_queue_stats *stats;
 	u64 bytes, packets, stop, wake;
 	struct mpnic_q_triad *qt;
 	unsigned int start;
-	u64 csum_partial;
 
 	if (!txr)
 		return;
@@ -228,12 +228,16 @@ static void mpnic_get_queue_stats_tx(struct net_device *dev, int idx,
 	do {
 		start = u64_stats_fetch_begin(&stats->syncp);
 		csum_partial = u64_stats_read(&stats->twq.csum_partial);
+		hw_gso = u64_stats_read(&stats->twq.hw_gso);
+		hw_gso_wire = u64_stats_read(&stats->twq.hw_gso_wire);
 		stop = u64_stats_read(&stats->twq.stop);
 	} while (u64_stats_fetch_retry(&stats->syncp, start));
 
 	tx->bytes = bytes;
 	tx->packets = packets;
-	tx->needs_csum = csum_partial;
+	tx->needs_csum = csum_partial + hw_gso_wire;
+	tx->hw_gso_packets = hw_gso;
+	tx->hw_gso_wire_packets = hw_gso_wire;
 	tx->stop = stop;
 	tx->wake = wake;
 }
@@ -249,7 +253,9 @@ static void mpnic_get_base_stats(struct net_device *dev,
 
 	tx->bytes = base->tx.bytes;
 	tx->packets = base->tx.packets;
-	tx->needs_csum = base->tx.csum_partial;
+	tx->needs_csum = base->tx.csum_partial + base->tx.hw_gso_wire;
+	tx->hw_gso_packets = base->tx.hw_gso;
+	tx->hw_gso_wire_packets = base->tx.hw_gso_wire;
 	tx->stop = base->tx.stop;
 	tx->wake = base->tx.wake;
 
@@ -322,7 +328,10 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 	netdev->features |=
 		NETIF_F_SG |
 		NETIF_F_HW_CSUM |
-		NETIF_F_RXCSUM;
+		NETIF_F_RXCSUM |
+		NETIF_F_TSO |
+		NETIF_F_TSO_ECN |
+		NETIF_F_TSO6;
 
 	netdev->hw_features |= netdev->features;
 	netdev->vlan_features |= netdev->features;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
index 76ec9811e79e..567a52d0b33d 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.h
@@ -17,6 +17,8 @@ struct mpnic_base_stats {
 		u64 bytes;
 		u64 dropped;
 		u64 csum_partial;
+		u64 hw_gso;
+		u64 hw_gso_wire;
 		u64 stop;
 		u64 wake;
 	} tx;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index 680625810ec0..e2e6ab6d3c59 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -16,11 +16,14 @@
 
 struct mpnic_xmit_cb {
 	u32 bytecount;
+	u16 gso_segs;
 	u8 desc_count;
 };
 
 struct mpnic_tx_hdrs {
 	unsigned int i3len;
+	unsigned int l3_type;
+	unsigned int l4_type;
 };
 
 #define MPNIC_XMIT_CB(__skb) ((struct mpnic_xmit_cb *)((__skb)->cb))
@@ -215,30 +218,82 @@ mpnic_tx_map(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
 
 static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
 {
-	hdrs->i3len = skb_checksum_start(skb) - skb_network_header(skb);
+	if (!skb_is_gso(skb)) {
+		hdrs->i3len = skb_checksum_start(skb) - skb_network_header(skb);
+		return;
+	}
+
+	hdrs->l4_type = MPNIC_TWD_L4_TYPE_TCP;
+	hdrs->i3len = skb_transport_offset(skb) - skb_network_offset(skb);
+	hdrs->l3_type = ip_hdr(skb)->version == 4 ?
+			MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
 }
 
-static void
+static bool
+mpnic_tx_lso(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
+{
+	struct skb_shared_info *shinfo;
+	unsigned int l4len, hdrlen;
+	struct tcphdr *tcph;
+	__be16 payload_len;
+
+	if (skb_cow_head(skb, 0))
+		return true;
+
+	shinfo = skb_shinfo(skb);
+
+	tcph = (struct tcphdr *)skb_checksum_start(skb);
+	l4len = __tcp_hdrlen(tcph);
+	payload_len = cpu_to_be16(skb->len - skb_checksum_start_offset(skb));
+	csum_replace_by_diff(&tcph->check, (__force __wsum)payload_len);
+
+	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L4_HLEN, l4len / 4) |
+			     FIELD_PREP(MPNIC_TWD_MSS, shinfo->gso_size) |
+			     MPNIC_TWD_FLAG_REQ_LSO);
+
+	hdrlen = skb_checksum_start_offset(skb) + l4len;
+	MPNIC_XMIT_CB(skb)->bytecount += (shinfo->gso_segs - 1) * hdrlen;
+	MPNIC_XMIT_CB(skb)->gso_segs = shinfo->gso_segs;
+
+	u64_stats_update_begin(&ring->stats.syncp);
+	u64_stats_inc(&ring->stats.twq.hw_gso);
+	u64_stats_add(&ring->stats.twq.hw_gso_wire, shinfo->gso_segs);
+	u64_stats_update_end(&ring->stats.syncp);
+
+	return false;
+}
+
+static bool
 mpnic_tx_offloads(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
 {
 	struct mpnic_tx_hdrs hdrs = {};
 
 	if (skb->ip_summed != CHECKSUM_PARTIAL)
-		return;
+		return false;
 
 	mpnic_tx_fill_hdrs(skb, &hdrs);
 
 	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_CSUM_OFFSET,
-					skb->csum_offset / 2) |
-			     MPNIC_TWD_FLAG_REQ_CSO);
+					skb->csum_offset / 2));
 
-	u64_stats_update_begin(&ring->stats.syncp);
-	u64_stats_inc(&ring->stats.twq.csum_partial);
-	u64_stats_update_end(&ring->stats.syncp);
+	if (skb_is_gso(skb)) {
+		if (mpnic_tx_lso(ring, skb, meta))
+			return true;
+	} else {
+		*meta |= cpu_to_le64(MPNIC_TWD_FLAG_REQ_CSO);
+
+		u64_stats_update_begin(&ring->stats.syncp);
+		u64_stats_inc(&ring->stats.twq.csum_partial);
+		u64_stats_update_end(&ring->stats.syncp);
+	}
 
 	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L2_HLEN,
 					skb_mac_header_len(skb) / 2) |
-			     FIELD_PREP(MPNIC_TWD_L3_IHLEN, hdrs.i3len / 2));
+			     FIELD_PREP(MPNIC_TWD_L3_IHLEN, hdrs.i3len / 2) |
+			     FIELD_PREP(MPNIC_TWD_L3_TYPE, hdrs.l3_type) |
+			     FIELD_PREP(MPNIC_TWD_L4_TYPE, hdrs.l4_type));
+
+	return false;
 }
 
 #define MPNIC_MIN_FRAME_LEN	60
@@ -261,9 +316,11 @@ static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
 	*meta = cpu_to_le64(MPNIC_TWD_FLAG_DEST_MAC);
 
 	MPNIC_XMIT_CB(skb)->bytecount = skb->len;
+	MPNIC_XMIT_CB(skb)->gso_segs = 1;
 	MPNIC_XMIT_CB(skb)->desc_count = 0;
 
-	mpnic_tx_offloads(ring, skb, meta);
+	if (mpnic_tx_offloads(ring, skb, meta))
+		goto err_free;
 
 	if (mpnic_tx_map(ring, skb, meta))
 		goto err_free;
@@ -312,6 +369,10 @@ netdev_features_t mpnic_features_check(struct sk_buff *skb,
 	    l2len + i3len + sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE > 256)
 		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
 
+	if (skb_is_gso(skb) &&
+	    !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size))
+		return features & ~NETIF_F_GSO_MASK;
+
 	return features;
 }
 
@@ -357,7 +418,7 @@ static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
 		}
 
 		total_bytes += MPNIC_XMIT_CB(skb)->bytecount;
-		total_packets++;
+		total_packets += MPNIC_XMIT_CB(skb)->gso_segs;
 
 		napi_consume_skb(skb, napi_budget);
 	}
@@ -811,6 +872,8 @@ static void mpnic_aggregate_ring_twq_counters(struct mpnic_base_stats *base,
 {
 	base->tx.dropped += u64_stats_read(&twq->stats.twq.dropped);
 	base->tx.csum_partial += u64_stats_read(&twq->stats.twq.csum_partial);
+	base->tx.hw_gso += u64_stats_read(&twq->stats.twq.hw_gso);
+	base->tx.hw_gso_wire += u64_stats_read(&twq->stats.twq.hw_gso_wire);
 	base->tx.stop += u64_stats_read(&twq->stats.twq.stop);
 }
 
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
index 714f09f4160e..70f321ac4368 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.h
@@ -90,6 +90,8 @@ struct mpnic_queue_stats {
 		struct {
 			u64_stats_t dropped;
 			u64_stats_t csum_partial;
+			u64_stats_t hw_gso;
+			u64_stats_t hw_gso_wire;
 			u64_stats_t stop;
 		} twq;
 		struct {

-- 
2.52.0

Re: [PATCH net-next 3/6] eth: mpnic: TCP segmentation offload

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mpnic_tx_lso() writes skb_shinfo(skb)->gso_size straight into
  the 14-bit MPNIC_TWD_MSS descriptor field with MPNIC_TWD_FLAG_REQ_LSO…
- [Medium] mpnic_xmit_frame_ring() pads every frame to 60 bytes in
  software (`skb_put_padto(skb, MPNIC_MIN_FRAME_LEN)`, line 307).
- [Low] The two places that feed the per-ring `twq.dropped` counter now
  count in different units.
- [Low] mpnic_tx_lso() bumps the new qstats counters `twq.hw_gso` and
  `twq.hw_gso_wire` (mpnic_txrx.c lines 259-260) before the frame is…

--

eth: mpnic: TCP segmentation offload

This adds hardware segmentation for non-encapsulated TCPv4/TCPv6 packets.
The TCP checksum is seeded with zero length, and the TOF block is set up
for incrementing IPv4 IDs and for clearing TCP flags per segment. GSO super
and wire packets are counted and reported through the netdev qstats API.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 680625810ec03..e2e6ab6d3c591 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -215,30 +218,82 @@ mpnic_tx_map(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)

[ ... ]

> +static bool
> +mpnic_tx_lso(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
> +{

[ ... ]

> +	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L4_HLEN, l4len / 4) |
> +			     FIELD_PREP(MPNIC_TWD_MSS, shinfo->gso_size) |
> +			     MPNIC_TWD_FLAG_REQ_LSO);

[Severity: Medium]
Can the device send runt frames for the last segment here?

mpnic_xmit_frame_ring() pads every frame in software:

	if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
		goto err_drop;

That suggests the TX path needs at least 60 bytes from the driver. For a
TSO skb the padding does nothing, because the super-packet is always longer
than 60 bytes, and the device builds each segment itself.

The last segment carries (skb->len - hdrlen) % gso_size bytes of payload.
For IPv4 TCP without options (54 bytes of headers) and a remainder of 1 to
5 bytes, that is a 55 to 59 byte frame. A TCP_NODELAY sender with
timestamps disabled can produce this, for example 1449 bytes at MSS 1448.

Neither mpnic_features_check() nor mpnic_tx_lso() guards against this.
mpnic_txglb_init() also doesn't configure any MAC or TOF auto-pad.

Does the segmentation engine pad the segments it generates to the Ethernet
minimum? If it doesn't, the link partner would drop those tail segments.
This also looks unchanged at the end of the series.

> +
> +	hdrlen = skb_checksum_start_offset(skb) + l4len;
> +	MPNIC_XMIT_CB(skb)->bytecount += (shinfo->gso_segs - 1) * hdrlen;
> +	MPNIC_XMIT_CB(skb)->gso_segs = shinfo->gso_segs;
> +
> +	u64_stats_update_begin(&ring->stats.syncp);
> +	u64_stats_inc(&ring->stats.twq.hw_gso);
> +	u64_stats_add(&ring->stats.twq.hw_gso_wire, shinfo->gso_segs);
> +	u64_stats_update_end(&ring->stats.syncp);

[Severity: Low]
Should hw_gso and hw_gso_wire be incremented only after the frame has
actually been handed to the device?

mpnic_tx_lso() is called from mpnic_tx_offloads(), which runs before
mpnic_tx_map() in mpnic_xmit_frame_ring():

	if (mpnic_tx_offloads(ring, skb, meta))
		goto err_free;

	if (mpnic_tx_map(ring, skb, meta))
		goto err_free;

Say mpnic_tx_map() then fails, either on a DMA mapping error or on a
fragment larger than MPNIC_TWD_LEN. The skb is freed and counted as
dropped, but hw_gso and hw_gso_wire are not rolled back. The same happens
to queued TSO skbs that mpnic_clean_twq0() discards with discard set.

mpnic_get_queue_stats_tx() and mpnic_get_base_stats() export these as
tx-hw-gso-packets and tx-hw-gso-wire-packets. They also add hw_gso_wire
into tx-needs-csum.

netdev.yaml defines these counters as packets the device segmented and the
wire packets it generated. On these paths the reported values include
super-packets the device never segmented. The existing csum_partial
counter has the same count-at-enqueue pattern.

[ ... ]

> @@ -312,6 +369,10 @@ netdev_features_t mpnic_features_check(struct sk_buff *skb,
>  	    l2len + i3len + sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE > 256)
>  		return features & ~(NETIF_F_CSUM_MASK | NETIF_F_GSO_MASK);
>  
> +	if (skb_is_gso(skb) &&
> +	    !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size))
> +		return features & ~NETIF_F_GSO_MASK;

[Severity: Medium]
Is the 14-bit field width the only limit the hardware puts on gso_size?

This check only rejects gso_size > 16383. There is no minimum MSS, and
nothing compares gso_size plus the header length against the largest frame
the device supports (MPNIC_MAX_JUMBO_FRAME_SIZE, 9742).

gso_size can come from an untrusted sender:

- virtio_net_hdr_to_skb() is used by tun, tap, macvtap and packet sockets
  with vnet headers. It only raises a small TCP gso_size to
  TCP_MIN_GSO_SIZE, which is 8, and marks the skb SKB_GSO_DODGY.
- mpnic doesn't advertise NETIF_F_GSO_ROBUST, so tcp_gso_segment() only
  recomputes gso_segs and passes the original skb to the driver.
- On bridged paths, is_skb_forwardable() skips the MTU check for GSO skbs.

So a guest or tap user could make mpnic_tx_lso() program MPNIC_TWD_MSS in
two problem ranges:

- 8, which means thousands of segments per 64KB descriptor.
- Roughly 9.7K to 16383, which asks for frames larger than the device
  supports.

How does the device handle either case?

Drivers such as i40e and ice clear NETIF_F_GSO_MASK in ndo_features_check
when the MSS is too small. Would a similar lower bound make sense here,
along with an upper bound tied to the maximum frame size? This also looks
unchanged at the end of the series.

>  	return features;
>  }
>  
> @@ -357,7 +418,7 @@ static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,
>  		}
>  
>  		total_bytes += MPNIC_XMIT_CB(skb)->bytecount;
> -		total_packets++;
> +		total_packets += MPNIC_XMIT_CB(skb)->gso_segs;

[Severity: Low]
Does this leave twq.dropped counting in two different units?

When mpnic_clean_twq0() runs with discard set, total_packets now adds wire
segments to the drop counter:

	if (discard) {
		...
		u64_stats_add(&ring->stats.twq.dropped, total_packets);

The error path in mpnic_xmit_frame_ring() still counts one per skb:

err_drop:
	...
	u64_stats_inc(&ring->stats.twq.dropped);

TSO skbs also reach that path when the new skb_cow_head() in
mpnic_tx_lso() fails, or when mpnic_tx_map() fails.

Both counts reach rtnl_link_stats64.tx_dropped through mpnic_get_stats64()
and mpnic_aggregate_ring_twq_counters(). A dropped TSO super-packet
therefore counts as gso_segs or as 1, depending on where it was dropped.
Before this patch both sites counted skbs. This also looks unchanged at the
end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com

[PATCH net-next 4/6] eth: mpnic: UDP segmentation offload

[Daniel Zahka]

Support segmentation of non-encapsulated udp4/udp6 packets. Like for
tcp, the segmentation engine expects the udp csum to be seeded with 0
length.

The HW inverts UDP checksums computed as 0 during segmentation.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    |  2 ++
 drivers/net/ethernet/meta/mpnic/mpnic_init.c   |  1 +
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c |  3 ++-
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 33 +++++++++++++++++++-------
 4 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 672ce0a98c22..2cd627d62bf5 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -20,6 +20,7 @@
 #define MPNIC_TWD_L4_TYPE		DESC_GENMASK(33, 32)
 enum {
 	MPNIC_TWD_L4_TYPE_TCP	= 1,
+	MPNIC_TWD_L4_TYPE_UDP	= 2,
 };
 
 #define MPNIC_TWD_FLAG_REQ_LSO		DESC_BIT(35)
@@ -335,6 +336,7 @@ enum {
 
 #define MPNIC_TOF_MODE_IPV4_ZERO_CSUM		CSR_BIT(1)
 #define MPNIC_TOF_MODE_TCP_ZERO_CSUM		CSR_BIT(2)
+#define MPNIC_TOF_MODE_UDP_ZERO_CSUM		CSR_BIT(3)
 #define MPNIC_TOF_MODE_L4_ZERO_CSUM		CSR_BIT(4)
 #define MPNIC_TOF_TCP_FLAG_CLR		0x7e3030		/* 0x1f8c0c0 */
 #define MPNIC_TOF_TCP_FLAG_CLR_FIRST_SEGMENT	CSR_GENMASK(8, 0)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
index b9a41656f8f7..ab832ec5de0e 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_init.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
@@ -304,6 +304,7 @@ static void mpnic_txglb_init(struct mpnic_dev *mpd)
 			      MPNIC_IPV4_ID_MODE_INCREMENT) |
 		   FIELD_PREP(MPNIC_TOF_MODE_IPV4_ZERO_CSUM, 1) |
 		   FIELD_PREP(MPNIC_TOF_MODE_TCP_ZERO_CSUM, 1) |
+		   FIELD_PREP(MPNIC_TOF_MODE_UDP_ZERO_CSUM, 0) |
 		   FIELD_PREP(MPNIC_TOF_MODE_L4_ZERO_CSUM, 0));
 	mpnic_wr64(mpd, MPNIC_TOF_TCP_FLAG_CLR,
 		   FIELD_PREP(MPNIC_TOF_TCP_FLAG_CLR_FIRST_SEGMENT,
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index 3a4aa5be57a1..83be45031a30 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -331,7 +331,8 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 		NETIF_F_RXCSUM |
 		NETIF_F_TSO |
 		NETIF_F_TSO_ECN |
-		NETIF_F_TSO6;
+		NETIF_F_TSO6 |
+		NETIF_F_GSO_UDP_L4;
 
 	netdev->hw_features |= netdev->features;
 	netdev->vlan_features |= netdev->features;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index e2e6ab6d3c59..9dd61b6d374c 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -223,29 +223,44 @@ static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
 		return;
 	}
 
-	hdrs->l4_type = MPNIC_TWD_L4_TYPE_TCP;
+	if (skb_shinfo(skb)->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))
+		hdrs->l4_type = MPNIC_TWD_L4_TYPE_TCP;
+	else
+		hdrs->l4_type = MPNIC_TWD_L4_TYPE_UDP;
+
 	hdrs->i3len = skb_transport_offset(skb) - skb_network_offset(skb);
 	hdrs->l3_type = ip_hdr(skb)->version == 4 ?
 			MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
 }
 
 static bool
-mpnic_tx_lso(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
+mpnic_tx_lso(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta,
+	     const struct mpnic_tx_hdrs *hdrs)
 {
 	struct skb_shared_info *shinfo;
 	unsigned int l4len, hdrlen;
-	struct tcphdr *tcph;
-	__be16 payload_len;
+	unsigned char *l4hdr;
 
 	if (skb_cow_head(skb, 0))
 		return true;
 
 	shinfo = skb_shinfo(skb);
+	l4hdr = skb_checksum_start(skb);
+
+	if (hdrs->l4_type == MPNIC_TWD_L4_TYPE_TCP) {
+		struct tcphdr *tcph = (struct tcphdr *)l4hdr;
+		__be16 payload_len;
 
-	tcph = (struct tcphdr *)skb_checksum_start(skb);
-	l4len = __tcp_hdrlen(tcph);
-	payload_len = cpu_to_be16(skb->len - skb_checksum_start_offset(skb));
-	csum_replace_by_diff(&tcph->check, (__force __wsum)payload_len);
+		l4len = __tcp_hdrlen(tcph);
+		payload_len = cpu_to_be16(skb->len -
+					  skb_checksum_start_offset(skb));
+		csum_replace_by_diff(&tcph->check, (__force __wsum)payload_len);
+	} else {
+		struct udphdr *udph = (struct udphdr *)l4hdr;
+
+		l4len = sizeof(*udph);
+		csum_replace_by_diff(&udph->check, (__force __wsum)udph->len);
+	}
 
 	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L4_HLEN, l4len / 4) |
 			     FIELD_PREP(MPNIC_TWD_MSS, shinfo->gso_size) |
@@ -277,7 +292,7 @@ mpnic_tx_offloads(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
 					skb->csum_offset / 2));
 
 	if (skb_is_gso(skb)) {
-		if (mpnic_tx_lso(ring, skb, meta))
+		if (mpnic_tx_lso(ring, skb, meta, &hdrs))
 			return true;
 	} else {
 		*meta |= cpu_to_le64(MPNIC_TWD_FLAG_REQ_CSO);

-- 
2.52.0

Re: [PATCH net-next 4/6] eth: mpnic: UDP segmentation offload

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With this patch, mpnic_netdev_alloc() advertises
  NETIF_F_GSO_UDP_L4.

--

eth: mpnic: UDP segmentation offload

This commit adds UDP segmentation offload for non-encapsulated udp4/udp6
packets by advertising NETIF_F_GSO_UDP_L4. For GSO skbs, mpnic_tx_lso()
now seeds the UDP checksum with zero length, the same way it already does
for TCP.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> index 3a4aa5be57a19..83be45031a30b 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> @@ -331,7 +331,8 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
>  		NETIF_F_RXCSUM |
>  		NETIF_F_TSO |
>  		NETIF_F_TSO_ECN |
> -		NETIF_F_TSO6;
> +		NETIF_F_TSO6 |
> +		NETIF_F_GSO_UDP_L4;

[Severity: Medium]
Now that NETIF_F_GSO_UDP_L4 is advertised, can UDP GSO skbs shorter than
MPNIC_MIN_FRAME_LEN reach the hardware with the zero padding counted as
payload?

An unprivileged process can set UDP_SEGMENT with a small gso_size and
send a short datagram. For example, IPv4 with gso_size=1 and 2 bytes of
data makes udp_send_skb() build a GSO skb with gso_segs=2 and a 44 byte
frame (14 + 20 + 8 + 2).

mpnic_xmit_frame_ring() pads every skb, GSO or not, before the offload
setup runs:

drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:mpnic_xmit_frame_ring() {
    ...
	if (skb_put_padto(skb, MPNIC_MIN_FRAME_LEN))
		goto err_drop;
    ...
}

The frame grows to 60 bytes. Then mpnic_tx_offloads()->mpnic_tx_lso() sets
MPNIC_TWD_FLAG_REQ_LSO with MSS=1 over a buffer that holds 18 bytes after
the 42 byte header, not 2.

If the segmentation engine works from the mapped length, would this send
18 one-byte datagrams instead of 2? 16 of them would be zero-filled and
delivered to the peer application.

That depends on whether the LSO engine uses the buffer length or the
IP/UDP length fields, and the driver source doesn't show which.

Even if the hardware ignores the padding, the accounting is still off.
MPNIC_XMIT_CB(skb)->bytecount, MPNIC_XMIT_CB(skb)->gso_segs and the
hw_gso_wire stat are all computed from shinfo->gso_segs. They won't match
the padded buffer the hardware is asked to segment.

Nothing appears to steer these skbs to software GSO. mpnic_features_check()
only checks an upper bound on gso_size:

drivers/net/ethernet/meta/mpnic/mpnic_txrx.c:mpnic_features_check() {
    ...
	if (skb_is_gso(skb) &&
	    !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size))
		return features & ~NETIF_F_GSO_MASK;
    ...
}

In net/core/dev.c, gso_features_check() and gso_within_dev_limits() only
check gso_max_segs and gso_max_size.

Before this patch only TCP GSO reached the driver, and locally generated
TCP GSO frames are well over 60 bytes. The later patch in the series,
"eth: mpnic: add GSO_PARTIAL for other tunnels", still pads every skb and
doesn't add a minimum length check.

Would it make sense to skip the padding for GSO skbs? Another option is to
clear NETIF_F_GSO_MASK in mpnic_features_check() when skb->len is below
MPNIC_MIN_FRAME_LEN.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com

[PATCH net-next 5/6] eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation

[Daniel Zahka]

The device can fully segment 4-in-6 and 6-in-6 packet types. Add
mpnic_features_check_encap_gso() to check the skb for some additional
constraints, such as ip6 non-atomic frag ext header.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    |  3 ++
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c |  2 ++
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 50 +++++++++++++++++++++++---
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 2cd627d62bf5..63282916eb13 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -14,6 +14,7 @@
 
 /* Transmit Work Descriptor Format */
 #define MPNIC_TWD_L2_HLEN		DESC_GENMASK(5, 0)
+#define MPNIC_TWD_L3_OHLEN		DESC_GENMASK(15, 8)
 #define MPNIC_TWD_L3_IHLEN		DESC_GENMASK(23, 16)
 #define MPNIC_TWD_CSUM_OFFSET		DESC_GENMASK(27, 24)
 #define MPNIC_TWD_L4_HLEN		DESC_GENMASK(31, 28)
@@ -30,6 +31,8 @@ enum {
 enum {
 	MPNIC_TWD_L3_TYPE_IPV4	= 1,
 	MPNIC_TWD_L3_TYPE_IPV6	= 2,
+	MPNIC_TWD_L3_TYPE_V6V6	= 3,
+	MPNIC_TWD_L3_TYPE_V6V4	= 4,
 };
 
 #define MPNIC_TWD_FLAG_DEST_MAC		DESC_BIT(43)
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index 83be45031a30..e391ca6483e0 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -326,6 +326,7 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 	mpn->num_napi = queues;
 
 	netdev->features |=
+		NETIF_F_GSO_IPXIP6 |
 		NETIF_F_SG |
 		NETIF_F_HW_CSUM |
 		NETIF_F_RXCSUM |
@@ -336,6 +337,7 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 
 	netdev->hw_features |= netdev->features;
 	netdev->vlan_features |= netdev->features;
+	netdev->hw_enc_features |= netdev->features;
 
 	netdev->min_mtu = IPV6_MIN_MTU;
 	netdev->max_mtu = MPNIC_MAX_JUMBO_FRAME_SIZE - ETH_HLEN;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index 9dd61b6d374c..c349e54a3547 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -22,6 +22,7 @@ struct mpnic_xmit_cb {
 
 struct mpnic_tx_hdrs {
 	unsigned int i3len;
+	unsigned int o3len;
 	unsigned int l3_type;
 	unsigned int l4_type;
 };
@@ -218,6 +219,9 @@ mpnic_tx_map(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
 
 static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
 {
+	unsigned int net_off = skb_network_offset(skb);
+	unsigned int inner_off;
+
 	if (!skb_is_gso(skb)) {
 		hdrs->i3len = skb_checksum_start(skb) - skb_network_header(skb);
 		return;
@@ -228,9 +232,17 @@ static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
 	else
 		hdrs->l4_type = MPNIC_TWD_L4_TYPE_UDP;
 
-	hdrs->i3len = skb_transport_offset(skb) - skb_network_offset(skb);
-	hdrs->l3_type = ip_hdr(skb)->version == 4 ?
-			MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
+	if (!skb->encapsulation) {
+		hdrs->i3len = skb_transport_offset(skb) - net_off;
+		hdrs->l3_type = ip_hdr(skb)->version == 4 ?
+				MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
+	} else {
+		inner_off = skb_inner_network_offset(skb);
+		hdrs->o3len = inner_off - net_off;
+		hdrs->i3len = skb_inner_transport_offset(skb) - inner_off;
+		hdrs->l3_type = inner_ip_hdr(skb)->version == 4 ?
+				MPNIC_TWD_L3_TYPE_V6V4 : MPNIC_TWD_L3_TYPE_V6V6;
+	}
 }
 
 static bool
@@ -305,6 +317,7 @@ mpnic_tx_offloads(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
 	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L2_HLEN,
 					skb_mac_header_len(skb) / 2) |
 			     FIELD_PREP(MPNIC_TWD_L3_IHLEN, hdrs.i3len / 2) |
+			     FIELD_PREP(MPNIC_TWD_L3_OHLEN, hdrs.o3len / 2) |
 			     FIELD_PREP(MPNIC_TWD_L3_TYPE, hdrs.l3_type) |
 			     FIELD_PREP(MPNIC_TWD_L4_TYPE, hdrs.l4_type));
 
@@ -363,6 +376,32 @@ netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
 	return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
 }
 
+static bool mpnic_ipxip6_gso_ok(struct sk_buff *skb)
+{
+	struct ipv6hdr *ip6h;
+	__be16 frag_off;
+	u8 nexthdr;
+	int start;
+
+	ip6h = ipv6_hdr(skb);
+	nexthdr = ip6h->nexthdr;
+	start = skb_network_offset(skb) + sizeof(*ip6h);
+	start = ipv6_skip_exthdr(skb, start, &nexthdr, &frag_off);
+
+	return !frag_off && start == skb_inner_network_offset(skb) &&
+	       (nexthdr == IPPROTO_IPV6 || nexthdr == IPPROTO_IPIP);
+}
+
+static netdev_features_t
+mpnic_features_check_encap_gso(struct sk_buff *skb, netdev_features_t features)
+{
+	if (skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6 &&
+	    mpnic_ipxip6_gso_ok(skb))
+		return features;
+
+	return features & ~NETIF_F_GSO_IPXIP6;
+}
+
 netdev_features_t mpnic_features_check(struct sk_buff *skb,
 				       struct net_device *dev,
 				       netdev_features_t features)
@@ -388,7 +427,10 @@ netdev_features_t mpnic_features_check(struct sk_buff *skb,
 	    !FIELD_FIT(MPNIC_TWD_MSS, skb_shinfo(skb)->gso_size))
 		return features & ~NETIF_F_GSO_MASK;
 
-	return features;
+	if (!skb->encapsulation || !skb_is_gso(skb))
+		return features;
+
+	return mpnic_features_check_encap_gso(skb, features);
 }
 
 static void mpnic_clean_twq0(struct mpnic_napi_vector *nv, int napi_budget,

-- 
2.52.0

Re: [PATCH net-next 5/6] eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mpnic_ipxip6_gso_ok()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c) never checks that the…
- [Medium] eth: mpnic: stale inner transport offset used in
  mpnic_tx_fill_hdrs()
- [Low] The commit message does not mention that this patch turns on
  hardware TX checksum offload for all encapsulated traffic.

--

eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation

This commit adds 4-in-6 and 6-in-6 TSO support to mpnic. It advertises
NETIF_F_GSO_IPXIP6, sets hw_enc_features, and fills the outer L3 header
length and the new V6V6/V6V4 L3 types in the TX descriptor. A new helper,
mpnic_features_check_encap_gso(), checks the outer IPv6 extension headers
before it allows hardware segmentation.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> index 83be45031a30b..e391ca6483e09 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c

[ ... ]

> @@ -336,6 +337,7 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
>  
>  	netdev->hw_features |= netdev->features;
>  	netdev->vlan_features |= netdev->features;
> +	netdev->hw_enc_features |= netdev->features;

[Severity: Low]
This line in mpnic_netdev_alloc() copies the whole feature set into
hw_enc_features, not only NETIF_F_GSO_IPXIP6. Was it meant to also turn on
NETIF_F_HW_CSUM, TSO, TSO6, TSO_ECN and GSO_UDP_L4 for all encapsulated
traffic?

Before this patch the driver did not set hw_enc_features. The only bits came
from register_netdevice():

    dev->hw_enc_features |= NETIF_F_SG | NETIF_F_GSO_PARTIAL;

netif_skb_features() masks encapsulated skbs with hw_enc_features. So until
now, CHECKSUM_PARTIAL tunnel packets were checksummed in software. That
covers VXLAN, GENEVE, GRE, IPIP/SIT, and the segments that software GSO
produces from them.

With this change those packets reach mpnic_tx_offloads() with
MPNIC_TWD_FLAG_REQ_CSO set. The non-GSO path in mpnic_tx_fill_hdrs() then
sets L3_IHLEN to cover everything from the outer IP header up to the inner
L4 header. Because TSO is now in hw_enc_features, register_netdevice() also
adds NETIF_F_TSO_MANGLEID there.

mpnic_features_check() still enforces the alignment and header length
limits, so this looks functionally ok. The commit message, though, only
describes 4-in-6 and 6-in-6 segmentation.

Could the commit message mention the tunnel checksum offload? If the wider
change isn't intended, could hw_enc_features be limited to the bits IPXIP6
needs?

>  
>  	netdev->min_mtu = IPV6_MIN_MTU;
>  	netdev->max_mtu = MPNIC_MAX_JUMBO_FRAME_SIZE - ETH_HLEN;
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 9dd61b6d374c7..c349e54a35470 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -363,6 +376,32 @@ netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
>  	return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
>  }
>  
> +static bool mpnic_ipxip6_gso_ok(struct sk_buff *skb)
> +{
> +	struct ipv6hdr *ip6h;
> +	__be16 frag_off;
> +	u8 nexthdr;
> +	int start;
> +
> +	ip6h = ipv6_hdr(skb);
> +	nexthdr = ip6h->nexthdr;
> +	start = skb_network_offset(skb) + sizeof(*ip6h);
> +	start = ipv6_skip_exthdr(skb, start, &nexthdr, &frag_off);

[Severity: Medium]
Should mpnic_ipxip6_gso_ok() check that ip6h->version == 6 before it
parses this header as IPv6?

A set SKB_GSO_IPXIP6 bit doesn't guarantee that the outermost header is
IPv6. iptunnel_handle_offloads() only resets the inner headers on the first
encapsulation, and it ORs in the gso_type bits.

Take a 4-in-6 ip6_tnl packet that is then sent over a sit tunnel. It ends
up with an outer IPv4 header and gso_type TCPV4 | IPXIP6 | IPXIP4.
inner_network_header still points at the innermost IPv4 header.

mpnic_tx_fill_hdrs() labels any encapsulated GSO skb as outer IPv6:

    hdrs->l3_type = inner_ip_hdr(skb)->version == 4 ?
            MPNIC_TWD_L3_TYPE_V6V4 : MPNIC_TWD_L3_TYPE_V6V6;

The fbnic gate this code appears to follow rejects this case in
fbnic_features_check_encap_gso():

    if (ip6_hdr->version != 6)
        return features & ~FBNIC_TUN_GSO_FEATURES;

With only this patch applied, the nested case still falls back to software
GSO, because IPXIP4 is not supported yet. The gate is still the same at the
end of the series, though.

After "eth: mpnic: add GSO_PARTIAL for other tunnels", a true return from
mpnic_ipxip6_gso_ok() makes mpnic_features_check_encap_gso() return early.
That skips the outer IPv4 check:

    if (iph->version == 4 && !(iph->frag_off & htons(IP_DF)))
        return features & ~NETIF_F_GSO_MASK;

It also skips the TSO_MANGLEID requirement.

With a sit tunnel using nopmtudisc, the outer DF bit is clear and byte 6 of
the IPv4 header is 0, which is read as NEXTHDR_HOP. ipv6_skip_exthdr() then
treats bytes of the middle IPv6 addresses as extension headers.

With addresses such as saddr[12..13] = 0x33,0x00 and daddr[4..5] =
0x04,0x01, the walk ends exactly at inner_network_offset with
nexthdr == IPPROTO_IPIP, and the gate passes.

Could that send every GSO_PARTIAL segment with the same outer IPv4 ID while
DF is clear? That is the case the later DF check is meant to prevent.

SKB_GSO_DODGY packets built by BPF (bpf_skb_adjust_room() with
BPF_F_ADJ_ROOM_ENCAP_L3_IPV6), or GRO packets with a bogus version nibble,
could also reach the device labelled V6V6/V6V4 when the outer version is
not 6. Software GSO doesn't check the version either, so the effect on the
hardware in that case is unclear.

Would a check like the one fbnic has be enough here?

    if (ip6h->version != 6)
        return false;

The inner version could optionally be checked against the nexthdr value
accepted here as well.

> +
> +	return !frag_off && start == skb_inner_network_offset(skb) &&
> +	       (nexthdr == IPPROTO_IPV6 || nexthdr == IPPROTO_IPIP);
> +}
> +
> +static netdev_features_t
> +mpnic_features_check_encap_gso(struct sk_buff *skb, netdev_features_t features)
> +{
> +	if (skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6 &&
> +	    mpnic_ipxip6_gso_ok(skb))
> +		return features;
> +
> +	return features & ~NETIF_F_GSO_IPXIP6;
> +}

[ ... ]

> @@ -228,9 +232,17 @@ static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
>  	else
>  		hdrs->l4_type = MPNIC_TWD_L4_TYPE_UDP;
>  
> -	hdrs->i3len = skb_transport_offset(skb) - skb_network_offset(skb);
> -	hdrs->l3_type = ip_hdr(skb)->version == 4 ?
> -			MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
> +	if (!skb->encapsulation) {
> +		hdrs->i3len = skb_transport_offset(skb) - net_off;
> +		hdrs->l3_type = ip_hdr(skb)->version == 4 ?
> +				MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
> +	} else {
> +		inner_off = skb_inner_network_offset(skb);
> +		hdrs->o3len = inner_off - net_off;
> +		hdrs->i3len = skb_inner_transport_offset(skb) - inner_off;
> +		hdrs->l3_type = inner_ip_hdr(skb)->version == 4 ?
> +				MPNIC_TWD_L3_TYPE_V6V4 : MPNIC_TWD_L3_TYPE_V6V6;
> +	}
>  }
>  
>  static bool

[Severity: Medium]
Can skb_inner_transport_offset() be relied on here? In the
encapsulated GSO branch, mpnic_tx_fill_hdrs() takes the inner L3
length from inner_transport_header. The rest of the TX path finds the
L4 header through skb_checksum_start():

  - mpnic_features_check() validates i3len that way.
  - mpnic_tx_lso() seeds the L4 checksum at skb_checksum_start(skb).
  - mpnic_tx_lso() also reads the L4 header length from there.

If the two offsets differ, MPNIC_TWD_L3_IHLEN no longer agrees with
where the driver says the L4 header is.

They can differ. Take an IPv6 GSO packet with an inner extension
header, such as destination options or a routing header.
ip6_rcv_core() sets transport_header to network_header +
sizeof(struct ipv6hdr), which points at the first extension header
and not at TCP/UDP.

Suppose the packet is then encapsulated by one of these:

  - bpf_lwt_push_ip_encap()
  - bpf_skb_adjust_room()
  - seg6_do_srh()
  - the IPVS tunnel xmit
  - the nf_flow_table tunnel push

In each case inner_transport_header is copied from that unadvanced
transport_header.

validate_xmit_skb() only calls skb_set_inner_transport_header() with
skb_checksum_start_offset() when netif_needs_gso() is false.
SKB_GSO_DODGY can be set by virtio_net_hdr_to_skb(), and
bpf_lwt_push_ip_encap() and bpf_skb_net_grow() add it
unconditionally. Without NETIF_F_GSO_ROBUST, netif_needs_gso() is
then true and __skb_gso_segment() runs.

For a dodgy skb, ip6ip6_gso_segment() -> ipv6_gso_segment() ->
tcp_gso_segment() only validates the packet, updates gso_segs and
returns NULL segs. The inner ipv6_gso_segment() then jumps to out
before skb_reset_inner_headers(). So when mpnic_xmit_frame() gets
the skb, inner_transport_header still points at the inner extension
header.

In that case hdrs->i3len is short by the length of the inner
extension headers. The descriptor tells the device that L4 starts
somewhere other than where mpnic_tx_lso() seeded the pseudo-header
checksum. That would corrupt the hardware-segmented frames on the
wire, and a guest or a BPF program can trigger it.

Would it be better to compute this as
skb_checksum_start_offset(skb) - inner_off, which matches
mpnic_features_check(), mpnic_tx_lso() and fbnic_tx_lso()? The
GSO_PARTIAL branch added later in the series also uses
skb_inner_transport_offset() and may need the same change.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com

[PATCH net-next 6/6] eth: mpnic: add GSO_PARTIAL for other tunnels

[Daniel Zahka]

Tunnels that are not natively supported for segmentation can be
supported with GSO_PARTIAL. For GSO_PARTIAL, the driver uses L3 type
OTHER, which tells the device to suppress any updates to the l3 headers
of the packet. As a consequence of this, MANGLEID is required when the
inner l3 is ipv4, and for tunnels that have an outer l3 of ipv4, we need
to check if DF is set in mpnic_features_check_encap_gso(), because the
device will not edit the IDs of the wire packets.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 drivers/net/ethernet/meta/mpnic/mpnic_csr.h    |  1 +
 drivers/net/ethernet/meta/mpnic/mpnic_netdev.c |  9 +++++++++
 drivers/net/ethernet/meta/mpnic/mpnic_txrx.c   | 21 ++++++++++++++++++++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
index 63282916eb13..03304e22d6ce 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_csr.h
@@ -29,6 +29,7 @@ enum {
 #define MPNIC_TWD_FLAG_REQ_COMPLETION	DESC_BIT(37)
 #define MPNIC_TWD_L3_TYPE		DESC_GENMASK(42, 40)
 enum {
+	MPNIC_TWD_L3_TYPE_OTHER	= 0,
 	MPNIC_TWD_L3_TYPE_IPV4	= 1,
 	MPNIC_TWD_L3_TYPE_IPV6	= 2,
 	MPNIC_TWD_L3_TYPE_V6V6	= 3,
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
index e391ca6483e0..fc19793036df 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
@@ -325,7 +325,15 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 	mpn->num_rx_queues = queues;
 	mpn->num_napi = queues;
 
+	netdev->gso_partial_features =
+		NETIF_F_GSO_GRE |
+		NETIF_F_GSO_GRE_CSUM |
+		NETIF_F_GSO_IPXIP4 |
+		NETIF_F_GSO_UDP_TUNNEL |
+		NETIF_F_GSO_UDP_TUNNEL_CSUM;
+
 	netdev->features |=
+		netdev->gso_partial_features |
 		NETIF_F_GSO_IPXIP6 |
 		NETIF_F_SG |
 		NETIF_F_HW_CSUM |
@@ -333,6 +341,7 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
 		NETIF_F_TSO |
 		NETIF_F_TSO_ECN |
 		NETIF_F_TSO6 |
+		NETIF_F_GSO_PARTIAL |
 		NETIF_F_GSO_UDP_L4;
 
 	netdev->hw_features |= netdev->features;
diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
index c349e54a3547..8b17acffe7e5 100644
--- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
+++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
@@ -236,6 +236,9 @@ static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
 		hdrs->i3len = skb_transport_offset(skb) - net_off;
 		hdrs->l3_type = ip_hdr(skb)->version == 4 ?
 				MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
+	} else if (skb_shinfo(skb)->gso_type & SKB_GSO_PARTIAL) {
+		hdrs->i3len = skb_inner_transport_offset(skb) - net_off;
+		hdrs->l3_type = MPNIC_TWD_L3_TYPE_OTHER;
 	} else {
 		inner_off = skb_inner_network_offset(skb);
 		hdrs->o3len = inner_off - net_off;
@@ -395,11 +398,27 @@ static bool mpnic_ipxip6_gso_ok(struct sk_buff *skb)
 static netdev_features_t
 mpnic_features_check_encap_gso(struct sk_buff *skb, netdev_features_t features)
 {
+	struct iphdr *iph;
+
 	if (skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6 &&
 	    mpnic_ipxip6_gso_ok(skb))
 		return features;
 
-	return features & ~NETIF_F_GSO_IPXIP6;
+	features &= ~NETIF_F_GSO_IPXIP6;
+
+	iph = ip_hdr(skb);
+	if (iph->version == 4 && !(iph->frag_off & htons(IP_DF)))
+		return features & ~NETIF_F_GSO_MASK;
+
+	/* HW can segment inner udp4 with fixed IDs, but there is no UDP
+	 * analog for MANGLEID, so defer to SW GSO.
+	 */
+	if (inner_ip_hdr(skb)->version == 4)
+		features &= ~NETIF_F_GSO_UDP_L4;
+	if (!(features & NETIF_F_TSO_MANGLEID))
+		features &= ~NETIF_F_TSO;
+
+	return features;
 }
 
 netdev_features_t mpnic_features_check(struct sk_buff *skb,

-- 
2.52.0

Re: [PATCH net-next 6/6] eth: mpnic: add GSO_PARTIAL for other tunnels

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message is incomplete about what the new GSO_PARTIAL
  tunnel offload covers by default.

--

eth: mpnic: add GSO_PARTIAL for other tunnels

This commit advertises GSO_PARTIAL for GRE, IPXIP4 and UDP tunnels. For
GSO_PARTIAL skbs the TX path uses L3 type OTHER, so the device does not
update the l3 headers. mpnic_features_check_encap_gso() now requires DF on
an outer IPv4 header and MANGLEID when the inner header is IPv4.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index c349e54a35470..8b17acffe7e5f 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -395,11 +398,27 @@ static bool mpnic_ipxip6_gso_ok(struct sk_buff *skb)
>  static netdev_features_t
>  mpnic_features_check_encap_gso(struct sk_buff *skb, netdev_features_t features)
>  {
> +	struct iphdr *iph;
> +
>  	if (skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6 &&
>  	    mpnic_ipxip6_gso_ok(skb))
>  		return features;
>
> -	return features & ~NETIF_F_GSO_IPXIP6;
> +	features &= ~NETIF_F_GSO_IPXIP6;
> +
> +	iph = ip_hdr(skb);
> +	if (iph->version == 4 && !(iph->frag_off & htons(IP_DF)))
> +		return features & ~NETIF_F_GSO_MASK;
> +
> +	/* HW can segment inner udp4 with fixed IDs, but there is no UDP
> +	 * analog for MANGLEID, so defer to SW GSO.
> +	 */
> +	if (inner_ip_hdr(skb)->version == 4)
> +		features &= ~NETIF_F_GSO_UDP_L4;
> +	if (!(features & NETIF_F_TSO_MANGLEID))
> +		features &= ~NETIF_F_TSO;

[Severity: Low]

This isn't a bug, but could the commit message say more about what the new
GSO_PARTIAL offload covers by default?

The subject says "add GSO_PARTIAL for other tunnels", and the body says
these tunnels "can be supported with GSO_PARTIAL".

mpnic_netdev_alloc() doesn't add NETIF_F_TSO_MANGLEID to netdev->features.
The core only makes it available as a hw_feature:

net/core/dev.c:register_netdevice() {
    ...
	if (dev->hw_features & NETIF_F_TSO)
		dev->hw_features |= NETIF_F_TSO_MANGLEID;
    ...
}

With default settings, this check clears NETIF_F_TSO. Then the
skb_gso_ok(skb, features | partial_features) check in __skb_gso_segment()
fails for SKB_GSO_TCPV4. As a result, inner-IPv4 TCP over GRE, IPXIP4 and
UDP tunnels is fully segmented in software until the user turns on
tx-tcp-mangleid-segmentation with ethtool.

Inner-UDPv4 tunnel GSO is never offloaded, because NETIF_F_GSO_UDP_L4 is
always cleared when the inner header is IPv4. The code comment explains
this, but the commit message doesn't mention it.

The message does say "MANGLEID is required when the inner l3 is ipv4".
Could it also say that, by default, only inner-IPv6 traffic over these
tunnels gets the new offload, and that inner-UDPv4 always uses software
GSO? fbnic has the same MANGLEID gate, so the behavior itself looks
intentional.

> +
> +	return features;
>  }
>
>  netdev_features_t mpnic_features_check(struct sk_buff *skb,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com