[PATCH 0/9] refine vhost-vdpa initialization

[PATCH 0/9] refine vhost-vdpa initialization

[Jason Wang]

Hi all:

This series refine vhost-vdpa initialization by:

- fixing SIGSEV when vhostdev is not specificed
- tweak the codes for vhost-vdpa initialization
- allow to accept preopen vhost-vdpa file descriptor

Please review.

Thanks

Jason Wang (9):
  vhost-vdpa: remove the default devname
  vhost-vdpa: mandate vhostdev
  vhost-vdpa: remove the unnecessary assert(s->vhost_net)
  vhost-vdpa: refine info string
  vhost-vdpa: remove the unnecessary initialization
  vhost-vdpa: remove the unnecessary queue index assignment
  vhost-vdpa: squash net_vhost_vdpa_init() into net_init_vhost_vdpa()
  vhost-vdpa: add accurate error string when fail to open vhost vDPA
    device
  vhost-vdpa: allow pre-opend file descriptor

 net/vhost-vdpa.c | 63 ++++++++++++++++++++++++++++++------------------
 qapi/net.json    |  6 +++--
 2 files changed, 43 insertions(+), 26 deletions(-)

-- 
2.20.1

[PATCH 1/9] vhost-vdpa: remove the default devname

[Jason Wang]

The code doesn't have a default vhostdev, so remove the default
description in net.json.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 qapi/net.json | 1 -
 1 file changed, 1 deletion(-)

diff --git a/qapi/net.json b/qapi/net.json
index ddb113e5e5..a2a94fad3e 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -438,7 +438,6 @@
 # specifications with a vendor specific control path.
 #
 # @vhostdev: path of vhost-vdpa device
-#            (default:'/dev/vhost-vdpa-0')
 #
 # @queues: number of queues to be created for multiqueue vhost-vdpa
 #          (default: 1)
-- 
2.20.1

[PATCH 2/9] vhost-vdpa: mandate vhostdev

[Jason Wang]

vhost-dev is mandatory for vhost-vdpa to be initialized otherwise
net_vhost_vdpa_init will pass an uninitialized pointer to qemu_open()
which will lead a SIGSEV:

#0  0x0000555555c3a640 in strstart (str=str@entry=0x0, val=val@entry=0x555555dbcb65 "/dev/fdset/", ptr=ptr@entry=0x7fffffffdfb8) at ../util/cutils.c:77
#1  0x0000555555c572a7 in qemu_open (name=name@entry=0x0, flags=flags@entry=2) at ../util/osdep.c:294
#2  0x000055555584314a in net_vhost_vdpa_init (device=0x555555c81baa "vhost-vdpa", vhostdev=0x0, name=0x555556396600 "hn0", peer=0x0) at ../net/vhost-vdpa.c:187
#3  0x000055555584314a in net_init_vhost_vdpa (netdev=<optimized out>, name=0x555556396600 "hn0", peer=0x0, errp=<optimized out>) at ../net/vhost-vdpa.c:227
#4  0x000055555587e8c9 in net_client_init1 (netdev=0x555556398790, is_netdev=is_netdev@entry=true, errp=errp@entry=0x7fffffffe290) at ../net/net.c:1008
#5  0x000055555587ecc7 in net_client_init (opts=0x555556192ff0, is_netdev=<optimized out>, errp=0x7fffffffe290) at ../net/net.c:1113
#6  0x0000555555c33082 in qemu_opts_foreach
    (list=<optimized out>, func=func@entry=0x55555587ed50 <net_init_netdev>, opaque=opaque@entry=0x0, errp=errp@entry=0x7fffffffe290) at ../util/qemu-option.c:1172
#7  0x0000555555880057 in net_init_clients (errp=errp@entry=0x7fffffffe290) at ../net/net.c:1494
#8  0x0000555555a7f8e9 in qemu_init (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../softmmu/vl.c:4250
#9  0x00005555557f26cd in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../softmmu/main.c:49

Cc: qemu-stable@nongnu.org
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bc0e0d2d35..b7221beaa1 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -206,7 +206,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
     }
     if (strcmp(netdev, name) == 0 &&
         !g_str_has_prefix(driver, "virtio-net-")) {
-        error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
+        error_setg(errp, "Vhost-vdpa requires frontend driver virtio-net-*");
         return -1;
     }
     return 0;
