[PATCH v2 0/4] net/gve: TSO descriptor count fixes for GVE DQO

[PATCH v2 0/4] net/gve: TSO descriptor count fixes for GVE DQO

[Joshua Washington]

https://inbox.dpdk.org/dev/20250707231812.1937260-7-joshwash@google.com/
introduced descriptor limit checks for TSO packets in DPDK.
Unfortunately, a couple of bugs have been found in the implementation.

This patch series introduces fixes for those bugs while correcting the
overly restrictive behavior recently introduced mandating that all
headers be in the same buffer.

The final patch in the series introduces tx_pkt_prepare as an option to
ensure that there is a way to get a clear signal of when packets violate
hardware limitations. Before a recently introduced patch, the GVE
behavior was to stop transmitting at the first invalid packet, which
would allow the application to do any necessary linearization or packet
modification before attempting to transmit again. It turns out that that
behavior is considered incorrect, so offer tx_pkt_prepare as an
alternative.

Depends-on: series-39707 ("net/gve: fix DQO Tx queue stalls")

Changes in v2:
- Rely on tx_pkt->nb_segs for descriptor limit calculation only as a
  precursor, falling back to the traditional gve_tx_pkt_nb_data_descs()
  if that optimization fails.
- Only call rte_net_intel_cksum_preapre on TSO packets that request
  checksum offloading. This enforces the additional requirement that
  headers are all in the same mbuf segment.
- Introduce new patch to validate minimum header length.

Joshua Washington (4):
  net/gve: fix TSO segment offset calculation
  net/gve: account for multi-mbuf headers when validating TSO
  net/gve: introduce Tx packet preparation for DQO format
  net/gve: validate minimum header length for TSO packets

 drivers/net/gve/base/gve_desc_dqo.h |   2 +
 drivers/net/gve/gve_ethdev.h        |   3 +
 drivers/net/gve/gve_tx_dqo.c        | 121 +++++++++++++++++++++++++---
 3 files changed, 113 insertions(+), 13 deletions(-)

-- 
2.56.0.317.g454f8302e1-goog

[PATCH v2 1/4] net/gve: fix TSO segment offset calculation

[Joshua Washington]

The descriptor limit validation for TSO packets was erroneously
computing the TSO segment offset at the end of an mbuf. The first bytes
of any mbuf will be included as part of the previous TSO segment which
has not yet been completed, as indicated by seg_remain. Subtract those
seg_remain bytes from the mbuf length before computing the segment
offset.

Visual representation:

Before:
------------------------------------------------------------------
|    tso_    |    tso_    |    tso_    |    tso_    |    tso_    |
|    segsz   |    segsz   |    segsz   |    segsz   |    segsz   |
------------------------------------------------------------------
|           16K               |            16K               |   |
------------------------------------------------------------------

After:
------------------------------------------------------------------
|rem |    tso_    |    tso_    |    tso_    |    tso_    | seg_  |
|ain |    segsz   |    segsz   |    segsz   |    segsz   |offset |
------------------------------------------------------------------
|           16K               |            16K               |   |
------------------------------------------------------------------
                                                               ^
                                                        mbuf_remain

In this example, the original code would have found that tso_segsz
evenly divides mbuf_len, and that there is no tail to be considered
split across two mbufs. So the number of descriptors it would have found
would be:

  1 (for the headers) +
  0 (no segment offset) +
  0 (mbuf_remain cannot be less than 0)

For a total of one descriptor counted in num_descs for the start of
the current TSO segment.

Meanwhile, the reality would be:

  1 (for the headers) +
  1 (seg_offset != 0) +
  1 (mbuf_remain < seg_offset)

For a total of 3 descriptors counted against the descriptor limit.

This undercount can cause TSO segments to incorrectly cross up to two
extra descriptors more than the hardware limit, leading to the datapath
being disabled.

Fixes: be8f0eb81f98 ("net/gve: fix DQO TSO descriptor limit")
Cc: stable@dpdk.org

Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Ankit Garg <nktgrg@google.com>
Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
---
Depends-on: series-39707 ("net/gve: fix DQO Tx queue stalls")
---
 drivers/net/gve/gve_tx_dqo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.c
