On Wed, May 03, 2023 at 11:15:56AM +0200, Stefano Garzarella wrote: > On Tue, May 02, 2023 at 03:02:32PM -0400, Stefan Hajnoczi wrote: > > On Tue, May 02, 2023 at 04:50:50PM +0200, Stefano Garzarella wrote: > > > The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the new > > > 'fd' property. Let's expose this to the user, so the management layer > > > can pass the file descriptor of an already opened vhost-vdpa character > > > device. This is useful especially when the device can only be accessed > > > with certain privileges. > > > > > > Signed-off-by: Stefano Garzarella <[email protected]> > > > --- > > > > > > Notes: > > > As an alternative we could support passing `/dev/fdset/N` via 'path', > > > always opening the path with qemu_open() and passing the fd to the > > > libblkio driver. > > > I preferred to add a new parameter though, because the code is > > > simpler without changing how path works (alternatively we should check > > > first if fd is supported by the driver or not). > > > > > > What do you think? > > > > I think the approach in this patch is fine. > > > > > > > > Thanks, > > > Stefano > > > > > > qapi/block-core.json | 6 +++++- > > > block/blkio.c | 45 +++++++++++++++++++++++++++++++++++++++++++- > > > 2 files changed, 49 insertions(+), 2 deletions(-) > > > > > > diff --git a/qapi/block-core.json b/qapi/block-core.json > > > index b57978957f..9f70777d49 100644 > > > --- a/qapi/block-core.json > > > +++ b/qapi/block-core.json > > > @@ -3841,10 +3841,14 @@ > > > # > > > # @path: path to the vhost-vdpa character device. > > > # > > > +# @fd: file descriptor of an already opened vhost-vdpa character device. > > > +# (Since 8.1) > > > +# > > > # Since: 7.2 > > > ## > > > { 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa', > > > - 'data': { 'path': 'str' }, > > > + 'data': { '*path': 'str', > > > + '*fd': 'str' }, > > > 'if': 'CONFIG_BLKIO' } > > > > > > ## > > > diff --git a/block/blkio.c b/block/blkio.c > > > index 0cdc99a729..98394b5745 100644 > > > --- a/block/blkio.c > > > +++ b/block/blkio.c > > > @@ -694,6 +694,49 @@ static int > > > blkio_virtio_blk_common_open(BlockDriverState *bs, > > > return 0; > > > } > > > > > > +static int blkio_virtio_blk_vhost_vdpa_open(BlockDriverState *bs, > > > + QDict *options, int flags, Error **errp) > > > +{ > > > + const char *path = qdict_get_try_str(options, "path"); > > > + const char *fd_str = qdict_get_try_str(options, "fd"); > > > + BDRVBlkioState *s = bs->opaque; > > > + int ret; > > > + > > > + if (path && fd_str) { > > > + error_setg(errp, "'path' and 'fd' options are mutually > > > exclusive"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!path && !fd_str) { > > > + error_setg(errp, "none of 'path' or 'fd' options was specified"); > > > + return -EINVAL; > > > + } > > > + > > > + if (path) { > > > + ret = blkio_set_str(s->blkio, "path", path); > > > + qdict_del(options, "path"); > > > + if (ret < 0) { > > > + error_setg_errno(errp, -ret, "failed to set path: %s", > > > + blkio_get_error_msg()); > > > + return ret; > > > + } > > > + } else { > > > + ret = blkio_set_str(s->blkio, "fd", fd_str); > > > > monitor_fd_param() is used by vhost-net, vhost-vsock, vhost-scsi, etc. > > > > I think QEMU should parse the fd string and resolve it to a file > > descriptor so the fd passing syntax matches the other vhost devices. > > Okay, but I have a linker issue if I use monitor_fd_param(). > IIUC because blkio is built as a module, so what about adding > qemu_fd_param() in libqemuutil?
Modules can access any extern function in QEMU so I don't think there is
a fundamental limitation there.
Maybe it's related to the dependencies between the blkio module and
monitor/ code. monitor_get_fd_param() is in softmmu_ss, which block
drivers don't directly depend on AFAICT.
>
> I mean something like this:
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 9eff0be95b..87360c983a 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -568,6 +568,7 @@ int qemu_lock_fd(int fd, int64_t start, int64_t len, bool
> exclusive);
> int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
> bool qemu_has_ofd_lock(void);
> +int qemu_fd_param(const char *fdname, Error **errp);
> #endif
>
> #if defined(__HAIKU__) && defined(__i386__)
> diff --git a/util/osdep.c b/util/osdep.c
> index e996c4744a..ed0832810b 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -234,6 +234,11 @@ bool qemu_has_ofd_lock(void)
> #endif
> }
>
> +int qemu_fd_param(const char *fdname, Error **errp)
> +{
> + return monitor_fd_param(monitor_cur(), fdname, errp);
> +}
I'm not sure. If it works with modules enabled/disabled,
qemu-io/qemu-img/etc, and qemu-user then I guess this solution is okay.
Sorry I don't know the answer!
Stefan
signature.asc
Description: PGP signature
