Patchwork [PATCH-RFC,09/13] tap: add vhost/vhostfd options

login
register
mail settings
Submitter Michael S. Tsirkin
Date Jan. 11, 2010, 5:22 p.m.
Message ID <20100111172217.GJ11936@redhat.com>
Download mbox | patch
Permalink /patch/42642/
State New
Headers show

Comments

Michael S. Tsirkin - Jan. 11, 2010, 5:22 p.m.
Looks like order got mixed up: vhost_net header
is added by a follow-up patch. Will be fixed
in the next revision.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 net.c           |    8 ++++++++
 net/tap.c       |   29 +++++++++++++++++++++++++++++
 qemu-options.hx |    4 +++-
 3 files changed, 40 insertions(+), 1 deletions(-)
Anthony Liguori - Jan. 12, 2010, 10:39 p.m.
On 01/11/2010 11:22 AM, Michael S. Tsirkin wrote:
> Looks like order got mixed up: vhost_net header
> is added by a follow-up patch. Will be fixed
> in the next revision.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>   net.c           |    8 ++++++++
>   net/tap.c       |   29 +++++++++++++++++++++++++++++
>   qemu-options.hx |    4 +++-
>   3 files changed, 40 insertions(+), 1 deletions(-)
>
> diff --git a/net.c b/net.c
> index 6ef93e6..b942d03 100644
> --- a/net.c
> +++ b/net.c
> @@ -976,6 +976,14 @@ static struct {
>                   .name = "vnet_hdr",
>                   .type = QEMU_OPT_BOOL,
>                   .help = "enable the IFF_VNET_HDR flag on the tap interface"
> +            }, {
> +                .name = "vhost",
> +                .type = QEMU_OPT_BOOL,
> +                .help = "enable vhost-net network accelerator",
> +            }, {
> +                .name = "vhostfd",
> +                .type = QEMU_OPT_STRING,
> +                .help = "file descriptor of an already opened vhost net device",
>               },
>    

Semantically, I think making vhost it's own backend makes more sense 
from a user perspective.

I don't think it's a significant code change.

Regards,

