|SUCCESS| [4/4] net/iavf: accept up to 32k unicast MAC addresses

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162853

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-03

Patch review for DPDK mailing list

Commit: 20260403091836.1073484-2-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>
Subject: [PATCH 1/4] ethdev: skip VMDq pools unless configured

This patch adds validation to ensure the mac_addr_add API only accepts
pools beyond 0 when VMDq has been enabled. It also cleans up related
operations for adding, removing, and restoring MAC addresses.

> diff --git a/drivers/net/cnxk/cnxk_ethdev_ops.c b/drivers/net/cnxk/cnxk_ethdev_ops.c
> index 49e77e49a6..75decf7098 100644
> --- a/drivers/net/cnxk/cnxk_ethdev_ops.c
> +++ b/drivers/net/cnxk/cnxk_ethdev_ops.c
> @@ -1240,7 +1240,6 @@ cnxk_nix_mc_addr_list_configure(struct rte_eth_dev *eth_dev, struct rte_ether_ad
>  		/* Update address in NIC data structure */
>  		rte_ether_addr_copy(&mc_addr_set[i], &data->mac_addrs[j]);
>  		rte_ether_addr_copy(&mc_addr_set[i], &dev->dmac_addrs[j]);
> -		data->mac_pool_sel[j] = RTE_BIT64(0);
>  	}
>  
>  	roc_nix_npc_promisc_ena_dis(nix, true);
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 2edc7a362e..9577b7d848 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1680,7 +1680,10 @@ eth_dev_mac_restore(struct rte_eth_dev *dev,
>  				continue;
>  
>  			pool = 0;
> -			pool_mask = dev->data->mac_pool_sel[i];
> +			if ((dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0)
> +				pool_mask = dev->data->mac_pool_sel[i];
> +			else
> +				pool_mask = 1;
>  
>  			do {
>  				if (pool_mask & UINT64_C(1))

Does the eth_dev_mac_restore() function properly access
dev->data->dev_conf.rxmode during a device stop and start cycle? Can
the rxmode configuration be unavailable or stale when this restore code
runs?

[ ... ]

> @@ -5416,6 +5420,12 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  		RTE_ETHDEV_LOG_LINE(ERR, "Pool ID must be 0-%d", RTE_ETH_64_POOLS - 1);
>  		return -EINVAL;
>  	}
> +	vmdq = (dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0;
> +	if (!vmdq && pool != 0) {
> +		RTE_ETHDEV_LOG_LINE(ERR, "Port %u: VMDq is not configured (pool %d)",
> +			port_id, pool);
> +		return -EINVAL;
> +	}
>  
>  	index = eth_dev_get_mac_addr_index(port_id, addr);
>  	if (index < 0) {
> @@ -5425,7 +5435,7 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  				port_id);
>  			return -ENOSPC;
>  		}
> -	} else {
> +	} else if (vmdq) {
>  		pool_mask = dev->data->mac_pool_sel[index];
>  
>  		/* Check if both MAC address and pool is already there, and do nothing */
> @@ -5440,8 +5450,10 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  		/* Update address in NIC data structure */
>  		rte_ether_addr_copy(addr, &dev->data->mac_addrs[index]);
>  
> -		/* Update pool bitmap in NIC data structure */
> -		dev->data->mac_pool_sel[index] |= RTE_BIT64(pool);
> +		if (vmdq) {
> +			/* Update pool bitmap in NIC data structure */
> +			dev->data->mac_pool_sel[index] |= RTE_BIT64(pool);
> +		}
>  	}

The early validation of vmdq means when we enter this final block at
line 5453, vmdq is guaranteed to be true. Does this code need the
redundant inner vmdq check?

Commit: 20260403091836.1073484-3-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>
Subject: [PATCH 2/4] ethdev: announce VMDq capability

This patch marks VMDq feature availability as a per device capability
and enforces API calls related to VMDq.

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 9577b7d848..7ba539e796 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1581,6 +1582,22 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>  		goto rollback;
>  	}
>  
> +	if (!(dev_info.dev_capa & RTE_ETH_DEV_CAPA_VMDQ)) {
> +		if ((dev_conf->rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0) {
> +			RTE_ETHDEV_LOG_LINE(ERR, "Ethdev port_id=%u does not support VMDq rx mode",
> +				port_id);
> +			ret = -EINVAL;
> +			goto rollback;
> +		}
> +		if (dev_conf->txmode.mq_mode == RTE_ETH_MQ_TX_VMDQ_DCB ||
> +				dev_conf->txmode.mq_mode == RTE_ETH_MQ_TX_VMDQ_ONLY) {
> +			RTE_ETHDEV_LOG_LINE(ERR, "Ethdev port_id=%u does not support VMDq tx mode",
> +				port_id);
> +			ret = -EINVAL;
> +			goto rollback;
> +		}
> +	}
> +
>  	/*
>  	 * Setup new number of Rx/Tx queues and reconfigure device.
>  	 */

Does this code properly handle the case where a device previously
configured with VMDq is then reconfigured without it? Can leftover state
from mac_pool_sel[] cause issues if not properly cleared during a
reconfiguration to non-VMDq mode?

Commit: 20260403091836.1073484-5-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>
Subject: [PATCH 4/4] net/iavf: accept up to 32k unicast MAC addresses

This patch increases the maximum number of secondary MAC addresses to
32768 on E810 hardware.

> diff --git a/drivers/net/intel/iavf/iavf.h b/drivers/net/intel/iavf/iavf.h
> index 403c61e2e8..f1dede0694 100644
> --- a/drivers/net/intel/iavf/iavf.h
> +++ b/drivers/net/intel/iavf/iavf.h
> @@ -31,7 +31,8 @@
>  #define IAVF_IRQ_MAP_NUM_PER_BUF	 128
>  #define IAVF_RXTX_QUEUE_CHUNKS_NUM	 2
>  
> -#define IAVF_NUM_MACADDR_MAX      64
> +#define IAVF_UC_MACADDR_MAX      32768
> +#define IAVF_MC_MACADDR_MAX      64

[ ... ]

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index 1eca20bc9a..c69a012d50 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c
> @@ -2822,11 +2822,11 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>  
>  	/* copy mac addr */
>  	eth_dev->data->mac_addrs = rte_zmalloc(
> -		"iavf_mac", RTE_ETHER_ADDR_LEN * IAVF_NUM_MACADDR_MAX, 0);
> +		"iavf_mac", RTE_ETHER_ADDR_LEN * IAVF_UC_MACADDR_MAX, 0);
>  	if (!eth_dev->data->mac_addrs) {
>  		PMD_INIT_LOG(ERR, "Failed to allocate %d bytes needed to"
>  			     " store MAC addresses",
> -			     RTE_ETHER_ADDR_LEN * IAVF_NUM_MACADDR_MAX);
> +			     RTE_ETHER_ADDR_LEN * IAVF_UC_MACADDR_MAX);
>  		ret = -ENOMEM;
>  		goto init_vf_err;
>  	}

Does the iavf_dev_init() function leak eth_dev->data->mac_addrs when
hitting the init_vf_err label? The allocation happens before the check,
but can the error path starting at init_vf_err properly clean up this
new larger memory region?