Skip to contentSkip to conversation

IXGBE IPsec fixes

PATCH Superseded Anatoly Burakov 3 participants 18 messages Aug 18 – Aug 19 Network drivers

lore
Share & tools
lore

Anatoly Burakov

author
#0 lore raw

cover IXGBE IPsec fixes

Small array of fixes and refactors for IXGBE IPsec implementation.

Anatoly Burakov (6):
net/ixgbe: add key length check in IPsec net/ixgbe: remove unnecessary dev pointer net/ixgbe: harmonize return values in IPsec net/ixgbe: get rid of unnecessary malloc in IPsec net/ixgbe: fix missing HW write in IPsec net/ixgbe: refactor IPsec code

 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 355 ++++++++++++--------------
 drivers/net/intel/ixgbe/ixgbe_ipsec.h |   1 +
 2 files changed, 161 insertions(+), 195 deletions(-)
Signature · 1 line
2.52.0
author
#1 lore raw

PATCH 1/6 net/ixgbe: add key length check in IPsec

The cryptodev API does not check for user parameters being within the bounds of what the driver supports. Currently, the only key size supported by ixgbe is 16 bytes, so that is what we will check for. Add the check.

Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +6 -0
index bdd34344e8..aaf657af5f 100644
@@ -384,6 +384,12 @@ ixgbe_crypto_create_session(void *device,
 	}
 	aead_xform = &conf->crypto_xform->aead;
 
+	/* Only 16-byte keys are supported. */
+	if (aead_xform->key.length != 16) {
+		PMD_DRV_LOG(ERR, "Unsupported key length %u", aead_xform->key.length);
+		return -ENOTSUP;
+	}
+
 	if (conf->ipsec.direction == RTE_SECURITY_IPSEC_SA_DIR_INGRESS) {
 		if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_SECURITY) {
 			ic_session->op = IXGBE_OP_AUTHENTICATED_DECRYPTION;
Signature · 1 line
2.52.0
author
#2 lore raw

PATCH 2/6 net/ixgbe: remove unnecessary dev pointer

In reply to Anatoly Burakov

The ic_session already contains a dev_data pointer, so there is no need to pass the dev pointer to ixgbe_crypto_remove_sa.

Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +4 -5
index aaf657af5f..e2ec402b52 100644
@@ -268,12 +268,11 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 }
 
 static int
-ixgbe_crypto_remove_sa(struct rte_eth_dev *dev,
-		       struct ixgbe_crypto_session *ic_session)
+ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 {
-	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(ic_session->dev_data->dev_private);
 	struct ixgbe_ipsec *priv =
-			IXGBE_DEV_PRIVATE_TO_IPSEC(dev->data->dev_private);
+			IXGBE_DEV_PRIVATE_TO_IPSEC(ic_session->dev_data->dev_private);
 	uint32_t reg_val;
 	int sa_index = -1;
 

@@ -441,7 +440,7 @@ ixgbe_crypto_remove_session(void *device,
 		return -ENODEV;
 	}
 
-	if (ixgbe_crypto_remove_sa(eth_dev, ic_session)) {
+	if (ixgbe_crypto_remove_sa(ic_session)) {
 		PMD_DRV_LOG(ERR, "Failed to remove session");
 		return -EFAULT;
 	}
Signature · 1 line
2.52.0
author
#3 lore raw

PATCH 3/6 net/ixgbe: harmonize return values in IPsec

In reply to Anatoly Burakov

Currently, some paths in add/remove SA code return negative errno values while others just return -1. Harmonize all of them to return appropriate negative errno values.

Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 30 +++++++++++----------------
 1 file changed, 12 insertions(+), 18 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +12 -18
index e2ec402b52..41fc7551ff 100644
@@ -119,9 +119,8 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 
 		/* Fail if no match and no free entries*/
 		if (ip_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "No free entry left in the Rx IP table");
-			return -1;
+			PMD_DRV_LOG(ERR, "No free entry left in the Rx IP table");
+			return -ENOSPC;
 		}
 
 		/* Find a free entry in the SA table*/

@@ -133,9 +132,8 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no free entries*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "No free entry left in the Rx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "No free entry left in the Rx SA table");
+			return -ENOSPC;
 		}
 
 		priv->rx_ip_tbl[ip_index].ip.ipv6[0] =

@@ -231,9 +229,8 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no free entries*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "No free entry left in the Tx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "No free entry left in the Tx SA table");
+			return -ENOSPC;
 		}
 
 		priv->tx_sa_tbl[sa_index].spi =

@@ -289,9 +286,8 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 
 		/* Fail if no match*/
 		if (ip_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "Entry not found in the Rx IP table");
-			return -1;
+			PMD_DRV_LOG(ERR, "Entry not found in the Rx IP table");
+			return -ENOENT;
 		}
 
 		/* Find a free entry in the SA table*/

@@ -304,9 +300,8 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no match*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "Entry not found in the Rx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "Entry not found in the Rx SA table");
+			return -ENOENT;
 		}
 
 		/* Disable and clear Rx SPI and key table entries*/

@@ -347,9 +342,8 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no match entries*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "Entry not found in the Tx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "Entry not found in the Tx SA table");
+			return -ENOENT;
 		}
 		reg_val = IPSRXIDX_WRITE | (sa_index << 3);
 		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0), 0);
Signature · 1 line
2.52.0
author
#4 lore raw

PATCH 4/6 net/ixgbe: get rid of unnecessary malloc in IPsec

In reply to Anatoly Burakov

Currently, when writing SA key to HW, a temporary buffer is allocated, to which the key is written. The allocation does not have to happen on the heap, so change the allocation to be on the stack, as key size is known at compile time and is pretty tiny.

Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +3 -19
index 41fc7551ff..1d001b955b 100644
@@ -94,10 +94,12 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 			dev_data->dev_private);
 	uint32_t reg_val;
 	int sa_index = -1;
+	uint8_t key[16] = {0};
+
+	memcpy(key, ic_session->key, ic_session->key_len);
 
 	if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION) {
 		int i, ip_index = -1;
-		uint8_t *key;
 
 		/* Find a match in the IP table*/
 		for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {

@@ -191,13 +193,6 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				priv->rx_sa_tbl[sa_index].ip_index);
 		IXGBE_WAIT_RWRITE;
 
-		/* write Key table entry*/
-		key = malloc(ic_session->key_len);
-		if (!key)
-			return -ENOMEM;
-
-		memcpy(key, ic_session->key, ic_session->key_len);
-
 		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
 				IPSRXIDX_TABLE_KEY | (sa_index << 3);
 		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0),

