Re: [Qemu-devel] [snabb-devel] Re: [PATCH v10 15/18] Add the vhost-user netdev backend to the command line

[Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>]

[-- Attachment #1: Type: text/plain, Size: 18459 bytes --]

On Mon, Jun 9, 2014 at 4:31 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Mon, Jun 09, 2014 at 04:28:23PM +0300, Nikolay Nikolaev wrote:
> > Hello,
> >
> >
> > On Thu, Jun 5, 2014 at 5:37 PM, Luiz Capitulino <lcapitulino@redhat.com>
> wrote:
> >
> >     On Tue, 27 May 2014 15:06:43 +0300
> >     Nikolay Nikolaev <n.nikolaev@virtualopensystems.com> wrote:
> >
> >     > The supplied chardev id will be inspected for supported options.
> Only
> >     > a socket backend, with a set path (i.e. a Unix socket) and
> optionally
> >     > the server parameter set, will be allowed. Other options (nowait,
> telnet)
> >     > will make the chardev unusable and the netdev will not be
> initialised.
> >     >
> >     > Additional checks for validity:
> >     >   - requires `-numa node,memdev=..`
> >     >   - requires `-device virtio-net-*`
> >     >
> >     > The `vhostforce` option is used to force vhost-net when we deal
> with
> >     > non-MSIX guests.
> >     >
> >     > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> >     > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com
> >
> >
> >     I gave a quick review and apart from some minor comments below it
> seems
> >     good to me, but I think it would be good to have Eric's review too:
> >
> >     Acked-by: Luiz Capitulino <lcapitulino@redhat.com>
> >
> > Thanks!
> >
> >
> >     > ---
> >     >  hmp-commands.hx    |    4 +-
> >     >  hw/net/vhost_net.c |    4 ++
> >     >  net/hub.c          |    1
> >     >  net/net.c          |   25 ++++++-----
> >     >  net/vhost-user.c   |  114
> >     +++++++++++++++++++++++++++++++++++++++++++++++++++-
> >     >  qapi-schema.json   |   19 ++++++++-
> >     >  qemu-options.hx    |   18 ++++++++
> >     >  7 files changed, 169 insertions(+), 16 deletions(-)
> >     >
> >     > diff --git a/hmp-commands.hx b/hmp-commands.hx
> >     > index 8971f1b..ef3782c 100644
> >     > --- a/hmp-commands.hx
> >     > +++ b/hmp-commands.hx
> >     > @@ -1205,7 +1205,7 @@ ETEXI
> >     >      {
> >     >          .name       = "host_net_add",
> >     >          .args_type  = "device:s,opts:s?",
> >     > -        .params     = "tap|user|socket|vde|netmap|dump [options]",
> >     > +        .params     = "tap|user|socket|vde|netmap|vhost-user|dump
> >     [options]",
> >     >          .help       = "add host VLAN client",
> >     >          .mhandler.cmd = net_host_device_add,
> >     >      },
> >     > @@ -1233,7 +1233,7 @@ ETEXI
> >     >      {
> >     >          .name       = "netdev_add",
> >     >          .args_type  = "netdev:O",
> >     > -        .params     =
> "[user|tap|socket|hubport|netmap],id=str[,prop=
> >     value][,...]",
> >     > +        .params     =
> "[user|tap|socket|hubport|netmap|vhost-user],id=
> >     str[,prop=value][,...]",
> >     >          .help       = "add host network device",
> >     >          .mhandler.cmd = hmp_netdev_add,
> >     >      },
> >     > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> >     > index 5f06736..7ac7c21 100644
> >     > --- a/hw/net/vhost_net.c
> >     > +++ b/hw/net/vhost_net.c
> >     > @@ -15,6 +15,7 @@
> >     >
> >     >  #include "net/net.h"
> >     >  #include "net/tap.h"
> >     > +#include "net/vhost-user.h"
> >     >
> >     >  #include "hw/virtio/virtio-net.h"
> >     >  #include "net/vhost_net.h"
> >     > @@ -360,6 +361,9 @@ VHostNetState *get_vhost_net(NetClientState
> *nc)
> >     >      case NET_CLIENT_OPTIONS_KIND_TAP:
> >     >          vhost_net = tap_get_vhost_net(nc);
> >     >          break;
> >     > +    case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
> >     > +        vhost_net = vhost_user_get_vhost_net(nc);
> >     > +        break;
> >     >      default:
> >     >          break;
> >     >      }
> >     > diff --git a/net/hub.c b/net/hub.c
> >     > index 33a99c9..7e0f2d6 100644
> >     > --- a/net/hub.c
> >     > +++ b/net/hub.c
> >     > @@ -322,6 +322,7 @@ void net_hub_check_clients(void)
> >     >              case NET_CLIENT_OPTIONS_KIND_TAP:
> >     >              case NET_CLIENT_OPTIONS_KIND_SOCKET:
> >     >              case NET_CLIENT_OPTIONS_KIND_VDE:
> >     > +            case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
> >     >                  has_host_dev = 1;
> >     >                  break;
> >     >              default:
> >     > diff --git a/net/net.c b/net/net.c
> >     > index 9db4dba..907f679 100644
> >     > --- a/net/net.c
> >     > +++ b/net/net.c
> >     > @@ -769,23 +769,24 @@ static int (* const net_client_init_fun
> >     [NET_CLIENT_OPTIONS_KIND_MAX])(
> >     >      const NetClientOptions *opts,
> >     >      const char *name,
> >     >      NetClientState *peer) = {
> >     > -        [NET_CLIENT_OPTIONS_KIND_NIC]       = net_init_nic,
> >     > +        [NET_CLIENT_OPTIONS_KIND_NIC]           = net_init_nic,
> >     >  #ifdef CONFIG_SLIRP
> >     > -        [NET_CLIENT_OPTIONS_KIND_USER]      = net_init_slirp,
> >     > +        [NET_CLIENT_OPTIONS_KIND_USER]          = net_init_slirp,
> >     >  #endif
> >     > -        [NET_CLIENT_OPTIONS_KIND_TAP]       = net_init_tap,
> >     > -        [NET_CLIENT_OPTIONS_KIND_SOCKET]    = net_init_socket,
> >     > +        [NET_CLIENT_OPTIONS_KIND_TAP]           = net_init_tap,
> >     > +        [NET_CLIENT_OPTIONS_KIND_SOCKET]        = net_init_socket,
> >     >  #ifdef CONFIG_VDE
> >     > -        [NET_CLIENT_OPTIONS_KIND_VDE]       = net_init_vde,
> >     > +        [NET_CLIENT_OPTIONS_KIND_VDE]           = net_init_vde,
> >     >  #endif
> >     >  #ifdef CONFIG_NETMAP
> >     > -        [NET_CLIENT_OPTIONS_KIND_NETMAP]    = net_init_netmap,
> >     > +        [NET_CLIENT_OPTIONS_KIND_NETMAP]        = net_init_netmap,
> >     >  #endif
> >     > -        [NET_CLIENT_OPTIONS_KIND_DUMP]      = net_init_dump,
> >     > +        [NET_CLIENT_OPTIONS_KIND_DUMP]          = net_init_dump,
> >     >  #ifdef CONFIG_NET_BRIDGE
> >     > -        [NET_CLIENT_OPTIONS_KIND_BRIDGE]    = net_init_bridge,
> >     > +        [NET_CLIENT_OPTIONS_KIND_BRIDGE]        = net_init_bridge,
> >
> >     These changes are unrelated.
> >
> > OK. Removing them.
> >
> >
> >     >  #endif
> >     > -        [NET_CLIENT_OPTIONS_KIND_HUBPORT]   = net_init_hubport,
> >     > +        [NET_CLIENT_OPTIONS_KIND_HUBPORT]       =
> net_init_hubport,
> >     > +        [NET_CLIENT_OPTIONS_KIND_VHOST_USER]    =
> net_init_vhost_user,
> >     >  };
> >     >
> >     >
> >     > @@ -819,6 +820,7 @@ static int net_client_init1(const void
> *object, int
> >     is_netdev, Error **errp)
> >     >          case NET_CLIENT_OPTIONS_KIND_BRIDGE:
> >     >  #endif
> >     >          case NET_CLIENT_OPTIONS_KIND_HUBPORT:
> >     > +        case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
> >     >              break;
> >     >
> >     >          default:
> >     > @@ -902,11 +904,12 @@ static int net_host_check_device(const char
> >     *device)
> >     >                                         , "bridge"
> >     >  #endif
> >     >  #ifdef CONFIG_SLIRP
> >     > -                                       ,"user"
> >     > +                                       , "user"
> >     >  #endif
> >     >  #ifdef CONFIG_VDE
> >     > -                                       ,"vde"
> >     > +                                       , "vde"
> >     >  #endif
> >     > +                                       , "vhost-user"
> >     >      };
> >     >      for (i = 0; i < ARRAY_SIZE(valid_param_list); i++) {
> >     >          if (!strncmp(valid_param_list[i], device,
> >     > diff --git a/net/vhost-user.c b/net/vhost-user.c
> >     > index 4bdd19d..69a5eb4 100644
> >     > --- a/net/vhost-user.c
> >     > +++ b/net/vhost-user.c
> >     > @@ -12,6 +12,7 @@
> >     >  #include "net/vhost_net.h"
> >     >  #include "net/vhost-user.h"
> >     >  #include "sysemu/char.h"
> >     > +#include "qemu/config-file.h"
> >     >  #include "qemu/error-report.h"
> >     >
> >     >  typedef struct VhostUserState {
> >     > @@ -21,9 +22,17 @@ typedef struct VhostUserState {
> >     >      VHostNetState *vhost_net;
> >     >  } VhostUserState;
> >     >
> >     > +typedef struct VhostUserChardevProps {
> >     > +    bool is_socket;
> >     > +    bool is_unix;
> >     > +    bool is_server;
> >     > +    bool has_unsupported;
> >     > +} VhostUserChardevProps;
> >     > +
> >     >  VHostNetState *vhost_user_get_vhost_net(NetClientState *nc)
> >     >  {
> >     >      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> >     > +    assert(nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> >     >      return s->vhost_net;
> >     >  }
> >     >
> >     > @@ -82,7 +91,7 @@ static bool vhost_user_has_ufo(NetClientState
> *nc)
> >     >  }
> >     >
> >     >  static NetClientInfo net_vhost_user_info = {
> >     > -        .type = 0,
> >     > +        .type = NET_CLIENT_OPTIONS_KIND_VHOST_USER,
> >     >          .size = sizeof(VhostUserState),
> >     >          .cleanup = vhost_user_cleanup,
> >     >          .has_vnet_hdr = vhost_user_has_vnet_hdr,
> >     > @@ -148,8 +157,109 @@ static int net_vhost_user_init(NetClientState
> >     *peer, const char *device,
> >     >      return 0;
> >     >  }
> >     >
> >     > +static int net_vhost_chardev_opts(const char *name, const char
> *value,
> >     > +        void *opaque)
> >     > +{
> >     > +    VhostUserChardevProps *props = opaque;
> >     > +
> >     > +    if (strcmp(name, "backend") == 0 && strcmp(value, "socket")
> == 0) {
> >     > +        props->is_socket = 1;
> >     > +    } else if (strcmp(name, "path") == 0) {
> >     > +        props->is_unix = 1;
> >     > +    } else if (strcmp(name, "server") == 0) {
> >     > +        props->is_server = 1;
> >     > +    } else {
> >     > +        error_report("vhost-user does not support a chardev"
> >     > +                     " with the following option:\n %s = %s",
> >     > +                     name, value);
> >     > +        props->has_unsupported = 1;
> >     > +        return -1;
> >     > +    }
> >     > +    return 0;
> >     > +}
> >     > +
> >     > +static CharDriverState *net_vhost_parse_chardev(
> >     > +        const NetdevVhostUserOptions *opts)
> >     > +{
> >     > +    CharDriverState *chr = qemu_chr_find(opts->chardev);
> >     > +    VhostUserChardevProps props;
> >     > +
> >     > +    if (chr == NULL) {
> >     > +        error_report("chardev \"%s\" not found\n", opts->chardev);
> >     > +        return 0;
> >     > +    }
> >     > +
> >     > +    /* inspect chardev opts */
> >     > +    memset(&props, 0, sizeof(props));
> >     > +    qemu_opt_foreach(chr->opts, net_vhost_chardev_opts, &props,
> false);
> >     > +
> >     > +    if (!props.is_socket || !props.is_unix) {
> >     > +        error_report("chardev \"%s\" is not a unix socket\n",
> >     > +                     opts->chardev);
> >     > +        return 0;
> >     > +    }
> >     > +
> >     > +    if (props.has_unsupported) {
> >     > +        error_report("chardev \"%s\" has an unsupported option\n",
> >     > +                opts->chardev);
> >     > +        return 0;
> >     > +    }
> >     > +
> >     > +    qemu_chr_fe_claim_no_fail(chr);
> >     > +
> >     > +    return chr;
> >     > +}
> >     > +
> >     > +static int net_vhost_check_net(QemuOpts *opts, void *opaque)
> >     > +{
> >     > +    const char *name = opaque;
> >     > +    const char *driver, *netdev;
> >     > +    const char virtio_name[] = "virtio-net-";
> >     > +
> >     > +    driver = qemu_opt_get(opts, "driver");
> >     > +    netdev = qemu_opt_get(opts, "netdev");
> >     > +
> >     > +    if (!driver || !netdev) {
> >     > +        return 0;
> >     > +    }
> >     > +
> >     > +    if ((strcmp(netdev, name) == 0)
> >     > +            && (strncmp(driver, virtio_name, strlen(virtio_name))
> != 0))
> >     {
> >     > +        error_report("vhost-user requires frontend driver
> >     virtio-net-*");
> >     > +        return -1;
> >     > +    }
> >     > +
> >     > +    return 0;
> >     > +}
> >     > +
> >     >  int net_init_vhost_user(const NetClientOptions *opts, const char
> *name,
> >     >                     NetClientState *peer)
> >     >  {
> >     > -    return net_vhost_user_init(peer, "vhost_user", 0, 0, 0);
> >     > +    const NetdevVhostUserOptions *vhost_user_opts;
> >     > +    CharDriverState *chr;
> >     > +    bool vhostforce;
> >     > +
> >     > +    assert(opts->kind == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
> >     > +    vhost_user_opts = opts->vhost_user;
> >     > +
> >     > +    chr = net_vhost_parse_chardev(vhost_user_opts);
> >     > +    if (!chr) {
> >     > +        error_report("No suitable chardev found\n");
> >     > +        return -1;
> >     > +    }
> >     > +
> >     > +    /* verify net frontend */
> >     > +    if (qemu_opts_foreach(qemu_find_opts("device"),
> net_vhost_check_net,
> >     > +                          (void *)name, true) == -1) {
> >     > +        return -1;
> >     > +    }
> >     > +
> >     > +    /* vhostforce for non-MSIX */
> >     > +    if (vhost_user_opts->has_vhostforce) {
> >     > +        vhostforce = vhost_user_opts->vhostforce;
> >     > +    } else {
> >     > +        vhostforce = false;
> >     > +    }
> >     > +
> >     > +    return net_vhost_user_init(peer, "vhost_user", name, chr,
> >     vhostforce);
> >     >  }
> >     > diff --git a/qapi-schema.json b/qapi-schema.json
> >     > index 1f28177..f458dd8 100644
> >     > --- a/qapi-schema.json
> >     > +++ b/qapi-schema.json
> >     > @@ -3264,6 +3264,22 @@
> >     >      '*devname':    'str' } }
> >     >
> >     >  ##
> >     > +# @NetdevVhostUserOptions
> >     > +#
> >     > +# Vhost-user network backend
> >     > +#
> >     > +# @path: control socket path
> >     > +#
> >     > +# @vhostforce: #optional vhost on for non-MSIX virtio guests
> (default:
> >     false).
> >     > +#
> >     > +# Since 2.1
> >     > +##
> >     > +{ 'type': 'NetdevVhostUserOptions',
> >     > +  'data': {
> >     > +    'chardev':        'str',
> >
> >     chardev or path?
> >
> > Right, it's chardev.
> >
> >
> >     > +    '*vhostforce':    'bool' } }
> >     > +
> >     > +##
> >     >  # @NetClientOptions
> >     >  #
> >     >  # A discriminated record of network device traits.
> >     > @@ -3281,7 +3297,8 @@
> >     >      'dump':     'NetdevDumpOptions',
> >     >      'bridge':   'NetdevBridgeOptions',
> >     >      'hubport':  'NetdevHubPortOptions',
> >     > -    'netmap':   'NetdevNetmapOptions' } }
> >     > +    'netmap':   'NetdevNetmapOptions',
> >     > +    'vhost-user': 'NetdevVhostUserOptions' } }
> >     >
> >     >  ##
> >     >  # @NetLegacy
> >     > diff --git a/qemu-options.hx b/qemu-options.hx
> >     > index 7f4ab83..2514264 100644
> >     > --- a/qemu-options.hx
> >     > +++ b/qemu-options.hx
> >     > @@ -1459,6 +1459,7 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
> >     >  #ifdef CONFIG_NETMAP
> >     >      "netmap|"
> >     >  #endif
> >     > +    "vhost-user|"
> >     >      "socket|"
> >     >      "hubport],id=str[,option][,option][,...]\n", QEMU_ARCH_ALL)
> >     >  STEXI
> >     > @@ -1790,6 +1791,23 @@ The hubport netdev lets you connect a NIC
> to a
> >     QEMU "vlan" instead of a single
> >     >  netdev.  @code{-net} and @code{-device} with parameter
> @option{vlan}
> >     create the
> >     >  required hub automatically.
> >     >
> >     > +@item -netdev vhost-user,chardev=@var{id}[,vhostforce=on|off]
> >     > +
> >     > +Establish a vhost-user netdev, backed by a chardev @var{id}. The
> chardev
> >     should
> >     > +be a unix domain socket backed one. The vhost-user uses a
> specifically
> >     defined
> >     > +protocol to pass vhost ioctl replacement messages to an
> application on
> >     the other
> >     > +end of the socket. On non-MSIX guests, the feature can be forced
> with
> >     > +@var{vhostforce}.
> >     > +
> >     > +Example:
> >     > +@example
> >     > +qemu -m 512 -object memory-file,id=mem,size=512M,mem-path=/
> >     hugetlbfs,share=on \
> >     > +     -numa node,memdev=mem \
> >     > +     -chardev socket,path=/path/to/socket \
> >     > +     -netdev type=vhost-user,id=net0,chardev=chr0 \
> >     > +     -device virtio-net-pci,netdev=net0
> >     > +@end example
> >     > +
> >     >  @item -net dump[,vlan=@var{n}][,file=@var{file}][,len=@var{len}]
> >     >  Dump network traffic on VLAN @var{n} to file @var{file} (@file
> >     {qemu-vlan0.pcap} by default).
> >     >  At most @var{len} bytes (64k by default) per packet are stored.
> The file
> >     format is
> >     >
> >     >
> >
> >     --
> >     You received this message because you are subscribed to the Google
> Groups
> >     "Snabb Switch development" group.
> >     To unsubscribe from this group and stop receiving emails from it,
> send an
> >     email to snabb-devel+unsubscribe@googlegroups.com.
> >     To post to this group, send an email to snabb-devel@googlegroups.com
> .
> >     Visit this group at http://groups.google.com/group/snabb-devel.
> >
> >
> > regards,
> > Nikolay Nikolaev
>
>
> Pls remember to base on top of vhost branch in my tree,
> this way you will not need to re-post merged patches.
>

OK - that's what we're doing. There are two questions though:

 - should we post with separate numbering - 15,16,18 will become 1/3, 2/3,
3/3? Or do you prefer to preserve the original numbering?

 - We can not test it. Probably we get something wrong, but vhost-user-v10
was rebased on Hu Tao's NUMA v3.2 series. And it uses
this on the command line to enalbe mmaped memory share flag (on HUGETLBFS):

     -object memory-file,id=mem,size=512M,mem-path=/hugetlbfs,share=on \
      -numa node,memdev=mem \

Now, this does not work, which will break the qtest (patch 18). Maybe we
misled you by saying it is rebased on memdev instead of naming it NUMA
patchseries.


>
> If you want me to drop some merged patch from my tree,
> include a revert and I'll figure it out.
>
> Will have this in mind - thanks.

>  --
> MST
>
> --
> You received this message because you are subscribed to the Google Groups
> "Snabb Switch development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to snabb-devel+unsubscribe@googlegroups.com.
> To post to this group, send an email to snabb-devel@googlegroups.com.
> Visit this group at http://groups.google.com/group/snabb-devel.
>

regards,
Nikolay Nikolaev

[-- Attachment #2: Type: text/html, Size: 26263 bytes --]