[libvirt] [PATCH 19/19] util: try *really* hard to set the MAC address of an SRIOV VF

[Michal Privoznik]

On 03/10/2017 09:35 PM, Laine Stump wrote:
> If an SRIOV VF has previously been used for VFIO device assignment,
> the "admin MAC" that is stored in the PF driver's table of VF info
> will have been set to the MAC address that the virtual machine wanted
> the device to have. Setting the admin MAC for a VF also sets a flag in
> the PF that is loosely called the "administratively set" flag. Once
> that flag is set, it is no longer possible for the net driver of the
> VF (either on the host or in a virtual machine) to directly set the
> VF's MAC again; this flag isn't reset until the *PF* driver is
> restarted, and that requires taking *all* VFs offline, so it's not
> really feasible to do.
>
> If the same SRIOV VF is later used for macvtap passthrough mode, the
> VF's MAC address must be set, but normally we don't unbind the VF from
> its host net driver (since we actually need the host net driver in
> this case). Since setting the VF MAC directly will fail, in the past
> "we" ("I") had tried to fix the problem by simply setting the admin MAC
> (via the PF) instead. This *appeared* to work (and might have at one
> time, due to promiscuous mode being turned on somewhere or something),
> but it currently creates a non-working interface because only the
> value for admin MAC is set to the desired value, *not* the actual MAC
> that the VF is using.
>
> Earlier patches in this series reverted that behavior, so that we once
> again set the MAC of the VF itself for macvtap passthrough operation,
> not the admin MAC. But that brings back the original bug - if the
> interface has been used for VFIO device assignment, you can no longer
> use it for macvtap passthrough.
>
> This patch solves that problem by noticing when virNetDevSetMAC()
> fails for a VF, and in that case it sets the desired MAC to the admin
> MAC via the PF, then "bounces" the VF driver (by unbinding and the
> immediately rebinding it to the VF). This causes the VF's MAC to be
> reinitialized from the admin MAC, and everybody is happy (until the
> *next* time someone wants to set the VF's MAC address, since the
> "administratively set" bit is still turned on).
> ---
>  src/util/virnetdev.c | 102 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 83 insertions(+), 19 deletions(-)
>
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index 6cf0463..861d725 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -1053,6 +1053,32 @@ virNetDevSysfsDeviceFile(char **pf_sysfs_device_link, const char *ifname,
>      return 0;
>  }
>
> +
> +static virPCIDevicePtr
> +virNetDevGetPCIDevice(const char *devName)
> +{
> +    char *vfSysfsDevicePath = NULL;
> +    virPCIDeviceAddressPtr vfPCIAddr = NULL;
> +    virPCIDevicePtr vfPCIDevice = NULL;
> +
> +    if (virNetDevSysfsFile(&vfSysfsDevicePath, devName, "device") < 0)
> +        goto cleanup;
> +
> +    vfPCIAddr = virPCIGetDeviceAddressFromSysfsLink(vfSysfsDevicePath);
> +    if (!vfPCIAddr)
> +        goto cleanup;
> +
> +    vfPCIDevice = virPCIDeviceNew(vfPCIAddr->domain, vfPCIAddr->bus,
> +                                  vfPCIAddr->slot, vfPCIAddr->function);
> +
> + cleanup:
> +    VIR_FREE(vfSysfsDevicePath);
> +    VIR_FREE(vfPCIAddr);
> +
> +    return vfPCIDevice;
> +}
> +
> +
>  /**
>   * virNetDevGetVirtualFunctions:
>   *
> @@ -1942,6 +1968,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
>      char *pfDevOrig = NULL;
>      char *vfDevOrig = NULL;
>      int vlanTag = -1;
> +    virPCIDevicePtr vfPCIDevice = NULL;
>
>      if (vf >= 0) {
>          /* linkdev is the PF */
> @@ -2037,6 +2064,8 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
>      }
>
>      if (MAC) {
> +        int setMACrc;
> +
>          if (!linkdev) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("VF %d of PF '%s' is not bound to a net driver, "
> @@ -2045,27 +2074,61 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
>              goto cleanup;
>          }
>
> -        if (virNetDevSetMAC(linkdev, MAC) < 0) {
> -            /* This may have failed due to the "administratively
> -             * set" flag being set in the PF for this VF. For now
> -             * we will just fail, but in the future we should
> -             * attempt to set the VF MAC via the PF.
> +        setMACrc = virNetDevSetMACInternal(linkdev, MAC, !!pfDevOrig);
> +        if (setMACrc < 0) {
> +            bool allowRetry = false;
> +            int retries = 100;
> +
> +            /* if pfDevOrig == NULL, this isn't a VF, so we've failed */
> +            if (!pfDevOrig || errno != EADDRNOTAVAIL)
> +                goto cleanup;
> +
> +            /* Otherwise this is a VF, and virNetDevSetMAC failed with
> +             * EADDRNOTAVAIL, which could be due to the
> +             * "administratively set" flag being set in the PF for
> +             * this VF.  When this happens, we can attempt to use an
> +             * alternate method to set the VF MAC: first set it into
> +             * the admin MAC for this VF in the PF, then unbind/rebind
> +             * the VF from its net driver. This causes the VF's MAC to
> +             * be initialized to whatever was stored in the admin MAC.
>               */
> -            goto cleanup;
> +
> +            if (virNetDevSetVfConfig(pfDevName, vf,
> +                                     MAC, vlanTag, &allowRetry) < 0) {
> +                goto cleanup;
> +            }
> +
> +            /* admin MAC is set, now we need to construct a virPCIDevice
> +             * object so we can call virPCIDeviceRebind()
> +             */
> +            if (!(vfPCIDevice = virNetDevGetPCIDevice(linkdev)))
> +                goto cleanup;
> +
> +            /* Rebind the device. This should set the proper MAC address */
> +            if (virPCIDeviceRebind(vfPCIDevice) < 0)
> +                goto cleanup;
> +
> +            /* Wait until virNetDevGetIndex for the VF netdev returns success.
> +             * This indicates that the device is ready to be used. If we don't
> +             * wait, then upcoming operations on the VF may fail.
> +             */
> +            while (retries-- > 0 && !virNetDevExists(linkdev))
> +               usleep(1000);
>          }
> -        if (pfDevOrig) {
> -            /* if pfDevOrig is set, it means that the caller was
> -             * *really* only interested in setting the MAC of the VF
> -             * itself, *not* the admin MAC via the PF. In those cases,
> -             * the adminMAC was only provided in case we need to set
> -             * the VF's MAC by temporarily unbinding/rebinding the
> -             * VF's net driver with the admin MAC set to the desired
> -             * MAC, and then want to restore the admin MAC to its
> -             * original setting when we're finished. We would only
> -             * need to do that if the virNetDevSetMAC() above had
> -             * failed; since it didn't, we don't need to set the
> -             * adminMAC, so we are NULLing it out here to avoid that
> -             * below.
> +
> +        if (pfDevOrig && setMACrc == 0) {
> +            /* if pfDevOrig is set, it that the caller was *really*
> +             * only interested in setting the MAC of the VF itself,
> +             * *not* the admin MAC via the PF. In those cases, the
> +             * adminMAC was only provided in case we need to set the
> +             * VF's MAC by temporarily unbinding/rebinding the VF's
> +             * net driver with the admin MAC set to the desired MAC,
> +             * and then want to restore the admin MAC to its original
> +             * setting when we're finished. We would only need to do
> +             * that if the virNetDevSetMAC() above had failed; since
> +             * setMACrc == 0, we know it didn't fail and we don't need
> +             * to set the adminMAC, so we are NULLing it out here to
> +             * avoid that below.
>
>               * (NB: since setting the admin MAC sets the
>               * "administratively set" flag for the VF in the PF's
> @@ -2109,6 +2172,7 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
>   cleanup:
>      VIR_FREE(pfDevOrig);
>      VIR_FREE(vfDevOrig);
> +    VIR_FREE(vfPCIDevice);

No. this needs to be virPCIDeviceFree().

ACK with that fixed.

Michal