[dpdk-dev] [PATCH] e1000: fix setting of VF MAC address

[dpdk-dev] [PATCH] e1000: fix setting of VF MAC address

[Bernard Iremonger]

Allow reprogramming of the RAR with a zero mac address,
to ensure that the VF traffic goes to the PF after
stop, close and detach of the VF.

Fixes: be2d648a2dd3 ("igb: add PF support")
Fixes: d82170d27918 ("igb: add VF support")
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 drivers/net/e1000/igb_ethdev.c | 12 +++++++++++-
 drivers/net/e1000/igb_pf.c     |  8 +++++---
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
index 4ed5e95..f1044b7 100644
--- a/drivers/net/e1000/igb_ethdev.c
+++ b/drivers/net/e1000/igb_ethdev.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -2819,6 +2819,7 @@ igbvf_dev_close(struct rte_eth_dev *dev)
 	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct e1000_adapter *adapter =
 		E1000_DEV_PRIVATE(dev->data->dev_private);
+	struct ether_addr addr;
 
 	PMD_INIT_FUNC_TRACE();
 
@@ -2827,6 +2828,15 @@ igbvf_dev_close(struct rte_eth_dev *dev)
 	igbvf_dev_stop(dev);
 	adapter->stopped = 1;
 	igb_dev_free_queues(dev);
+
+	/**
+	 * reprogram the RAR with a zero mac address,
+	 * to ensure that the VF traffic goes to the PF
+	 * after stop, close and detach of the VF.
+	 **/
+
+	memset(&addr, 0, sizeof(addr));
+	igbvf_default_mac_addr_set(dev, &addr);
 }
 
 static int igbvf_set_vfta(struct e1000_hw *hw, uint16_t vid, bool on)
