[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] util: set MAC address for VF via netlink message to PF + VF# when possible




On 04/10/2015 01:47 PM, Laine Stump wrote:
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1113474
> 
> When we set the MAC address of a network device as a part of setting
> up macvtap "passthrough" mode (where the domain has an emulated netdev
> connected to a host macvtap device that has exclusive use of the
> physical device, and sets the device MAC address to match its own,
> i.e. "<interface type='direct'> <source mode='passthrough' .../>"), we
> use ioctl(SIOCSIFHWADDR) giving it the name of that device. This is
> true even if it is an SRIOV Virtual Function (VF).
> 
> But, when we are setting the MAC address / vlan ID of a VF in
> preparation for "hostdev network" passthrough (this is where we set
> the MAC address and vlan id of the VF before detaching the host net
> driver and assigning the device to the domain with PCI passthrough,
> i.e. "<interface type='hostdev'>", we do the setting via a netlink
                                  ^^
Looks like a close ")" is needed...

> RTM_SETLINK message for that VF's Physical Function (PF), telling it
> the VF# we want to change. This sets an "administratively changed MAC"
> flag for that VF in the PF's driver, and from that point on (until the
> PF's driver is reloaded) that VF's MAC address can't be changed.
> 
> This means that if a VF is used for hostdev passthrough, it will have
> the admin flag set, and future attempts to use that VF for macvtap
> passthrough will fail.
> 
> The solution to this problem is to check if a device being used for
> macvtap passthrough is actually a VF; if so, we use the netlink
> RTM_SETLINK message to the PF to set the VF's mac address instead of
> ioctl(SIOCSIFHWADDR); if not, behavior does not change from previously.
> 
> There are three pieces to making this work:
> 
> 1) virNetDevMacVLan(Create|Delete)WithVPortProfile() now call
>    virNetDev(Replace|Restore)NetConfig() rather than
>    virNetDev(Replace|Restore)MacAddress() (simply passing -1 for VF#
>    and vlanid).
> 
> 2) virNetDev(Replace|Restore)NetConfig() check to see if the device is
>    a VF. If so, they find the PF's name and VF#, allowing them to call
>    virNetDev(Replace|Restore)VfConfig().
> 
> 3) To prevent mixups when detaching a macvtap passthrough device that
>    had been attached while running an older version of libvirt,
>    virNetDevRestoreVfConfig() is potentially given the preserved name
>    of the VF, and if the proper statefile for a VF can't be found in
>    the stateDir (${stateDir}/${pfname}_vf${vfid}),
>    virNetDevRestoreMacAddress() is called instead (which will look in
>    the file named ${stateDir}/${vfname}).
> 
> This problem has existed in every version of libvirt that has both
> macvtap passthrough and interface type='hostdev'. Fortunately people
> seem to use one or the other though.
> ---
> 
> (I'm still doing some testing on this, but as long as this approach
> doesn't cause any other regressions (so far it doesn't appear to),
> this is the final form. So any ACK given will be contingent on the
> functional tests passing.)
> 
>  src/util/virnetdev.c        | 66 ++++++++++++++++++++++++++++++++++++++++-----
>  src/util/virnetdevmacvlan.c |  4 +--
>  2 files changed, 62 insertions(+), 8 deletions(-)
> 

Perhaps in order to avoid someone externally calling
virNetDevRestoreMacAddress and virNetDevReplaceMacAddress, they should
be come local/static to virnetdev.c thus forcing other module callers to
use virNetDevRestoreVfConfig or virNetDevReplaceNetConfig

Not a requirement, but it potentially avoids future issues...


In any case, ACK as long as your testing went well as your explanation
seems perfectly reasonable and succinct

