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

[Carolina Jubran <cjubran@nvidia.com>]

On 12/09/2025 18:49, Simon Horman wrote:
> 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 {


Hi Simon,

Thanks for the suggestion!

The goal with this patch was to clearly show which bits are used for
each feature in the metadata field, rather than compressing everything
under a single mask. That’s why we chose to explicitly define MACsec,
FS_ID_MASK, and the shift separately. This way, its easy to see at a
glance that MACsec uses bit 1, and bits 2–5 are reserved for the fs_id.

Using FIELD_PREP can work, but it hides the bit layout behind one
mask, which makes it harder to reason about when multiple features
share the same 32-bit field. We wanted to keep things more readable
and maintainable by showing the bit assignments explicitly.

Carolina