|SUCCESS| [v3,5/5] net/iavf: fix duplicate MAC addresses install

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-10

commit 20260510170306.3406045-2-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

ethdev: skip VMDq pools unless configured

The mac_addr_add API describes that only the 0 pool should be passed
unless VMDq has been enabled, though there was no validation so far.
Add such a check, then cleanup the MAC related operations (adding,
removing, restoring).

As a side effect, the net/cnxk does not need to manually reset the
mac_pool_sel[] array.

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index eae34954e2..a628a7661a 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c

[ ... ]

> @@ -1685,9 +1685,11 @@ eth_dev_mac_restore(struct rte_eth_dev *dev,
>  	if (dev->dev_ops->mac_addr_set != NULL)
>  		dev->dev_ops->mac_addr_set(dev, addr);
>  	else if (dev->dev_ops->mac_addr_add != NULL)
> -		dev->dev_ops->mac_addr_add(dev, addr, 0, pool);
> +		dev->dev_ops->mac_addr_add(dev, addr, 0, 0);
>  
>  	if (dev->dev_ops->mac_addr_add != NULL) {
> +		bool vmdq = (dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_VMDQ_FLAG) != 0;
> +
>  		for (i = 1; i < dev_info->max_mac_addrs; i++) {
>  			addr = &dev->data->mac_addrs[i];
>  
> @@ -1695,15 +1697,19 @@ eth_dev_mac_restore(struct rte_eth_dev *dev,
>  			if (rte_is_zero_ether_addr(addr))
>  				continue;
>  
> -			pool = 0;
> -			pool_mask = dev->data->mac_pool_sel[i];
> -
> -			do {
> -				if (pool_mask & UINT64_C(1))
> -					dev->dev_ops->mac_addr_add(dev, addr, i, pool);
> -				pool_mask >>= 1;
> -				pool++;
> -			} while (pool_mask);
> +			if (!vmdq) {
> +				dev->dev_ops->mac_addr_add(dev, addr, i, 0);
> +			} else {
> +				pool = 0;
> +				pool_mask = dev->data->mac_pool_sel[i];
> +
> +				do {
> +					if (pool_mask & UINT64_C(1))
> +						dev->dev_ops->mac_addr_add(dev, addr, i, pool);
> +					pool_mask >>= 1;
> +					pool++;
> +				} while (pool_mask);
> +			}
>  		}
>  	}
>  }

Does this code skip calling the device op if VMDq is configured but none
of the mac_pool_sel bits are set for a given MAC address?  In
eth_dev_mac_restore()->if(vmdq) path, does the while loop avoid calling
dev->dev_ops->mac_addr_add when pool_mask is initially zero?

> @@ -5432,6 +5439,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) {
> @@ -5442,6 +5455,9 @@ rte_eth_dev_mac_addr_add(uint16_t port_id, struct rte_ether_addr *addr,
>  			return -ENOSPC;
>  		}
>  	} else {
> +		if (!vmdq)
> +			return 0;
> +

Does this path create a situation where mac_pool_sel is not updated but
the code returns success?  When VMDq is disabled and a duplicate MAC
address is added via rte_eth_dev_mac_addr_add, does the driver op get
skipped but the function still returns 0?

> @@ -5456,8 +5472,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);
> +		}
>  	}

Does this code ensure mac_pool_sel array state stays consistent with
dev->data->mac_addrs when vmdq is false?


commit 20260510170306.3406045-4-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

net/iavf: accept up to 32k unicast MAC addresses

E810 hardware provides 32k switch lookups.
Thanks to this, it is possible to allow a lot more secondary mac
addresses than what is possible today.

In practice, the maximum number of macs available per port may be lower
and depends on usage by other (trusted?) VFs on the same PF.
There is no way to figure out this limit but to try adding a mac address
and get an error from the PF driver.

Mailbox exchanges are limited to IAVF_AQ_BUF_SZ, segment messages
accordingly.

> diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
> index 08dd6f2d7f..d0fc8dd54b 100644
> --- a/drivers/net/intel/iavf/iavf_vchnl.c
> +++ b/drivers/net/intel/iavf/iavf_vchnl.c

[ ... ]

