Message-ID: <CAKgT0UfUjuEcZo4JVS-8Q5+nTGKWBv0yZa69eMT894KvqkCOOA@mail.gmail.com>
Date:   Wed, 24 Jan 2018 18:29:42 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        Netdev <netdev@...r.kernel.org>,
        Neil Horman <nhorman@...hat.com>, sassmann@...hat.com,
        John Greene <jogreene@...hat.com>
Subject: Re: [net-next 0/8][pull request] 1GbE Intel Wired LAN Driver Updates 2018-01-24

On Wed, Jan 24, 2018 at 1:10 PM, David Miller <davem@...emloft.net> wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> Date: Wed, 24 Jan 2018 12:55:12 -0800
>
>> This series contains updates to igb and e1000e only.
>
> Pulled, however:
>
>> Corinna Vinschen implements the ability to set the VF MAC to
>> 00:00:00:00:00:00 via RTM_SETLINK on the PF, to prevent receiving
>> "invlaid argument" when libvirt attempts to restore the MAC address back
>> to its original state of 00:00:00:00:00:00.
>
> This is really a mess and the wrong way to go about this.
>
> No interface, even a VF, should come up or ever have an invalid
> MAC addres like all-zeros.  That's the fundamental problem and
> once you fix that all of this other crazy logic and workarounds
> no longer become necessary.

In the case of igbvf the VFs never come up with 0s in their MAC
address. An all 0's MAC address basically leaves it open to VF's
choice for assigning themselves a MAC address, or at least that is the
way I recall coding it back in the day.

There are a few issues with making changes to this at this point. The
first being that this concept is pretty much baked into the VF driver
logic for most drivers supporting legacy SR-IOV, and as pointed out in
the patch comments the libvirt interface is writing 0's to disable the
VF MAC address when it is not in use. At this point we cannot change
this without breaking the libvirt userspace.

One of the motivations for clearing this is to avoid having the PF
misdirect traffic as having a MAC address mapped to a
disabled/unassigned VF could result in traffic being dropped when it
should be directed elsewhere such as a bridge on the PF, or out to
some other PF that is now running the VM there.

> Whatever it takes, just do it.  We can even come up with a global
> MAC address range that on a Linux system is reserved for VFs to
> come up with.

That is normally how the VFs handle this on their side. The code was
setup such that if the PF provided an all 0's MAC address then the VF
would assign itself a locally administered address so that it wouldn't
come up with an address of 0s. If you are saying the VFs shouldn't be
allowed to come up with an all 0's MAC address I believe that none of
them do. I believe they either fail to come up at all or report a
locally administered address for themselves. I can double check that
though (at least for Intel) to verify that it is in fact a consistent
behavior. In theory there isn't likely to be a VF bound to the
interface anyway, usually when the MAC address is invalidated it is
because a VM has been terminated and the VF driver is just in limbo
since it is usually assigned to a VFIO interface which doesn't
actually expose the network interface to the kernel.

I suppose we could look at pushing the LAA generation up into the PF,
but we would still want to maintain the all 0's address while the VF
is inactive since we need to clear the stale VF addresses from the MAC
address table in the event of a VM being relocated to a different
server and taking the MAC address with it.

The good news to all this is that this is going to be fading out and
going away anyway as SwitchDev takes over for SR-IOV.

> Thanks.

I'll double check our VF drivers and make sure none of them are
exposing a netdevice with an all 0's MAC address, and see what we can
do about relocating the locally administered address generation into
the PF.

Thanks.

- Alex

Powered by blists - more mailing lists