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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