@@ -214,10 +209,7 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				priv->rx_sa_tbl[sa_index].mode);
 		IXGBE_WAIT_RWRITE;
 
-		free(key);
-
 	} else { /* sess->dir == RTE_CRYPTO_OUTBOUND */
-		uint8_t *key;
 		int i;
 
 		/* Find a free entry in the SA table*/

@@ -238,12 +230,6 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		priv->tx_sa_tbl[i].used = 1;
 		ic_session->sa_index = sa_index;
 
-		key = malloc(ic_session->key_len);
-		if (!key)
-			return -ENOMEM;
-
-		memcpy(key, ic_session->key, ic_session->key_len);
-
 		/* write Key table entry*/
 		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE | (sa_index << 3);
 		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0),

@@ -257,8 +243,6 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT,
 				rte_cpu_to_be_32(ic_session->salt));
 		IXGBE_WAIT_TWRITE;
-
-		free(key);
 	}
 
 	return 0;
Signature · 1 line
2.52.0
author
#5 lore raw

PATCH 5/6 net/ixgbe: fix missing HW write in IPsec

In reply to Anatoly Burakov

When removing an SA and finding ourselves with zero refcount on an IP table entry, we zero out the IP table registers, but do not write them. Add the missing write.

Fixes 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
Cc [email protected]
Cc [email protected]
Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +1 -0
index 1d001b955b..2d1b3a43ca 100644
@@ -312,6 +312,7 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
 			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
 			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), 0);
+			IXGBE_WAIT_RWRITE;
 		}
 	} else { /* session->dir == RTE_CRYPTO_OUTBOUND */
 		int i;
Signature · 1 line
2.52.0
author
#6 lore raw

PATCH 6/6 net/ixgbe: refactor IPsec code

In reply to Anatoly Burakov

Currently, the IPsec code mixes SW bookkeeping with HW writes quite a lot, which results in difficulty extending the code, lots of duplication, and potential for bugs. Refactor the code to be more readable by separating out the HW writing parts into helper functions.

Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 291 ++++++++++++--------------
 drivers/net/intel/ixgbe/ixgbe_ipsec.h |   1 +
 2 files changed, 137 insertions(+), 155 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +136 -155
index 2d1b3a43ca..c024892e1c 100644
@@ -36,6 +36,85 @@
 	(a).ipv6[2] == (b).ipv6[2] && \
 	(a).ipv6[3] == (b).ipv6[3])
 
+static inline void
+ixgbe_crypto_write_rx_ip(struct ixgbe_hw *hw, uint32_t idx,
+		const struct ipaddr *ip, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_IP | (idx << 3);
+	uint32_t addr[4] = {0};
+
+	if (enable)
+		reg_val |= IPSRXIDX_RX_EN;
+
+	if (ip->type == IPv4)
+		/* only write last 4 bytes */
+		addr[3] = ip->ipv4;
+	else
+		memcpy(addr, ip->ipv6, sizeof(addr));
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), addr[0]);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), addr[1]);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), addr[2]);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), addr[3]);
+	IXGBE_WAIT_RWRITE;
+}
+
+static inline void
+ixgbe_crypto_write_rx_spi(struct ixgbe_hw *hw, uint32_t idx,
+		uint32_t spi, uint32_t ip_idx, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_SPI | (idx << 3);
+
+	if (enable)
+		reg_val |= IPSRXIDX_RX_EN;
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, spi);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, ip_idx);
+	IXGBE_WAIT_RWRITE;
+}
+
+static inline void
+ixgbe_crypto_write_rx_key(struct ixgbe_hw *hw, uint32_t idx,
+		const uint8_t *key, uint32_t salt, uint32_t mode, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_KEY | (idx << 3);
+
+	if (enable)
+		reg_val |= IPSRXIDX_RX_EN;
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[12]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[8]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[4]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[0]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, rte_cpu_to_be_32(salt));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, mode);
+	IXGBE_WAIT_RWRITE;
+}
+
+static inline void
+ixgbe_crypto_write_tx_key(struct ixgbe_hw *hw, uint32_t idx,
+		const uint8_t *key, uint32_t salt, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | (idx << 3);
+
+	if (enable)
+		reg_val |= IPSRXIDX_TX_EN;
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[12]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[8]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[4]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[0]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, rte_cpu_to_be_32(salt));
+	IXGBE_WAIT_TWRITE;
+}
 
 static void
 ixgbe_crypto_clear_ipsec_tables(struct rte_eth_dev *dev)

@@ -43,41 +122,19 @@ ixgbe_crypto_clear_ipsec_tables(struct rte_eth_dev *dev)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ixgbe_ipsec *priv = IXGBE_DEV_PRIVATE_TO_IPSEC(
 				dev->data->dev_private);
+	const struct ipaddr ip = {0};
+	const uint8_t key[16] = {0};
 	int i = 0;
 
 	/* clear Rx IP table*/
-	for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {
-		uint16_t index = i << 3;
-		uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_IP | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), 0);
-		IXGBE_WAIT_RWRITE;
-	}
+	for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++)
+		ixgbe_crypto_write_rx_ip(hw, i, &ip, false);
 
 	/* clear Rx SPI and Rx/Tx SA tables*/
 	for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {
-		uint32_t index = i << 3;
-		uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_SPI | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, 0);
-		IXGBE_WAIT_RWRITE;
-		reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_KEY | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, 0);
-		IXGBE_WAIT_RWRITE;
-		reg_val = IPSRXIDX_WRITE | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, 0);
-		IXGBE_WAIT_TWRITE;
+		ixgbe_crypto_write_rx_spi(hw, i, 0, 0, false);
+		ixgbe_crypto_write_rx_key(hw, i, key, 0, 0, false);
+		ixgbe_crypto_write_tx_key(hw, i, key, 0, false);
 	}
 
 	memset(priv->rx_ip_tbl, 0, sizeof(priv->rx_ip_tbl));

@@ -90,16 +147,16 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 {
 	struct rte_eth_dev_data *dev_data = ic_session->dev_data;
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev_data->dev_private);
-	struct ixgbe_ipsec *priv = IXGBE_DEV_PRIVATE_TO_IPSEC(
-			dev_data->dev_private);
-	uint32_t reg_val;
-	int sa_index = -1;
+	struct ixgbe_ipsec *priv = IXGBE_DEV_PRIVATE_TO_IPSEC(dev_data->dev_private);
+	int i, sa_index = -1;
 	uint8_t key[16] = {0};
 
 	memcpy(key, ic_session->key, ic_session->key_len);
 
 	if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION) {
-		int i, ip_index = -1;
+		struct ixgbe_crypto_rx_ip_table *rxip;
+		struct ixgbe_crypto_rx_sa_table *rxsa;
+		int ip_index = -1, free_index = -1;
 
 		/* Find a match in the IP table*/
 		for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {

@@ -108,22 +165,19 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				ip_index = i;
 				break;
 			}
+			if (free_index == -1 && priv->rx_ip_tbl[i].ref_count == 0)
+				free_index = i;
 		}
 		/* If no match, find a free entry in the IP table*/