@@ -224,5 +224,9 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                           (char *)name, errp)) {
         return -1;
     }
+    if (!opts->has_vhostdev) {
+        error_setg(errp, "vhost-vdpa requires vhostdev to be set");
+        return -1;
+    }
     return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
 }
-- 
2.20.1

[PATCH 3/9] vhost-vdpa: remove the unnecessary assert(s->vhost_net)

[Jason Wang]

vhost_vdpa_add() can fail before s->vhost_net is set and
net_vhost_vdpa_init() can report error. So there's no need for this
assert. Let's remove it.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index b7221beaa1..c4568b885e 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -190,7 +190,7 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
     }
     s->vhost_vdpa.device_fd = vdpa_device_fd;
     ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
-    assert(s->vhost_net);
+
     return ret;
 }
 
-- 
2.20.1

[PATCH 4/9] vhost-vdpa: refine info string

[Jason Wang]

We use "vhost-vdpa" as nic info string which is not useful, let's add
the vhostdev path.

before:
info network
v0: index=0,type=vhost-vdpa,vhost-vdpa

after:
info network
v0: index=0,type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index c4568b885e..397d4d3082 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -181,7 +181,7 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
     int ret = 0;
     assert(name);
     nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
-    snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
+    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
     nc->queue_index = 0;
     s = DO_UPCAST(VhostVDPAState, nc, nc);
     vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
-- 
2.20.1

[PATCH 5/9] vhost-vdpa: remove the unnecessary initialization

[Jason Wang]

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 397d4d3082..bcbf49d55b 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -175,10 +175,10 @@ static NetClientInfo net_vhost_vdpa_info = {
 static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
                                const char *name, const char *vhostdev)
 {
-    NetClientState *nc = NULL;
+    NetClientState *nc;
     VhostVDPAState *s;
-    int vdpa_device_fd = -1;
-    int ret = 0;
+    int vdpa_device_fd;
+    int ret;
     assert(name);
     nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
     snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
-- 
2.20.1

[PATCH 6/9] vhost-vdpa: remove the unnecessary queue index assignment

[Jason Wang]

The nc is allocated through g_malloc0(), so no need to assign its
queue_index to zero.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index bcbf49d55b..1d3ac89135 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -182,7 +182,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
     assert(name);
     nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
     snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
-    nc->queue_index = 0;
     s = DO_UPCAST(VhostVDPAState, nc, nc);
     vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
     if (vdpa_device_fd == -1) {
-- 
2.20.1

[PATCH 7/9] vhost-vdpa: squash net_vhost_vdpa_init() into net_init_vhost_vdpa()

[Jason Wang]

This patch squashes net_vhost_vdpa_init() into
net_init_vhost_vdpa(). This will simplify adding pre open file
descriptor support.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 1d3ac89135..f5cc4e8326 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -172,27 +172,6 @@ static NetClientInfo net_vhost_vdpa_info = {
         .has_ufo = vhost_vdpa_has_ufo,
 };
 
-static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
-                               const char *name, const char *vhostdev)
-{
-    NetClientState *nc;
-    VhostVDPAState *s;
-    int vdpa_device_fd;
-    int ret;
-    assert(name);
-    nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
-    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
-    s = DO_UPCAST(VhostVDPAState, nc, nc);
-    vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
-    if (vdpa_device_fd == -1) {
-        return -errno;
-    }
-    s->vhost_vdpa.device_fd = vdpa_device_fd;
-    ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
-
-    return ret;
-}
-
 static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
 {
     const char *name = opaque;
@@ -215,6 +194,10 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                         NetClientState *peer, Error **errp)
 {
     const NetdevVhostVDPAOptions *opts;
+    NetClientState *nc;
+    VhostVDPAState *s;
+    int vdpa_device_fd;
+    int ret;
 
     assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
     opts = &netdev->u.vhost_vdpa;
@@ -227,5 +210,19 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
         error_setg(errp, "vhost-vdpa requires vhostdev to be set");
         return -1;
     }
-    return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
+
+    assert(name);
+
+    nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, TYPE_VHOST_VDPA, name);
+    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", opts->vhostdev);
+
+    s = DO_UPCAST(VhostVDPAState, nc, nc);
+    vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
+    if (vdpa_device_fd == -1) {
+        return -errno;
+    }
+    s->vhost_vdpa.device_fd = vdpa_device_fd;
+    ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
+
+    return ret;
 }