diff --git a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c
index 1d00dda..95204e9 100644
--- a/drivers/net/e1000/igb_pf.c
+++ b/drivers/net/e1000/igb_pf.c
@@ -1,7 +1,7 @@
 /*-
  *   BSD LICENSE
  *
- *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
+ *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
  *   All rights reserved.
  *
  *   Redistribution and use in source and binary forms, with or without
@@ -332,8 +332,10 @@ igb_vf_set_mac_addr(struct rte_eth_dev *dev, uint32_t vf, uint32_t *msgbuf)
 	int rar_entry = hw->mac.rar_entry_count - (vf + 1);
 	uint8_t *new_mac = (uint8_t *)(&msgbuf[1]);
 
-	if (is_valid_assigned_ether_addr((struct ether_addr*)new_mac)) {
-		rte_memcpy(vfinfo[vf].vf_mac_addresses, new_mac, 6);
+	if (is_unicast_ether_addr((struct ether_addr *)new_mac)) {
+		if (!is_zero_ether_addr((struct ether_addr *)new_mac))
+			rte_memcpy(vfinfo[vf].vf_mac_addresses, new_mac,
+				sizeof(vfinfo[vf].vf_mac_addresses));
 		hw->mac.ops.rar_set(hw, new_mac, rar_entry);
 		return 0;
 	}
-- 
2.6.3

Re: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address

[Lu, Wenzhuo]

Hi Bernard,


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bernard Iremonger
> Sent: Thursday, March 3, 2016 12:09 AM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address
> 
> Allow reprogramming of the RAR with a zero mac address, to ensure that the VF
> traffic goes to the PF after stop, close and detach of the VF.
> 
> Fixes: be2d648a2dd3 ("igb: add PF support")
> Fixes: d82170d27918 ("igb: add VF support")
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  drivers/net/e1000/igb_ethdev.c | 12 +++++++++++-
>  drivers/net/e1000/igb_pf.c     |  8 +++++---
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/e1000/igb_ethdev.c b/drivers/net/e1000/igb_ethdev.c
> index 4ed5e95..f1044b7 100644
> --- a/drivers/net/e1000/igb_ethdev.c
> +++ b/drivers/net/e1000/igb_ethdev.c
> @@ -1,7 +1,7 @@
>  /*-
>   *   BSD LICENSE
>   *
> - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
>   *   All rights reserved.
>   *
>   *   Redistribution and use in source and binary forms, with or without
> @@ -2819,6 +2819,7 @@ igbvf_dev_close(struct rte_eth_dev *dev)
>  	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data-
> >dev_private);
>  	struct e1000_adapter *adapter =
>  		E1000_DEV_PRIVATE(dev->data->dev_private);
> +	struct ether_addr addr;
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> @@ -2827,6 +2828,15 @@ igbvf_dev_close(struct rte_eth_dev *dev)
>  	igbvf_dev_stop(dev);
>  	adapter->stopped = 1;
>  	igb_dev_free_queues(dev);
> +
> +	/**
> +	 * reprogram the RAR with a zero mac address,
> +	 * to ensure that the VF traffic goes to the PF
> +	 * after stop, close and detach of the VF.
> +	 **/
> +
> +	memset(&addr, 0, sizeof(addr));
> +	igbvf_default_mac_addr_set(dev, &addr);
>  }
> 
>  static int igbvf_set_vfta(struct e1000_hw *hw, uint16_t vid, bool on) diff --git
> a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c index
> 1d00dda..95204e9 100644
> --- a/drivers/net/e1000/igb_pf.c
> +++ b/drivers/net/e1000/igb_pf.c
> @@ -1,7 +1,7 @@
>  /*-
>   *   BSD LICENSE
>   *
> - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
>   *   All rights reserved.
>   *
>   *   Redistribution and use in source and binary forms, with or without
> @@ -332,8 +332,10 @@ igb_vf_set_mac_addr(struct rte_eth_dev *dev,
> uint32_t vf, uint32_t *msgbuf)
>  	int rar_entry = hw->mac.rar_entry_count - (vf + 1);
>  	uint8_t *new_mac = (uint8_t *)(&msgbuf[1]);
> 
> -	if (is_valid_assigned_ether_addr((struct ether_addr*)new_mac)) {
> -		rte_memcpy(vfinfo[vf].vf_mac_addresses, new_mac, 6);
> +	if (is_unicast_ether_addr((struct ether_addr *)new_mac)) {
> +		if (!is_zero_ether_addr((struct ether_addr *)new_mac))
> +			rte_memcpy(vfinfo[vf].vf_mac_addresses, new_mac,
> +				sizeof(vfinfo[vf].vf_mac_addresses));
>  		hw->mac.ops.rar_set(hw, new_mac, rar_entry);
If the new mac is 0, after this, the rar is 0, but the address stored in vfinfo is not changed and surely not 0. Right?
So, they're not align with each other. Could it cause some problem?

>  		return 0;
>  	}
> --
> 2.6.3

Re: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address

[Iremonger, Bernard]

Hi Wenzhuo,

> -----Original Message-----
> From: Lu, Wenzhuo
> Sent: Thursday, March 3, 2016 2:34 AM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address
> 
> Hi Bernard,
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bernard
> Iremonger
> > Sent: Thursday, March 3, 2016 12:09 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address
> >
> > Allow reprogramming of the RAR with a zero mac address, to ensure that
> > the VF traffic goes to the PF after stop, close and detach of the VF.
> >
> > Fixes: be2d648a2dd3 ("igb: add PF support")
> > Fixes: d82170d27918 ("igb: add VF support")
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  drivers/net/e1000/igb_ethdev.c | 12 +++++++++++-
> >  drivers/net/e1000/igb_pf.c     |  8 +++++---
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/e1000/igb_ethdev.c
> > b/drivers/net/e1000/igb_ethdev.c index 4ed5e95..f1044b7 100644
> > --- a/drivers/net/e1000/igb_ethdev.c
> > +++ b/drivers/net/e1000/igb_ethdev.c
> > @@ -1,7 +1,7 @@
> >  /*-
> >   *   BSD LICENSE
> >   *
> > - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> > + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> >   *   All rights reserved.
> >   *
> >   *   Redistribution and use in source and binary forms, with or without
> > @@ -2819,6 +2819,7 @@ igbvf_dev_close(struct rte_eth_dev *dev)
> >  	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data-
> > >dev_private);
> >  	struct e1000_adapter *adapter =
> >  		E1000_DEV_PRIVATE(dev->data->dev_private);
> > +	struct ether_addr addr;
> >
> >  	PMD_INIT_FUNC_TRACE();
> >
> > @@ -2827,6 +2828,15 @@ igbvf_dev_close(struct rte_eth_dev *dev)
> >  	igbvf_dev_stop(dev);
> >  	adapter->stopped = 1;
> >  	igb_dev_free_queues(dev);
> > +
> > +	/**
> > +	 * reprogram the RAR with a zero mac address,
> > +	 * to ensure that the VF traffic goes to the PF
> > +	 * after stop, close and detach of the VF.
> > +	 **/
> > +
> > +	memset(&addr, 0, sizeof(addr));
> > +	igbvf_default_mac_addr_set(dev, &addr);
> >  }
> >
> >  static int igbvf_set_vfta(struct e1000_hw *hw, uint16_t vid, bool on)
> > diff --git a/drivers/net/e1000/igb_pf.c b/drivers/net/e1000/igb_pf.c
> > index
> > 1d00dda..95204e9 100644
> > --- a/drivers/net/e1000/igb_pf.c
> > +++ b/drivers/net/e1000/igb_pf.c
> > @@ -1,7 +1,7 @@
> >  /*-
> >   *   BSD LICENSE
> >   *
> > - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> > + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> >   *   All rights reserved.
> >   *
> >   *   Redistribution and use in source and binary forms, with or without
> > @@ -332,8 +332,10 @@ igb_vf_set_mac_addr(struct rte_eth_dev *dev,
> > uint32_t vf, uint32_t *msgbuf)
> >  	int rar_entry = hw->mac.rar_entry_count - (vf + 1);
> >  	uint8_t *new_mac = (uint8_t *)(&msgbuf[1]);
> >
> > -	if (is_valid_assigned_ether_addr((struct ether_addr*)new_mac)) {
> > -		rte_memcpy(vfinfo[vf].vf_mac_addresses, new_mac, 6);
> > +	if (is_unicast_ether_addr((struct ether_addr *)new_mac)) {
> > +		if (!is_zero_ether_addr((struct ether_addr *)new_mac))
> > +			rte_memcpy(vfinfo[vf].vf_mac_addresses,
> new_mac,
> > +				sizeof(vfinfo[vf].vf_mac_addresses));
> >  		hw->mac.ops.rar_set(hw, new_mac, rar_entry);
> If the new mac is 0, after this, the rar is 0, but the address stored in vfinfo is
> not changed and surely not 0. Right?
> So, they're not align with each other. Could it cause some problem?
> 
> >  		return 0;
> >  	}
> > --
> > 2.6.3

 vfinfo[vf].vf_mac_addresses  contains the perm_addr MAC address.
I do not want to overwrite this with a zero address. If the VF is attached again it uses the perm_addr  MAC address.
The follow sequence works fine for me in testpmd, the VF is port 1:

Testpmd> port stop 1
Testpmd> port close 1 		/* VF MAC address is set to zero here */
Testpmd> port detach 1
Testpmd> port attach 0000:04:10.0  
Testpmd> show port info 1	/* VF MAC address is perm_addr MAC address */