-		if (ip_index < 0) {
-			for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {
-				if (priv->rx_ip_tbl[i].ref_count == 0) {
-					ip_index = i;
-					break;
-				}
-			}
-		}
+		if (ip_index < 0)
+			ip_index = free_index;
 
 		/* Fail if no match and no free entries*/
 		if (ip_index < 0) {
 			PMD_DRV_LOG(ERR, "No free entry left in the Rx IP table");
 			return -ENOSPC;
 		}
+		rxip = &priv->rx_ip_tbl[ip_index];
 
 		/* Find a free entry in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {

@@ -137,80 +191,31 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "No free entry left in the Rx SA table");
 			return -ENOSPC;
 		}
+		rxsa = &priv->rx_sa_tbl[sa_index];
 
-		priv->rx_ip_tbl[ip_index].ip.ipv6[0] =
-				ic_session->dst_ip.ipv6[0];
-		priv->rx_ip_tbl[ip_index].ip.ipv6[1] =
-				ic_session->dst_ip.ipv6[1];
-		priv->rx_ip_tbl[ip_index].ip.ipv6[2] =
-				ic_session->dst_ip.ipv6[2];
-		priv->rx_ip_tbl[ip_index].ip.ipv6[3] =
-				ic_session->dst_ip.ipv6[3];
-		priv->rx_ip_tbl[ip_index].ref_count++;
+		rxip->ref_count++;
+		memcpy(&rxip->ip, &ic_session->dst_ip, sizeof(rxip->ip));
 
-		priv->rx_sa_tbl[sa_index].spi =
-			rte_cpu_to_be_32(ic_session->spi);
-		priv->rx_sa_tbl[sa_index].ip_index = ip_index;
-		priv->rx_sa_tbl[sa_index].mode = IPSRXMOD_VALID;
-		if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION)
-			priv->rx_sa_tbl[sa_index].mode |=
-					(IPSRXMOD_PROTO | IPSRXMOD_DECRYPT);
-		if (ic_session->dst_ip.type == IPv6) {
-			priv->rx_sa_tbl[sa_index].mode |= IPSRXMOD_IPV6;
-			priv->rx_ip_tbl[ip_index].ip.type = IPv6;
-		} else if (ic_session->dst_ip.type == IPv4)
-			priv->rx_ip_tbl[ip_index].ip.type = IPv4;
+		rxsa->spi = rte_cpu_to_be_32(ic_session->spi);
+		rxsa->ip_index = ip_index;
+		rxsa->mode = IPSRXMOD_VALID | IPSRXMOD_PROTO | IPSRXMOD_DECRYPT;
+		if (ic_session->dst_ip.type == IPv6)
+			rxsa->mode |= IPSRXMOD_IPV6;
 
-		priv->rx_sa_tbl[sa_index].used = 1;
+		rxsa->used = 1;
 
 		/* write IP table entry*/
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
-				IPSRXIDX_TABLE_IP | (ip_index << 3);
-		if (priv->rx_ip_tbl[ip_index].ip.type == IPv4) {
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3),
-					priv->rx_ip_tbl[ip_index].ip.ipv4);
-		} else {
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[0]);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[1]);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[2]);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[3]);
-		}
-		IXGBE_WAIT_RWRITE;
+		ixgbe_crypto_write_rx_ip(hw, ip_index, &rxip->ip, true);
 
 		/* write SPI table entry*/
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
-				IPSRXIDX_TABLE_SPI | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI,
-				priv->rx_sa_tbl[sa_index].spi);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX,
-				priv->rx_sa_tbl[sa_index].ip_index);
-		IXGBE_WAIT_RWRITE;
+		ixgbe_crypto_write_rx_spi(hw, sa_index, rxsa->spi, ip_index, true);
 
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
-				IPSRXIDX_TABLE_KEY | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0),
-			rte_cpu_to_be_32(*(uint32_t *)&key[12]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1),
-			rte_cpu_to_be_32(*(uint32_t *)&key[8]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2),
-			rte_cpu_to_be_32(*(uint32_t *)&key[4]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3),
-			rte_cpu_to_be_32(*(uint32_t *)&key[0]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT,
-				rte_cpu_to_be_32(ic_session->salt));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD,
-				priv->rx_sa_tbl[sa_index].mode);
-		IXGBE_WAIT_RWRITE;
+		/* write Key table entry*/
+		ixgbe_crypto_write_rx_key(hw, sa_index, key,
+				ic_session->salt, rxsa->mode, true);
 
 	} else { /* sess->dir == RTE_CRYPTO_OUTBOUND */
-		int i;
+		struct ixgbe_crypto_tx_sa_table *txsa;
 
 		/* Find a free entry in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {

@@ -224,25 +229,14 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "No free entry left in the Tx SA table");
 			return -ENOSPC;
 		}
+		txsa = &priv->tx_sa_tbl[sa_index];
 
-		priv->tx_sa_tbl[sa_index].spi =
-			rte_cpu_to_be_32(ic_session->spi);
-		priv->tx_sa_tbl[i].used = 1;
+		txsa->spi = rte_cpu_to_be_32(ic_session->spi);
+		txsa->used = 1;
 		ic_session->sa_index = sa_index;
 
 		/* write Key table entry*/
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0),
-			rte_cpu_to_be_32(*(uint32_t *)&key[12]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1),
-			rte_cpu_to_be_32(*(uint32_t *)&key[8]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2),
-			rte_cpu_to_be_32(*(uint32_t *)&key[4]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3),
-			rte_cpu_to_be_32(*(uint32_t *)&key[0]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT,
-				rte_cpu_to_be_32(ic_session->salt));
-		IXGBE_WAIT_TWRITE;
+		ixgbe_crypto_write_tx_key(hw, sa_index, key, ic_session->salt, true);
 	}
 
 	return 0;

@@ -254,11 +248,13 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(ic_session->dev_data->dev_private);
 	struct ixgbe_ipsec *priv =
 			IXGBE_DEV_PRIVATE_TO_IPSEC(ic_session->dev_data->dev_private);
