Re: [PATCH v3 05/19] net/ixgbe: reimplement ethertype parser

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

On 9/16/2026 1:18 PM, Anatoly Burakov wrote:
> Use the new flow graph API and the common parsing framework to implement
> flow parser for ethertype.
>
> The old ethertype parser was accepting certain things that were later
> rejected by the actual ethertype installation code, in particular DROP
> action as well as dst MAC address filtering. This was removed from the
> graph parser.
>
> The ethertype filter tracking table is used by the rte_flow ethertype
> engine, but it is also in use by other features, so the filter tracking
> is refactored to be properly shared between the engine and other features
> that write into the same table.
>
> Signed-off-by: Anatoly Burakov<anatoly.burakov@intel.com>
> ---
<snip>
> @@ -7198,8 +7177,11 @@ static int
>   ixgbe_timesync_enable(struct rte_eth_dev *dev)
>   {
>   	struct ixgbe_hw *hw = IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	struct ixgbe_filter_info *filter_info =
> +		IXGBE_DEV_PRIVATE_TO_FILTER_INFO(dev->data->dev_private);
>   	uint32_t tsync_ctl;
>   	uint32_t tsauxc;
> +	uint32_t etqf;
>   	struct timespec ts;
>   
>   	memset(&ts, 0, sizeof(struct timespec));
> @@ -7221,10 +7203,19 @@ ixgbe_timesync_enable(struct rte_eth_dev *dev)
>   	ixgbe_start_timecounters(dev);
>   
>   	/* Enable L2 filtering of IEEE1588/802.1AS Ethernet frame types. */
> -	IXGBE_WRITE_REG(hw, IXGBE_ETQF(IXGBE_ETQF_FILTER_1588),
> -			(RTE_ETHER_TYPE_1588 |
> -			 IXGBE_ETQF_FILTER_EN |
> -			 IXGBE_ETQF_1588));
> +	etqf = RTE_ETHER_TYPE_1588 | IXGBE_ETQF_FILTER_EN | IXGBE_ETQF_1588;
> +	if (!filter_info->timesync_installed) {
> +		int idx = ixgbe_ethertype_table_add(&filter_info->ethertype_table,
> +				RTE_ETHER_TYPE_1588, etqf, 0);
if I understood this part correctly, from now Ethertype filters are no 
longer statically dedicated for predefined set of ethertypes (ref 
IXGBE_ETQF_FILTER_1588) and now are dynamically managed. It is probably 
worth to separate this to another commit, or at least mention about in 
commit message
> +
> +		if (idx < 0) {
> +			PMD_DRV_LOG(ERR, "no free ETQF slot for 1588 timestamping");
> +			return idx;
> +		}
> +		filter_info->timesync_idx = idx;
> +		filter_info->timesync_installed = true;
> +	}
> +	ixgbe_ethertype_filter_program(hw, filter_info->timesync_idx, etqf, 0);
>   
<snip>
> +
> +static int
> +ixgbe_flow_ethertype_register(struct ci_flow *flow, struct rte_flow_error *error)
> +{
> +	struct ixgbe_ethertype_flow *ethertype_flow = (struct ixgbe_ethertype_flow *)flow;
> +	struct ixgbe_filter_info *filter_info =
> +		IXGBE_DEV_PRIVATE_TO_FILTER_INFO(flow->dev_data->dev_private);
> +	int idx;
> +
> +	idx = ixgbe_ethertype_table_add(&filter_info->ethertype_table,
> +			ethertype_flow->ether_type, ethertype_flow->etqf,
> +			ethertype_flow->etqs);

nit: is it worth to check if idx is negative instead of handling only 2 
error codes?

> +	if (idx == -EEXIST) {
> +		return rte_flow_error_set(error, EEXIST,
> +				RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +				"Ethertype filter already exists");
> +	}
> +	if (idx == -ENOSPC) {
> +		return rte_flow_error_set(error, ENOSPC,
> +				RTE_FLOW_ERROR_TYPE_HANDLE, NULL,
> +				"Ethertype filters are full");
> +	}
> +	ethertype_flow->index = idx;
> +
> +	return 0;
> +}
> +
<snip>
> @@ -168,38 +180,29 @@ ixgbe_add_tx_flow_control_drop_filter(struct rte_eth_dev *eth_dev)
>   	struct ixgbe_filter_info *filter_info =
>   		IXGBE_DEV_PRIVATE_TO_FILTER_INFO(eth_dev->data->dev_private);
>   	uint16_t vf_num;
> +	uint32_t etqf, etqs;
>   	int i;
> -	struct ixgbe_ethertype_filter ethertype_filter;
>   
>   	if (!hw->mac.ops.set_ethertype_anti_spoofing) {
>   		PMD_DRV_LOG(INFO, "ether type anti-spoofing is not supported.");
>   		return;
>   	}
>   
> -	i = ixgbe_ethertype_filter_lookup(filter_info,
> -					  IXGBE_ETHERTYPE_FLOW_CTRL);
> -	if (i >= 0) {
> -		PMD_DRV_LOG(ERR, "A ether type filter entity for flow control already exists!");
> -		return;
> -	}
> +	etqf = IXGBE_ETQF_FILTER_EN | IXGBE_ETQF_TX_ANTISPOOF |
> +			IXGBE_ETHERTYPE_FLOW_CTRL;
> +	etqs = 0;
> +	if (!filter_info->antispoof_installed) {
> +		int idx = ixgbe_ethertype_table_add(&filter_info->ethertype_table,
> +				IXGBE_ETHERTYPE_FLOW_CTRL, etqf, etqs);
>   
> -	ethertype_filter.ethertype = IXGBE_ETHERTYPE_FLOW_CTRL;
> -	ethertype_filter.etqf = IXGBE_ETQF_FILTER_EN |
> -				IXGBE_ETQF_TX_ANTISPOOF |
> -				IXGBE_ETHERTYPE_FLOW_CTRL;
> -	ethertype_filter.etqs = 0;
> -	ethertype_filter.conf = TRUE;
> -	i = ixgbe_ethertype_filter_insert(filter_info,
> -					  &ethertype_filter);
> -	if (i < 0) {
> -		PMD_DRV_LOG(ERR, "Cannot find an unused ether type filter entity for flow control.");
> -		return;
> +		if (idx < 0) {
> +			PMD_DRV_LOG(ERR, "no free ETQF slot for Tx anti-spoof filter");
here and for ixgbe_timesync_enable(), what if 
ixgbe_ethertype_table_add() returns -EEXIST? It is better to add since 
ixgbe_ethertype_filter_lookup() was removed
> +			return;
> +		}
> +		filter_info->antispoof_idx = idx;
> +		filter_info->antispoof_installed = true;
>   	}
> -
> -	IXGBE_WRITE_REG(hw, IXGBE_ETQF(i),
> -			(IXGBE_ETQF_FILTER_EN |
> -			IXGBE_ETQF_TX_ANTISPOOF |
> -			IXGBE_ETHERTYPE_FLOW_CTRL));
> +	ixgbe_ethertype_filter_program(hw, filter_info->antispoof_idx, etqf, etqs);
>   
>   	vf_num = dev_num_vf(eth_dev);
>   	for (i = 0; i < vf_num; i++)
> diff --git a/drivers/net/intel/ixgbe/meson.build b/drivers/net/intel/ixgbe/meson.build
> index 0531d37acd3..f2857feab76 100644
> --- a/drivers/net/intel/ixgbe/meson.build
> +++ b/drivers/net/intel/ixgbe/meson.build
> @@ -26,6 +26,7 @@ sources += files(
>           'ixgbe_ethdev.c',
>           'ixgbe_fdir.c',
>           'ixgbe_flow.c',
> +        'ixgbe_flow_ethertype.c',
>           'ixgbe_ipsec.c',
>           'ixgbe_pf.c',
>           'ixgbe_rxtx.c',

-- 
Regards,
Vladimir