[PATCH] igb: Allow to remove administratively set MAC on VFs

[PATCH] igb: Allow to remove administratively set MAC on VFs

[Corinna Vinschen]

  Before libvirt modifies the MAC address and vlan tag for an SRIOV VF
  for use by a virtual machine (either using vfio device assignment or
  macvtap passthru mode), it saves the current MAC address and vlan tag
  so that it can reset them to their original value when the guest is
  done.  Libvirt can't leave the VF MAC set to the value used by the
  now-defunct guest since it may be started again later using a
  different VF, but it certainly shouldn't just pick any random value,
  either. So it saves the state of everything prior to using the VF, and
  resets it to that.

  The igb driver initializes the MAC addresses of all VFs to
  00:00:00:00:00:00, and reports that when asked (via an RTM_GETLINK
  netlink message, also visible in the list of VFs in the output of "ip
  link show"). But when libvirt attempts to restore the MAC address back
  to 00:00:00:00:00:00 (using an RTM_SETLINK netlink message) the kernel
  responds with "Invalid argument".

  Forbidding a reset back to the original value leaves the VF MAC at the
  value set for the now-defunct virtual machine. Especially on a system
  with NetworkManager enabled, this has very bad consequences, since
  NetworkManager forces all interfacess to be IFF_UP all the time - if
  the same virtual machine is restarted using a different VF (or even on
  a different host), there will be multiple interfaces watching for
  traffic with the same MAC address.

  To allow libvirt to revert to the original state, we need a way to
  remove the administrative set MAC on a VF, to allow normal host
  operation again, and to reset/overwrite the VF MAC via VF netdev.

  This patch implements the outlined scenario by allowing to set the
  VF MAC to 00:00:00:00:00:00 via RTM_SETLINK on the PF.
  igb_ndo_set_vf_mac resets the IGB_VF_FLAG_PF_SET_MAC flag to 0,
  so it's possible to reset the VF MAC back to the original value via
  the VF netdev.

  Note: Recent patches to libvirt allow for a workaround if the NIC
  isn't capable of resetting the administrative MAC back to all 0, but
  in theory the NIC should allow resetting the MAC in the fisr place.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 26a821f..e7a61b1 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8125,12 +8125,27 @@ static int igb_set_vf_mac(struct igb_adapter *adapter,
 static int igb_ndo_set_vf_mac(struct net_device *netdev, int vf, u8 *mac)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
-	if (!is_valid_ether_addr(mac) || (vf >= adapter->vfs_allocated_count))
+
+	if (vf >= adapter->vfs_allocated_count)
+		return -EINVAL;
+	/* Setting the VF MAC to 0 reverts the IGB_VF_FLAG_PF_SET_MAC
+	   flag and allows to overwrite the MAC via VF netdev.  This
+	   is necessary to allow libvirt a way to restore the original
+	   MAC after unbinding vfio-pci and reloading igbvf after shutting
+	   down a VM. */
+	if (is_zero_ether_addr(mac)) {
+		adapter->vf_data[vf].flags &= ~IGB_VF_FLAG_PF_SET_MAC;
+		dev_info(&adapter->pdev->dev,
+			 "remove administratively set MAC on VF %d\n",
+			 vf);
+	} else if (is_valid_ether_addr (mac)) {
+		adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
+		dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n",
+			 mac, vf);
+		dev_info(&adapter->pdev->dev,
+			 "Reload the VF driver to make this change effective.");
+	} else
 		return -EINVAL;
-	adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
-	dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n", mac, vf);
-	dev_info(&adapter->pdev->dev,
-		 "Reload the VF driver to make this change effective.");
 	if (test_bit(__IGB_DOWN, &adapter->state)) {
 		dev_warn(&adapter->pdev->dev,
 			 "The VF MAC address has been set, but the PF device is not up.\n");
-- 
2.9.3

RE: [Intel-wired-lan] [PATCH] igb: Allow to remove administratively set MAC on VFs

[Duyck, Alexander H]

> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Corinna Vinschen
> Sent: Tuesday, April 4, 2017 8:11 AM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH] igb: Allow to remove administratively set MAC
> on VFs
> 
>   Before libvirt modifies the MAC address and vlan tag for an SRIOV VF
>   for use by a virtual machine (either using vfio device assignment or
>   macvtap passthru mode), it saves the current MAC address and vlan tag
>   so that it can reset them to their original value when the guest is
>   done.  Libvirt can't leave the VF MAC set to the value used by the
>   now-defunct guest since it may be started again later using a
>   different VF, but it certainly shouldn't just pick any random value,
>   either. So it saves the state of everything prior to using the VF, and
>   resets it to that.
> 
>   The igb driver initializes the MAC addresses of all VFs to
>   00:00:00:00:00:00, and reports that when asked (via an RTM_GETLINK
>   netlink message, also visible in the list of VFs in the output of "ip
>   link show"). But when libvirt attempts to restore the MAC address back
>   to 00:00:00:00:00:00 (using an RTM_SETLINK netlink message) the kernel
>   responds with "Invalid argument".
> 
>   Forbidding a reset back to the original value leaves the VF MAC at the
>   value set for the now-defunct virtual machine. Especially on a system
>   with NetworkManager enabled, this has very bad consequences, since
>   NetworkManager forces all interfacess to be IFF_UP all the time - if
>   the same virtual machine is restarted using a different VF (or even on
>   a different host), there will be multiple interfaces watching for
>   traffic with the same MAC address.
> 
>   To allow libvirt to revert to the original state, we need a way to
>   remove the administrative set MAC on a VF, to allow normal host
>   operation again, and to reset/overwrite the VF MAC via VF netdev.
> 
>   This patch implements the outlined scenario by allowing to set the
>   VF MAC to 00:00:00:00:00:00 via RTM_SETLINK on the PF.
>   igb_ndo_set_vf_mac resets the IGB_VF_FLAG_PF_SET_MAC flag to 0,
>   so it's possible to reset the VF MAC back to the original value via
>   the VF netdev.
> 
>   Note: Recent patches to libvirt allow for a workaround if the NIC
>   isn't capable of resetting the administrative MAC back to all 0, but
>   in theory the NIC should allow resetting the MAC in the fisr place.

Minor typo here. I assume you mean "first place".

> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index 26a821f..e7a61b1 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8125,12 +8125,27 @@ static int igb_set_vf_mac(struct igb_adapter
> *adapter,  static int igb_ndo_set_vf_mac(struct net_device *netdev, int vf, u8
> *mac)  {
>  	struct igb_adapter *adapter = netdev_priv(netdev);
> -	if (!is_valid_ether_addr(mac) || (vf >= adapter->vfs_allocated_count))
> +
> +	if (vf >= adapter->vfs_allocated_count)
> +		return -EINVAL;

I would add an blank line here just for readability.

> +	/* Setting the VF MAC to 0 reverts the IGB_VF_FLAG_PF_SET_MAC
> +	   flag and allows to overwrite the MAC via VF netdev.  This
> +	   is necessary to allow libvirt a way to restore the original
> +	   MAC after unbinding vfio-pci and reloading igbvf after shutting
> +	   down a VM. */

Minor coding style issue here. The "*/" should be on a separate line.

> +	if (is_zero_ether_addr(mac)) {
> +		adapter->vf_data[vf].flags &= ~IGB_VF_FLAG_PF_SET_MAC;
> +		dev_info(&adapter->pdev->dev,
> +			 "remove administratively set MAC on VF %d\n",
> +			 vf);
> +	} else if (is_valid_ether_addr (mac)) {
> +		adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
> +		dev_info(&adapter->pdev->dev, "setting MAC %pM on VF
> %d\n",
> +			 mac, vf);
> +		dev_info(&adapter->pdev->dev,
> +			 "Reload the VF driver to make this change effective.");
> +	} else
>  		return -EINVAL;

Minor coding style issue here. The else should also have "{}" wrapping the statement. Generally if any one of the statements in an if/else series needs the braces they should all have the braces.

> -	adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
> -	dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n", mac,
> vf);
> -	dev_info(&adapter->pdev->dev,
> -		 "Reload the VF driver to make this change effective.");
>  	if (test_bit(__IGB_DOWN, &adapter->state)) {

You might need to change this to allow us to clear the VF MAC address while the PF is down without the message. It doesn't add much in this case and allowing us to clear it would make sense.

>  		dev_warn(&adapter->pdev->dev,
>  			 "The VF MAC address has been set, but the PF device is
> not up.\n");
> --
> 2.9.3
> 
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@lists.osuosl.org
> http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [PATCH] igb: Allow to remove administratively set MAC on VFs

[Alexander Duyck]

On Tue, Apr 4, 2017 at 10:16 AM, Duyck, Alexander H
<alexander.h.duyck@intel.com> wrote:
>> -----Original Message-----
>> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
>> Behalf Of Corinna Vinschen
>> Sent: Tuesday, April 4, 2017 8:11 AM
>> To: intel-wired-lan@lists.osuosl.org
>> Cc: netdev@vger.kernel.org
>> Subject: [Intel-wired-lan] [PATCH] igb: Allow to remove administratively set MAC
>> on VFs
>>
>>   Before libvirt modifies the MAC address and vlan tag for an SRIOV VF
>>   for use by a virtual machine (either using vfio device assignment or
>>   macvtap passthru mode), it saves the current MAC address and vlan tag
>>   so that it can reset them to their original value when the guest is
>>   done.  Libvirt can't leave the VF MAC set to the value used by the
>>   now-defunct guest since it may be started again later using a
>>   different VF, but it certainly shouldn't just pick any random value,
>>   either. So it saves the state of everything prior to using the VF, and
>>   resets it to that.
>>
>>   The igb driver initializes the MAC addresses of all VFs to
>>   00:00:00:00:00:00, and reports that when asked (via an RTM_GETLINK
>>   netlink message, also visible in the list of VFs in the output of "ip
>>   link show"). But when libvirt attempts to restore the MAC address back
>>   to 00:00:00:00:00:00 (using an RTM_SETLINK netlink message) the kernel
>>   responds with "Invalid argument".
>>
>>   Forbidding a reset back to the original value leaves the VF MAC at the
>>   value set for the now-defunct virtual machine. Especially on a system
>>   with NetworkManager enabled, this has very bad consequences, since
>>   NetworkManager forces all interfacess to be IFF_UP all the time - if
>>   the same virtual machine is restarted using a different VF (or even on
>>   a different host), there will be multiple interfaces watching for
>>   traffic with the same MAC address.
>>
>>   To allow libvirt to revert to the original state, we need a way to
>>   remove the administrative set MAC on a VF, to allow normal host
>>   operation again, and to reset/overwrite the VF MAC via VF netdev.
>>
>>   This patch implements the outlined scenario by allowing to set the
>>   VF MAC to 00:00:00:00:00:00 via RTM_SETLINK on the PF.
>>   igb_ndo_set_vf_mac resets the IGB_VF_FLAG_PF_SET_MAC flag to 0,
>>   so it's possible to reset the VF MAC back to the original value via
>>   the VF netdev.
>>
>>   Note: Recent patches to libvirt allow for a workaround if the NIC
>>   isn't capable of resetting the administrative MAC back to all 0, but
>>   in theory the NIC should allow resetting the MAC in the fisr place.
>
> Minor typo here. I assume you mean "first place".
>
>> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
>> ---
>>  drivers/net/ethernet/intel/igb/igb_main.c | 25 ++++++++++++++++++++-----
>>  1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
>> b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 26a821f..e7a61b1 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -8125,12 +8125,27 @@ static int igb_set_vf_mac(struct igb_adapter
>> *adapter,  static int igb_ndo_set_vf_mac(struct net_device *netdev, int vf, u8
>> *mac)  {
>>       struct igb_adapter *adapter = netdev_priv(netdev);
>> -     if (!is_valid_ether_addr(mac) || (vf >= adapter->vfs_allocated_count))
>> +
>> +     if (vf >= adapter->vfs_allocated_count)
>> +             return -EINVAL;
>
> I would add an blank line here just for readability.
>
>> +     /* Setting the VF MAC to 0 reverts the IGB_VF_FLAG_PF_SET_MAC
>> +        flag and allows to overwrite the MAC via VF netdev.  This
>> +        is necessary to allow libvirt a way to restore the original
>> +        MAC after unbinding vfio-pci and reloading igbvf after shutting
>> +        down a VM. */
>
> Minor coding style issue here. The "*/" should be on a separate line.
>
>> +     if (is_zero_ether_addr(mac)) {
>> +             adapter->vf_data[vf].flags &= ~IGB_VF_FLAG_PF_SET_MAC;
>> +             dev_info(&adapter->pdev->dev,
>> +                      "remove administratively set MAC on VF %d\n",
>> +                      vf);
>> +     } else if (is_valid_ether_addr (mac)) {
>> +             adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
>> +             dev_info(&adapter->pdev->dev, "setting MAC %pM on VF
>> %d\n",
>> +                      mac, vf);
>> +             dev_info(&adapter->pdev->dev,
>> +                      "Reload the VF driver to make this change effective.");
>> +     } else
>>               return -EINVAL;
>
> Minor coding style issue here. The else should also have "{}" wrapping the statement. Generally if any one of the statements in an if/else series needs the braces they should all have the braces.
>
>> -     adapter->vf_data[vf].flags |= IGB_VF_FLAG_PF_SET_MAC;
>> -     dev_info(&adapter->pdev->dev, "setting MAC %pM on VF %d\n", mac,
>> vf);
>> -     dev_info(&adapter->pdev->dev,
>> -              "Reload the VF driver to make this change effective.");
>>       if (test_bit(__IGB_DOWN, &adapter->state)) {
>
> You might need to change this to allow us to clear the VF MAC address while the PF is down without the message. It doesn't add much in this case and allowing us to clear it would make sense.
>
>>               dev_warn(&adapter->pdev->dev,
>>                        "The VF MAC address has been set, but the PF device is
>> not up.\n");

So I just realized there is one other minor issue I just found. In
igb_rar_set_qsel you should probably add a check for
"is_valid_ether_addr(addr)" before you set the E1000_RAH_AV bit. For
the zeroed MAC address it should be cleared so that we aren't
filtering on a MAC address of all 0's for the VF.

- Alex

Re: [Intel-wired-lan] [PATCH] igb: Allow to remove administratively set MAC on VFs

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]

Hi Alex,

On Apr  4 10:33, Alexander Duyck wrote:
> On Tue, Apr 4, 2017 at 10:16 AM, Duyck, Alexander H
> <alexander.h.duyck@intel.com> wrote:
> >> -----Original Message-----
> >> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> >> Behalf Of Corinna Vinschen
> >> Sent: Tuesday, April 4, 2017 8:11 AM
> >> To: intel-wired-lan@lists.osuosl.org
> >> Cc: netdev@vger.kernel.org
> >> Subject: [Intel-wired-lan] [PATCH] igb: Allow to remove administratively set MAC
> >> on VFs
> >> [...]
> 
> So I just realized there is one other minor issue I just found. In
> igb_rar_set_qsel you should probably add a check for
> "is_valid_ether_addr(addr)" before you set the E1000_RAH_AV bit. For
> the zeroed MAC address it should be cleared so that we aren't
> filtering on a MAC address of all 0's for the VF.
> 
> - Alex

I see your point, but I'm a bit reluctant to do that because
igb_vf_configure() calls igb_set_vf_mac() with addr set to the
zeroed MAC explicitely:


  eth_zero_addr(mac_addr);
  igb_set_vf_mac(adapter, vf, mac_addr);

So in this case the zero MAC is already treated as valid address
and the E1000_RAH_AV bit is set.  Is that just a bug?


Corinna

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]