[PATCH] net/iavf: fix mac address with i40e PF Linux driver

[PATCH] net/iavf: fix mac address with i40e PF Linux driver

[David Marchand]

Following an upstream Linux kernel change (see link), the mac address of
a iavf port, serviced by a i40e PF driver, is reset to 0 during the
VIRTCHNL_OP_GET_VF_RESOURCES query.

The DPDK iavf driver then assigns a new random mac address.

Such sequence is triggered every time the VF is initialised which means
that, from a DPDK application point of view, the mac address gets changed
for every VF reset event. And obviously, two runs of a DPDK application
get a different mac address.

The i40e PF driver change is pretty obscure but the iavf Linux driver does
set VIRTCHNL_VF_OFFLOAD_USO.
Announcing such a capability in the DPDK driver does not seem to be an
issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.

Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266

Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_vchnl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 6d5969f084..a894a80007 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -710,6 +710,7 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
 		VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF |
 		VIRTCHNL_VF_OFFLOAD_FSUB_PF |
 		VIRTCHNL_VF_OFFLOAD_REQ_QUEUES |
+		VIRTCHNL_VF_OFFLOAD_USO |
 		VIRTCHNL_VF_OFFLOAD_CRC |
 		VIRTCHNL_VF_OFFLOAD_VLAN_V2 |
 		VIRTCHNL_VF_LARGE_NUM_QPAIRS |
-- 
2.46.0

[PATCH v2] net/iavf: preserve MAC address with i40e PF Linux driver

[David Marchand]

Following two upstream Linux kernel changes (see links), the mac address
of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
driver probes the port again (which may be triggered at any point of a
DPDK application life, like when a reset event is triggered by the PF).

A first change results in the mac address of the VF port being reset to 0
during the VIRTCHNL_OP_GET_VF_RESOURCES query.
The i40e PF driver change is pretty obscure but the iavf Linux driver does
set VIRTCHNL_VF_OFFLOAD_USO.
Announcing such a capability in the DPDK driver does not seem to be an
issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.

Then a second change in the kernel results in the VF mac address being
cleared when the VF driver remove its default mac address.
Removing (unicast or multicast) mac addresses is not done by the kernel VF
driver in general.
The reason why the DPDK driver behaves like this is undocumented
(and lost because the authors are not active anymore).
Aligning DPDK behavior to the upstream kernel driver is safer in any
case.

Cc: stable@dpdk.org

Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_ethdev.c | 22 +++++-----------------
 drivers/net/iavf/iavf_vchnl.c  |  1 +
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index 11036bc179..97cdb1cbe0 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1044,7 +1044,7 @@ iavf_dev_start(struct rte_eth_dev *dev)
 		if (iavf_configure_queues(adapter,
 				IAVF_CFG_Q_NUM_PER_BUF, index) != 0) {
 			PMD_DRV_LOG(ERR, "configure queues failed");
-			goto err_queue;
+			goto error;
 		}
 		num_queue_pairs -= IAVF_CFG_Q_NUM_PER_BUF;
 		index += IAVF_CFG_Q_NUM_PER_BUF;
@@ -1052,12 +1052,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
 
 	if (iavf_configure_queues(adapter, num_queue_pairs, index) != 0) {
 		PMD_DRV_LOG(ERR, "configure queues failed");
-		goto err_queue;
+		goto error;
 	}
 
 	if (iavf_config_rx_queues_irqs(dev, intr_handle) != 0) {
 		PMD_DRV_LOG(ERR, "configure irq failed");
-		goto err_queue;
+		goto error;
 	}
 	/* re-enable intr again, because efd assign may change */
 	if (dev->data->dev_conf.intr_conf.rxq != 0) {
@@ -1077,14 +1077,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
 
 	if (iavf_start_queues(dev) != 0) {
 		PMD_DRV_LOG(ERR, "enable queues failed");
-		goto err_mac;
+		goto error;
 	}
 
 	return 0;
 
-err_mac:
-	iavf_add_del_all_mac_addr(adapter, false);
-err_queue:
+error:
 	return -1;
 }
 
@@ -1113,16 +1111,6 @@ iavf_dev_stop(struct rte_eth_dev *dev)
 	/* Rx interrupt vector mapping free */
 	rte_intr_vec_list_free(intr_handle);
 
-	/* adminq will be disabled when vf is resetting. */
-	if (!vf->in_reset_recovery) {
-		/* remove all mac addrs */
-		iavf_add_del_all_mac_addr(adapter, false);
-
-		/* remove all multicast addresses */
-		iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
-				  false);
-	}
-
 	iavf_stop_queues(dev);
 
 	adapter->stopped = 1;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 69420bc9b6..065ab3594c 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -710,6 +710,7 @@ iavf_get_vf_resource(struct iavf_adapter *adapter)
 		VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF |
 		VIRTCHNL_VF_OFFLOAD_FSUB_PF |
 		VIRTCHNL_VF_OFFLOAD_REQ_QUEUES |
+		VIRTCHNL_VF_OFFLOAD_USO |
 		VIRTCHNL_VF_OFFLOAD_CRC |
 		VIRTCHNL_VF_OFFLOAD_VLAN_V2 |
 		VIRTCHNL_VF_LARGE_NUM_QPAIRS |
-- 
2.46.2

Re: [PATCH v2] net/iavf: preserve MAC address with i40e PF Linux driver

[Bruce Richardson]