-- 
2.20.1

[PATCH 8/9] vhost-vdpa: add accurate error string when fail to open vhost vDPA device

[Jason Wang]

This patch adds more accurate error string when fail to open vhost
vDPA device.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index f5cc4e8326..9a6f0b63d3 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -219,6 +219,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
     s = DO_UPCAST(VhostVDPAState, nc, nc);
     vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
     if (vdpa_device_fd == -1) {
+        error_setg(errp, "Fail to open vhost-vdpa device %s", opts->vhostdev);
         return -errno;
     }
     s->vhost_vdpa.device_fd = vdpa_device_fd;
-- 
2.20.1

[PATCH 9/9] vhost-vdpa: allow pre-opend file descriptor

[Jason Wang]

This patch allows to initialize vhost-vdpa network backend with pre
opened vhost-vdpa file descriptor. This is useful for running
unprivileged qemu through libvirt.

Cc: Eric Blake <eblake@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 net/vhost-vdpa.c | 24 +++++++++++++++++++-----
 qapi/net.json    |  5 ++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 9a6f0b63d3..f6385cd264 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -206,20 +206,34 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
                           (char *)name, errp)) {
         return -1;
     }
-    if (!opts->has_vhostdev) {
-        error_setg(errp, "vhost-vdpa requires vhostdev to be set");
+    if (!(opts->has_vhostdev ^ opts->has_fd)) {
+        error_setg(errp, "Vhost-vdpa requires either vhostdev or fd to be set");
         return -1;
     }
 
     assert(name);
 
     nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, TYPE_VHOST_VDPA, name);
-    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", opts->vhostdev);
+    if (opts->has_vhostdev) {
+        snprintf(nc->info_str, sizeof(nc->info_str),
+                 "vhostdev=%s", opts->vhostdev);
+        vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
+        if (vdpa_device_fd == -1) {
+            error_setg(errp, "Fail to open vhost-vdpa device %s",
+                       opts->vhostdev);
+            return -errno;
+        }
+    } else {
+        snprintf(nc->info_str, sizeof(nc->info_str), "fd=%s", opts->fd);
+        vdpa_device_fd = monitor_fd_param(cur_mon, opts->fd, errp);
+        if (vdpa_device_fd == -1) {
+            return -1;
+        }
+    }
 
     s = DO_UPCAST(VhostVDPAState, nc, nc);
-    vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
     if (vdpa_device_fd == -1) {
-        error_setg(errp, "Fail to open vhost-vdpa device %s", opts->vhostdev);
+
         return -errno;
     }
     s->vhost_vdpa.device_fd = vdpa_device_fd;
diff --git a/qapi/net.json b/qapi/net.json
index a2a94fad3e..5ad60c3045 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -442,12 +442,15 @@
 # @queues: number of queues to be created for multiqueue vhost-vdpa
 #          (default: 1)
 #
+# @fd: file descriptor of an already opened vhost-vdpa (since 5.2)
+#
 # Since: 5.1
 ##
 { 'struct': 'NetdevVhostVDPAOptions',
   'data': {
     '*vhostdev':     'str',
-    '*queues':       'int' } }
+    '*queues':       'int',
+    '*fd':           'str' } }
 
 ##
 # @NetClientDriver:
-- 
2.20.1

Re: [PATCH 9/9] vhost-vdpa: allow pre-opend file descriptor

[Cindy Lu]

