[PATCH v2 00/20] sxe2: address review comments - testpmd restructuring, devargs documentation, and code cleanup

[liujie5@linkdatatechnology.com]

Hi all,

Thank you for the detailed review of the sxe2 PMD patch series. We have
prepared a v2 addressing the feedback. Below is our point-by-point response.

---

### 1. strtoul() out-of-range handling

> This memory stuff looks problematic and needs more review. At a minimum
> I see a pattern of not handling values from strtoul() that are out of range.

After re-examining the devargs parsing code in sxe2_ethdev.c, we confirmed
that the existing parsers handle out-of-range values:

- sxe2_parse_u8(): checks val > UINT8_MAX, returns -ERANGE
- sxe2_parse_bool(): checks bool_val != 0 && bool_val != 1
- sxe2_parse_fnav_stat_type(): checks val > the maximum valid enum value
- sxe2_parse_sched_layer_mode(): checks val > SXE2_TXSCH_NODE_ADJ_LVL_MAX
- sxe2_parse_high_performance_mode(): checks val != 1

Each function sets errno = 0 before strtoul() and checks errno != 0
afterward, which catches ERANGE (overflow). This is followed by a semantic
range check specific to each parameter.

Nevertheless, to make the overflow path more explicit and defensive, we will
add explicit ULONG_MAX comparisons after each strtoul() call in the v2.

---

### 2. testpmd command symbols exported from driver library

> Error: the command logic is placed in sxe2_testpmd_lib.c, compiled into
> the driver library, and exposed through 14 new RTE_EXPORT_EXPERIMENTAL_SYMBOL
> entries. No upstream driver exports symbols for its testpmd commands; all six
> existing drivers with testpmd integration compile their *_testpmd.c into
> testpmd via testpmd_sources and use internal access. These exports are
> vendor public API that any application can link against.

Agreed. This is a valid concern. We will restructure as follows:

- Move the testpmd command implementation from sxe2_testpmd_lib.c into
  sxe2_testpmd.c, compiled via testpmd_sources in the meson build,
  matching the pattern used by i40e, ixgbe, mlx5, bnxt, and others.
- Remove all 14 RTE_EXPORT_EXPERIMENTAL_SYMBOL entries. These were never
  intended as public API.
- Keep driver-internal functions accessible through internal headers only
  (they are already declared in driver-local headers under drivers/net/sxe2/).
- Move application state variables (g_tx_session[][], g_rx_session[][],
  g_esp_header_offset[], g_sess_pool) into the testpmd-side code.

---

### 3. Commands duplicating standard testpmd functionality

> Error: three commands duplicate standard testpmd functionality the driver
> already supports.
> "sxe2 flow rule dump" -> implement the rte_flow dev_dump op
> "sxe2 udp_tunnel_port add|rm" -> port config udp_tunnel_port add|rm
> "sxe2 show stats" -> show port xstats

Agreed on all three:

- **sxe2 flow rule dump**: We will implement the rte_flow_dev_dump() op in
  the driver. After that, the standard "flow dump <port_id> all" command
  works for every application. The private command will be removed.

- **sxe2 udp_tunnel_port add|rm**: This calls rte_eth_dev_udp_tunnel_port_add/
  rm, which is already exposed by "port config <port_id> udp_tunnel_port
  add|rm". The private command is redundant and will be removed.

- **sxe2 show stats**: We will audit the xstats implementation and ensure
  all statistics currently shown by the private formatter are available
  through rte_eth_xstats_get(). Any missing counters will be added as
  xstats entries. The private command will be removed; users will use
  "show port xstats <port_id>" instead.

---

### 4. IPsec SA management suite in testpmd commands

> Warning: the 9-subcommand ipsec suite (egress/ingress add/rm/show,
> session-id and esp-hdr-offset set/get, flush, stats) is an SA management
> application embedded in the driver. Inline crypto is exercised with
> examples/ipsec-secgw, as done for other inline-crypto PMDs.

Understood. We will:

- Remove the ipsec SA management commands from the testpmd extension.
- Document that examples/ipsec-secgw is the supported method for
  exercising inline crypto on this driver, consistent with other
  inline-crypto PMDs (e.g., mlx5, ice).
- If interactive SA management in testpmd is needed in the future,
  we will propose it as generic testpmd commands over rte_security,
  benefiting all drivers equally.

