[libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00

Laine Stump posted 19 patches 8 years, 11 months ago
There is a newer version of this series
[libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00
Posted by Laine Stump 8 years, 11 months ago
Some PF drivers allow setting the admin MAC (that is the MAC address
that the VF will be initialized to the next time the VF's driver is
loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
initialize the admin MACs to all 0, but don't allow setting it to that
very same value. It has been an uphill battle convincing the driver
people that it's reasonable to expect The argument that's used is
that an all 0 device MAC address on a device is invalid; however, from
an outsider's point of view, when the admin MAC is set to 0 at the
time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
random non-0 value. But that's beside the point - even if I could
convince one or two SRIOV driver maintainers to permit setting the
admin MAC to 0, there are still several other drivers.

So rather than fighting that losing battle, this patch checks for a
failure to set the admin MAC due to an all 0 value, and retries it
with 02:00:00:00:00:00. That won't result in a random value being set
in the VF MAC at next VF driver init, but that's okay, because we
always want to set a specific value anyway. Rather, the "almost 0"
setting makes it easy to visually detect from the output of "ip link
show" which VFs are currently in use and which are free.
---
 src/util/virnetdev.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 2b1cebc..6cf0463 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1387,6 +1387,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED,
 #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
 
 
+static virMacAddr zeroMAC = { 0 };
+
+/* if a net driver doesn't allow setting MAC to all 0, try setting
+ * to this (the only bit that is set is the "locally administered" bit")
+ */
+static virMacAddr altZeroMAC = { .addr = { 0x02, 0, 0, 0, 0, 0, } };
+
+
 static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
     [IFLA_VF_MAC]       = { .type = NLA_UNSPEC,
                             .maxlen = sizeof(struct ifla_vf_mac) },
@@ -1397,7 +1405,8 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
 
 static int
 virNetDevSetVfConfig(const char *ifname, int vf,
-                     const virMacAddr *macaddr, int vlanid)
+                     const virMacAddr *macaddr, int vlanid,
+                     bool *allowRetry)
 {
     int rc = -1;
     struct nlmsghdr *resp = NULL;
@@ -1474,7 +1483,15 @@ virNetDevSetVfConfig(const char *ifname, int vf,
         if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
             goto malformed_resp;
 
-        if (err->error) {
+        /* if allowRetry is true and the error was EINVAL, then
+         * silently return a failure so the caller can retry with a
+         * different MAC address
+         */
+        if (-err->error == EINVAL && *allowRetry &&
+            macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) {
+            goto cleanup;
+        } else if (err->error) {
+            /* other errors are permanent */
             char macstr[VIR_MAC_STRING_BUFLEN];
 
             virReportSystemError(-err->error,
@@ -1486,6 +1503,7 @@ virNetDevSetVfConfig(const char *ifname, int vf,
                                  vlanid,
                                  ifname ? ifname : "(unspecified)",
                                  vf);
+            *allowRetry = false; /* no use retrying */
             goto cleanup;
         }
         break;
@@ -2067,8 +2085,24 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
          * guest or host). if there is a vlanTag to set, it will take
          * effect immediately though.
          */
-        if (virNetDevSetVfConfig(pfDevName, vf, adminMAC, vlanTag) < 0)
-            goto cleanup;
+        bool allowRetry = true;
+
+        if (virNetDevSetVfConfig(pfDevName, vf,
+                                 adminMAC, vlanTag, &allowRetry) < 0) {
+            /* allowRetry will still be true if the failure was due to
+             * trying to set the MAC address to all 0. In that case,
+             * we can retry with "altZeroMAC", which is just an all-0 MAC
+             * with the "locally administered" bit set.
+             */
+            if (!allowRetry)
+                goto cleanup;
+
+            allowRetry = false;
+            if (virNetDevSetVfConfig(pfDevName, vf,
+                                     &altZeroMAC, vlanTag, &allowRetry) < 0) {
+                goto cleanup;
+            }
+        }
     }
 
     ret = 0;
-- 
2.9.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00
Posted by Michal Privoznik 8 years, 10 months ago
On 03/10/2017 09:35 PM, Laine Stump wrote:
> Some PF drivers allow setting the admin MAC (that is the MAC address
> that the VF will be initialized to the next time the VF's driver is
> loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
> initialize the admin MACs to all 0, but don't allow setting it to that
> very same value. It has been an uphill battle convincing the driver
> people that it's reasonable to expect The argument that's used is
> that an all 0 device MAC address on a device is invalid; however, from
> an outsider's point of view, when the admin MAC is set to 0 at the
> time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
> random non-0 value. But that's beside the point - even if I could
> convince one or two SRIOV driver maintainers to permit setting the
> admin MAC to 0, there are still several other drivers.
>
> So rather than fighting that losing battle, this patch checks for a
> failure to set the admin MAC due to an all 0 value, and retries it
> with 02:00:00:00:00:00. That won't result in a random value being set
> in the VF MAC at next VF driver init, but that's okay, because we
> always want to set a specific value anyway. Rather, the "almost 0"
> setting makes it easy to visually detect from the output of "ip link
> show" which VFs are currently in use and which are free.
> ---
>  src/util/virnetdev.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 2b1cebc..6cf0463 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1387,6 +1387,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link ATTRIBUTE_UNUSED,
>  #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
>
>
> +static virMacAddr zeroMAC = { 0 };
> +
> +/* if a net driver doesn't allow setting MAC to all 0, try setting
> + * to this (the only bit that is set is the "locally administered" bit")
> + */
> +static virMacAddr altZeroMAC = { .addr = { 0x02, 0, 0, 0, 0, 0, } };
> +
> +
>  static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>      [IFLA_VF_MAC]       = { .type = NLA_UNSPEC,
>                              .maxlen = sizeof(struct ifla_vf_mac) },
> @@ -1397,7 +1405,8 @@ static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>
>  static int
>  virNetDevSetVfConfig(const char *ifname, int vf,
> -                     const virMacAddr *macaddr, int vlanid)
> +                     const virMacAddr *macaddr, int vlanid,
> +                     bool *allowRetry)
>  {
>      int rc = -1;
>      struct nlmsghdr *resp = NULL;
> @@ -1474,7 +1483,15 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>          if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
>              goto malformed_resp;
>
> -        if (err->error) {
> +        /* if allowRetry is true and the error was EINVAL, then
> +         * silently return a failure so the caller can retry with a
> +         * different MAC address
> +         */
> +        if (-err->error == EINVAL && *allowRetry &&

No, please no. if (err->error == -EINVAL ...) is way better.

> +            macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) {
> +            goto cleanup;
> +        } else if (err->error) {
> +            /* other errors are permanent */
>              char macstr[VIR_MAC_STRING_BUFLEN];
>
>              virReportSystemError(-err->error,

ACK with that fixed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 18/19] util: if setting admin MAC to 00:00:00:00:00:00 fails, try 02:00:00:00:00:00
Posted by Laine Stump 8 years, 10 months ago
On 03/17/2017 09:32 AM, Michal Privoznik wrote:
> On 03/10/2017 09:35 PM, Laine Stump wrote:
>> Some PF drivers allow setting the admin MAC (that is the MAC address
>> that the VF will be initialized to the next time the VF's driver is
>> loaded) to 00:00:00:00:00:00, and some don't. Multiple drivers
>> initialize the admin MACs to all 0, but don't allow setting it to that
>> very same value. It has been an uphill battle convincing the driver
>> people that it's reasonable to expect The argument that's used is
>> that an all 0 device MAC address on a device is invalid; however, from
>> an outsider's point of view, when the admin MAC is set to 0 at the
>> time the VF driver is loaded, the VF's MAC is *not* set to 0, but to a
>> random non-0 value. But that's beside the point - even if I could
>> convince one or two SRIOV driver maintainers to permit setting the
>> admin MAC to 0, there are still several other drivers.
>>
>> So rather than fighting that losing battle, this patch checks for a
>> failure to set the admin MAC due to an all 0 value, and retries it
>> with 02:00:00:00:00:00. That won't result in a random value being set
>> in the VF MAC at next VF driver init, but that's okay, because we
>> always want to set a specific value anyway. Rather, the "almost 0"
>> setting makes it easy to visually detect from the output of "ip link
>> show" which VFs are currently in use and which are free.
>> ---
>>  src/util/virnetdev.c | 42 ++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 38 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
>> index 2b1cebc..6cf0463 100644
>> --- a/src/util/virnetdev.c
>> +++ b/src/util/virnetdev.c
>> @@ -1387,6 +1387,14 @@ virNetDevSysfsFile(char **pf_sysfs_device_link
>> ATTRIBUTE_UNUSED,
>>  #if defined(__linux__) && defined(HAVE_LIBNL) && defined(IFLA_VF_MAX)
>>
>>
>> +static virMacAddr zeroMAC = { 0 };
>> +
>> +/* if a net driver doesn't allow setting MAC to all 0, try setting
>> + * to this (the only bit that is set is the "locally administered" bit")
>> + */
>> +static virMacAddr altZeroMAC = { .addr = { 0x02, 0, 0, 0, 0, 0, } };
>> +
>> +
>>  static struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
>>      [IFLA_VF_MAC]       = { .type = NLA_UNSPEC,
>>                              .maxlen = sizeof(struct ifla_vf_mac) },
>> @@ -1397,7 +1405,8 @@ static struct nla_policy
>> ifla_vf_policy[IFLA_VF_MAX+1] = {
>>
>>  static int
>>  virNetDevSetVfConfig(const char *ifname, int vf,
>> -                     const virMacAddr *macaddr, int vlanid)
>> +                     const virMacAddr *macaddr, int vlanid,
>> +                     bool *allowRetry)
>>  {
>>      int rc = -1;
>>      struct nlmsghdr *resp = NULL;
>> @@ -1474,7 +1483,15 @@ virNetDevSetVfConfig(const char *ifname, int vf,
>>          if (resp->nlmsg_len < NLMSG_LENGTH(sizeof(*err)))
>>              goto malformed_resp;
>>
>> -        if (err->error) {
>> +        /* if allowRetry is true and the error was EINVAL, then
>> +         * silently return a failure so the caller can retry with a
>> +         * different MAC address
>> +         */
>> +        if (-err->error == EINVAL && *allowRetry &&
> 
> No, please no. if (err->error == -EINVAL ...) is way better.

Yeah, sure. I just copy-pasta'd that from somewhere else. Consider it done.

> 
>> +            macaddr && !virMacAddrCmp(macaddr, &zeroMAC)) {
>> +            goto cleanup;
>> +        } else if (err->error) {
>> +            /* other errors are permanent */
>>              char macstr[VIR_MAC_STRING_BUFLEN];
>>
>>              virReportSystemError(-err->error,
> 
> ACK with that fixed.
> 
> Michal
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list