On Mon, 31 Aug 2020 at 16:30, Jason Wang <jasowang@redhat.com> wrote:
>
> This patch allows to initialize vhost-vdpa network backend with pre
> opened vhost-vdpa file descriptor. This is useful for running
> unprivileged qemu through libvirt.
>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 24 +++++++++++++++++++-----
>  qapi/net.json    |  5 ++++-
>  2 files changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 9a6f0b63d3..f6385cd264 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -206,20 +206,34 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                            (char *)name, errp)) {
>          return -1;
>      }
> -    if (!opts->has_vhostdev) {
> -        error_setg(errp, "vhost-vdpa requires vhostdev to be set");
> +    if (!(opts->has_vhostdev ^ opts->has_fd)) {
> +        error_setg(errp, "Vhost-vdpa requires either vhostdev or fd to be set");
>          return -1;
>      }
>
>      assert(name);
>
>      nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, TYPE_VHOST_VDPA, name);
> -    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", opts->vhostdev);
> +    if (opts->has_vhostdev) {
> +        snprintf(nc->info_str, sizeof(nc->info_str),
> +                 "vhostdev=%s", opts->vhostdev);
> +        vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
> +        if (vdpa_device_fd == -1) {
> +            error_setg(errp, "Fail to open vhost-vdpa device %s",
> +                       opts->vhostdev);
> +            return -errno;
> +        }
> +    } else {
> +        snprintf(nc->info_str, sizeof(nc->info_str), "fd=%s", opts->fd);
> +        vdpa_device_fd = monitor_fd_param(cur_mon, opts->fd, errp);
> +        if (vdpa_device_fd == -1) {
> +            return -1;
> +        }
> +    }
>
>      s = DO_UPCAST(VhostVDPAState, nc, nc);
> -    vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
>      if (vdpa_device_fd == -1) {
> -        error_setg(errp, "Fail to open vhost-vdpa device %s", opts->vhostdev);
> +
>          return -errno;
>      }
>      s->vhost_vdpa.device_fd = vdpa_device_fd;
> diff --git a/qapi/net.json b/qapi/net.json
> index a2a94fad3e..5ad60c3045 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -442,12 +442,15 @@
>  # @queues: number of queues to be created for multiqueue vhost-vdpa
>  #          (default: 1)
>  #
> +# @fd: file descriptor of an already opened vhost-vdpa (since 5.2)
> +#
>  # Since: 5.1
>  ##
>  { 'struct': 'NetdevVhostVDPAOptions',
>    'data': {
>      '*vhostdev':     'str',
> -    '*queues':       'int' } }
> +    '*queues':       'int',
> +    '*fd':           'str' } }
>
>  ##
>  # @NetClientDriver:
> --
> 2.20.1
>
I think the latest  code supported this part.
you can pass a pre open file descriptor to it via the add-fd QMP
command to /dev/fdset/NNN, and then pass the string
"/dev/fdset/NNN" as vhostdev.  so we don't need a special fd parameter here.

Thanks
Cindy

Re: [PATCH 9/9] vhost-vdpa: allow pre-opend file descriptor

[Jason Wang]

On 2020/8/31 下午7:16, Cindy Lu wrote:
> On Mon, 31 Aug 2020 at 16:30, Jason Wang <jasowang@redhat.com> wrote:
>> This patch allows to initialize vhost-vdpa network backend with pre
>> opened vhost-vdpa file descriptor. This is useful for running
>> unprivileged qemu through libvirt.
>>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   net/vhost-vdpa.c | 24 +++++++++++++++++++-----
>>   qapi/net.json    |  5 ++++-
>>   2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 9a6f0b63d3..f6385cd264 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -206,20 +206,34 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>>                             (char *)name, errp)) {
>>           return -1;
>>       }
>> -    if (!opts->has_vhostdev) {
>> -        error_setg(errp, "vhost-vdpa requires vhostdev to be set");
>> +    if (!(opts->has_vhostdev ^ opts->has_fd)) {
>> +        error_setg(errp, "Vhost-vdpa requires either vhostdev or fd to be set");
>>           return -1;
>>       }
>>
>>       assert(name);
>>
>>       nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, TYPE_VHOST_VDPA, name);
>> -    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", opts->vhostdev);
>> +    if (opts->has_vhostdev) {
>> +        snprintf(nc->info_str, sizeof(nc->info_str),
>> +                 "vhostdev=%s", opts->vhostdev);
>> +        vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
>> +        if (vdpa_device_fd == -1) {
>> +            error_setg(errp, "Fail to open vhost-vdpa device %s",
>> +                       opts->vhostdev);
>> +            return -errno;
>> +        }
>> +    } else {
>> +        snprintf(nc->info_str, sizeof(nc->info_str), "fd=%s", opts->fd);
>> +        vdpa_device_fd = monitor_fd_param(cur_mon, opts->fd, errp);
>> +        if (vdpa_device_fd == -1) {
>> +            return -1;
>> +        }
>> +    }
>>
>>       s = DO_UPCAST(VhostVDPAState, nc, nc);
>> -    vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
>>       if (vdpa_device_fd == -1) {
>> -        error_setg(errp, "Fail to open vhost-vdpa device %s", opts->vhostdev);
>> +
>>           return -errno;
>>       }
>>       s->vhost_vdpa.device_fd = vdpa_device_fd;
>> diff --git a/qapi/net.json b/qapi/net.json
>> index a2a94fad3e..5ad60c3045 100644
>> --- a/qapi/net.json
>> +++ b/qapi/net.json
>> @@ -442,12 +442,15 @@
>>   # @queues: number of queues to be created for multiqueue vhost-vdpa
>>   #          (default: 1)
>>   #
>> +# @fd: file descriptor of an already opened vhost-vdpa (since 5.2)
>> +#
>>   # Since: 5.1
>>   ##
>>   { 'struct': 'NetdevVhostVDPAOptions',
>>     'data': {
>>       '*vhostdev':     'str',
>> -    '*queues':       'int' } }
>> +    '*queues':       'int',
>> +    '*fd':           'str' } }
>>
>>   ##
>>   # @NetClientDriver:
>> --
>> 2.20.1
>>
> I think the latest  code supported this part.
> you can pass a pre open file descriptor to it via the add-fd QMP
> command to /dev/fdset/NNN, and then pass the string
> "/dev/fdset/NNN" as vhostdev.  so we don't need a special fd parameter here.


