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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 9/19/2026 6:10 PM, Medvedkin, Vladimir wrote:
> 
> 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>
>> ---

Hi Vladimir!

> <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

Yes, I'll separate it out.

>> +
>> +        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?

Will fix.

> 
>> +    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

This should not be possible. If the entry is already installed, 
antispoof_installed will be true. If it's false, then the only way in 
which we could have the same filter already installed is through 
rte_flow which would not produce such specific ETQF values.

-- 
Thanks,
Anatoly