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).
> 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.
> + 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!
> + 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.
> 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.
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list