Right, I forgot the fdset tricks.


Thanks



> Thanks
> Cindy
>
>

Re: [PATCH 1/9] vhost-vdpa: remove the default devname

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> The code doesn't have a default vhostdev, so remove the default
> description in net.json.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  qapi/net.json | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/qapi/net.json b/qapi/net.json
> index ddb113e5e5..a2a94fad3e 100644
> --- a/qapi/net.json
> +++ b/qapi/net.json
> @@ -438,7 +438,6 @@
>  # specifications with a vendor specific control path.
>  #
>  # @vhostdev: path of vhost-vdpa device
> -#            (default:'/dev/vhost-vdpa-0')
>  #
>  # @queues: number of queues to be created for multiqueue vhost-vdpa
>  #          (default: 1)
> 

If you remove the default, you must also set the parameter as not
optional (remove the star in front of its declaration):

diff --git a/qapi/net.json b/qapi/net.json
index ddb113e5e5a8..012830ca1a27 100644
--- a/qapi/net.json
+++ b/qapi/net.json
@@ -447,7 +447,7 @@
 ##
 { 'struct': 'NetdevVhostVDPAOptions',
   'data': {
-    '*vhostdev':     'str',
+    'vhostdev':     'str',
     '*queues':       'int' } }

 ##


And then you'll have:

$ ./qemu-system-x86_64  -netdev vhost-vdpa,id=hostnet1
qemu-system-x86_64: Parameter 'vhostdev' is missing

And PATCH 2 becomes useless.

Thanks,
Laurent