-	uint32_t reg_val;
-	int sa_index = -1;
+	const uint8_t key[16] = {0};
+	int i, sa_index = -1;
 
 	if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION) {
-		int i, ip_index = -1;
+		struct ixgbe_crypto_rx_ip_table *rxip;
+		struct ixgbe_crypto_rx_sa_table *rxsa;
+		int ip_index = -1;
 
 		/* Find a match in the IP table*/
 		for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {

@@ -273,6 +269,7 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "Entry not found in the Rx IP table");
 			return -ENOENT;
 		}
+		rxip = &priv->rx_ip_tbl[ip_index];
 
 		/* Find a free entry in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {

@@ -287,35 +284,24 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "Entry not found in the Rx SA table");
 			return -ENOENT;
 		}
+		rxsa = &priv->rx_sa_tbl[sa_index];
 
 		/* Disable and clear Rx SPI and key table entries*/
-		reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_SPI | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, 0);
-		IXGBE_WAIT_RWRITE;
-		reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_KEY | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, 0);
-		IXGBE_WAIT_RWRITE;
-		priv->rx_sa_tbl[sa_index].used = 0;
+		ixgbe_crypto_write_rx_spi(hw, sa_index, 0, 0, false);
+		ixgbe_crypto_write_rx_key(hw, sa_index, key, 0, 0, false);
+
+		/* Clear the SA table entry*/
+		*rxsa = (struct ixgbe_crypto_rx_sa_table){0};
 
 		/* If last used then clear the IP table entry*/
-		priv->rx_ip_tbl[ip_index].ref_count--;
-		if (priv->rx_ip_tbl[ip_index].ref_count == 0) {
-			reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_IP |
-					(ip_index << 3);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), 0);
-			IXGBE_WAIT_RWRITE;
+		rxip->ref_count--;
+		if (rxip->ref_count == 0) {
+			const struct ipaddr ip = {0};
+			ixgbe_crypto_write_rx_ip(hw, ip_index, &ip, false);
+			*rxip = (struct ixgbe_crypto_rx_ip_table){0};
 		}
 	} else { /* session->dir == RTE_CRYPTO_OUTBOUND */
-		int i;
+		struct ixgbe_crypto_tx_sa_table *txsa;
 
 		/* Find a match in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {

@@ -330,15 +316,10 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "Entry not found in the Tx SA table");
 			return -ENOENT;
 		}
-		reg_val = IPSRXIDX_WRITE | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, 0);
-		IXGBE_WAIT_TWRITE;
+		txsa = &priv->tx_sa_tbl[sa_index];
 
-		priv->tx_sa_tbl[sa_index].used = 0;
+		ixgbe_crypto_write_tx_key(hw, sa_index, key, 0, false);
+		*txsa = (struct ixgbe_crypto_tx_sa_table){0};
 	}
 
 	return 0;
drivers/net/intel/ixgbe/ixgbe_ipsec.h +1 -0
index 356817c61b..1099b5f598 100644
@@ -12,6 +12,7 @@
 #include <rte_flow.h>
 
 #define IPSRXIDX_RX_EN                                    0x00000001
+#define IPSRXIDX_TX_EN                                    0x00000001
 #define IPSRXIDX_TABLE_IP                                 0x00000002
 #define IPSRXIDX_TABLE_SPI                                0x00000004
 #define IPSRXIDX_TABLE_KEY                                0x00000006
Signature · 1 line
2.52.0
#7 lore raw
In reply to Anatoly Burakov
Quoted 10 lines · Anatoly Burakov

Small array of fixes and refactors for IXGBE IPsec implementation.

Anatoly Burakov (6):
net/ixgbe: add key length check in IPsec net/ixgbe: remove unnecessary dev pointer net/ixgbe: harmonize return values in IPsec net/ixgbe: get rid of unnecessary malloc in IPsec net/ixgbe: fix missing HW write in IPsec net/ixgbe: refactor IPsec code

Quoted 3 lines
  drivers/net/intel/ixgbe/ixgbe_ipsec.c | 355 ++++++++++++--------------
  drivers/net/intel/ixgbe/ixgbe_ipsec.h |   1 +
  2 files changed, 161 insertions(+), 195 deletions(-)
Series-acked-by Radu Nicolau <[email protected]>
author
#8 lore raw

v2 cover IXGBE IPsec fixes

In reply to Anatoly Burakov

Small array of fixes and refactors for IXGBE IPsec implementation.

v1 -> v2:
- Added rte_memzero_explicit calls on account of AI review
- Added a small patch fixing wrong variable usage
- Reworked patch 7 (was 6) to not perform SPI byteswap until register writes

Anatoly Burakov (7):
net/ixgbe: add key length check in IPsec net/ixgbe: remove unnecessary dev pointer net/ixgbe: harmonize return values in IPsec net/ixgbe: get rid of unnecessary malloc in IPsec net/ixgbe: use correct index variable in IPsec net/ixgbe: fix missing HW write in IPsec net/ixgbe: refactor IPsec code

 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 355 ++++++++++++--------------
 drivers/net/intel/ixgbe/ixgbe_ipsec.h |   1 +
 2 files changed, 163 insertions(+), 193 deletions(-)
Signature · 1 line
2.52.0
author
#9 lore raw

PATCH v2 1/7 net/ixgbe: add key length check in IPsec

The cryptodev API does not check for user parameters being within the bounds of what the driver supports. Currently, the only key size supported by ixgbe is 16 bytes, so that is what we will check for. Add the check.

Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Radu Nicolau <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +6 -0
index bdd34344e8..aaf657af5f 100644
@@ -384,6 +384,12 @@ ixgbe_crypto_create_session(void *device,
 	}
 	aead_xform = &conf->crypto_xform->aead;
 
+	/* Only 16-byte keys are supported. */
+	if (aead_xform->key.length != 16) {
+		PMD_DRV_LOG(ERR, "Unsupported key length %u", aead_xform->key.length);
+		return -ENOTSUP;
+	}
+
 	if (conf->ipsec.direction == RTE_SECURITY_IPSEC_SA_DIR_INGRESS) {
 		if (dev_conf->rxmode.offloads & RTE_ETH_RX_OFFLOAD_SECURITY) {
 			ic_session->op = IXGBE_OP_AUTHENTICATED_DECRYPTION;
Signature · 1 line
2.52.0
author
#10 lore raw

PATCH v2 2/7 net/ixgbe: remove unnecessary dev pointer

In reply to Anatoly Burakov

The ic_session already contains a dev_data pointer, so there is no need to pass the dev pointer to ixgbe_crypto_remove_sa.

Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Radu Nicolau <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +4 -5
index aaf657af5f..e2ec402b52 100644
@@ -268,12 +268,11 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 }
 
 static int
-ixgbe_crypto_remove_sa(struct rte_eth_dev *dev,
-		       struct ixgbe_crypto_session *ic_session)
+ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 {
-	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(ic_session->dev_data->dev_private);
 	struct ixgbe_ipsec *priv =
-			IXGBE_DEV_PRIVATE_TO_IPSEC(dev->data->dev_private);
+			IXGBE_DEV_PRIVATE_TO_IPSEC(ic_session->dev_data->dev_private);
 	uint32_t reg_val;
 	int sa_index = -1;
 

@@ -441,7 +440,7 @@ ixgbe_crypto_remove_session(void *device,
 		return -ENODEV;
 	}
 
-	if (ixgbe_crypto_remove_sa(eth_dev, ic_session)) {
+	if (ixgbe_crypto_remove_sa(ic_session)) {
 		PMD_DRV_LOG(ERR, "Failed to remove session");
 		return -EFAULT;
 	}
Signature · 1 line
2.52.0
author
#11 lore raw

PATCH v2 3/7 net/ixgbe: harmonize return values in IPsec

In reply to Anatoly Burakov

Currently, some paths in add/remove SA code return negative errno values while others just return -1. Harmonize all of them to return appropriate negative errno values.

Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Radu Nicolau <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 30 +++++++++++----------------
 1 file changed, 12 insertions(+), 18 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +12 -18
index e2ec402b52..41fc7551ff 100644
@@ -119,9 +119,8 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 
 		/* Fail if no match and no free entries*/
 		if (ip_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "No free entry left in the Rx IP table");
-			return -1;
+			PMD_DRV_LOG(ERR, "No free entry left in the Rx IP table");
+			return -ENOSPC;
 		}
 
 		/* Find a free entry in the SA table*/

@@ -133,9 +132,8 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no free entries*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "No free entry left in the Rx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "No free entry left in the Rx SA table");
+			return -ENOSPC;
 		}
 
 		priv->rx_ip_tbl[ip_index].ip.ipv6[0] =

@@ -231,9 +229,8 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no free entries*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "No free entry left in the Tx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "No free entry left in the Tx SA table");
+			return -ENOSPC;
 		}
 
 		priv->tx_sa_tbl[sa_index].spi =

@@ -289,9 +286,8 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 
 		/* Fail if no match*/
 		if (ip_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "Entry not found in the Rx IP table");
-			return -1;
+			PMD_DRV_LOG(ERR, "Entry not found in the Rx IP table");
+			return -ENOENT;
 		}
 
 		/* Find a free entry in the SA table*/

