[PATCH 0/2] Fix a doorbell race on Cortex-A78AE

[PATCH 0/2] Fix a doorbell race on Cortex-A78AE

[Shani Peretz]

Fix a doorbell race on Cortex-A78AE where the compiler reorders a
64-bit WQE load before the 32-bit stores that construct it, causing
stale data to reach the hardware doorbell.
The first patch fixes the root cause by passing a pointer and adding
a compiler barrier;
The second patch reverts the crypto march extension workaround that is
no longer needed and that broke builds without AES support.

Shani Peretz (2):
  common/mlx5: fix doorbell race from compiler reordering
  config/arm: revert forced crypto extension for Cortex-A78AE

 config/arm/meson.build                   |  7 ++-----
 drivers/common/mlx5/mlx5_common.h        | 12 ++++++++----
 drivers/compress/mlx5/mlx5_compress.c    |  2 +-
 drivers/crypto/mlx5/mlx5_crypto_gcm.c    |  6 +++---
 drivers/crypto/mlx5/mlx5_crypto_xts.c    |  2 +-
 drivers/net/mlx5/mlx5_flow_aso.c         | 12 ++++++------
 drivers/net/mlx5/mlx5_flow_quota.c       |  2 +-
 drivers/net/mlx5/mlx5_rxq.c              |  3 ++-
 drivers/net/mlx5/mlx5_tx.h               |  4 ++--
 drivers/net/mlx5/mlx5_txpp.c             |  4 ++--
 drivers/regex/mlx5/mlx5_regex_fastpath.c |  2 +-
 drivers/vdpa/mlx5/mlx5_vdpa_event.c      |  2 +-
 12 files changed, 30 insertions(+), 28 deletions(-)

--
2.34.1

[PATCH 1/2] common/mlx5: fix doorbell race from compiler reordering

[Shani Peretz]

The first 8 bytes of a WQE are written to the hardware doorbell.
When compiling with -mcpu=cortex-a78ae, the compiler reorders the
64-bit load of these bytes before the 32-bit stores that construct them.
This happens because mlx5_doorbell_ring() receives the WQE value
by-value as a function argument, so the compiler evaluates the load
at the call site. The A78AE-specific instruction scheduler then hoists
this load above the WQE construction stores, as it fails to detect
the memory alias between the addresses used for the store and load.

The result is that stale data is written to the hardware doorbell,
corrupting the WQE and causing an infinite loop in the crypto
datapath.

Fix this by changing mlx5_doorbell_ring() to accept a pointer to the
WQE value instead of the value itself. A compiler barrier
(rte_compiler_barrier) is placed before the dereference to ensure all
WQE construction stores are visible before the value is read.

Fixes: 5dfa003db53f ("common/mlx5: fix post doorbell barrier")
Cc: stable@dpdk.org

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
Acked-by: Bing Zhao <bingz@nvidia.com>
---
 drivers/common/mlx5/mlx5_common.h        | 12 ++++++++----
 drivers/compress/mlx5/mlx5_compress.c    |  2 +-
 drivers/crypto/mlx5/mlx5_crypto_gcm.c    |  6 +++---
 drivers/crypto/mlx5/mlx5_crypto_xts.c    |  2 +-
 drivers/net/mlx5/mlx5_flow_aso.c         | 12 ++++++------
 drivers/net/mlx5/mlx5_flow_quota.c       |  2 +-
 drivers/net/mlx5/mlx5_rxq.c              |  3 ++-
 drivers/net/mlx5/mlx5_tx.h               |  4 ++--
 drivers/net/mlx5/mlx5_txpp.c             |  4 ++--
 drivers/regex/mlx5/mlx5_regex_fastpath.c |  2 +-
 drivers/vdpa/mlx5/mlx5_vdpa_event.c      |  2 +-
 11 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_common.h b/drivers/common/mlx5/mlx5_common.h
