Re: [PATCH net-next 12/15] net/mlx5: E-switch, match master egress rule by vport metadata

[Shay Drory <shayd@nvidia.com>]

On 03/11/2026 19:10, Tariq Toukan wrote:
> From: Shay Drory <shayd@nvidia.com>
>
> The master egress-ACL bounce group and rule matched the slave uplink by
> source_port + source_eswitch_owner_vhca_id. Virt LAG master cannot match
> that way; it must match by vport metadata (reg_c_0).
>
> Virt LAG only supports MPESW, where every uplink has a unique metadata.
> Hence, use metadata matching when LAG is virt LAG, and keep the
> source_port + source_eswitch_owner_vhca_id match otherwise.
>
> Signed-off-by: Shay Drory <shayd@nvidia.com>
> Reviewed-by: Mark Bloch <mbloch@nvidia.com>
> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
> Reviewed-by: Akiva Goldberger <agoldberger@nvidia.com>
> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> ---
>  .../mellanox/mlx5/core/eswitch_offloads.c     | 78 +++++++++++++------
>  1 file changed, 55 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index ef1043517e9e..d633c63b309b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> @@ -3000,6 +3000,26 @@ static int esw_set_slave_root_fdb(struct mlx5_core_dev *master,
>  	return err;
>  }
>  
> +static void mlx5_esw_set_spec_source_port_vhca(struct mlx5_flow_spec *spec,
> +					       u16 vport, u16 vhca_id)
> +{
> +	void *misc;
> +
> +	spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
> +
> +	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
> +			    misc_parameters);
> +	MLX5_SET(fte_match_set_misc, misc, source_port, vport);
> +	MLX5_SET(fte_match_set_misc, misc, source_eswitch_owner_vhca_id,
> +		 vhca_id);
> +
> +	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
> +			    misc_parameters);
> +	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
> +	MLX5_SET_TO_ONES(fte_match_set_misc, misc,
> +			 source_eswitch_owner_vhca_id);
> +}
> +
>  static int __esw_set_master_egress_rule(struct mlx5_core_dev *master,
>  					struct mlx5_core_dev *slave,
>  					struct mlx5_vport *vport,
> @@ -3011,22 +3031,22 @@ static int __esw_set_master_egress_rule(struct mlx5_core_dev *master,
>  	struct mlx5_flow_act flow_act = {};
>  	struct mlx5_flow_spec *spec;
>  	int err = 0;
> -	void *misc;
> +
> +	if (mlx5_virt_lag_is_supported(master) &&
> +	    !mlx5_eswitch_vport_match_metadata_enabled(slave->priv.eswitch))
> +		return -EINVAL;
>  
>  	spec = kvzalloc_obj(*spec);
>  	if (!spec)
>  		return -ENOMEM;
>  
> -	spec->match_criteria_enable = MLX5_MATCH_MISC_PARAMETERS;
> -	misc = MLX5_ADDR_OF(fte_match_param, spec->match_value,
> -			    misc_parameters);
> -	MLX5_SET(fte_match_set_misc, misc, source_port, MLX5_VPORT_UPLINK);
> -	MLX5_SET(fte_match_set_misc, misc, source_eswitch_owner_vhca_id, slave_index);
> -
> -	misc = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters);
> -	MLX5_SET_TO_ONES(fte_match_set_misc, misc, source_port);
> -	MLX5_SET_TO_ONES(fte_match_set_misc, misc,
> -			 source_eswitch_owner_vhca_id);
> +	if (mlx5_virt_lag_is_supported(master) &&
> +	    mlx5_eswitch_vport_match_metadata_enabled(slave->priv.eswitch))
> +		mlx5_esw_set_spec_source_port(slave->priv.eswitch,
> +					      MLX5_VPORT_UPLINK, spec);
> +	else
> +		mlx5_esw_set_spec_source_port_vhca(spec, MLX5_VPORT_UPLINK,
> +						   slave_index);

answers to net-dev sashiko

Sashiko (Medium):
Is mlx5_virt_lag_is_supported() enough to decide on reg_c_0 matching here?
It only checks static capability bits:
lag/lag.h:mlx5_virt_lag_is_supported() {
    if (!MLX5_CAP_GEN(dev, multi_uplink_eswitch) ||
        !MLX5_CAP_GEN(dev, lag_master) ||
        MLX5_CAP_GEN(dev, num_lag_ports) >= 2)
        return false;
    return true;
}
That says nothing about the mode the shared FDB was actually created in.
The commit message says "Virt LAG only supports MPESW, where every uplink
has a unique metadata". The code relies on that, but it never checks that
mlx5_mpesw_metadata_set() has run.
Outside MPESW, each uplink gets its default metadata from
mlx5_esw_match_metadata_reserved(), which returns
MLX5_ESW_METADATA_RSVD_UPLINK. The comment there says both uplinks share
this value in shared FDB mode.
Suppose a virt-LAG capable master gets a shared FDB in a non-MPESW mode.
Would this bounce FTE then match reg_c_0 == MLX5_ESW_METADATA_RSVD_UPLINK?
That is also the master's own uplink metadata. If so, master uplink
traffic would be sent to the slave manager vport.
Rules for several slaves could also collide in one group. The group mask
from mlx5_esw_set_flow_group_source_port() in
esw_master_egress_create_resources() no longer includes
source_eswitch_owner_vhca_id.
For VF/SF virt LAG at the end of the series, this case appears
unreachable:
- the SRIOV shared FDB needs MLX5_LAG_FLAG_NDEVS_READY, which is never set
  for virt_lag
- multipath is blocked for virt LAG
- SD is PF-only
One case is left: a PF that reports the virt LAG capability bits and also
supports SD. In that case sd.c would call:
    err = mlx5_lag_shared_fdb_create(ldev, NULL, 0, sd->group_id);
with only the reserved uplink metadata in place. The commit "add
match-metadata allocator for nested VF/SF eswitches" says PFs may have
virt LAG capabilities.

[SD] This is not correct, PFs cannot have virt LAG caps; FW exposes
them only on VF/SF nested eswitch managers. The wording in the
"add match-metadata allocator" commit message is misleading, and will
be fixed in case of v2.

Could this be gated on runtime state instead, for example ldev->virt_lag
together with MPESW mode or pf_metadata being set? Or could the invariant
at least be documented?
The new -EINVAL above, for virt-LAG capable devices with metadata matching
disabled, is also not mentioned in the commit message. Could the message
describe this behavior change?
At this commit the new branch is not reachable, because
mlx5_lag_add_mdev() still requires mlx5_lag_is_supported()
(num_lag_ports >= 2).

>  
>  	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
>  	dest.type = MLX5_FLOW_DESTINATION_TYPE_VPORT;
> @@ -3179,6 +3199,21 @@ static void esw_unset_slave_egress_rule(struct mlx5_core_dev *master,
>  	esw_slave_egress_destroy_resources(slave_vport);
>  }
>  
> +static void mlx5_esw_set_flow_group_source_port_vhca(u32 *flow_group_in)
> +{
> +	void *match_criteria = MLX5_ADDR_OF(create_flow_group_in,
> +					    flow_group_in, match_criteria);
> +
> +	MLX5_SET_TO_ONES(fte_match_param, match_criteria,
> +			 misc_parameters.source_port);
> +	MLX5_SET_TO_ONES(fte_match_param, match_criteria,
> +			 misc_parameters.source_eswitch_owner_vhca_id);
> +	MLX5_SET(create_flow_group_in, flow_group_in,
> +		 match_criteria_enable, MLX5_MATCH_MISC_PARAMETERS);
> +	MLX5_SET(create_flow_group_in, flow_group_in,
> +		 source_eswitch_owner_vhca_id_valid, 1);
> +}
> +
>  static int esw_master_egress_create_resources(struct mlx5_eswitch *esw,
>  					      struct mlx5_flow_namespace *egress_ns,
>  					      struct mlx5_vport *vport, size_t count)
> @@ -3189,13 +3224,16 @@ static int esw_master_egress_create_resources(struct mlx5_eswitch *esw,
>  	};
>  	struct mlx5_flow_table *acl;
>  	struct mlx5_flow_group *g;
> -	void *match_criteria;
>  	u32 *flow_group_in;
>  	int err;
>  
>  	if (vport->egress.acl)
>  		return 0;
>  
> +	if (mlx5_virt_lag_is_supported(esw->dev) &&
> +	    !mlx5_eswitch_vport_match_metadata_enabled(esw))
> +		return -EINVAL;
> +
>  	flow_group_in = kvzalloc(inlen, GFP_KERNEL);
>  	if (!flow_group_in)
>  		return -ENOMEM;
> @@ -3209,17 +3247,11 @@ static int esw_master_egress_create_resources(struct mlx5_eswitch *esw,
>  		goto out;
>  	}
>  
> -	match_criteria = MLX5_ADDR_OF(create_flow_group_in, flow_group_in,
> -				      match_criteria);
> -	MLX5_SET_TO_ONES(fte_match_param, match_criteria,
> -			 misc_parameters.source_port);
> -	MLX5_SET_TO_ONES(fte_match_param, match_criteria,
> -			 misc_parameters.source_eswitch_owner_vhca_id);
> -	MLX5_SET(create_flow_group_in, flow_group_in, match_criteria_enable,
> -		 MLX5_MATCH_MISC_PARAMETERS);
> -
> -	MLX5_SET(create_flow_group_in, flow_group_in,
> -		 source_eswitch_owner_vhca_id_valid, 1);
> +	if (mlx5_virt_lag_is_supported(esw->dev) &&
> +	    mlx5_eswitch_vport_match_metadata_enabled(esw))
> +		mlx5_esw_set_flow_group_source_port(esw, flow_group_in, 0);
> +	else
> +		mlx5_esw_set_flow_group_source_port_vhca(flow_group_in);
>  	MLX5_SET(create_flow_group_in, flow_group_in, start_flow_index, 0);
>  	MLX5_SET(create_flow_group_in, flow_group_in, end_flow_index, count);
>