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

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.

John



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