Re: [PATCH net-next V4 09/14] net/mlx5e: psp: Add an rx_decap steering table

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] accel_psp_fs_rx_decap_ft_create()
  (drivers/net/ethernet/mellanox/mlx5/core/en_accel/psp.c) creates the…
- [Medium] accel_psp_fs_rx_decrypt_ft_create() now unconditionally
  allocates a modify-header context that copies…

--

net/mlx5e: psp: Add an rx_decap steering table

This adds a new PSP RX decapsulation steering table with one rule per
supported PSP version (transport-mode decap, SPI copy into reg_b, flow tag
marker) plus a drop rule for unsupported versions. The mode is tracked in
fs.decap_enabled and switched through accel_psp_fs_rx_reconfigure(), which
is called with decap_wanted == false in this commit.

A couple of questions below.

[ ... ]

[ ... ]

[Severity: High]
Should ft_attr.flags here include MLX5_FLOW_TABLE_TUNNEL_EN_REFORMAT?

Every rule installed into this table below carries
MLX5_FLOW_CONTEXT_ACTION_PACKET_REFORMAT with a
MLX5_REFORMAT_TYPE_REMOVE_PSP_TRANSPORT context, but the table flag is the
only source of flow_table_context.reformat_en:

fs_cmd.c:mlx5_cmd_create_flow_table() {
	int en_encap = !!(ft->flags & MLX5_FLOW_TABLE_TUNNEL_EN_REFORMAT);
	...
	MLX5_SET(create_flow_table_in, in, flow_table_context.reformat_en,
		 en_encap);
}

The SWS and HWS backends derive reformat_en from the same flag, so with
flags == 0 the table is created with reformat disabled while its FTEs
request a reformat action.

The directly analogous header-removal case in the same driver does set the
flag, for the MACsec RX check table that hosts
MLX5_REFORMAT_TYPE_DEL_MACSEC:

lib/macsec_fs.c:macsec_fs_rx_create() {
	flow_table = macsec_fs_auto_group_table_create(ns,
						       MLX5_FLOW_TABLE_TUNNEL_EN_REFORMAT,
						       RX_CHECK_TABLE_LEVEL,
						       RX_CHECK_TABLE_NUM_FTE);
}

IPsec does the same on rx->ft.sa / tx->ft.sa in en_accel/ipsec_fs.c, and so
do esw/bridge.c, lib/fs_chains.c, esw/vporttbl.c and eswitch_offloads.c for
their reformat-bearing tables.

If firmware rejects the reformat FTE, mlx5_add_flow_rules() fails in the
version loop, accel_psp_fs_rx_decap_ft_create() unwinds, and
accel_psp_fs_rx_reconfigure() only prints "HW GRO for PSP unavailable", so
the feature stays inert even on HW/FW advertising
reformat_del_psp_transport. If a backend instead accepts the FTE without
performing the reformat, packets that still carry UDP+PSP headers get the
PSP-decap flow tag and are interpreted as decapsulated by the RX handler
added later in the series.

Note there is no software-side check in fs_core.c for this mismatch, so it
only shows up as a firmware syndrome at rule installation time. Also, as
IPsec shows (mlx5_eswitch_block_encap() before setting the flag), adding the
flag alone may not be the whole fix.

At the end of the series (selftests: drv-net: Add PSP HW GRO conformance
tests) psp.c still has no ft_attr.flags assignment and no
MLX5_FLOW_TABLE_TUNNEL_EN_REFORMAT anywhere.

[ ... ]

[ ... ]

[ ... ]

[Severity: Medium]
Should this SPI copy be gated on the same capability as the decap table?

The only consumer of REG_C_0 here is the optional decap table, which
re-copies REG_C_0 into REG_B and is gated in accel_psp_fs_rx_reconfigure()
on:

	bool decap_supported =
		MLX5_CAP_FLOWTABLE(fs->mdev,
				   flow_table_properties_nic_receive.reformat_del_psp_transport);

with failures there being warn-only. On the decrypt path, however, the
MLX5_ACTION_IN_FIELD_PSP_HEADER_1 copy is unconditional, and a failure is
fatal for the whole feature:

accel_psp_fs_rx_create()
  accel_psp_fs_rx_decrypt_ft_create()   /* -> goto out_err */
    NL_SET_ERR_MSG(extack, "Failed creating RX decrypt steering table");

mlx5_cmd_modify_header_alloc() validates only num_actions against
max_modify_header_actions, so the source field is validated by firmware
alone and there is no capability bit for it. Both
MLX5_ACTION_IN_FIELD_PSP_HEADER_1 and the reformat_del_psp_transport bit
came in with commit bee40a7d0bd1 ("net/mlx5: Add PSP related fields to the
mlx5_ifc"), while PSP crypto offload shipped earlier with commit
04a3134f88a4. On FW that has PSP crypto offload but not the newer PSP header
field, does PSP offload stop being enable-able at all rather than just
losing decap/GRO?

That would not match the commit message:

    The intention is for failures creating the new table to not block
    feature reconfig/PSP toggling.
    ...
    Nothing happens on older HW/FW without the ability to decapsulate PSP
    transport.

The change to the pre-existing decrypt rule is not mentioned in the commit
message at all; could it be described there?

Two smaller points in the same block. This is the only failure branch of
accel_psp_fs_rx_decrypt_ft_create() without an mlx5_core_err(), while the
code it replaces did log ("fail to alloc psp set modify_header_id err=%d").
And since accel_psp_fs_rx_create() calls accel_psp_fs_rx_reconfigure(fs,
false) in this commit, the extra MOD_HDR action runs for every PSP RX packet
with nothing reading REG_C_0 until the follow-up patch enables decap. At the
end of the series the decrypt-side copy is still unconditional and still
unlogged.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908065244.3799142-1-tariqt%40nvidia.com