index 47fcc3fb06..7e8d6349e4 100644
--- a/drivers/common/mlx5/mlx5_common.h
+++ b/drivers/common/mlx5/mlx5_common.h
@@ -420,8 +420,8 @@ struct mlx5_uar {
  *
  * @param uar
  *   Pointer to UAR data structure.
- * @param val
- *   value to write in big endian format.
+ * @param wqe
+ *   Pointer to the first 64 bits of the WQE to write in big endian format.
  * @param index
  *   Index of doorbell record.
  * @param db_rec
@@ -430,9 +430,13 @@ struct mlx5_uar {
  *   Decide whether to flush the DB writing using a memory barrier.
  */
 static __rte_always_inline void
-mlx5_doorbell_ring(struct mlx5_uar_data *uar, uint64_t val, uint32_t index,
-		   volatile uint32_t *db_rec, bool flash)
+mlx5_doorbell_ring(struct mlx5_uar_data *uar, volatile uint64_t *wqe,
+		   uint32_t index, volatile uint32_t *db_rec, bool flash)
 {
+	uint64_t val;
+
+	rte_compiler_barrier();
+	val = *wqe;
 	rte_io_wmb();
 	*db_rec = rte_cpu_to_be_32(index);
 	/* Ensure ordering between DB record actual update and UAR access. */
diff --git a/drivers/compress/mlx5/mlx5_compress.c b/drivers/compress/mlx5/mlx5_compress.c
index e5325c6150..fa687b0eec 100644
--- a/drivers/compress/mlx5/mlx5_compress.c
+++ b/drivers/compress/mlx5/mlx5_compress.c
@@ -589,7 +589,7 @@ mlx5_compress_enqueue_burst(void *queue_pair, struct rte_comp_op **ops,
 		qp->pi++;
 	} while (--remain);
 	qp->stats.enqueued_count += nb_ops;
-	mlx5_doorbell_ring(&qp->priv->uar.bf_db, *(volatile uint64_t *)wqe,
+	mlx5_doorbell_ring(&qp->priv->uar.bf_db, (volatile uint64_t *)wqe,
 			   qp->pi, &qp->qp.db_rec[MLX5_SND_DBR],
 			   !qp->priv->uar.dbnc);
 	return nb_ops;
diff --git a/drivers/crypto/mlx5/mlx5_crypto_gcm.c b/drivers/crypto/mlx5/mlx5_crypto_gcm.c
index 89f32c7722..8875f9b71c 100644
--- a/drivers/crypto/mlx5/mlx5_crypto_gcm.c
+++ b/drivers/crypto/mlx5/mlx5_crypto_gcm.c
@@ -858,12 +858,12 @@ mlx5_crypto_gcm_enqueue_burst(void *queue_pair,
 		RTE_BE32(MLX5_COMP_ALWAYS << MLX5_COMP_MODE_OFFSET);
 	/* Only when there are no pending SEND_EN WQEs in background. */
 	if (!umr_cnt && !qp->has_umr) {
-		mlx5_doorbell_ring(&priv->uar.bf_db, *(volatile uint64_t *)qp->wqe,
+		mlx5_doorbell_ring(&priv->uar.bf_db, (volatile uint64_t *)qp->wqe,
 				   qp->pi, &qp->qp_obj.db_rec[MLX5_SND_DBR],
 				   !priv->uar.dbnc);
 	} else {
 		mlx5_crypto_gcm_build_send_en(qp);
-		mlx5_doorbell_ring(&priv->uar.bf_db, *(volatile uint64_t *)qp->umr_wqe,
+		mlx5_doorbell_ring(&priv->uar.bf_db, (volatile uint64_t *)qp->umr_wqe,
 				   qp->umr_pi, &qp->umr_qp_obj.db_rec[MLX5_SND_DBR],
 				   !priv->uar.dbnc);
 		qp->last_gga_pi = qp->pi;
@@ -1078,7 +1078,7 @@ mlx5_crypto_gcm_ipsec_enqueue_burst(void *queue_pair,
 	/* Update the last GGA cseg with COMP. */
 	((struct mlx5_wqe_cseg *)qp->wqe)->flags =
 		RTE_BE32(MLX5_COMP_ALWAYS << MLX5_COMP_MODE_OFFSET);
-	mlx5_doorbell_ring(&priv->uar.bf_db, *(volatile uint64_t *)qp->wqe,
+	mlx5_doorbell_ring(&priv->uar.bf_db, (volatile uint64_t *)qp->wqe,
 			   qp->pi, &qp->qp_obj.db_rec[MLX5_SND_DBR],
 			   !priv->uar.dbnc);
 	return nb_ops;
diff --git a/drivers/crypto/mlx5/mlx5_crypto_xts.c b/drivers/crypto/mlx5/mlx5_crypto_xts.c
index 1c914caa85..e62641d154 100644
--- a/drivers/crypto/mlx5/mlx5_crypto_xts.c
+++ b/drivers/crypto/mlx5/mlx5_crypto_xts.c
@@ -353,7 +353,7 @@ mlx5_crypto_xts_enqueue_burst(void *queue_pair, struct rte_crypto_op **ops,
 		qp->pi++;
 	} while (--remain);
 	qp->stats.enqueued_count += nb_ops;
-	mlx5_doorbell_ring(&priv->uar.bf_db, *(volatile uint64_t *)qp->wqe,
+	mlx5_doorbell_ring(&priv->uar.bf_db, (volatile uint64_t *)qp->wqe,
 			   qp->db_pi, &qp->qp_obj.db_rec[MLX5_SND_DBR],
 			   !priv->uar.dbnc);
 	return nb_ops;
diff --git a/drivers/net/mlx5/mlx5_flow_aso.c b/drivers/net/mlx5/mlx5_flow_aso.c
index 5e2a81ef9c..3091cec411 100644
--- a/drivers/net/mlx5/mlx5_flow_aso.c
+++ b/drivers/net/mlx5/mlx5_flow_aso.c
@@ -469,7 +469,7 @@ mlx5_aso_sq_enqueue_burst(struct mlx5_dev_ctx_shared *sh, uint16_t n)
 	} while (max);
 	wqe->general_cseg.flags = RTE_BE32(MLX5_COMP_ALWAYS <<
 							 MLX5_COMP_MODE_OFFSET);
-	mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)wqe,
+	mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)wqe,
 			   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 			   !sh->tx_uar.dbnc);
 	return sq->elts[start_head & mask].burst_size;
@@ -576,7 +576,7 @@ mlx5_aso_push_wqe(struct mlx5_dev_ctx_shared *sh,
 {
 	if (sq->db_pi == sq->pi)
 		return;
-	mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)sq->db,
+	mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)sq->db,
 			   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 			   !sh->tx_uar.dbnc);
 	sq->db_pi = sq->pi;
@@ -885,7 +885,7 @@ mlx5_aso_mtr_sq_enqueue_single(struct mlx5_dev_ctx_shared *sh,
 	sq->head++;
 	sq->pi += 2;/* Each WQE contains 2 WQEBB's. */
 	if (push) {
-		mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)wqe,
+		mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)wqe,
 			   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 			   !sh->tx_uar.dbnc);
 		sq->db_pi = sq->pi;
@@ -1338,7 +1338,7 @@ mlx5_aso_ct_sq_enqueue_single(struct mlx5_dev_ctx_shared *sh,
 	sq->head++;
 	sq->pi += 2; /* Each WQE contains 2 WQEBB's. */
 	if (push) {
-		mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)wqe,
+		mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)wqe,
 				   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 				   !sh->tx_uar.dbnc);
 		sq->db_pi = sq->pi;
@@ -1470,7 +1470,7 @@ mlx5_aso_ct_sq_query_single(struct mlx5_dev_ctx_shared *sh,
 	 */
 	sq->pi += 2;
 	if (push) {
-		mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)wqe,
+		mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)wqe,
 				   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 				   !sh->tx_uar.dbnc);
 		sq->db_pi = sq->pi;
@@ -1904,7 +1904,7 @@ mlx5_aso_cnt_sq_enqueue_burst(struct mlx5_hws_cnt_pool *cpool,
 	} while (max);
 	wqe->general_cseg.flags = RTE_BE32(MLX5_COMP_ALWAYS <<
 							 MLX5_COMP_MODE_OFFSET);
-	mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)wqe,
+	mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)wqe,
 			   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 			   !sh->tx_uar.dbnc);
 	bursted_cnts = RTE_MIN((uint32_t)(sq->elts[0].burst_size * 4), n);