@@ -304,9 +300,8 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no match*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "Entry not found in the Rx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "Entry not found in the Rx SA table");
+			return -ENOENT;
 		}
 
 		/* Disable and clear Rx SPI and key table entries*/

@@ -347,9 +342,8 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 		}
 		/* Fail if no match entries*/
 		if (sa_index < 0) {
-			PMD_DRV_LOG(ERR,
-				    "Entry not found in the Tx SA table");
-			return -1;
+			PMD_DRV_LOG(ERR, "Entry not found in the Tx SA table");
+			return -ENOENT;
 		}
 		reg_val = IPSRXIDX_WRITE | (sa_index << 3);
 		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0), 0);
Signature · 1 line
2.52.0
author
#12 lore raw

PATCH v2 4/7 net/ixgbe: get rid of unnecessary malloc in IPsec

In reply to Anatoly Burakov

Currently, when writing SA key to HW, a temporary buffer is allocated, to which the key is written. The allocation does not have to happen on the heap, so change the allocation to be on the stack, as key size is known at compile time and is pretty tiny. Also, zero it out explicitly after use.

Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Radu Nicolau <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +3 -13
index 41fc7551ff..9c4937fcba 100644
@@ -94,10 +94,10 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 			dev_data->dev_private);
 	uint32_t reg_val;
 	int sa_index = -1;
+	uint8_t key[16] = {0};
 
 	if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION) {
 		int i, ip_index = -1;
-		uint8_t *key;
 
 		/* Find a match in the IP table*/
 		for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {

@@ -191,11 +191,6 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				priv->rx_sa_tbl[sa_index].ip_index);
 		IXGBE_WAIT_RWRITE;
 
-		/* write Key table entry*/
-		key = malloc(ic_session->key_len);
-		if (!key)
-			return -ENOMEM;
-
 		memcpy(key, ic_session->key, ic_session->key_len);
 
 		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |

@@ -214,10 +209,9 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				priv->rx_sa_tbl[sa_index].mode);
 		IXGBE_WAIT_RWRITE;
 
-		free(key);
+		rte_memzero_explicit(key, sizeof(key));
 
 	} else { /* sess->dir == RTE_CRYPTO_OUTBOUND */
-		uint8_t *key;
 		int i;
 
 		/* Find a free entry in the SA table*/

@@ -238,10 +232,6 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 		priv->tx_sa_tbl[i].used = 1;
 		ic_session->sa_index = sa_index;
 
-		key = malloc(ic_session->key_len);
-		if (!key)
-			return -ENOMEM;
-
 		memcpy(key, ic_session->key, ic_session->key_len);
 
 		/* write Key table entry*/

@@ -258,7 +248,7 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				rte_cpu_to_be_32(ic_session->salt));
 		IXGBE_WAIT_TWRITE;
 
-		free(key);
+		rte_memzero_explicit(key, sizeof(key));
 	}
 
 	return 0;
Signature · 1 line
2.52.0
author
#13 lore raw

PATCH v2 5/7 net/ixgbe: use correct index variable in IPsec

In reply to Anatoly Burakov

Currently, one of the Tx SA table accesses uses i iterator instead of the sa_index variable that was assigned from said iterator. While this is technically not a bug because i and sa_index always carry the same value, it should still be fixed for readability purposes.

Signed-off-by Anatoly Burakov <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +1 -1
index 9c4937fcba..f026a41690 100644
@@ -229,7 +229,7 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 
 		priv->tx_sa_tbl[sa_index].spi =
 			rte_cpu_to_be_32(ic_session->spi);
-		priv->tx_sa_tbl[i].used = 1;
+		priv->tx_sa_tbl[sa_index].used = 1;
 		ic_session->sa_index = sa_index;
 
 		memcpy(key, ic_session->key, ic_session->key_len);
Signature · 1 line
2.52.0
author
#15 lore raw

PATCH v2 6/7 net/ixgbe: fix missing HW write in IPsec

In reply to Anatoly Burakov

