Message ID | 20221028054835.29674-6-jasowang@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/26] virtio-net: fix bottom-half packet TX on asynchronous completion | expand |
On Fri, 28 Oct 2022 at 06:55, Jason Wang <jasowang@redhat.com> wrote: > > From: Si-Wei Liu <si-wei.liu@oracle.com> > > Similar to other vhost backends, vhostfd can be passed to vhost-vdpa > backend as another parameter to instantiate vhost-vdpa net client. > This would benefit the use case where only open file descriptors, as > opposed to raw vhost-vdpa device paths, are accessible from the QEMU > process. > > (qemu) netdev_add type=vhost-vdpa,vhostfd=61,id=vhost-vdpa1 > > @@ -634,14 +634,29 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, > > assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); > opts = &netdev->u.vhost_vdpa; > - if (!opts->vhostdev) { > - error_setg(errp, "vdpa character device not specified with vhostdev"); > + if (!opts->has_vhostdev && !opts->has_vhostfd) { > + error_setg(errp, > + "vhost-vdpa: neither vhostdev= nor vhostfd= was specified"); > return -1; > } > > - vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR, errp); > - if (vdpa_device_fd == -1) { > - return -errno; > + if (opts->has_vhostdev && opts->has_vhostfd) { > + error_setg(errp, > + "vhost-vdpa: vhostdev= and vhostfd= are mutually exclusive"); > + return -1; > + } > + > + if (opts->has_vhostdev) { > + vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR, errp); > + if (vdpa_device_fd == -1) { > + return -errno; > + } > + } else if (opts->has_vhostfd) { > + vdpa_device_fd = monitor_fd_param(monitor_cur(), opts->vhostfd, errp); > + if (vdpa_device_fd == -1) { > + error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: "); > + return -1; > + } > } Hi; this doesn't compile with clang: ../../net/vhost-vdpa.c:654:16: error: variable 'vdpa_device_fd' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized] } else if (opts->has_vhostfd) { ^~~~~~~~~~~~~~~~~ ../../net/vhost-vdpa.c:662:33: note: uninitialized use occurs here r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp); ^~~~~~~~~~~~~~ ../../net/vhost-vdpa.c:654:12: note: remove the 'if' if its condition is always true } else if (opts->has_vhostfd) { ^~~~~~~~~~~~~~~~~~~~~~~ ../../net/vhost-vdpa.c:629:23: note: initialize the variable 'vdpa_device_fd' to silence this warning int vdpa_device_fd; ^ = 0 1 error generated. (clang version 10.0.0-4ubuntu1). It's a false positive -- the compiler doesn't manage to figure out that the error checks further up mean that there's no code path where vdpa_device_fd isn't initialized. Put another way, the problem is that we check "if (opts->has_vhostfd)" when in fact that condition must always be true. We could fix this by for instance: diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 854ebd61ae6..b1572ea00bc 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -634,11 +634,6 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); opts = &netdev->u.vhost_vdpa; - if (!opts->has_vhostdev && !opts->has_vhostfd) { - error_setg(errp, - "vhost-vdpa: neither vhostdev= nor vhostfd= was specified"); - return -1; - } if (opts->has_vhostdev && opts->has_vhostfd) { error_setg(errp, @@ -657,6 +652,10 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: "); return -1; } + } else { + error_setg(errp, + "vhost-vdpa: neither vhostdev= nor vhostfd= was specified"); + return -1; } r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp); or alternatively by diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 854ebd61ae6..e370ecb8ebd 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -651,7 +651,8 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, if (vdpa_device_fd == -1) { return -errno; } - } else if (opts->has_vhostfd) { + } else { + /* has_vhostfd */ vdpa_device_fd = monitor_fd_param(monitor_cur(), opts->vhostfd, errp); if (vdpa_device_fd == -1) { error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: "); I think I prefer the second of these. (I'll post a patch properly in a moment I guess.) -- PMM
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 6d64000..9c1b25d 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -634,14 +634,29 @@ int net_init_vhost_vdpa(const Netdev *netdev, const char *name, assert(netdev->type == NET_CLIENT_DRIVER_VHOST_VDPA); opts = &netdev->u.vhost_vdpa; - if (!opts->vhostdev) { - error_setg(errp, "vdpa character device not specified with vhostdev"); + if (!opts->has_vhostdev && !opts->has_vhostfd) { + error_setg(errp, + "vhost-vdpa: neither vhostdev= nor vhostfd= was specified"); return -1; } - vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR, errp); - if (vdpa_device_fd == -1) { - return -errno; + if (opts->has_vhostdev && opts->has_vhostfd) { + error_setg(errp, + "vhost-vdpa: vhostdev= and vhostfd= are mutually exclusive"); + return -1; + } + + if (opts->has_vhostdev) { + vdpa_device_fd = qemu_open(opts->vhostdev, O_RDWR, errp); + if (vdpa_device_fd == -1) { + return -errno; + } + } else if (opts->has_vhostfd) { + vdpa_device_fd = monitor_fd_param(monitor_cur(), opts->vhostfd, errp); + if (vdpa_device_fd == -1) { + error_prepend(errp, "vhost-vdpa: unable to parse vhostfd: "); + return -1; + } } r = vhost_vdpa_get_features(vdpa_device_fd, &features, errp); diff --git a/qapi/net.json b/qapi/net.json index dd088c0..926ecc8 100644 --- a/qapi/net.json +++ b/qapi/net.json @@ -442,6 +442,8 @@ # @vhostdev: path of vhost-vdpa device # (default:'/dev/vhost-vdpa-0') # +# @vhostfd: file descriptor of an already opened vhost vdpa device +# # @queues: number of queues to be created for multiqueue vhost-vdpa # (default: 1) # @@ -456,6 +458,7 @@ { 'struct': 'NetdevVhostVDPAOptions', 'data': { '*vhostdev': 'str', + '*vhostfd': 'str', '*queues': 'int', '*x-svq': {'type': 'bool', 'features' : [ 'unstable'] } } } diff --git a/qemu-options.hx b/qemu-options.hx index eb38e5d..876a70a 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -2790,8 +2790,10 @@ DEF("netdev", HAS_ARG, QEMU_OPTION_netdev, " configure a vhost-user network, backed by a chardev 'dev'\n" #endif #ifdef __linux__ - "-netdev vhost-vdpa,id=str,vhostdev=/path/to/dev\n" + "-netdev vhost-vdpa,id=str[,vhostdev=/path/to/dev][,vhostfd=h]\n" " configure a vhost-vdpa network,Establish a vhost-vdpa netdev\n" + " use 'vhostdev=/path/to/dev' to open a vhost vdpa device\n" + " use 'vhostfd=h' to connect to an already opened vhost vdpa device\n" #endif #ifdef CONFIG_VMNET "-netdev vmnet-host,id=str[,isolated=on|off][,net-uuid=uuid]\n" @@ -3296,7 +3298,7 @@ SRST -netdev type=vhost-user,id=net0,chardev=chr0 \ -device virtio-net-pci,netdev=net0 -``-netdev vhost-vdpa,vhostdev=/path/to/dev`` +``-netdev vhost-vdpa[,vhostdev=/path/to/dev][,vhostfd=h]`` Establish a vhost-vdpa netdev. vDPA device is a device that uses a datapath which complies with