diff --git a/drivers/net/mlx5/mlx5_flow_quota.c b/drivers/net/mlx5/mlx5_flow_quota.c
index d94167d0b0..72d2c545aa 100644
--- a/drivers/net/mlx5/mlx5_flow_quota.c
+++ b/drivers/net/mlx5/mlx5_flow_quota.c
@@ -312,7 +312,7 @@ mlx5_quota_cmd_wqe(struct rte_eth_dev *dev, struct mlx5_quota *quota_obj,
 	sq->head++;
 	sq->pi += 2; /* Each WQE contains 2 WQEBB */
 	if (push) {
-		mlx5_doorbell_ring(&sh->tx_uar.bf_db, *(volatile uint64_t *)wqe,
+		mlx5_doorbell_ring(&sh->tx_uar.bf_db, (volatile uint64_t *)wqe,
 				   sq->pi, &sq->sq_obj.db_rec[MLX5_SND_DBR],
 				   !sh->tx_uar.dbnc);
 		sq->db_pi = sq->pi;
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 48d982a8c2..5e9bc0e666 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1265,7 +1265,8 @@ mlx5_arm_cq(struct mlx5_rxq_data *rxq, int sq_n_rxq)
 	doorbell_hi = sq_n << MLX5_CQ_SQN_OFFSET | (rxq->cq_ci & MLX5_CI_MASK);
 	doorbell = (uint64_t)doorbell_hi << 32;
 	doorbell |= rxq->cqn;
-	mlx5_doorbell_ring(&rxq->uar_data, rte_cpu_to_be_64(doorbell),
+	doorbell = rte_cpu_to_be_64(doorbell);
+	mlx5_doorbell_ring(&rxq->uar_data, &doorbell,
 			   doorbell_hi, &rxq->cq_db[MLX5_CQ_ARM_DB], 0);
 }
 
diff --git a/drivers/net/mlx5/mlx5_tx.h b/drivers/net/mlx5/mlx5_tx.h
index 0134a2e003..cd35a89ff8 100644
--- a/drivers/net/mlx5/mlx5_tx.h
+++ b/drivers/net/mlx5/mlx5_tx.h
@@ -313,7 +313,7 @@ mlx5_tx_bfreg(struct mlx5_txq_data *txq)
 static __rte_always_inline void
 mlx5_tx_dbrec(struct mlx5_txq_data *txq, volatile struct mlx5_wqe *wqe)
 {
-	mlx5_doorbell_ring(mlx5_tx_bfreg(txq), *(volatile uint64_t *)wqe,
+	mlx5_doorbell_ring(mlx5_tx_bfreg(txq), (volatile uint64_t *)wqe,
 			   txq->wqe_ci, txq->qp_db, 1);
 }
 
@@ -3796,7 +3796,7 @@ mlx5_tx_burst_tmpl(struct mlx5_txq_data *__rte_restrict txq,
 	 *   the next burst (after descriptor writing, at least).
 	 */
 	mlx5_doorbell_ring(mlx5_tx_bfreg(txq),
-			   *(volatile uint64_t *)loc.wqe_last, txq->wqe_ci,
+			   (volatile uint64_t *)loc.wqe_last, txq->wqe_ci,
 			   txq->qp_db, !txq->db_nc &&
 			   (!txq->db_heu || pkts_n % MLX5_TX_DEFAULT_BURST));
 	/* Not all of the mbufs may be stored into elts yet. */
diff --git a/drivers/net/mlx5/mlx5_txpp.c b/drivers/net/mlx5/mlx5_txpp.c
index 0e99b58bde..ef94a5a7d5 100644
--- a/drivers/net/mlx5/mlx5_txpp.c
+++ b/drivers/net/mlx5/mlx5_txpp.c
@@ -171,7 +171,7 @@ mlx5_txpp_doorbell_rearm_queue(struct mlx5_dev_ctx_shared *sh, uint16_t ci)
 			(wqe[ci & (wq->sq_size - 1)].ctrl[0]) | (ci - 1) << 8);
 	cs.w32[1] = wqe[ci & (wq->sq_size - 1)].ctrl[1];
 	/* Update SQ doorbell record with new SQ ci. */
-	mlx5_doorbell_ring(&sh->tx_uar.bf_db, cs.w64, wq->sq_ci,
+	mlx5_doorbell_ring(&sh->tx_uar.bf_db, &cs.w64, wq->sq_ci,
 			   wq->sq_obj.db_rec, !sh->tx_uar.dbnc);
 }
 
@@ -480,7 +480,7 @@ mlx5_txpp_cq_arm(struct mlx5_dev_ctx_shared *sh)
 	uint64_t db_be =
 		rte_cpu_to_be_64(((uint64_t)db_hi << 32) | aq->cq_obj.cq->id);
 
-	mlx5_doorbell_ring(&sh->tx_uar.cq_db, db_be, db_hi,
+	mlx5_doorbell_ring(&sh->tx_uar.cq_db, &db_be, db_hi,
 			   &aq->cq_obj.db_rec[MLX5_CQ_ARM_DB], 0);
 	aq->arm_sn++;
 }
diff --git a/drivers/regex/mlx5/mlx5_regex_fastpath.c b/drivers/regex/mlx5/mlx5_regex_fastpath.c
index 3207bcbc60..52a0a93ad9 100644
--- a/drivers/regex/mlx5/mlx5_regex_fastpath.c
+++ b/drivers/regex/mlx5/mlx5_regex_fastpath.c
@@ -215,7 +215,7 @@ send_doorbell(struct mlx5_regex_priv *priv, struct mlx5_regex_hw_qp *qp)
 
 	/* Or the fm_ce_se instead of set, avoid the fence be cleared. */
 	((struct mlx5_wqe_ctrl_seg *)wqe)->fm_ce_se |= MLX5_WQE_CTRL_CQ_UPDATE;
-	mlx5_doorbell_ring(&priv->uar.bf_db, *(volatile uint64_t *)wqe,
+	mlx5_doorbell_ring(&priv->uar.bf_db, (volatile uint64_t *)wqe,
 			   actual_pi, &qp->qp_obj.db_rec[MLX5_SND_DBR],
 			   !priv->uar.dbnc);
 }
diff --git a/drivers/vdpa/mlx5/mlx5_vdpa_event.c b/drivers/vdpa/mlx5/mlx5_vdpa_event.c
index 43585a94b2..b9983a61ef 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa_event.c
+++ b/drivers/vdpa/mlx5/mlx5_vdpa_event.c
@@ -77,7 +77,7 @@ mlx5_vdpa_cq_arm(struct mlx5_vdpa_priv *priv, struct mlx5_vdpa_cq *cq)
 	uint64_t doorbell = ((uint64_t)doorbell_hi << 32) | cq->cq_obj.cq->id;
 	uint64_t db_be = rte_cpu_to_be_64(doorbell);
 
-	mlx5_doorbell_ring(&priv->uar.cq_db, db_be, doorbell_hi,
+	mlx5_doorbell_ring(&priv->uar.cq_db, &db_be, doorbell_hi,
 			   &cq->cq_obj.db_rec[MLX5_CQ_ARM_DB], 0);
 	cq->arm_sn++;
 	cq->armed = 1;
-- 
2.34.1

[PATCH 2/2] config/arm: revert forced crypto extension for Cortex-A78AE

[Shani Peretz]

Revert commit 6e6cc1dc3548
("config/arm: support crypto with Cortex-A78AE").

The change introduced an explicit crypto march extension as a workaround
for an infinite loop in crypto performance tests. The root cause was a
doorbell race due to compiler instruction reordering, now fixed by commit
0986acdceab5 ("common/mlx5: fix doorbell race from compiler reordering").
With the root cause addressed, the workaround is no longer needed.

Additionally, the explicit crypto march extension causes compilation
failures on setups where the toolchain or environment does not support
AES instructions.

Fixes: 6e6cc1dc3548 ("config/arm: support crypto with Cortex-A78AE")
Cc: stable@dpdk.org

Signed-off-by: Shani Peretz <shperetz@nvidia.com>
---
 config/arm/meson.build | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 523b0fc0ed..c2e5f1bce6 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -83,7 +83,8 @@ part_number_config_arm = {
         ]
     },
     '0xd42': {
-        'mcpu' : 'mcpu_cortex-a78ae',
+        'march': 'armv8.4-a',
+        'mcpu' : 'cortex-a78ae',
         'flags': [
             ['RTE_MAX_LCORE', 16],
             ['RTE_MAX_NUMA_NODES', 1]
@@ -704,10 +705,6 @@ mcpu_defs = {
         'march': 'armv8-a',
         'march_extensions': ['crc']
     },
-    'mcpu_cortex-a78ae': {
-        'march': 'armv8.4-a',
-        'march_extensions': ['crypto']
-    },
     'mcpu_ft2000plus': {
         'march': 'armv8-a',
         'march_extensions': ['crc']
-- 
2.34.1