Re: [PATCH mlx5-next 2/3] net/mlx5: Refactor MACsec WQE metadata shifts

[Simon Horman <horms@kernel.org>]

On Thu, Sep 11, 2025 at 10:10:18AM +0300, Tariq Toukan wrote:
> From: Carolina Jubran <cjubran@nvidia.com>
> 
> Introduce MLX5_ETH_WQE_FT_META_SHIFT as a shared base offset for
> features that use the lower 8 bits of the WQE flow_table_metadata
> field, currently used for timestamping, IPsec, and MACsec.
> 
> Define MLX5_ETH_WQE_FT_META_MACSEC_FS_ID_MASK so that fs_id occupies
> bits 2–5, making it clear that fs_id occupies bits in the metadata.
> 
> Set MLX5_ETH_WQE_FT_META_MACSEC_MASK as the OR of the MACsec flag and
> MLX5_ETH_WQE_FT_META_MACSEC_FS_ID_MASK, corresponding to the original
> 0x3E mask.
> 
> Update the fs_id macro to right-shift the MACsec flag by
> MLX5_ETH_WQE_FT_META_SHIFT and update the RoCE modify-header action to
> use it.
> 
> Introduce the helper macro MLX5_MACSEC_TX_METADATA(fs_id) to compose
> the full shifted MACsec metadata value.
> 
> These changes make it explicit exactly which metadata bits carry MACsec
> information, simplifying future feature exclusions when multiple
> features share the WQE flowtable metadata.
> 
> In addition, drop the incorrect “RX flow steering” comment, since this
> applies to TX flow steering.
> 
> Signed-off-by: Carolina Jubran <cjubran@nvidia.com>
> Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>

Hi Carolina, Tariq, all,

I'm wondering if dropping _SHIFT and making use of FIELD_PREP
would lead to a cleaner and more idiomatic implementation.

I'm thinking that such an approach would involve
updating MLX5_ETH_WQE_FT_META_MACSEC_MASK rather
than MLX5_ETH_WQE_FT_META_MACSEC_SHIFT in the following patch.

I'm thinking of something along the lines of following incremental patch.

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
index 9ec450603176..58c0ff4af78f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.c
@@ -2218,7 +2218,7 @@ static int mlx5_macsec_fs_add_roce_rule_tx(struct mlx5_macsec_fs *macsec_fs, u32
 	MLX5_SET(set_action_in, action, data,
 		 mlx5_macsec_fs_set_tx_fs_id(fs_id));
 	MLX5_SET(set_action_in, action, offset,
-		 MLX5_ETH_WQE_FT_META_MACSEC_SHIFT);
+		 __bf_shf(MLX5_ETH_WQE_FT_META_MACSEC_MASK));
 	MLX5_SET(set_action_in, action, length, 32);
 
 	modify_hdr = mlx5_modify_header_alloc(mdev, MLX5_FLOW_NAMESPACE_RDMA_TX_MACSEC,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.h
index 15acaff43641..402840cb3110 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/macsec_fs.h
@@ -13,18 +13,15 @@
 #define MLX5_MACSEC_RX_METADAT_HANDLE(metadata)  ((metadata) & MLX5_MACSEC_RX_FS_ID_MASK)
 
 /* MACsec TX flow steering */
-#define MLX5_ETH_WQE_FT_META_MACSEC_MASK \
-	(MLX5_ETH_WQE_FT_META_MACSEC | MLX5_ETH_WQE_FT_META_MACSEC_FS_ID_MASK)
-#define MLX5_ETH_WQE_FT_META_MACSEC_SHIFT MLX5_ETH_WQE_FT_META_SHIFT
+#define MLX5_ETH_WQE_FT_META_MACSEC_MASK GENMASK(7, 0)
 
 /* MACsec fs_id handling for steering */
 #define mlx5_macsec_fs_set_tx_fs_id(fs_id) \
-	(((MLX5_ETH_WQE_FT_META_MACSEC) >> MLX5_ETH_WQE_FT_META_MACSEC_SHIFT) \
-	 | ((fs_id) << 2))
+	(MLX5_ETH_WQE_FT_META_IPSEC | (fs_id) << 2)
 
 #define MLX5_MACSEC_TX_METADATA(fs_id) \
-	(mlx5_macsec_fs_set_tx_fs_id(fs_id) << \
-	 MLX5_ETH_WQE_FT_META_MACSEC_SHIFT)
+	FIELD_PREP(MLX5_ETH_WQE_FT_META_MACSEC_MASK, \
+		   mlx5_macsec_fs_set_tx_fs_id(fs_id))
 
 /* MACsec fs_id uses 4 bits, supports up to 16 interfaces */
 #define MLX5_MACSEC_NUM_OF_SUPPORTED_INTERFACES 16
diff --git a/include/linux/mlx5/qp.h b/include/linux/mlx5/qp.h
index b21be7630575..5546c7bd2c83 100644
--- a/include/linux/mlx5/qp.h
+++ b/include/linux/mlx5/qp.h
@@ -251,14 +251,9 @@ enum {
 	MLX5_ETH_WQE_SWP_OUTER_L4_UDP   = 1 << 5,
 };
 
-/* Base shift for metadata bits used by timestamping, IPsec, and MACsec */
-#define MLX5_ETH_WQE_FT_META_SHIFT 0
-
 enum {
-	MLX5_ETH_WQE_FT_META_IPSEC = BIT(0) << MLX5_ETH_WQE_FT_META_SHIFT,
-	MLX5_ETH_WQE_FT_META_MACSEC = BIT(1) << MLX5_ETH_WQE_FT_META_SHIFT,
-	MLX5_ETH_WQE_FT_META_MACSEC_FS_ID_MASK =
-		GENMASK(5, 2) << MLX5_ETH_WQE_FT_META_SHIFT,
+	MLX5_ETH_WQE_FT_META_IPSEC = BIT(0),
+	MLX5_ETH_WQE_FT_META_MACSEC = BIT(1),
 };
 
 struct mlx5_wqe_eth_seg {