[libvirt] [PATCH] util: set vlan tag for macvtap passthrough mode on SRIOV VFs
Laine Stump
laine at laine.org
Tue May 10 18:07:13 UTC 2016
On 05/06/2016 03:26 PM, John Ferlan wrote:
>
> 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><virtualport type='openvswitch'/></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><vlan></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).
Fixed.
>
>> 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><tag></code>, specifies which vlan tag
>> - to use (for example: <code><tag id='42'/></code>. If an
>> - interface has more than one <code><vlan></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.
Both fixed.
>
>> + separate <code><tag></code> subelement
>> + of <code><vlan></code> (for example: <code><tag
>> + id='42'/></code>). For VLAN trunking of multiple tags (which
>> + is supported only on Open vSwitch connections),
>> + multiple <code><tag></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!
Yeah, I can't really think of a better way to say it (except for just
using some synonym for "multiple" in one of the cases).
>
>> + 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><tag></code>
>> - element: <code>nativeMode</code> may be set to 'tagged' or
>> - 'untagged'. The id attribute of the element sets the native vlan.
>> + <code><vlan></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><tag></code> subelement: <code>nativeMode</code>
>> + may be set to 'tagged' or 'untagged'. The <code>id</code>
>> + attribute of the <code><tag></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.
Which "two complex statements" are you talking about? The one in
networkValidate and the one in networkAllocateActualDevice? They can't
be combined because one of them is looking at whether or not a
particular network allows vlan tagging (and has no interface to look
at), while the other is looking at whether or not a particular interface
is on a connection that allows vlan tagging (and the interface may or
may not be associated with a network, so it's possible there will be no
network object to look at).
>
>> 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.
Okay, fixed that too.
>
> ACK with the nits fixed.
Thanks for the review. I just pushed the fixed patch.
More information about the libvir-list
mailing list