Re: [libvirt] [PATCH] util: set vlan tag for macvtap passthrough mode on SRIOV VFs

On 05/05/2016 12:39 PM, Laine Stump wrote: > SRIOV VFs used in macvtap passthrough mode can take advantage of the > SRIOV card's transparent vlan tagging. All the code was there to set > the vlan tag, and it has been used for SRIOV VFs used for hostdev > interfaces for several years, but for some reason, the vlan tag for > macvtap passthrough devices was stubbed out with a -1. > > This patch moves a bit of common validation down to a lower level > (virNetDevReplaceNetConfig()) so it is shared by hostdev and macvtap > modes, and updates the macvtap caller to actually send the vlan config > instead of -1. > --- > > While adding the info to the docs, I got a bit carried away fixing up > the vlan-specific paragraphs. It someone really wants me to I can move > it into a separate patch, but it seemed like more trouble than it was > worth at the time... > > > docs/formatdomain.html.in | 61 +++++++++++++++++++++++++++------------------ > src/lxc/lxc_process.c | 3 ++- > src/network/bridge_driver.c | 17 ++++++++----- > src/qemu/qemu_interface.c | 1 + > src/util/virhostdev.c | 23 ++--------------- > src/util/virnetdev.c | 29 +++++++++++++++++---- > src/util/virnetdev.h | 6 +++-- > src/util/virnetdevmacvlan.c | 4 ++- > src/util/virnetdevmacvlan.h | 2 ++ > 9 files changed, 86 insertions(+), 60 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 97794b7..68f0295 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -4032,7 +4032,7 @@ > <p> > On Linux systems, the bridge device is normally a standard Linux > host bridge. On hosts that support Open vSwitch, it is also > - possible to connect to an open vSwitch bridge device by adding > + possible to connect to an Open vSwitch bridge device by adding > a <code>&lt;virtualport type='openvswitch'/&gt;</code> to the > interface definition. (<span class="since">Since > 0.9.11</span>). The Open vSwitch type virtualport accepts two > @@ -4816,34 +4816,47 @@ qemu-kvm -net nic,model=? /dev/null > > <p> > If (and only if) the network connection used by the guest > - supports vlan tagging transparent to the guest, an > + supports VLAN tagging transparent to the guest, an > optional <code>&lt;vlan&gt;</code> element can specify one or > - more vlan tags to apply to the guest's network > - traffic <span class="since">Since 0.10.0</span>. (openvswitch ^ [1] note: Formerly the "open" parenthesis... > - and type='hostdev' SR-IOV interfaces do support transparent vlan > - tagging of guest traffic; everything else, including standard > + more VLAN tags to apply to the guest's network > + traffic <span class="since">Since 0.10.0</span>. Network > + connections that support guest-transparent VLAN tagging include > + 1) type='bridge' interfaces connected to an Open vSwitch bridge > + <span class="since">Since 0.10.0</span>, 2) SRIOV Virtual > + Functions (VF) used via type='hostdev' (direct device > + assignment) <span class="since">Since 0.10.0</span>, and 3) > + SRIOV VFs used via type='direct' with mode='passthrough' > + (macvtap "passthru" mode) <span class="since">Since > + 1.3.4</span>; all other connection types, including standard nit: "s/; all/. All/" (already a long enough).

> linux bridges and libvirt's own virtual networks, <b>do not</b> > support it. 802.1Qbh (vn-link) and 802.1Qbg (VEPA) switches > provide their own way (outside of libvirt) to tag guest traffic > - onto specific vlans.) To allow for specification of multiple > - tags (in the case of vlan trunking), a > - subelement, <code>&lt;tag&gt;</code>, specifies which vlan tag > - to use (for example: <code>&lt;tag id='42'/&gt;</code>. If an > - interface has more than one <code>&lt;vlan&gt;</code> element > - defined, it is assumed that the user wants to do VLAN trunking > - using all the specified tags. In the case that vlan trunking > - with a single tag is desired, the optional > + onto specific vlans.) Each tag is given in a Should this be VLANs? or "onto a specific VLAN." ? [1] Don't need the close parentheis ')' since the open one was removed.

> + separate <code>&lt;tag&gt;</code> subelement > + of <code>&lt;vlan&gt;</code> (for example: <code>&lt;tag > + id='42'/&gt;</code>). For VLAN trunking of multiple tags (which > + is supported only on Open vSwitch connections), > + multiple <code>&lt;tag&gt;</code> subelements can be specified, This reads strange... There's multiple multiples - one "multiple tags" and one "multiple <tag> subelements"... I guess it's right, just was a tough read the first time through!

> + which implies that the user wants to do VLAN trunking on the > + interface for all the specified tags. In the case that VLAN > + trunking of a single tag is desired, the optional > attribute <code>trunk='yes'</code> can be added to the toplevel > - vlan element. > - </p> > - > - <p> > - For network connections using openvswitch it is possible to > - configure the 'native-tagged' and 'native-untagged' vlan modes > - <span class="since">Since 1.1.0.</span> This uses the optional > - <code>nativeMode</code> attribute on the <code>&lt;tag&gt;</code> > - element: <code>nativeMode</code> may be set to 'tagged' or > - 'untagged'. The id attribute of the element sets the native vlan. > + <code>&lt;vlan&gt;</code> element to differentiate trunking of a > + single tag from normal tagging. > + </p> > + > + <p> > + For network connections using Open vSwitch it is also possible > + to configure 'native-tagged' and 'native-untagged' VLAN modes > + <span class="since">Since 1.1.0.</span> This is done with the > + optional <code>nativeMode</code> attribute on > + the <code>&lt;tag&gt;</code> subelement: <code>nativeMode</code> > + may be set to 'tagged' or 'untagged'. The <code>id</code> > + attribute of the <code>&lt;tag&gt;</code> subelement > + containing <code>nativeMode</code> sets which VLAN is considered > + to be the "native" VLAN for this interface, and > + the <code>nativeMode</code> attribute determines whether or not > + traffic for that VLAN will be tagged. > </p> > > <h5><a name="elementLink">Modifying virtual link state</a></h5> > diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c > index 0044ee5..8981d9a 100644 > --- a/src/lxc/lxc_process.c > +++ b/src/lxc/lxc_process.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2010-2015 Red Hat, Inc. > + * Copyright (C) 2010-2016 Red Hat, Inc. > * Copyright IBM Corp. 2008 > * > * lxc_process.c: LXC process lifecycle management > @@ -343,6 +343,7 @@ char *virLXCProcessSetupInterfaceDirect(virConnectPtr conn, > net->ifname, &net->mac, > linkdev, > virDomainNetGetActualDirectMode(net), > + virDomainNetGetActualVlan(net), > def->uuid, > prof, > &res_ifname, > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index a34da2a..7a2cadb 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -3058,11 +3058,12 @@ networkValidate(virNetworkDriverStatePtr driver, > * a pool, and those using an Open vSwitch bridge. > */ > > - vlanAllowed = ((def->forward.type == VIR_NETWORK_FORWARD_BRIDGE && > + vlanAllowed = (def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV || > + def->forward.type == VIR_NETWORK_FORWARD_PASSTHROUGH || > + (def->forward.type == VIR_NETWORK_FORWARD_BRIDGE && > def->virtPortProfile && > def->virtPortProfile->virtPortType > - == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) || > - def->forward.type == VIR_NETWORK_FORWARD_HOSTDEV); > + == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH)); > > vlanUsed = def->vlan.nTags > 0; > for (i = 0; i < def->nPortGroups; i++) { > @@ -4277,11 +4278,15 @@ networkAllocateActualDevice(virDomainDefPtr dom, > */ > > if (virDomainNetGetActualVlan(iface)) { > - /* vlan configuration via libvirt is only supported for > - * PCI Passthrough SR-IOV devices and openvswitch bridges. > - * otherwise log an error and fail > + /* vlan configuration via libvirt is only supported for PCI > + * Passthrough SR-IOV devices (hostdev or macvtap passthru > + * mode) and openvswitch bridges. Otherwise log an error and > + * fail > */ > if (!(actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV || > + (actualType == VIR_DOMAIN_NET_TYPE_DIRECT && > + virDomainNetGetActualDirectMode(iface) > + == VIR_NETDEV_MACVLAN_MODE_PASSTHRU) || > (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE && > virtport && virtport->virtPortType > == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH))) { Might be easier if these two rather complex statements could be put into one... one seems to use "def->forward.type" and the other "actualType"... one uses "def->virtPortProfile" and the other virtport... the difference seems to be the passthrough vs. actualType == DIRECT.. Oh well.

> diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c > index ef789fa..b48ae50 100644 > --- a/src/qemu/qemu_interface.c > +++ b/src/qemu/qemu_interface.c > @@ -266,6 +266,7 @@ qemuInterfaceDirectConnect(virDomainDefPtr def, > &net->mac, > virDomainNetGetActualDirectDev(net), > virDomainNetGetActualDirectMode(net), > + virDomainNetGetActualVlan(net), > def->uuid, > virDomainNetGetActualVirtPortProfile(net), > &res_ifname, > diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c > index 933c942..980e590 100644 > --- a/src/util/virhostdev.c > +++ b/src/util/virhostdev.c > @@ -1,6 +1,6 @@ > /* virhostdev.c: hostdev management > * > - * Copyright (C) 2006-2007, 2009-2015 Red Hat, Inc. > + * Copyright (C) 2006-2007, 2009-2016 Red Hat, Inc. > * Copyright (C) 2006 Daniel P. Berrange > * Copyright (C) 2014 SUSE LINUX Products GmbH, Nuernberg, Germany. > * > @@ -387,7 +387,6 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, > virNetDevVPortProfilePtr virtPort; > int ret = -1; > int vf = -1; > - int vlanid = -1; > bool port_profile_associate = true; > > if (virHostdevIsVirtualFunction(hostdev) != 1) { > @@ -416,27 +415,9 @@ virHostdevNetConfigReplace(virDomainHostdevDefPtr hostdev, > port_profile_associate); > } else { > /* Set only mac and vlan */ > - if (vlan) { > - if (vlan->nTags != 1 || vlan->trunk) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > - _("vlan trunking is not supported " > - "by SR-IOV network devices")); > - goto cleanup; > - } > - if (vf == -1) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - _("vlan can only be set for SR-IOV VFs, but " > - "%s is not a VF"), linkdev); > - goto cleanup; > - } > - vlanid = vlan->tag[0]; > - } else if (vf >= 0) { > - vlanid = 0; /* assure any current vlan tag is reset */ > - } > - > ret = virNetDevReplaceNetConfig(linkdev, vf, > &hostdev->parent.data.net->mac, > - vlanid, stateDir); > + vlan, stateDir); > } > cleanup: > VIR_FREE(linkdev); > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c > index bb17b84..7db4497 100644 > --- a/src/util/virnetdev.c > +++ b/src/util/virnetdev.c > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2007-2015 Red Hat, Inc. > + * Copyright (C) 2007-2016 Red Hat, Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -2547,7 +2547,8 @@ virNetDevRestoreVfConfig(const char *pflinkdev, > */ > int > virNetDevReplaceNetConfig(const char *linkdev, int vf, > - const virMacAddr *macaddress, int vlanid, > + const virMacAddr *macaddress, > + virNetDevVlanPtr vlan, > const char *stateDir) > { > int ret = -1; > @@ -2566,11 +2567,29 @@ virNetDevReplaceNetConfig(const char *linkdev, int vf, > linkdev = pfdevname; > } > > - if (vf == -1) > + if (vf == -1) { > + if (vlan) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("vlan can only be set for SR-IOV VFs, but " > + "%s is not a VF"), linkdev); > + goto cleanup; > + } > ret = virNetDevReplaceMacAddress(linkdev, macaddress, stateDir); > - else > + } else { > + int vlanid = 0; /* assure any current vlan tag is reset */ > + > + if (vlan) { > + if (vlan->nTags != 1 || vlan->trunk) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("vlan trunking is not supported " > + "by SR-IOV network devices")); > + goto cleanup; > + } > + vlanid = vlan->tag[0]; > + } > ret = virNetDevReplaceVfConfig(linkdev, vf, macaddress, vlanid, > stateDir); > + } > > cleanup: > VIR_FREE(pfdevname); > @@ -2636,7 +2655,7 @@ int > virNetDevReplaceNetConfig(const char *linkdev ATTRIBUTE_UNUSED, > int vf ATTRIBUTE_UNUSED, > const virMacAddr *macaddress ATTRIBUTE_UNUSED, > - int vlanid ATTRIBUTE_UNUSED, > + virNetDevVlanPtr vlan ATTRIBUTE_UNUSED, > const char *stateDir ATTRIBUTE_UNUSED) > { > virReportSystemError(ENOSYS, "%s", > diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h > index dcc81a6..cbe7938 100644 > --- a/src/util/virnetdev.h > +++ b/src/util/virnetdev.h > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2007-2015 Red Hat, Inc. > + * Copyright (C) 2007-2016 Red Hat, Inc. > * > * This library is free software; you can redistribute it and/or > * modify it under the terms of the GNU Lesser General Public > @@ -30,6 +30,7 @@ > # include "virnetlink.h" > # include "virmacaddr.h" > # include "virpci.h" > +# include "virnetdevvlan.h" > # include "device_conf.h" > > # ifdef HAVE_STRUCT_IFREQ > @@ -175,7 +176,8 @@ int virNetDevLinkDump(const char *ifname, int ifindex, > ATTRIBUTE_RETURN_CHECK; > > int virNetDevReplaceNetConfig(const char *linkdev, int vf, > - const virMacAddr *macaddress, int vlanid, > + const virMacAddr *macaddress, > + virNetDevVlanPtr vlan, > const char *stateDir) > ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); > > diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c > index d755b93..c05c67f 100644 > --- a/src/util/virnetdevmacvlan.c > +++ b/src/util/virnetdevmacvlan.c > @@ -975,6 +975,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, > const virMacAddr *macaddress, > const char *linkdev, > virNetDevMacVLanMode mode, > + virNetDevVlanPtr vlan, > const unsigned char *vmuuid, > virNetDevVPortProfilePtr virtPortProfile, > char **ifnameResult, > @@ -1021,7 +1022,7 @@ virNetDevMacVLanCreateWithVPortProfile(const char *ifnameRequested, > if (virNetDevReplaceMacAddress(linkdev, macaddress, stateDir) < 0) > return -1; > } else { > - if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, -1, stateDir) < 0) > + if (virNetDevReplaceNetConfig(linkdev, -1, macaddress, vlan, stateDir) < 0) > return -1; > } > } > @@ -1281,6 +1282,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname ATTRIBUTE_UNUSED, > const virMacAddr *macaddress ATTRIBUTE_UNUSED, > const char *linkdev ATTRIBUTE_UNUSED, > virNetDevMacVLanMode mode ATTRIBUTE_UNUSED, > + virNetDevVlanPtr vlan ATTRIBUTE_UNUSED, > const unsigned char *vmuuid ATTRIBUTE_UNUSED, > virNetDevVPortProfilePtr virtPortProfile ATTRIBUTE_UNUSED, > char **res_ifname ATTRIBUTE_UNUSED, > diff --git a/src/util/virnetdevmacvlan.h b/src/util/virnetdevmacvlan.h > index 560593e..179a8a1 100644 > --- a/src/util/virnetdevmacvlan.h > +++ b/src/util/virnetdevmacvlan.h > @@ -28,6 +28,7 @@ > # include "virsocketaddr.h" > # include "virnetdevbandwidth.h" > # include "virnetdevvportprofile.h" > +# include "virnetdevvlan.h" > > /* the mode type for macvtap devices */ > typedef enum { > @@ -69,6 +70,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *ifname, > const virMacAddr *macaddress, > const char *linkdev, > virNetDevMacVLanMode mode, > + virNetDevVlanPtr vlan, > const unsigned char *vmuuid, > virNetDevVPortProfilePtr virtPortProfile, > char **res_ifname, You need to update the ATTRIBUTE_NONNULL's here... Since the called code checks "if (vlan)" before using, then it seems the new param5 doesn't need the ATTRIBUTE_NONNULL This one upsets the coverity build.

ACK with the nits fixed.