On Tue, Oct 01, 2024 at 11:12:54AM +0200, David Marchand wrote:
> Following two upstream Linux kernel changes (see links), the mac address
> of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
> driver probes the port again (which may be triggered at any point of a
> DPDK application life, like when a reset event is triggered by the PF).
> 
> A first change results in the mac address of the VF port being reset to 0
> during the VIRTCHNL_OP_GET_VF_RESOURCES query.
> The i40e PF driver change is pretty obscure but the iavf Linux driver does
> set VIRTCHNL_VF_OFFLOAD_USO.
> Announcing such a capability in the DPDK driver does not seem to be an
> issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.
> 
> Then a second change in the kernel results in the VF mac address being
> cleared when the VF driver remove its default mac address.
> Removing (unicast or multicast) mac addresses is not done by the kernel VF
> driver in general.
> The reason why the DPDK driver behaves like this is undocumented
> (and lost because the authors are not active anymore).
> Aligning DPDK behavior to the upstream kernel driver is safer in any
> case.
> 

One question inline below.

/Bruce

> Cc: stable@dpdk.org
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 22 +++++-----------------
>  drivers/net/iavf/iavf_vchnl.c  |  1 +
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index 11036bc179..97cdb1cbe0 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -1044,7 +1044,7 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  		if (iavf_configure_queues(adapter,
>  				IAVF_CFG_Q_NUM_PER_BUF, index) != 0) {
>  			PMD_DRV_LOG(ERR, "configure queues failed");
> -			goto err_queue;
> +			goto error;
>  		}
>  		num_queue_pairs -= IAVF_CFG_Q_NUM_PER_BUF;
>  		index += IAVF_CFG_Q_NUM_PER_BUF;
> @@ -1052,12 +1052,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  
>  	if (iavf_configure_queues(adapter, num_queue_pairs, index) != 0) {
>  		PMD_DRV_LOG(ERR, "configure queues failed");
> -		goto err_queue;
> +		goto error;
>  	}
>  
>  	if (iavf_config_rx_queues_irqs(dev, intr_handle) != 0) {
>  		PMD_DRV_LOG(ERR, "configure irq failed");
> -		goto err_queue;
> +		goto error;
>  	}
>  	/* re-enable intr again, because efd assign may change */
>  	if (dev->data->dev_conf.intr_conf.rxq != 0) {
> @@ -1077,14 +1077,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  
>  	if (iavf_start_queues(dev) != 0) {
>  		PMD_DRV_LOG(ERR, "enable queues failed");
> -		goto err_mac;
> +		goto error;
>  	}
>  
>  	return 0;
>  
> -err_mac:
> -	iavf_add_del_all_mac_addr(adapter, false);
> -err_queue:
> +error:
>  	return -1;
>  }
>  
> @@ -1113,16 +1111,6 @@ iavf_dev_stop(struct rte_eth_dev *dev)
>  	/* Rx interrupt vector mapping free */
>  	rte_intr_vec_list_free(intr_handle);
>  
> -	/* adminq will be disabled when vf is resetting. */
> -	if (!vf->in_reset_recovery) {
> -		/* remove all mac addrs */
> -		iavf_add_del_all_mac_addr(adapter, false);
> -
> -		/* remove all multicast addresses */
> -		iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
> -				  false);
> -	}
> -

Question on this: while I understand we don't want to remove the default
mac address, should all other non-default macs not still be removed?

Re: [PATCH v2] net/iavf: preserve MAC address with i40e PF Linux driver

[David Marchand]

On Tue, Oct 1, 2024 at 12:09 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > @@ -1113,16 +1111,6 @@ iavf_dev_stop(struct rte_eth_dev *dev)
> >       /* Rx interrupt vector mapping free */
> >       rte_intr_vec_list_free(intr_handle);
> >
> > -     /* adminq will be disabled when vf is resetting. */
> > -     if (!vf->in_reset_recovery) {
> > -             /* remove all mac addrs */
> > -             iavf_add_del_all_mac_addr(adapter, false);
> > -
> > -             /* remove all multicast addresses */
> > -             iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
> > -                               false);
> > -     }
> > -
>
> Question on this: while I understand we don't want to remove the default
> mac address, should all other non-default macs not still be removed?

I would say that we don't care.
Right after this, the VF driver sends a VF reset (following your
recent change), and the PF flushes any mac associated to the VF from
hw filters.

The kernel VF driver does nothing about mac addresses when uninitialising.


-- 
David Marchand

Re: [PATCH v2] net/iavf: preserve MAC address with i40e PF Linux driver

[Bruce Richardson]

On Tue, Oct 01, 2024 at 11:12:54AM +0200, David Marchand wrote:
> Following two upstream Linux kernel changes (see links), the mac address
> of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
> driver probes the port again (which may be triggered at any point of a
> DPDK application life, like when a reset event is triggered by the PF).
> 
> A first change results in the mac address of the VF port being reset to 0
> during the VIRTCHNL_OP_GET_VF_RESOURCES query.
> The i40e PF driver change is pretty obscure but the iavf Linux driver does
> set VIRTCHNL_VF_OFFLOAD_USO.
> Announcing such a capability in the DPDK driver does not seem to be an
> issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.
> 
> Then a second change in the kernel results in the VF mac address being
> cleared when the VF driver remove its default mac address.
> Removing (unicast or multicast) mac addresses is not done by the kernel VF
> driver in general.
> The reason why the DPDK driver behaves like this is undocumented
> (and lost because the authors are not active anymore).
> Aligning DPDK behavior to the upstream kernel driver is safer in any
> case.
> 
> Cc: stable@dpdk.org
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Seems reasonable to me.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>