Re: [PATCH 2/9] vhost-vdpa: mandate vhostdev

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> vhost-dev is mandatory for vhost-vdpa to be initialized otherwise
> net_vhost_vdpa_init will pass an uninitialized pointer to qemu_open()
> which will lead a SIGSEV:
> 
> #0  0x0000555555c3a640 in strstart (str=str@entry=0x0, val=val@entry=0x555555dbcb65 "/dev/fdset/", ptr=ptr@entry=0x7fffffffdfb8) at ../util/cutils.c:77
> #1  0x0000555555c572a7 in qemu_open (name=name@entry=0x0, flags=flags@entry=2) at ../util/osdep.c:294
> #2  0x000055555584314a in net_vhost_vdpa_init (device=0x555555c81baa "vhost-vdpa", vhostdev=0x0, name=0x555556396600 "hn0", peer=0x0) at ../net/vhost-vdpa.c:187
> #3  0x000055555584314a in net_init_vhost_vdpa (netdev=<optimized out>, name=0x555556396600 "hn0", peer=0x0, errp=<optimized out>) at ../net/vhost-vdpa.c:227
> #4  0x000055555587e8c9 in net_client_init1 (netdev=0x555556398790, is_netdev=is_netdev@entry=true, errp=errp@entry=0x7fffffffe290) at ../net/net.c:1008
> #5  0x000055555587ecc7 in net_client_init (opts=0x555556192ff0, is_netdev=<optimized out>, errp=0x7fffffffe290) at ../net/net.c:1113
> #6  0x0000555555c33082 in qemu_opts_foreach
>     (list=<optimized out>, func=func@entry=0x55555587ed50 <net_init_netdev>, opaque=opaque@entry=0x0, errp=errp@entry=0x7fffffffe290) at ../util/qemu-option.c:1172
> #7  0x0000555555880057 in net_init_clients (errp=errp@entry=0x7fffffffe290) at ../net/net.c:1494
> #8  0x0000555555a7f8e9 in qemu_init (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../softmmu/vl.c:4250
> #9  0x00005555557f26cd in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at ../softmmu/main.c:49
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index bc0e0d2d35..b7221beaa1 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -206,7 +206,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
>      }
>      if (strcmp(netdev, name) == 0 &&
>          !g_str_has_prefix(driver, "virtio-net-")) {
> -        error_setg(errp, "vhost-vdpa requires frontend driver virtio-net-*");
> +        error_setg(errp, "Vhost-vdpa requires frontend driver virtio-net-*");
>          return -1;
>      }
>      return 0;
> @@ -224,5 +224,9 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                            (char *)name, errp)) {
>          return -1;
>      }
> +    if (!opts->has_vhostdev) {
> +        error_setg(errp, "vhost-vdpa requires vhostdev to be set");
> +        return -1;
> +    }

Useless if you remove the '*' in qapi/net.json.

Thanks,
Laurent

Re: [PATCH 4/9] vhost-vdpa: refine info string

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> We use "vhost-vdpa" as nic info string which is not useful, let's add
> the vhostdev path.
> 
> before:
> info network
> v0: index=0,type=vhost-vdpa,vhost-vdpa
> 
> after:
> info network
> v0: index=0,type=vhost-vdpa,vhostdev=/dev/vhost-vdpa-0
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index c4568b885e..397d4d3082 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -181,7 +181,7 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
>      int ret = 0;
>      assert(name);
>      nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
> -    snprintf(nc->info_str, sizeof(nc->info_str), TYPE_VHOST_VDPA);
> +    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
>      nc->queue_index = 0;
>      s = DO_UPCAST(VhostVDPAState, nc, nc);
>      vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 5/9] vhost-vdpa: remove the unnecessary initialization

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 397d4d3082..bcbf49d55b 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -175,10 +175,10 @@ static NetClientInfo net_vhost_vdpa_info = {
>  static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
>                                 const char *name, const char *vhostdev)
>  {
> -    NetClientState *nc = NULL;
> +    NetClientState *nc;
>      VhostVDPAState *s;
> -    int vdpa_device_fd = -1;
> -    int ret = 0;
> +    int vdpa_device_fd;
> +    int ret;
>      assert(name);
>      nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
>      snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 6/9] vhost-vdpa: remove the unnecessary queue index assignment

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> The nc is allocated through g_malloc0(), so no need to assign its
> queue_index to zero.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index bcbf49d55b..1d3ac89135 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -182,7 +182,6 @@ static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
>      assert(name);
>      nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
>      snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
> -    nc->queue_index = 0;
>      s = DO_UPCAST(VhostVDPAState, nc, nc);
>      vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
>      if (vdpa_device_fd == -1) {
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 7/9] vhost-vdpa: squash net_vhost_vdpa_init() into net_init_vhost_vdpa()

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> This patch squashes net_vhost_vdpa_init() into
> net_init_vhost_vdpa(). This will simplify adding pre open file
> descriptor support.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 1d3ac89135..f5cc4e8326 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -172,27 +172,6 @@ static NetClientInfo net_vhost_vdpa_info = {
>          .has_ufo = vhost_vdpa_has_ufo,
>  };
>  
> -static int net_vhost_vdpa_init(NetClientState *peer, const char *device,
> -                               const char *name, const char *vhostdev)
> -{
> -    NetClientState *nc;
> -    VhostVDPAState *s;
> -    int vdpa_device_fd;
> -    int ret;
> -    assert(name);
> -    nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, device, name);
> -    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", vhostdev);
> -    s = DO_UPCAST(VhostVDPAState, nc, nc);
> -    vdpa_device_fd = qemu_open(vhostdev, O_RDWR);
> -    if (vdpa_device_fd == -1) {
> -        return -errno;
> -    }
> -    s->vhost_vdpa.device_fd = vdpa_device_fd;
> -    ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
> -
> -    return ret;
> -}
> -
>  static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
>  {
>      const char *name = opaque;
> @@ -215,6 +194,10 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>                          NetClientState *peer, Error **errp)
>  {
>      const NetdevVhostVDPAOptions *opts;
> +    NetClientState *nc;
> +    VhostVDPAState *s;
> +    int vdpa_device_fd;
> +    int ret;
>  
>      assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>      opts = &netdev->u.vhost_vdpa;
> @@ -227,5 +210,19 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,
>          error_setg(errp, "vhost-vdpa requires vhostdev to be set");
>          return -1;
>      }
> -    return net_vhost_vdpa_init(peer, TYPE_VHOST_VDPA, name, opts->vhostdev);
> +
> +    assert(name);
> +
> +    nc = qemu_new_net_client(&net_vhost_vdpa_info, peer, TYPE_VHOST_VDPA, name);
> +    snprintf(nc->info_str, sizeof(nc->info_str), "vhostdev=%s", opts->vhostdev);
> +
> +    s = DO_UPCAST(VhostVDPAState, nc, nc);
> +    vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
> +    if (vdpa_device_fd == -1) {
> +        return -errno;
> +    }
> +    s->vhost_vdpa.device_fd = vdpa_device_fd;
> +    ret = vhost_vdpa_add(nc, (void *)&s->vhost_vdpa);
> +
> +    return ret;