Regards,

Bernard.

Re: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address

[Lu, Wenzhuo]

Hi,

> > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>

> > > ---
> > >  drivers/net/e1000/igb_ethdev.c | 12 +++++++++++-
> > >  drivers/net/e1000/igb_pf.c     |  8 +++++---
> > >  2 files changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/e1000/igb_ethdev.c
> > > b/drivers/net/e1000/igb_ethdev.c index 4ed5e95..f1044b7 100644
> > > --- a/drivers/net/e1000/igb_ethdev.c
> > > +++ b/drivers/net/e1000/igb_ethdev.c
> > > @@ -1,7 +1,7 @@
> > >  /*-
> > >   *   BSD LICENSE
> > >   *
> > > - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> > > + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> > >   *   All rights reserved.
> > >   *
> > >   *   Redistribution and use in source and binary forms, with or without
> > > @@ -2819,6 +2819,7 @@ igbvf_dev_close(struct rte_eth_dev *dev)
> > >  	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data-
> > > >dev_private);
> > >  	struct e1000_adapter *adapter =
> > >  		E1000_DEV_PRIVATE(dev->data->dev_private);
> > > +	struct ether_addr addr;
> > >
> > >  	PMD_INIT_FUNC_TRACE();
> > >
> > > @@ -2827,6 +2828,15 @@ igbvf_dev_close(struct rte_eth_dev *dev)
> > >  	igbvf_dev_stop(dev);
> > >  	adapter->stopped = 1;
> > >  	igb_dev_free_queues(dev);
> > > +
> > > +	/**
> > > +	 * reprogram the RAR with a zero mac address,
> > > +	 * to ensure that the VF traffic goes to the PF
> > > +	 * after stop, close and detach of the VF.
> > > +	 **/
> > > +
> > > +	memset(&addr, 0, sizeof(addr));
> > > +	igbvf_default_mac_addr_set(dev, &addr);
> > >  }
> > >
> > >  static int igbvf_set_vfta(struct e1000_hw *hw, uint16_t vid, bool
> > > on) diff --git a/drivers/net/e1000/igb_pf.c
> > > b/drivers/net/e1000/igb_pf.c index
> > > 1d00dda..95204e9 100644
> > > --- a/drivers/net/e1000/igb_pf.c
> > > +++ b/drivers/net/e1000/igb_pf.c
> > > @@ -1,7 +1,7 @@
> > >  /*-
> > >   *   BSD LICENSE
> > >   *
> > > - *   Copyright(c) 2010-2015 Intel Corporation. All rights reserved.
> > > + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> > >   *   All rights reserved.
> > >   *
> > >   *   Redistribution and use in source and binary forms, with or without
> > > @@ -332,8 +332,10 @@ igb_vf_set_mac_addr(struct rte_eth_dev *dev,
> > > uint32_t vf, uint32_t *msgbuf)
> > >  	int rar_entry = hw->mac.rar_entry_count - (vf + 1);
> > >  	uint8_t *new_mac = (uint8_t *)(&msgbuf[1]);
> > >
> > > -	if (is_valid_assigned_ether_addr((struct ether_addr*)new_mac)) {
> > > -		rte_memcpy(vfinfo[vf].vf_mac_addresses, new_mac, 6);
> > > +	if (is_unicast_ether_addr((struct ether_addr *)new_mac)) {
> > > +		if (!is_zero_ether_addr((struct ether_addr *)new_mac))
> > > +			rte_memcpy(vfinfo[vf].vf_mac_addresses,
> > new_mac,
> > > +				sizeof(vfinfo[vf].vf_mac_addresses));
> > >  		hw->mac.ops.rar_set(hw, new_mac, rar_entry);
> > If the new mac is 0, after this, the rar is 0, but the address stored
> > in vfinfo is not changed and surely not 0. Right?
> > So, they're not align with each other. Could it cause some problem?
> >
> > >  		return 0;
> > >  	}
> > > --
> > > 2.6.3
> 
>  vfinfo[vf].vf_mac_addresses  contains the perm_addr MAC address.
> I do not want to overwrite this with a zero address. If the VF is attached again it
> uses the perm_addr  MAC address.
> The follow sequence works fine for me in testpmd, the VF is port 1:
> 
> Testpmd> port stop 1
> Testpmd> port close 1 		/* VF MAC address is set to zero here */
> Testpmd> port detach 1
> Testpmd> port attach 0000:04:10.0
> Testpmd> show port info 1	/* VF MAC address is perm_addr MAC address
> */
I think you're right. Only when the rar is 0, the vf_mac_addresses is different from rar. So, we can use it to keep the permanent address.

> 
> Regards,
> 
> Bernard.

Re: [dpdk-dev] [PATCH] e1000: fix setting of VF MAC address

[Bruce Richardson]

On Fri, Mar 04, 2016 at 01:16:51AM +0000, Lu, Wenzhuo wrote:
> Hi,
> 
> > > > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> Acked-by: Wenzhuo Lu <wenzhuo.lu@intel.com>
> 
Applied to dpdk-next-net/rel_16_04

/Bruce