John
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index a7903c3..a3f302f 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -2243,7 +2243,8 @@ virNetDevReplaceVfConfig(const char *pflinkdev, int vf,
>  }
>  
>  static int
> -virNetDevRestoreVfConfig(const char *pflinkdev, int vf,
> +virNetDevRestoreVfConfig(const char *pflinkdev,
> +                         int vf, const char *vflinkdev,
>                           const char *stateDir)
>  {
>      int rc = -1;
> @@ -2258,6 +2259,17 @@ virNetDevRestoreVfConfig(const char *pflinkdev, int vf,
>                      stateDir, pflinkdev, vf) < 0)
>          return rc;
>  
> +    if (vflinkdev && !virFileExists(path)) {
> +        /* this VF's config may have been stored with
> +         * virNetDevReplaceMacAddress while running an older version
> +         * of libvirt. If so, the ${pf}_vf${id} file won't exist. In
> +         * that case, try to restore using the older method with the
> +         * VF's name directly.
> +         */
> +        rc = virNetDevRestoreMacAddress(vflinkdev, stateDir);
> +        goto cleanup;
> +    }
> +
>      if (virFileReadAll(path, 128, &fileData) < 0)
>          goto cleanup;
>  
> @@ -2311,11 +2323,31 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf,
>                            const virMacAddr *macaddress, int vlanid,
>                            const char *stateDir)
>  {
> +    int ret = -1;
> +    char *pfdevname = NULL;
> +
> +    if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) {
> +        /* If this really *is* a VF and the caller just didn't know
> +         * it, we should set the MAC address via PF+vf# instead of
> +         * setting directly via VF, because the latter will be
> +         * rejected any time after the former has been done.
> +         */
> +        if (virNetDevGetPhysicalFunction(linkdev, &pfdevname) < 0)
> +            goto cleanup;
> +        if (virNetDevGetVirtualFunctionIndex(pfdevname, linkdev, &vf) < 0)
> +            goto cleanup;
> +        linkdev = pfdevname;
> +    }
> +
>      if (vf == -1)
> -        return virNetDevReplaceMacAddress(linkdev, macaddress, stateDir);
> +        ret = virNetDevReplaceMacAddress(linkdev, macaddress, stateDir);
>      else
> -        return virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid,
> -                                        stateDir);
> +        ret = virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid,
> +                                       stateDir);
> +
> + cleanup:
> +    VIR_FREE(pfdevname);
> +    return ret;
>  }
>  
>  /**
> @@ -2330,10 +2362,32 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf,
>  int
>  virNetDevRestoreNetConfig(const char *linkdev, int vf, const char *stateDir)
>  {
> +    int ret = -1;
> +    char *pfdevname = NULL;
> +    const char *vfdevname = NULL;
> +
> +    if (vf == -1 && virNetDevIsVirtualFunction(linkdev)) {
> +        /* If this really *is* a VF and the caller just didn't know
> +         * it, we should set the MAC address via PF+vf# instead of
> +         * setting directly via VF, because the latter will be
> +         * rejected any time after the former has been done.
> +         */
> +        if (virNetDevGetPhysicalFunction(linkdev, &pfdevname) < 0)
> +            goto cleanup;
> +        if (virNetDevGetVirtualFunctionIndex(pfdevname, linkdev, &vf) < 0)
> +            goto cleanup;
> +        vfdevname = linkdev;
> +        linkdev = pfdevname;
> +    }
> +
>      if (vf == -1)
> -        return virNetDevRestoreMacAddress(linkdev, stateDir);
> +        ret = virNetDevRestoreMacAddress(linkdev, stateDir);
>      else
> -        return virNetDevRestoreVfConfig(linkdev, vf, stateDir);
> +        ret = virNetDevRestoreVfConfig(linkdev, vf, vfdevname, stateDir);
> +
> + cleanup:
> +    VIR_FREE(pfdevname);
> +    return ret;
>  }
>  
>  #else /* defined(__linux__) && defined(HAVE_LIBNL) */
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index 5fd2097..ea628c0 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -779,7 +779,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
>       * emulate their switch in firmware.
>       */
>      if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) {
> -        if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0)
> +        if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0)
>              return -1;
>      }
>  
> @@ -914,7 +914,7 @@ int virNetDevMacVLanDeleteWithVPortProfile(const char *ifname,
>      int vf = -1;
>  
>      if (mode == VIR_NETDEV_MACVLAN_MODE_PASSTHRU)
> -        ignore_value(virNetDevRestoreMacAddress(linkdev, stateDir));
> +        ignore_value(virNetDevRestoreNetConfig(linkdev, vf, stateDir));
>  
>      if (ifname) {
>          if (virNetDevVPortProfileDisassociate(ifname,
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]