When removing an SA and finding ourselves with zero refcount on an IP table entry, we zero out the IP table registers, but do not write them. Add the missing write.

Fixes 9a0752f498d2 ("net/ixgbe: enable inline IPsec")
Cc [email protected]
Cc [email protected]
Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Radu Nicolau <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 1 +
 1 file changed, 1 insertion(+)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +1 -0
index f026a41690..4fe4dc9ba0 100644
@@ -318,6 +318,7 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
 			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
 			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), 0);
+			IXGBE_WAIT_RWRITE;
 		}
 	} else { /* session->dir == RTE_CRYPTO_OUTBOUND */
 		int i;
Signature · 1 line
2.52.0
author
#16 lore raw

PATCH v2 7/7 net/ixgbe: refactor IPsec code

In reply to Anatoly Burakov

Currently, the IPsec code mixes SW bookkeeping with HW writes quite a lot, which results in difficulty extending the code, lots of duplication, and potential for bugs. Refactor the code to be more readable by separating out the HW writing parts into helper functions.

Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Radu Nicolau <[email protected]>
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 297 ++++++++++++--------------
 drivers/net/intel/ixgbe/ixgbe_ipsec.h |   1 +
 2 files changed, 139 insertions(+), 159 deletions(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +138 -159
index 4fe4dc9ba0..3c35326016 100644
@@ -36,6 +36,85 @@
 	(a).ipv6[2] == (b).ipv6[2] && \
 	(a).ipv6[3] == (b).ipv6[3])
 
+static inline void
+ixgbe_crypto_write_rx_ip(struct ixgbe_hw *hw, uint32_t idx,
+		const struct ipaddr *ip, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_IP | (idx << 3);
+	uint32_t addr[4] = {0};
+
+	if (enable)
+		reg_val |= IPSRXIDX_RX_EN;
+
+	if (ip->type == IPv4)
+		/* only write last 4 bytes */
+		addr[3] = ip->ipv4;
+	else
+		memcpy(addr, ip->ipv6, sizeof(addr));
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), addr[0]);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), addr[1]);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), addr[2]);
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), addr[3]);
+	IXGBE_WAIT_RWRITE;
+}
+
+static inline void
+ixgbe_crypto_write_rx_spi(struct ixgbe_hw *hw, uint32_t idx,
+		uint32_t spi, uint32_t ip_idx, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_SPI | (idx << 3);
+
+	if (enable)
+		reg_val |= IPSRXIDX_RX_EN;
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, rte_cpu_to_be_32(spi));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, ip_idx);
+	IXGBE_WAIT_RWRITE;
+}
+
+static inline void
+ixgbe_crypto_write_rx_key(struct ixgbe_hw *hw, uint32_t idx,
+		const uint8_t *key, uint32_t salt, uint32_t mode, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_KEY | (idx << 3);
+
+	if (enable)
+		reg_val |= IPSRXIDX_RX_EN;
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[12]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[8]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[4]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[0]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, rte_cpu_to_be_32(salt));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, mode);
+	IXGBE_WAIT_RWRITE;
+}
+
+static inline void
+ixgbe_crypto_write_tx_key(struct ixgbe_hw *hw, uint32_t idx,
+		const uint8_t *key, uint32_t salt, bool enable)
+{
+	uint32_t reg_val = IPSRXIDX_WRITE | (idx << 3);
+
+	if (enable)
+		reg_val |= IPSRXIDX_TX_EN;
+
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[12]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[8]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[4]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3),
+		rte_cpu_to_be_32(*(const uint32_t *)&key[0]));
+	IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, rte_cpu_to_be_32(salt));
+	IXGBE_WAIT_TWRITE;
+}
 
 static void
 ixgbe_crypto_clear_ipsec_tables(struct rte_eth_dev *dev)

@@ -43,41 +122,19 @@ ixgbe_crypto_clear_ipsec_tables(struct rte_eth_dev *dev)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct ixgbe_ipsec *priv = IXGBE_DEV_PRIVATE_TO_IPSEC(
 				dev->data->dev_private);
+	const struct ipaddr ip = {0};
+	const uint8_t key[16] = {0};
 	int i = 0;
 
 	/* clear Rx IP table*/
-	for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {
-		uint16_t index = i << 3;
-		uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_IP | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), 0);
-		IXGBE_WAIT_RWRITE;
-	}
+	for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++)
+		ixgbe_crypto_write_rx_ip(hw, i, &ip, false);
 
 	/* clear Rx SPI and Rx/Tx SA tables*/
 	for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {
-		uint32_t index = i << 3;
-		uint32_t reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_SPI | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, 0);
-		IXGBE_WAIT_RWRITE;
-		reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_KEY | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, 0);
-		IXGBE_WAIT_RWRITE;
-		reg_val = IPSRXIDX_WRITE | index;
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, 0);
-		IXGBE_WAIT_TWRITE;
+		ixgbe_crypto_write_rx_spi(hw, i, 0, 0, false);
+		ixgbe_crypto_write_rx_key(hw, i, key, 0, 0, false);
+		ixgbe_crypto_write_tx_key(hw, i, key, 0, false);
 	}
 
 	memset(priv->rx_ip_tbl, 0, sizeof(priv->rx_ip_tbl));

@@ -90,14 +147,14 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 {
 	struct rte_eth_dev_data *dev_data = ic_session->dev_data;
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev_data->dev_private);
-	struct ixgbe_ipsec *priv = IXGBE_DEV_PRIVATE_TO_IPSEC(
-			dev_data->dev_private);
-	uint32_t reg_val;
-	int sa_index = -1;
+	struct ixgbe_ipsec *priv = IXGBE_DEV_PRIVATE_TO_IPSEC(dev_data->dev_private);
+	int i, sa_index = -1;
 	uint8_t key[16] = {0};
 
 	if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION) {
-		int i, ip_index = -1;
+		struct ixgbe_crypto_rx_ip_table *rxip;
+		struct ixgbe_crypto_rx_sa_table *rxsa;
+		int ip_index = -1, free_index = -1;
 
 		/* Find a match in the IP table*/
 		for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {

@@ -106,22 +163,19 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 				ip_index = i;
 				break;
 			}
+			if (free_index == -1 && priv->rx_ip_tbl[i].ref_count == 0)
+				free_index = i;
 		}
 		/* If no match, find a free entry in the IP table*/
-		if (ip_index < 0) {
-			for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {
-				if (priv->rx_ip_tbl[i].ref_count == 0) {
-					ip_index = i;
-					break;
-				}
-			}
-		}
+		if (ip_index < 0)
+			ip_index = free_index;
 
 		/* Fail if no match and no free entries*/
 		if (ip_index < 0) {
 			PMD_DRV_LOG(ERR, "No free entry left in the Rx IP table");
 			return -ENOSPC;
 		}