You can avoid to declare and use "ret" by doing directly

  "return vhost_vdpa_add()"

Anyway, the code move is correct:

Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Re: [PATCH 9/9] vhost-vdpa: allow pre-opend file descriptor

[Eric Blake]

On 8/31/20 6:16 AM, Cindy Lu wrote:
> On Mon, 31 Aug 2020 at 16:30, Jason Wang <jasowang@redhat.com> wrote:
>>
>> This patch allows to initialize vhost-vdpa network backend with pre
>> opened vhost-vdpa file descriptor. This is useful for running
>> unprivileged qemu through libvirt.
>>
>> Cc: Eric Blake <eblake@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---

>> +++ b/qapi/net.json
>> @@ -442,12 +442,15 @@
>>   # @queues: number of queues to be created for multiqueue vhost-vdpa
>>   #          (default: 1)
>>   #
>> +# @fd: file descriptor of an already opened vhost-vdpa (since 5.2)
>> +#
>>   # Since: 5.1
>>   ##
>>   { 'struct': 'NetdevVhostVDPAOptions',
>>     'data': {
>>       '*vhostdev':     'str',
>> -    '*queues':       'int' } }
>> +    '*queues':       'int',
>> +    '*fd':           'str' } }
>>
>>   ##
>>   # @NetClientDriver:
>> --
>> 2.20.1
>>
> I think the latest  code supported this part.
> you can pass a pre open file descriptor to it via the add-fd QMP
> command to /dev/fdset/NNN, and then pass the string
> "/dev/fdset/NNN" as vhostdev.  so we don't need a special fd parameter here.

Correct - the 'vhostdev' parameter + magic filename is all the more that 
is needed to access a pre-opened fd, so no 'fd' parameter should be added.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 8/9] vhost-vdpa: add accurate error string when fail to open vhost vDPA device

[Laurent Vivier]

On 31/08/2020 10:27, Jason Wang wrote:
> This patch adds more accurate error string when fail to open vhost
> vDPA device.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  net/vhost-vdpa.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index f5cc4e8326..9a6f0b63d3 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -219,6 +219,7 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name,

I think you can also remove the "assert(name)", qemu_net_client_setup()
in qemu_new_net_client() uses a default value if NULL.

>      s = DO_UPCAST(VhostVDPAState, nc, nc);
>      vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR);
>      if (vdpa_device_fd == -1) {
> +        error_setg(errp, "Fail to open vhost-vdpa device %s", opts->vhostdev);
>          return -errno;

you can "return -1" now: no one checks for the exact returned value, all
the other init functions from net_client_init_fun[] return -1 or 0, and
the errno is now in the error string.

>      }
>      s->vhost_vdpa.device_fd = vdpa_device_fd;
> 

Thanks,
Laurent