Anthony Liguori
Anthony Liguori - Jan. 12, 2010, 10:42 p.m.
On 01/11/2010 11:22 AM, Michael S. Tsirkin wrote:
> Looks like order got mixed up: vhost_net header
> is added by a follow-up patch. Will be fixed
> in the next revision.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>   net.c           |    8 ++++++++
>   net/tap.c       |   29 +++++++++++++++++++++++++++++
>   qemu-options.hx |    4 +++-
>   3 files changed, 40 insertions(+), 1 deletions(-)
>
> diff --git a/net.c b/net.c
> index 6ef93e6..b942d03 100644
> --- a/net.c
> +++ b/net.c
> @@ -976,6 +976,14 @@ static struct {
>                   .name = "vnet_hdr",
>                   .type = QEMU_OPT_BOOL,
>                   .help = "enable the IFF_VNET_HDR flag on the tap interface"
> +            }, {
> +                .name = "vhost",
> +                .type = QEMU_OPT_BOOL,
> +                .help = "enable vhost-net network accelerator",
> +            }, {
> +                .name = "vhostfd",
> +                .type = QEMU_OPT_STRING,
> +                .help = "file descriptor of an already opened vhost net device",
>               },
>   #endif /* _WIN32 */
>               { /* end of list */ }
> diff --git a/net/tap.c b/net/tap.c
> index 7e9ca79..d9f2e41 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -41,6 +41,8 @@
>
>   #include "net/tap-linux.h"
>
> +#include "hw/vhost_net.h"
> +
>   /* Maximum GSO packet size (64k) plus plenty of room for
>    * the ethernet and virtio_net headers
>    */
> @@ -57,6 +59,7 @@ typedef struct TAPState {
>       unsigned int has_vnet_hdr : 1;
>       unsigned int using_vnet_hdr : 1;
>       unsigned int has_ufo: 1;
> +    struct vhost_net *vhost_net;
>   } TAPState;
>
>   static int launch_script(const char *setup_script, const char *ifname, int fd);
> @@ -252,6 +255,10 @@ static void tap_cleanup(VLANClientState *nc)
>   {
>       TAPState *s = DO_UPCAST(TAPState, nc, nc);
>
> +    if (s->vhost_net) {
> +        vhost_net_cleanup(s->vhost_net);
> +    }
> +
>       qemu_purge_queued_packets(nc);
>
>       if (s->down_script[0])
> @@ -307,6 +314,7 @@ static TAPState *net_tap_fd_init(VLANState *vlan,
>       s->has_ufo = tap_probe_has_ufo(s->fd);
>       tap_set_offload(&s->nc, 0, 0, 0, 0, 0);
>       tap_read_poll(s, 1);
> +    s->vhost_net = NULL;
>       return s;
>   }
>
> @@ -456,6 +464,27 @@ int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
>           }
>       }
>
> +    if (qemu_opt_get_bool(opts, "vhost", 0)) {
> +        int vhostfd, r;
> +        if (qemu_opt_get(opts, "vhostfd")) {
> +            r = net_handle_fd_param(mon, qemu_opt_get(opts, "vhostfd"));
> +            if (r == -1) {
> +                return -1;
> +            }
> +            vhostfd = r;
> +        } else {
> +            vhostfd = -1;
> +        }
> +        s->vhost_net = vhost_net_init(&s->nc, vhostfd);
>    

Hrm, I don't see a patch that introduces this function?  Am I missing 
something obvious?

Regards,

Anthony Liguori
Michael S. Tsirkin - Jan. 13, 2010, 10:59 a.m.
On Tue, Jan 12, 2010 at 04:39:52PM -0600, Anthony Liguori wrote:
> On 01/11/2010 11:22 AM, Michael S. Tsirkin wrote:
>> Looks like order got mixed up: vhost_net header
>> is added by a follow-up patch. Will be fixed
>> in the next revision.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> ---
>>   net.c           |    8 ++++++++
>>   net/tap.c       |   29 +++++++++++++++++++++++++++++
>>   qemu-options.hx |    4 +++-
>>   3 files changed, 40 insertions(+), 1 deletions(-)
>>
>> diff --git a/net.c b/net.c
>> index 6ef93e6..b942d03 100644
>> --- a/net.c
>> +++ b/net.c
>> @@ -976,6 +976,14 @@ static struct {
>>                   .name = "vnet_hdr",
>>                   .type = QEMU_OPT_BOOL,
>>                   .help = "enable the IFF_VNET_HDR flag on the tap interface"
>> +            }, {
>> +                .name = "vhost",
>> +                .type = QEMU_OPT_BOOL,
>> +                .help = "enable vhost-net network accelerator",
>> +            }, {
>> +                .name = "vhostfd",
>> +                .type = QEMU_OPT_STRING,
>> +                .help = "file descriptor of an already opened vhost net device",
>>               },
>>    
>
> Semantically, I think making vhost it's own backend makes more sense  
> from a user perspective.


It doesn't. Users mostly do not care that vhost is used: they just get
fast virtio and that's all.  Users do care about e.g. tap because they
need setup scripts, understand bridging etc.


Do you propose -net tap be replaced with -net vhost?  This means vhost will
need to get tap flags if it is attached to tap and raw flags if attached
to raw, etc.

A separate backend that only works with virtio frontend is also
ugly. OTOH an option that has effect only with virtio frontend
is pretty usual, vnet_hdr is one such example.


>
> I don't think it's a significant code change.
>
> Regards,
>
> Anthony Liguori

Patch

diff --git a/net.c b/net.c
index 6ef93e6..b942d03 100644
--- a/net.c
+++ b/net.c
@@ -976,6 +976,14 @@  static struct {
                 .name = "vnet_hdr",
                 .type = QEMU_OPT_BOOL,
                 .help = "enable the IFF_VNET_HDR flag on the tap interface"
+            }, {
+                .name = "vhost",
+                .type = QEMU_OPT_BOOL,
+                .help = "enable vhost-net network accelerator",
+            }, {
+                .name = "vhostfd",
+                .type = QEMU_OPT_STRING,
+                .help = "file descriptor of an already opened vhost net device",
             },
 #endif /* _WIN32 */
             { /* end of list */ }
diff --git a/net/tap.c b/net/tap.c
index 7e9ca79..d9f2e41 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -41,6 +41,8 @@ 
 
 #include "net/tap-linux.h"
 
+#include "hw/vhost_net.h"
+
 /* Maximum GSO packet size (64k) plus plenty of room for
  * the ethernet and virtio_net headers
  */
@@ -57,6 +59,7 @@  typedef struct TAPState {
     unsigned int has_vnet_hdr : 1;
     unsigned int using_vnet_hdr : 1;
     unsigned int has_ufo: 1;
+    struct vhost_net *vhost_net;
 } TAPState;
 
 static int launch_script(const char *setup_script, const char *ifname, int fd);
@@ -252,6 +255,10 @@  static void tap_cleanup(VLANClientState *nc)
 {
     TAPState *s = DO_UPCAST(TAPState, nc, nc);
 
+    if (s->vhost_net) {
+        vhost_net_cleanup(s->vhost_net);
+    }
+
     qemu_purge_queued_packets(nc);
 
     if (s->down_script[0])
@@ -307,6 +314,7 @@  static TAPState *net_tap_fd_init(VLANState *vlan,
     s->has_ufo = tap_probe_has_ufo(s->fd);
     tap_set_offload(&s->nc, 0, 0, 0, 0, 0);
     tap_read_poll(s, 1);
+    s->vhost_net = NULL;
     return s;
 }
 
@@ -456,6 +464,27 @@  int net_init_tap(QemuOpts *opts, Monitor *mon, const char *name, VLANState *vlan
         }
     }
 
+    if (qemu_opt_get_bool(opts, "vhost", 0)) {
+        int vhostfd, r;
+        if (qemu_opt_get(opts, "vhostfd")) {
+            r = net_handle_fd_param(mon, qemu_opt_get(opts, "vhostfd"));
+            if (r == -1) {
+                return -1;
+            }
+            vhostfd = r;
+        } else {
+            vhostfd = -1;
+        }
+        s->vhost_net = vhost_net_init(&s->nc, vhostfd);
+        if (!s->vhost_net) {
+            qemu_error("vhost-net requested but could not be initialized\n");
+            return -1;
+        }
+    } else if (qemu_opt_get(opts, "vhostfd")) {
+        qemu_error("vhostfd= is not valid without vhost\n");
+        return -1;
+    }
+
     if (vlan) {
         vlan->nb_host_devs++;
     }
diff --git a/qemu-options.hx b/qemu-options.hx
index ecd50eb..ef185dd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -813,7 +813,7 @@  DEF("net", HAS_ARG, QEMU_OPTION_net,
     "-net tap[,vlan=n][,name=str],ifname=name\n"
     "                connect the host TAP network interface to VLAN 'n'\n"
 #else
-    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off]\n"
+    "-net tap[,vlan=n][,name=str][,fd=h][,ifname=name][,script=file][,downscript=dfile][,sndbuf=nbytes][,vnet_hdr=on|off][,vhost=on|off][,vhostfd=h]\n"
     "                connect the host TAP network interface to VLAN 'n' and use the\n"
     "                network scripts 'file' (default=%s)\n"
     "                and 'dfile' (default=%s);\n"
@@ -823,6 +823,8 @@  DEF("net", HAS_ARG, QEMU_OPTION_net,
     "                default of 'sndbuf=1048576' can be disabled using 'sndbuf=0'\n"
     "                use vnet_hdr=off to avoid enabling the IFF_VNET_HDR tap flag; use\n"
     "                vnet_hdr=on to make the lack of IFF_VNET_HDR support an error condition\n"
+    "                use vhost=on to enable experimental in kernel accelerator\n"
+    "                use 'vhostfd=h' to connect to an already opened vhost net device\n"
 #endif
     "-net socket[,vlan=n][,name=str][,fd=h][,listen=[host]:port][,connect=host:port]\n"
     "                connect the vlan 'n' to another VLAN using a socket connection\n"