[libvirt] [PATCH] util: set MAC address for VF via netlink message to PF + VF# when possible
John Ferlan
jferlan at redhat.com
Wed Apr 15 18:47:44 UTC 2015
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,
>
More information about the libvir-list
mailing list