> @@ -1437,48 +1437,97 @@ iavf_config_irq_map_lv(struct iavf_adapter *adapter, uint16_t num)
>  	return 0;
>  }
>  
> -void
> -iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
> +static int
> +iavf_add_del_uc_addr_bulk(struct iavf_adapter *adapter, struct rte_ether_addr *addrs,
> +			  uint32_t nb_addrs, bool add)
>  {
> +#define IAVF_ETH_ADDR_PER_REQ \
> +	((IAVF_AQ_BUF_SZ - sizeof(struct virtchnl_ether_addr_list)) / \
> +	 sizeof(struct virtchnl_ether_addr))
>  	struct {
>  		struct virtchnl_ether_addr_list list;
> -		struct virtchnl_ether_addr addr[IAVF_NUM_MACADDR_MAX];
> -	} list_req = {0};
> -	struct virtchnl_ether_addr_list *list = &list_req.list;
> +		struct virtchnl_ether_addr addr[IAVF_ETH_ADDR_PER_REQ];
> +	} cmd_buffer;
> +#undef IAVF_ETH_ADDR_PER_REQ
> +	struct virtchnl_ether_addr_list *list = &cmd_buffer.list;
>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> -	struct iavf_cmd_info args = {0};
> -	int err, i;
> -	size_t buf_len;
>  
> -	for (i = 0; i < IAVF_NUM_MACADDR_MAX; i++) {
> -		struct rte_ether_addr *addr = &adapter->dev_data->mac_addrs[i];
> -		struct virtchnl_ether_addr *vc_addr = &list->list[list->num_elements];
> +	for (uint32_t i = 0; i < nb_addrs; i++) {
> +		struct iavf_cmd_info args;
> +		uint32_t batch;
> +		int err;
>  
> -		/* ignore empty addresses */
> -		if (rte_is_zero_ether_addr(addr))
> -			continue;
> +		batch = i % RTE_DIM(cmd_buffer.addr);
> +
> +		if (batch == 0) {
> +			memset(&cmd_buffer, 0, sizeof(cmd_buffer));
> +			list->vsi_id = vf->vsi_res->vsi_id;
> +			list->num_elements = 0;
> +		}
> +
> +		rte_memcpy(list->list[batch].addr, addrs[i].addr_bytes,
> +			   sizeof(list->list[batch].addr));
> +		list->list[batch].type = VIRTCHNL_ETHER_ADDR_EXTRA;
>  		list->num_elements++;
>  
> -		memcpy(vc_addr->addr, addr->addr_bytes, sizeof(addr->addr_bytes));
> -		vc_addr->type = (list->num_elements == 1) ?
> -				VIRTCHNL_ETHER_ADDR_PRIMARY :
> -				VIRTCHNL_ETHER_ADDR_EXTRA;
> +		if (batch != RTE_DIM(cmd_buffer.addr) - 1 && i != nb_addrs - 1)
> +			continue;
> +
> +		memset(&args, 0, sizeof(args));
> +		args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR : VIRTCHNL_OP_DEL_ETH_ADDR;
> +		args.in_args = (uint8_t *)list;
> +		args.in_args_size = sizeof(struct virtchnl_ether_addr_list) +
> +			sizeof(struct virtchnl_ether_addr) * list->num_elements;
> +		args.out_buffer = vf->aq_resp;
> +		args.out_size = IAVF_AQ_BUF_SZ;
> +		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
> +		if (err != 0) {
> +			PMD_DRV_LOG(ERR, "fail to execute command %s for %u macs",
> +				add ? "VIRTCHNL_OP_ADD_ETH_ADDR" : "VIRTCHNL_OP_DEL_ETH_ADDR",
> +				list->num_elements);
> +			return err;
> +		}

Does this code leak partially installed MAC addresses if
iavf_execute_vf_cmd_safe fails partway through a sequence of batches?

When iavf_add_del_uc_addr_bulk is called with add=true for 1000
addresses and the 10th batch fails, do the first 9 batches remain
installed without any cleanup or rollback?

> +void
> +iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
> +{
> +	int start = -1;
> +	int i;
> +
> +	/* Handle primary address (index 0) separately */
> +	if (!rte_is_zero_ether_addr(&adapter->dev_data->mac_addrs[0]))
> +		iavf_add_del_eth_addr(adapter, &adapter->dev_data->mac_addrs[0], add,
> +			VIRTCHNL_ETHER_ADDR_PRIMARY);
> +
> +	/* Process secondary addresses in contiguous blocks */
> +	for (i = 1; i < IAVF_UC_MACADDR_MAX; i++) {
> +		struct rte_ether_addr *addr = &adapter->dev_data->mac_addrs[i];
> +
> +		if (!rte_is_zero_ether_addr(addr)) {
> +			if (start == -1)
> +				start = i;
> +			continue;
> +		}
> +
> +		if (start != -1) {
> +			iavf_add_del_uc_addr_bulk(adapter, &adapter->dev_data->mac_addrs[start],
> +				i - start, add);
> +			start = -1;
> +		}
> +	}
> +
> +	if (start != -1) {
> +		iavf_add_del_uc_addr_bulk(adapter, &adapter->dev_data->mac_addrs[start],
> +			i - start, add);
> +	}

Does this function check the return values from iavf_add_del_eth_addr
and iavf_add_del_uc_addr_bulk?

Can iavf_add_del_all_mac_addr silently ignore errors from the bulk add
operations during device configuration?


commit 20260510170306.3406045-6-david.marchand at redhat.com
Author: David Marchand <david.marchand at redhat.com>

net/iavf: fix duplicate MAC addresses install

On port restart, all MAC addresses get pushed *twice* to the hardware,
once by the driver and once by the eth_dev_mac_restore() in ethdev.

On the other hand, MAC address filters are reset in the hardware
by the PF only when a VF reset is triggered.

Strictly speaking, the mac restore on port (re)start is unneeded,
if no VF reset happened, so we can announce to ethdev that no mac
restoration is needed via a get_restore_flags callback.

Then, move the mac restoration to the VF reset handler.

> diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
> index edbbc34cc5..504eabebe7 100644
> --- a/drivers/net/intel/iavf/iavf_ethdev.c
> +++ b/drivers/net/intel/iavf/iavf_ethdev.c

[ ... ]

> @@ -1039,15 +1049,14 @@ iavf_dev_start(struct