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

Re: [libvirt] [PATCH] Add support for 'passthru' mode for direct network interfaces



On Tue, May 17, 2011 at 08:19:17PM -0600, Eric Blake wrote:
> On 05/17/2011 05:26 AM, D. Herrendoerfer wrote:
> > This patch includes the documentation part.
> > 
> > The error returned by libvirt when a using an older kernel
> > is imho sufficient to point to the macvtap setup. Apart from that
> > I doubt that someone will use this feature by chance. But still - I
> > believe the error returned is detailed enough for now.
> 
> Fair enough for me.
> 
> Your patch came through rather mangled by your mailer; sometimes, it is
> worth sending a patch to yourself as a trial run to see if others would
> have a hard time running 'git am' on the resulting email, then figure
> out how to tune 'git send-email' to do what you wanted for sending the
> patch.  The git mailing list or IRC channel is a good resource for
> figuring out configuration emails that make patch submission easier.
> 
> > 
> > Signed-off-by: Dirk Herrendoerfer <d.herrendoerfer at herrendoerfer.name>
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 989dcf6..3f6bec8 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -1429,6 +1429,12 @@
> >       external router or gateway and that device sends them back to the
> >       host. This procedure is followed if either the source or destination
> >       device is in <code>private</code> mode.</dd>
> > +      <dt><code>passthru</code></dt>
> > +      <dd>This feature allows to attach a virtual function of a SRIOV
> > capable
> 
> s/allows to attach/attaches/
> 
> > +      NIC directly to a VM without loosing the migration capability.
> 
> s/loosing/losing/
> 
> > +      All packets are sent to the VF/IF of the configured network device.
> > +      Depending on the capabilities of the device additional
> > prerequisites or
> > +      limitations may apply. <span class="since">(Since Linux
> > 2.6.38.)</span></dd>
> 
> Here's where part of the patch botching showed up.  Also, when possible,
> we like to keep lines within 80 columns.
> 
> The <span class="since"> annotations are for when a feature was added to
> libvirt (0.9.2 in this case), not when it was added elsewhere.
> 
> >     </dl>
> > 
> > <pre>
> > diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
> > index 7163c6e..e2a66ae 100644
> > --- a/docs/schemas/domain.rng
> > +++ b/docs/schemas/domain.rng
> > @@ -2351,7 +2351,7 @@
> >   </define>
> >   <define name="bridgeMode">
> >     <data type="string">
> > -      <param name="pattern">(vepa|bridge|private)</param>
> > +      <param name="pattern">(vepa|bridge|private|passthru)</param>
> 
> We already have other cases of "passthrough" in our XML schema; let's be
> consistent and avoid abbreviations in the public interface.
> 
> >     </data>
> >   </define>
> >   <define name="addrMAC">
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index d3efec6..4c4bfad 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -383,7 +383,8 @@ VIR_ENUM_IMPL(virDomainSeclabel,
> > VIR_DOMAIN_SECLABEL_LAST,
> > VIR_ENUM_IMPL(virDomainNetdevMacvtap, VIR_DOMAIN_NETDEV_MACVTAP_MODE_LAST,
> >               "vepa",
> >               "private",
> > -              "bridge")
> > +              "bridge",
> > +              "passthru" )
> 
> Odd spacing, and same spelling issue.
> 
> > 
> > VIR_ENUM_IMPL(virVirtualPort, VIR_VIRTUALPORT_TYPE_LAST,
> >               "none",
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index a0f820c..70943f9 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -335,6 +335,7 @@ enum virDomainNetdevMacvtapType {
> >     VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA,
> >     VIR_DOMAIN_NETDEV_MACVTAP_MODE_PRIVATE,
> >     VIR_DOMAIN_NETDEV_MACVTAP_MODE_BRIDGE,
> > +    VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU,
> 
> But here, the abbreviation is fine (it's internal to the code).
> 
> > 
> >     VIR_DOMAIN_NETDEV_MACVTAP_MODE_LAST,
> > };
> > diff --git a/src/util/macvtap.c b/src/util/macvtap.c
> > index a7af0cb..1b274cc 100644
> > --- a/src/util/macvtap.c
> > +++ b/src/util/macvtap.c
> > @@ -473,6 +473,9 @@ macvtapModeFromInt(enum virDomainNetdevMacvtapType
> > mode)
> >     case VIR_DOMAIN_NETDEV_MACVTAP_MODE_BRIDGE:
> >         return MACVLAN_MODE_BRIDGE;
> > 
> > +    case VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU:
> > +        return MACVLAN_MODE_PASSTHRU;
> 
> This fails to compile for me on kernel-2.6.35.13-91.fc14.x86_64, but I
> see on rawhide that this is 8, so we need a #define fallback value.
> Shoot, in rawhide, it's an enum rather than a #define, so we need a
> configure check rather than a simpler preprocessor check.
> 
> This is what I'll probably push.
> 
> 
> From f977a09221757826ee91dd966e8d726b0bc22501 Mon Sep 17 00:00:00 2001
> From: Dirk Herrendorefer <d herrendoerfer herrendoerfer name>
> Date: Tue, 17 May 2011 13:26:09 +0200
> Subject: [PATCH] Add support for 'passthru' mode for direct network
>  interfaces
> 
> starting with kernel 2.6.38 macvtap supports a 'passthru' mode for
> attaching virtual functions of a SRIOV capable network card directly to
> a VM.
> This patch adds the capability to configure such a device.
> 
> Signed-off-by: Dirk Herrendoerfer <d herrendoerfer herrendoerfer name>
> ---
>  AUTHORS                   |    1 +
>  configure.ac              |    7 +++++++
>  docs/formatdomain.html.in |    7 +++++++
>  docs/schemas/domain.rng   |    2 +-
>  src/conf/domain_conf.c    |    3 ++-
>  src/conf/domain_conf.h    |    1 +
>  src/util/macvtap.c        |   12 ++++++++++--
>  src/util/macvtap.h        |    1 +
>  8 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/AUTHORS b/AUTHORS
> index 1bb1f0f..a1e93db 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -170,6 +170,7 @@ Patches have also been contributed by:
> index 233e4af..8db3226 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2240,6 +2240,13 @@ if test "$with_macvtap" != "no" ; then
>  fi
>  AM_CONDITIONAL([WITH_MACVTAP], [test "$with_macvtap" = "yes"])
>  AC_MSG_RESULT([$with_macvtap])
> +if test "$with_macvtap" = yes; then
> +    AC_CHECK_DECLS([MACVLAN_MODE_PASSTHRU], [], [], [[
> +      #include <sys/socket.h>
> +      #include <linux/if_link.h>
> +    ]])
> +fi
> +
> 
>  AC_ARG_WITH([virtualport],
>    AC_HELP_STRING([--with-virtualport],[enable virtual port support
> @<:@default=check@:>@]),
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 989dcf6..70a120b 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1429,6 +1429,13 @@
>        external router or gateway and that device sends them back to the
>        host. This procedure is followed if either the source or destination
>        device is in <code>private</code> mode.</dd>
> +      <dt><code>passthru</code></dt>
> +      <dd>This feature attaches a virtual function of a SRIOV capable
> +      NIC directly to a VM without losing the migration capability.
> +      All packets are sent to the VF/IF of the configured network device.
> +      Depending on the capabilities of the device additional
> prerequisites or
> +      limitations may apply; for example, on Linux this requires
> +      kernel 2.6.38 or newer. <span class="since">Since 0.9.2</span></dd>
>      </dl>
> 
>  <pre>
> diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng
> index 7163c6e..b252547 100644
> --- a/docs/schemas/domain.rng
> +++ b/docs/schemas/domain.rng
> @@ -2351,7 +2351,7 @@
>    </define>
>    <define name="bridgeMode">
>      <data type="string">
> -      <param name="pattern">(vepa|bridge|private)</param>
> +      <param name="pattern">(vepa|bridge|private|passthrough)</param>
>      </data>
>    </define>
>    <define name="addrMAC">
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 498438a..3298c80 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -433,7 +433,8 @@ VIR_ENUM_IMPL(virDomainSeclabel,
> VIR_DOMAIN_SECLABEL_LAST,
>  VIR_ENUM_IMPL(virDomainNetdevMacvtap, VIR_DOMAIN_NETDEV_MACVTAP_MODE_LAST,
>                "vepa",
>                "private",
> -              "bridge")
> +              "bridge",
> +              "passthrough")
> 
>  VIR_ENUM_IMPL(virVirtualPort, VIR_VIRTUALPORT_TYPE_LAST,
>                "none",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index fe42f21..5fe31d4 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -335,6 +335,7 @@ enum virDomainNetdevMacvtapType {
>      VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA,
>      VIR_DOMAIN_NETDEV_MACVTAP_MODE_PRIVATE,
>      VIR_DOMAIN_NETDEV_MACVTAP_MODE_BRIDGE,
> +    VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU,
> 
>      VIR_DOMAIN_NETDEV_MACVTAP_MODE_LAST,
>  };
> diff --git a/src/util/macvtap.c b/src/util/macvtap.c
> index a7af0cb..068638e 100644
> --- a/src/util/macvtap.c
> +++ b/src/util/macvtap.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2010 Red Hat, Inc.
> + * Copyright (C) 2010-2011 Red Hat, Inc.
>   * Copyright (C) 2010 IBM Corporation
>   *
>   * This library is free software; you can redistribute it and/or
> @@ -45,6 +45,11 @@
> 
>  # include <netlink/msg.h>
> 
> +/* Older kernels lacked this enum value.  */
> +# if !HAVE_DECL_MACVLAN_MODE_PASSTHRU
> +#  define MACVLAN_MODE_PASSTHRU 8
> +# endif
> +
>  #endif /* WITH_MACVTAP || WITH_VIRTUALPORT */
> 
>  #include "util.h"
> @@ -473,6 +478,9 @@ macvtapModeFromInt(enum virDomainNetdevMacvtapType mode)
>      case VIR_DOMAIN_NETDEV_MACVTAP_MODE_BRIDGE:
>          return MACVLAN_MODE_BRIDGE;
> 
> +    case VIR_DOMAIN_NETDEV_MACVTAP_MODE_PASSTHRU:
> +        return MACVLAN_MODE_PASSTHRU;
> +
>      case VIR_DOMAIN_NETDEV_MACVTAP_MODE_VEPA:
>      default:
>          return MACVLAN_MODE_VEPA;
> @@ -546,7 +554,7 @@ configMacvtapTap(int tapfd, int vnet_hdr)
>   *    be NULL if this function is supposed to choose a name
>   * @macaddress: The MAC address for the macvtap device
>   * @linkdev: The interface name of the NIC to connect to the external
> bridge
> - * @mode: int describing the mode for 'bridge', 'vepa' or 'private'.
> + * @mode: int describing the mode for 'bridge', 'vepa', 'private' or
> 'passthru'.
>   * @vnet_hdr: 1 to enable IFF_VNET_HDR, 0 to disable it
>   * @vmuuid: The UUID of the VM the macvtap belongs to
>   * @virtPortProfile: pointer to object holding the virtual port profile
> data
> diff --git a/src/util/macvtap.h b/src/util/macvtap.h
> index 54205c7..c45dd13 100644
> --- a/src/util/macvtap.h
> +++ b/src/util/macvtap.h
> @@ -92,6 +92,7 @@ void delMacvtap(const char *ifname,
>  #  define MACVTAP_MODE_PRIVATE_STR  "private"
>  #  define MACVTAP_MODE_VEPA_STR     "vepa"
>  #  define MACVTAP_MODE_BRIDGE_STR   "bridge"
> +#  define MACVTAP_MODE_PASSTHRU_STR "passthru"
> 
>  int vpAssociatePortProfileId(const char *macvtap_ifname,
>                               const unsigned char *macvtap_macaddr,

  That patch looks fine to me, ACK,
The only unresolved uncertainty to me is what happen if you try to use
it on a kernel without the support ?

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel veillard com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/


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