+		rxip = &priv->rx_ip_tbl[ip_index];
 
 		/* Find a free entry in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {

@@ -135,84 +189,35 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "No free entry left in the Rx SA table");
 			return -ENOSPC;
 		}
+		rxsa = &priv->rx_sa_tbl[sa_index];
 
-		priv->rx_ip_tbl[ip_index].ip.ipv6[0] =
-				ic_session->dst_ip.ipv6[0];
-		priv->rx_ip_tbl[ip_index].ip.ipv6[1] =
-				ic_session->dst_ip.ipv6[1];
-		priv->rx_ip_tbl[ip_index].ip.ipv6[2] =
-				ic_session->dst_ip.ipv6[2];
-		priv->rx_ip_tbl[ip_index].ip.ipv6[3] =
-				ic_session->dst_ip.ipv6[3];
-		priv->rx_ip_tbl[ip_index].ref_count++;
+		rxip->ref_count++;
+		memcpy(&rxip->ip, &ic_session->dst_ip, sizeof(rxip->ip));
 
-		priv->rx_sa_tbl[sa_index].spi =
-			rte_cpu_to_be_32(ic_session->spi);
-		priv->rx_sa_tbl[sa_index].ip_index = ip_index;
-		priv->rx_sa_tbl[sa_index].mode = IPSRXMOD_VALID;
-		if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION)
-			priv->rx_sa_tbl[sa_index].mode |=
-					(IPSRXMOD_PROTO | IPSRXMOD_DECRYPT);
-		if (ic_session->dst_ip.type == IPv6) {
-			priv->rx_sa_tbl[sa_index].mode |= IPSRXMOD_IPV6;
-			priv->rx_ip_tbl[ip_index].ip.type = IPv6;
-		} else if (ic_session->dst_ip.type == IPv4)
-			priv->rx_ip_tbl[ip_index].ip.type = IPv4;
+		rxsa->spi = ic_session->spi;
+		rxsa->ip_index = ip_index;
+		rxsa->mode = IPSRXMOD_VALID | IPSRXMOD_PROTO | IPSRXMOD_DECRYPT;
+		if (ic_session->dst_ip.type == IPv6)
+			rxsa->mode |= IPSRXMOD_IPV6;
 
-		priv->rx_sa_tbl[sa_index].used = 1;
+		rxsa->used = 1;
 
 		/* write IP table entry*/
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
-				IPSRXIDX_TABLE_IP | (ip_index << 3);
-		if (priv->rx_ip_tbl[ip_index].ip.type == IPv4) {
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3),
-					priv->rx_ip_tbl[ip_index].ip.ipv4);
-		} else {
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[0]);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[1]);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[2]);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3),
-					priv->rx_ip_tbl[ip_index].ip.ipv6[3]);
-		}
-		IXGBE_WAIT_RWRITE;
+		ixgbe_crypto_write_rx_ip(hw, ip_index, &rxip->ip, true);
 
 		/* write SPI table entry*/
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
-				IPSRXIDX_TABLE_SPI | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI,
-				priv->rx_sa_tbl[sa_index].spi);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX,
-				priv->rx_sa_tbl[sa_index].ip_index);
-		IXGBE_WAIT_RWRITE;
+		ixgbe_crypto_write_rx_spi(hw, sa_index, rxsa->spi, ip_index, true);
 
+		/* write Key table entry*/
 		memcpy(key, ic_session->key, ic_session->key_len);
 
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE |
-				IPSRXIDX_TABLE_KEY | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0),
-			rte_cpu_to_be_32(*(uint32_t *)&key[12]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1),
-			rte_cpu_to_be_32(*(uint32_t *)&key[8]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2),
-			rte_cpu_to_be_32(*(uint32_t *)&key[4]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3),
-			rte_cpu_to_be_32(*(uint32_t *)&key[0]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT,
-				rte_cpu_to_be_32(ic_session->salt));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD,
-				priv->rx_sa_tbl[sa_index].mode);
-		IXGBE_WAIT_RWRITE;
+		ixgbe_crypto_write_rx_key(hw, sa_index, key,
+				ic_session->salt, rxsa->mode, true);
 
 		rte_memzero_explicit(key, sizeof(key));
 
 	} else { /* sess->dir == RTE_CRYPTO_OUTBOUND */
-		int i;
+		struct ixgbe_crypto_tx_sa_table *txsa;
 
 		/* Find a free entry in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {

@@ -226,27 +231,16 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "No free entry left in the Tx SA table");
 			return -ENOSPC;
 		}
+		txsa = &priv->tx_sa_tbl[sa_index];
 
-		priv->tx_sa_tbl[sa_index].spi =
-			rte_cpu_to_be_32(ic_session->spi);
-		priv->tx_sa_tbl[sa_index].used = 1;
+		txsa->spi = ic_session->spi;
+		txsa->used = 1;
 		ic_session->sa_index = sa_index;
 
 		memcpy(key, ic_session->key, ic_session->key_len);
 
 		/* write Key table entry*/
-		reg_val = IPSRXIDX_RX_EN | IPSRXIDX_WRITE | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0),
-			rte_cpu_to_be_32(*(uint32_t *)&key[12]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1),
-			rte_cpu_to_be_32(*(uint32_t *)&key[8]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2),
-			rte_cpu_to_be_32(*(uint32_t *)&key[4]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3),
-			rte_cpu_to_be_32(*(uint32_t *)&key[0]));
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT,
-				rte_cpu_to_be_32(ic_session->salt));
-		IXGBE_WAIT_TWRITE;
+		ixgbe_crypto_write_tx_key(hw, sa_index, key, ic_session->salt, true);
 
 		rte_memzero_explicit(key, sizeof(key));
 	}

@@ -260,11 +254,13 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(ic_session->dev_data->dev_private);
 	struct ixgbe_ipsec *priv =
 			IXGBE_DEV_PRIVATE_TO_IPSEC(ic_session->dev_data->dev_private);
-	uint32_t reg_val;
-	int sa_index = -1;
+	const uint8_t key[16] = {0};
+	int i, sa_index = -1;
 
 	if (ic_session->op == IXGBE_OP_AUTHENTICATED_DECRYPTION) {
-		int i, ip_index = -1;
+		struct ixgbe_crypto_rx_ip_table *rxip;
+		struct ixgbe_crypto_rx_sa_table *rxsa;
+		int ip_index = -1;
 
 		/* Find a match in the IP table*/
 		for (i = 0; i < IPSEC_MAX_RX_IP_COUNT; i++) {

@@ -279,11 +275,11 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "Entry not found in the Rx IP table");
 			return -ENOENT;
 		}