---

### 5. Private devargs

> Warning: seven private devargs are added with no documentation.
> Each surviving devarg needs documentation and a rationale for why
> no standard API covers it.

Documentation has been added to doc/guides/nics/sxe2.rst (Runtime
Configuration section) for all seven devargs, along with
RTE_PMD_REGISTER_PARAM_STRING for discoverability.

Below is the rationale for each devarg that we believe should be retained:

**flow-duplicate-pattern**
This controls whether the hardware switch engine accepts flow rules with
identical match patterns. It does NOT change rte_flow semantics; it
configures a hardware-specific behavior. The switch engine supports two
modes: (a) reject duplicate rules (useful for catching configuration
errors), and (b) allow duplicates by setting a per-rule metadata flag.
This is a hardware capability toggle, analogous to how some NICs support
configurable TCAM overlap behavior. Default value (1 = allow duplicates)
ensures standard rte_flow behavior out of the box. The standard rte_flow
API does not provide a mechanism to control switch-level duplicate rule
behavior.

**fnav-stat-type**
This selects how the Fnav flow engine partitions its hardware counter
resources (packets only, bytes only, or both). This is set at device
initialization and cannot be changed dynamically; it determines the
hardware resource allocation. When only packet counts are needed, the
hardware can allocate the full counter width to packet counting. The
resulting counters are queried via the standard rte_flow_query() API.
No standard API currently exists to select hardware counter resource
allocation mode at initialization time; it is a hardware-specific
configuration decision.

**drv-sw-stats**
Hardware packet statistics counters may be inaccurate for certain packet
types due to hardware design limitations. This devarg enables a software
accumulation path in the Rx data path as a workaround. When enabled, the
driver classifies each received packet (unicast/multicast/broadcast/drop)
and accumulates counts in software. The counters are exposed as xstats
(rx_sw_unicast_packets, etc.). The devarg controls whether the per-packet
software accumulation overhead is incurred; it is an optimization choice
for users who do not require this level of accuracy. The rationale for
keeping this as a devarg rather than always-on xstats is that the
software accumulation adds overhead to the Rx fast path; users who do
not need per-packet classification accuracy should not pay this cost.

**sched-layer-mode**
Different hardware SKUs support different numbers of Tx scheduling levels
(0-3). This parameter caps the maximum scheduling depth at initialization.
This is NOT a TM topology configuration -- the application still builds
its rte_tm hierarchy as usual. Rather, it constrains the hierarchy depth
to match hardware capability across SKU variants. The rte_tm API does not
currently provide a mechanism to query or constrain the maximum scheduling
depth before configuring the hierarchy, so a devarg is the practical
approach.

**high-performance-mode**
When enabled, the Tx path bypasses the hardware scheduling module and
sends packets directly to the port. This trades TM scheduling capability
for lower latency and higher throughput. It is not appropriate to make
this the default because TM functionality is important for many users.
The devarg (now documented) makes the performance trade-off explicit.

**rx-low-latency**
Controls whether Rx interrupt moderation is tuned for minimum latency vs.
batching efficiency. This is a hardware-specific tuning parameter with
no equivalent in the standard Ethdev API. Many drivers have analogous
low-latency devargs (e.g., mlx5 has rxqs_min_bth for similar purposes).

**function-flow-direct**
Controls whether flow rules can directly target a specific PF/VF function.
This is a hardware switch engine feature that affects the scope of flow
redirection. The standard rte_flow API does not have a per-rule flag for
this; the devarg controls the default redirect behavior.

---

### Summary of changes for v2

1. Add explicit ULONG_MAX checks after strtoul() in all devarg parsers
2. Move testpmd command implementation out of driver library into
   testpmd_sources; remove all 14 RTE_EXPORT_EXPERIMENTAL_SYMBOL
3. Implement rte_flow_dev_dump op; remove "sxe2 flow rule dump" command
4. Remove "sxe2 udp_tunnel_port add|rm" command (redundant)
5. Add missing counters to xstats; remove "sxe2 show stats" command
6. Remove ipsec SA management testpmd commands
7. Retain 7 devargs with documentation and rationale in sxe2.rst
8. Add RTE_PMD_REGISTER_PARAM_STRING for all devargs

We will send v2 shortly. Please let us know if there are any further
concerns.

Thanks,
[sender name]