index 52fcdb285a..3a20109a5d 100644
--- a/drivers/net/gve/gve_tx_dqo.c
+++ b/drivers/net/gve/gve_tx_dqo.c
@@ -191,7 +191,7 @@ gve_tx_validate_descs(const struct gve_tx_queue *txq, struct rte_mbuf *tx_pkt,
 			return false;
 
 		if (seg_remain < mbuf_len) {
-			seg_offset = mbuf_len % tso_segsz;
+			seg_offset = (mbuf_len - seg_remain) % tso_segsz;
 			/* The MSS is bound from above by 9728B, so a
 			 * single TSO segment in the middle of an mbuf
 			 * will be part of at most two descriptors, and
-- 
2.56.0.317.g454f8302e1-goog

[PATCH v2 2/4] net/gve: account for multi-mbuf headers when validating TSO

[Joshua Washington]

The DQ queue format allows TSO headers to span up to 3 descriptors.
Before this patch, the TSO descriptor limit check logic assumed that
headers would only be in a single descriptor. This can cause under
counting when processing descriptors, which can lead to hardware
failures.

As an example: suppose a header spans 3 mbufs. In that case, 3
descriptors will need to be reserved per TSO segment. Now imagine that
there is a TSO segment whose payload spans 8 buffers. The previous logic
would add one for the headers, making a total of 9, which would be
considered valid. In actuality, since the header spans 3 descriptors
themselves, there are actually 11 descriptors for the one wire segment,
which is invalid.

Compute the number of descriptors needed for the TSO header, reject
mbufs in which the headers span more than 3 buffers, and count the
number of descriptors needed against the descriptor budget per wire
packet/TSO segment.

Also, relax the strong restriction that all headers be in the same
buffer, as this is not a requirement by the hardware.

Fixes: be8f0eb81f98 ("net/gve: fix DQO TSO descriptor limit")
Fixes: 6b0452f76bf0 ("net/gve: enforce DQO Tx packet length limits")
Cc: stable@dpdk.org

Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Ankit Garg <nktgrg@google.com>
Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
---
Depends-on: series-39707 ("net/gve: fix DQO Tx queue stalls")
---
 drivers/net/gve/base/gve_desc_dqo.h |  1 +
 drivers/net/gve/gve_tx_dqo.c        | 38 ++++++++++++++++++++++-------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/net/gve/base/gve_desc_dqo.h b/drivers/net/gve/base/gve_desc_dqo.h
index 53e394f01c..b7d8b27d68 100644
--- a/drivers/net/gve/base/gve_desc_dqo.h
+++ b/drivers/net/gve/base/gve_desc_dqo.h
@@ -45,6 +45,7 @@ GVE_CHECK_STRUCT_LEN(16, gve_tx_pkt_desc_dqo);
 
 /* Maximum number of data descriptors allowed per packet, or per-TSO segment. */
 #define GVE_TX_MAX_DATA_DESCS 10
+#define GVE_TX_MAX_HEADER_DESCS_TSO 3
 
 /* Min gap between tail and head to avoid cacheline overlap */
 #define GVE_TX_MIN_DESC_PREVENT_CACHE_OVERLAP 4
diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.c
index 3a20109a5d..00a4ea9d27 100644
--- a/drivers/net/gve/gve_tx_dqo.c
+++ b/drivers/net/gve/gve_tx_dqo.c
@@ -138,7 +138,7 @@ gve_tx_validate_pkt(const struct rte_mbuf *tx_pkt, bool is_tso)
 	 */
 	headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len;
 	if (tx_pkt->l4_len == 0 || headlen > GVE_TX_MAX_HDR_SIZE_DQO ||
-	    tx_pkt->data_len < headlen || tx_pkt->pkt_len <= headlen)
+	    tx_pkt->pkt_len <= headlen)
 		return false;
 
 	return tx_pkt->pkt_len - headlen <= GVE_TX_MAX_TSO_LEN_DQO &&
@@ -167,20 +167,41 @@ static inline bool
 gve_tx_validate_descs(const struct gve_tx_queue *txq, struct rte_mbuf *tx_pkt,
 		      uint16_t nb_descs, bool is_tso)
 {
+	int headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len;
+	int num_descs, seg_offset, mbuf_len;
+	int tso_segsz = tx_pkt->tso_segsz;
+	int header_remain = headlen;
+	uint16_t num_header_descs;
+
 	if (nb_descs > gve_tx_max_pkt_descs(txq))
 		return false;
 
 	if (!is_tso)
 		return nb_descs <= GVE_TX_MAX_DATA_DESCS;
 
-	int tso_segsz = tx_pkt->tso_segsz;
-	int num_descs, seg_offset, mbuf_len;
-	int headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len;
+	/* Process headers. Headers can span up to 3 descriptors. */
+	if (headlen == 0)
+		return false;
+
+	num_header_descs = 1;
+	while (tx_pkt != NULL) {
+		if (header_remain <= tx_pkt->data_len)
+			break;
+
+		if (likely(tx_pkt->data_len > 0)) {
+			header_remain -= tx_pkt->data_len;
+			num_header_descs++;
+		}
 
-	/* Headers will be split into their own buffer. */
-	num_descs = 1;
+		tx_pkt = tx_pkt->next;
+	}
+
+	if (tx_pkt == NULL || num_header_descs > GVE_TX_MAX_HEADER_DESCS_TSO)
+		return false;
+
+	num_descs = num_header_descs;
 	seg_offset = 0;
-	mbuf_len = tx_pkt->data_len - headlen;
+	mbuf_len = tx_pkt->data_len - header_remain;
 
 	while (tx_pkt) {
 		if (!mbuf_len)
@@ -207,7 +228,7 @@ gve_tx_validate_descs(const struct gve_tx_queue *txq, struct rte_mbuf *tx_pkt,
 			 * segment in this mbuf is divided between two
 			 * descriptors, both must be counted.
 			 */
-			num_descs = 1 + !!(seg_offset) +
+			num_descs = num_header_descs + !!(seg_offset) +
 				(mbuf_remain < seg_offset && mbuf_remain);
 		} else {
 			seg_offset += mbuf_len;
@@ -220,7 +241,6 @@ gve_tx_validate_descs(const struct gve_tx_queue *txq, struct rte_mbuf *tx_pkt,
 			mbuf_len = tx_pkt->data_len;
 	}
 
-
 	return true;
 }
 
-- 
2.56.0.317.g454f8302e1-goog

[PATCH v2 3/4] net/gve: introduce Tx packet preparation for DQO format

[Joshua Washington]

Before this patch, packets that are invalid in the DQO format with
respect to offloads, descriptor limits, or packet size limits, are
dropped by the driver without any form of recourse for the application,
potentially leading to a disruption in traffic that can be hard to
debug.

Introduce tx_pkt_prepare for the DQ queue format to avoid dropping
packets without first giving the application a way to know that they
are invalid. For TSO packets, this method also computes the TCP
pseudo-header for packets with checksum offloading enabled. Note that
in this case, such packets must have L2/L3/L4 headers in the same mbuf
segment to pass tx_pkt_prepare.

Fixes: 84472d3edffa ("net/gve: fix Tx stall on invalid packets")
Cc: stable@dpdk.org

Signed-off-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Praveen Kaligineedi <pkaligineedi@google.com>
Reviewed-by: Ankit Garg <nktgrg@google.com>
---
Depends-on: series-39707 ("net/gve: fix DQO Tx queue stalls")

Changes in v2:
- Rely on tx_pkt->nb_segs for descriptor limit calculation only as a
  precursor, falling back to the traditional gve_tx_pkt_nb_data_descs()
  if that optimization fails.
- Only call rte_net_intel_cksum_preapre on TSO packets that request
  checksum offloading. This enforces the additional requirement that
  headers are all in the same mbuf segment.
---
 drivers/net/gve/gve_ethdev.h |  3 ++
 drivers/net/gve/gve_tx_dqo.c | 79 +++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gve/gve_ethdev.h b/drivers/net/gve/gve_ethdev.h
index 26c6a304a6..ab5af33ca9 100644
--- a/drivers/net/gve/gve_ethdev.h
+++ b/drivers/net/gve/gve_ethdev.h
@@ -649,6 +649,9 @@ gve_rx_burst_dqo(void *rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts);
 uint16_t
 gve_tx_burst_dqo(void *txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
 
+uint16_t
+gve_tx_pkt_prepare_dqo(void *txq, struct rte_mbuf **tx_pkts, uint16_t nb_pkts);
+
 void
 gve_set_rx_function_dqo(struct rte_eth_dev *dev);
 
diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.c
index 00a4ea9d27..d6ed238849 100644
--- a/drivers/net/gve/gve_tx_dqo.c
+++ b/drivers/net/gve/gve_tx_dqo.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2022-2023 Intel Corporation
  */
 
+#include <rte_net.h>
+
 #include "gve_ethdev.h"
 #include "base/gve_adminq.h"
 #include "rte_malloc.h"
@@ -125,7 +127,7 @@ gve_tx_clean_descs_dqo(struct gve_tx_queue *txq, uint16_t nb_descs) {
 
 /* Validate packet lengths against the device limits. */
 static inline bool
-gve_tx_validate_pkt(const struct rte_mbuf *tx_pkt, bool is_tso)
+gve_tx_validate_pkt_limits(const struct rte_mbuf *tx_pkt, bool is_tso)
 {
 	uint32_t headlen;
 
@@ -303,6 +305,78 @@ gve_fill_tx_pkt_desc(struct gve_tx_queue *txq, uint16_t *tx_id,
 	}
 }
 
+uint16_t
+gve_tx_pkt_prepare_dqo(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
+{
+	struct gve_tx_queue *txq = tx_queue;
+	uint16_t i;
+
+	for (i = 0; i < nb_pkts; i++) {
+		struct rte_mbuf *tx_pkt;
+		uint16_t nb_descs;
+		bool tso, csum;
+		int err;
+
+		tx_pkt = tx_pkts[i];
+
+#ifdef RTE_ETHDEV_DEBUG_TX
+		const char *reason;
+
+		if (rte_mbuf_check(tx_pkt, true, &reason)) {
+			rte_errno = EINVAL;
+			return i;
+		}
+
+		err = rte_validate_tx_offload(tx_pkt);
+		if (err != 0) {
+			rte_errno = -err;
+			return i;
+		}
+#endif
+
+		tso = !!(tx_pkt->ol_flags & RTE_MBUF_F_TX_TCP_SEG);
+		if (!gve_tx_validate_pkt_limits(tx_pkt, tso)) {
+			rte_errno = EINVAL;
+			return i;
+		}
+
+		if (tso) {
+			nb_descs = gve_tx_pkt_nb_data_descs(tx_pkt) + 1;
+		} else {
+			/* Handle non-TSO packets by directly checking nb_segs.
+			 * The packet has already been validated to be no larger
+			 * than 9728B, so every descriptor is guaranteed to only
+			 * require a single descriptor.
+			 * If the packet does not meet the hardware descriptor
+			 * limits, fall back to the mbuf walk in case the
+			 * mbuf has empty segments.
+			 */
+			if (tx_pkt->nb_segs <= GVE_TX_MAX_DATA_DESCS)
+				continue;
+			nb_descs = gve_tx_pkt_nb_data_descs(tx_pkt);
+		}
+		if (!gve_tx_validate_descs(txq, tx_pkt, nb_descs, tso)) {
+			rte_errno = ENOTSUP;
+			return i;
+		}
+
+		/* If checksum offloading is requested, the L4 pseudo header
+		 * checksum must be seeded. In this case, headers must all be in
+		 * the same buffer.
+		 */
+		csum = !!(tx_pkt->ol_flags & GVE_TX_CKSUM_OFFLOAD_MASK_DQO);
+		if (csum && tso) {
+			err = rte_net_intel_cksum_prepare(tx_pkt);
+			if (err != 0) {
+				rte_errno = -err;
+				return i;
+			}
+		}
+	}
+
+	return nb_pkts;
+}
+
 uint16_t
 gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 {
@@ -349,7 +423,7 @@ gve_tx_burst_dqo(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 		tso = !!(ol_flags & RTE_MBUF_F_TX_TCP_SEG);
 		csum = !!(ol_flags & GVE_TX_CKSUM_OFFLOAD_MASK_DQO);
 
-		if (!gve_tx_validate_pkt(tx_pkt, tso)) {
+		if (!gve_tx_validate_pkt_limits(tx_pkt, tso)) {
 			txq->stats.invalid_pkts++;
 			rte_pktmbuf_free(tx_pkt);
 			continue;
@@ -703,4 +777,5 @@ void
 gve_set_tx_function_dqo(struct rte_eth_dev *dev)
 {
 	dev->tx_pkt_burst = gve_tx_burst_dqo;
+	dev->tx_pkt_prepare = gve_tx_pkt_prepare_dqo;
 }
-- 
2.56.0.317.g454f8302e1-goog

[PATCH v2 4/4] net/gve: validate minimum header length for TSO packets

[Joshua Washington]

In addition to the limits added in commit
03b62d39c1c8 ("net/gve: enforce DQO TSO header and length limits"), TSO
packets also have a minimum header length of 16B. Enforce this limit as
well.

Fixes: 403c671a46b6 ("net/gve: support TSO in DQO RDA")
Fixes: be8f0eb81f98 ("net/gve: fix DQO TSO descriptor limit")
Cc: stable@dpdk.org

Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/gve/base/gve_desc_dqo.h | 1 +
 drivers/net/gve/gve_tx_dqo.c        | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gve/base/gve_desc_dqo.h b/drivers/net/gve/base/gve_desc_dqo.h
index b7d8b27d68..5d51c70aa9 100644
--- a/drivers/net/gve/base/gve_desc_dqo.h
+++ b/drivers/net/gve/base/gve_desc_dqo.h
@@ -14,6 +14,7 @@
 #define GVE_TX_MIN_TSO_MSS_DQO 88
 #define GVE_TX_MIN_PKT_SIZE_DQO 17
 #define GVE_TX_MAX_PKT_SIZE_DQO 9728
+#define GVE_TX_MIN_TSO_HDR_LEN_DQO 16
 #define GVE_TX_MAX_TSO_LEN_DQO ((256 * 1024) - 1)
 
 /* Basic TX descriptor (DTYPE 0x0C) */
diff --git a/drivers/net/gve/gve_tx_dqo.c b/drivers/net/gve/gve_tx_dqo.c
index d6ed238849..45f5a79b2e 100644
--- a/drivers/net/gve/gve_tx_dqo.c
+++ b/drivers/net/gve/gve_tx_dqo.c
@@ -139,8 +139,8 @@ gve_tx_validate_pkt_limits(const struct rte_mbuf *tx_pkt, bool is_tso)
 	 * contained in the first segment; the payload must not be empty.
 	 */
 	headlen = tx_pkt->l2_len + tx_pkt->l3_len + tx_pkt->l4_len;
-	if (tx_pkt->l4_len == 0 || headlen > GVE_TX_MAX_HDR_SIZE_DQO ||
-	    tx_pkt->pkt_len <= headlen)
+	if (tx_pkt->l4_len == 0 || headlen < GVE_TX_MIN_TSO_HDR_LEN_DQO ||
+	    headlen > GVE_TX_MAX_HDR_SIZE_DQO || tx_pkt->pkt_len <= headlen)
 		return false;
 
 	return tx_pkt->pkt_len - headlen <= GVE_TX_MAX_TSO_LEN_DQO &&
-- 
2.56.0.317.g454f8302e1-goog