+		rxip = &priv->rx_ip_tbl[ip_index];
 
 		/* Find a free entry in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {
-			if (priv->rx_sa_tbl[i].spi ==
-				  rte_cpu_to_be_32(ic_session->spi)) {
+			if (priv->rx_sa_tbl[i].spi == ic_session->spi) {
 				sa_index = i;
 				break;
 			}

@@ -293,40 +289,28 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "Entry not found in the Rx SA table");
 			return -ENOENT;
 		}
+		rxsa = &priv->rx_sa_tbl[sa_index];
 
 		/* Disable and clear Rx SPI and key table entries*/
-		reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_SPI | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSPI, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPIDX, 0);
-		IXGBE_WAIT_RWRITE;
-		reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_KEY | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXSALT, 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSRXMOD, 0);
-		IXGBE_WAIT_RWRITE;
-		priv->rx_sa_tbl[sa_index].used = 0;
+		ixgbe_crypto_write_rx_spi(hw, sa_index, 0, 0, false);
+		ixgbe_crypto_write_rx_key(hw, sa_index, key, 0, 0, false);
+
+		/* Clear the SA table entry*/
+		*rxsa = (struct ixgbe_crypto_rx_sa_table){0};
 
 		/* If last used then clear the IP table entry*/
-		priv->rx_ip_tbl[ip_index].ref_count--;
-		if (priv->rx_ip_tbl[ip_index].ref_count == 0) {
-			reg_val = IPSRXIDX_WRITE | IPSRXIDX_TABLE_IP |
-					(ip_index << 3);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(0), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(1), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(2), 0);
-			IXGBE_WRITE_REG(hw, IXGBE_IPSRXIPADDR(3), 0);
-			IXGBE_WAIT_RWRITE;
+		rxip->ref_count--;
+		if (rxip->ref_count == 0) {
+			const struct ipaddr ip = {0};
+			ixgbe_crypto_write_rx_ip(hw, ip_index, &ip, false);
+			*rxip = (struct ixgbe_crypto_rx_ip_table){0};
 		}
 	} else { /* session->dir == RTE_CRYPTO_OUTBOUND */
-		int i;
+		struct ixgbe_crypto_tx_sa_table *txsa;
 
 		/* Find a match in the SA table*/
 		for (i = 0; i < IPSEC_MAX_SA_COUNT; i++) {
-			if (priv->tx_sa_tbl[i].spi ==
-				    rte_cpu_to_be_32(ic_session->spi)) {
+			if (priv->tx_sa_tbl[i].spi == ic_session->spi) {
 				sa_index = i;
 				break;
 			}

@@ -336,15 +320,10 @@ ixgbe_crypto_remove_sa(struct ixgbe_crypto_session *ic_session)
 			PMD_DRV_LOG(ERR, "Entry not found in the Tx SA table");
 			return -ENOENT;
 		}
-		reg_val = IPSRXIDX_WRITE | (sa_index << 3);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(0), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(1), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(2), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXKEY(3), 0);
-		IXGBE_WRITE_REG(hw, IXGBE_IPSTXSALT, 0);
-		IXGBE_WAIT_TWRITE;
+		txsa = &priv->tx_sa_tbl[sa_index];
 
-		priv->tx_sa_tbl[sa_index].used = 0;
+		ixgbe_crypto_write_tx_key(hw, sa_index, key, 0, false);
+		*txsa = (struct ixgbe_crypto_tx_sa_table){0};
 	}
 
 	return 0;
drivers/net/intel/ixgbe/ixgbe_ipsec.h +1 -0
index 356817c61b..1099b5f598 100644
@@ -12,6 +12,7 @@
 #include <rte_flow.h>
 
 #define IPSRXIDX_RX_EN                                    0x00000001
+#define IPSRXIDX_TX_EN                                    0x00000001
 #define IPSRXIDX_TABLE_IP                                 0x00000002
 #define IPSRXIDX_TABLE_SPI                                0x00000004
 #define IPSRXIDX_TABLE_KEY                                0x00000006
Signature · 1 line
2.52.0
Acked-by on v2 5/7
#14 lore raw

Re: [PATCH v2 5/7] net/ixgbe: use correct index variable in IPsec

In reply to Anatoly Burakov
Quoted 6 lines · Anatoly Burakov

Currently, one of the Tx SA table accesses uses i iterator instead of the sa_index variable that was assigned from said iterator. While this is technically not a bug because i and sa_index always carry the same value, it should still be fixed for readability purposes.

Signed-off-by Anatoly Burakov <[email protected]>
Acked-by Bruce Richardson <[email protected]>
Quoted 20 lines
---
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/intel/ixgbe/ixgbe_ipsec.c +1 -1
index 9c4937fcba..f026a41690 100644
@@ -229,7 +229,7 @@ ixgbe_crypto_add_sa(struct ixgbe_crypto_session *ic_session)
 
 		priv->tx_sa_tbl[sa_index].spi =
 			rte_cpu_to_be_32(ic_session->spi);
-		priv->tx_sa_tbl[i].used = 1;
+		priv->tx_sa_tbl[sa_index].used = 1;
 		ic_session->sa_index = sa_index;
 
 		memcpy(key, ic_session->key, ic_session->key_len);
Signature · 1 line
2.52.0
#17 lore raw
In reply to Anatoly Burakov
Quoted 16 lines · Anatoly Burakov

Small array of fixes and refactors for IXGBE IPsec implementation.

v1 -> v2:
- Added rte_memzero_explicit calls on account of AI review
- Added a small patch fixing wrong variable usage
- Reworked patch 7 (was 6) to not perform SPI byteswap until register writes

Anatoly Burakov (7):
net/ixgbe: add key length check in IPsec net/ixgbe: remove unnecessary dev pointer net/ixgbe: harmonize return values in IPsec net/ixgbe: get rid of unnecessary malloc in IPsec net/ixgbe: use correct index variable in IPsec net/ixgbe: fix missing HW write in IPsec net/ixgbe: refactor IPsec code

Quoted 4 lines
 drivers/net/intel/ixgbe/ixgbe_ipsec.c | 355 ++++++++++++--------------
 drivers/net/intel/ixgbe/ixgbe_ipsec.h |   1 +
 2 files changed, 163 insertions(+), 193 deletions(-)

Series applied to next-net-intel.
Thanks,
/Bruce