qapi/block-core.json | 6 +++++- block/blkio.c | 45 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-)
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 <sgarzare@redhat.com>
---
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?
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);
+ qdict_del(options, "fd");
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "failed to set fd: %s",
+ blkio_get_error_msg());
+ return ret;
+ }
+ }
+
+ if (!(flags & BDRV_O_NOCACHE)) {
+ error_setg(errp, "cache.direct=off is not supported");
+ return -EINVAL;
+ }
+ return 0;
+}
+
static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -717,7 +760,7 @@ static int blkio_file_open(BlockDriverState *bs, QDict *options, int flags,
} else if (strcmp(blkio_driver, DRIVER_VIRTIO_BLK_VHOST_USER) == 0) {
ret = blkio_virtio_blk_common_open(bs, options, flags, errp);
} else if (strcmp(blkio_driver, DRIVER_VIRTIO_BLK_VHOST_VDPA) == 0) {
- ret = blkio_virtio_blk_common_open(bs, options, flags, errp);
+ ret = blkio_virtio_blk_vhost_vdpa_open(bs, options, flags, errp);
} else {
g_assert_not_reached();
}
--
2.40.1On 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 <sgarzare@redhat.com>
> ---
>
> 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.
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 <sgarzare@redhat.com>
>> ---
>>
>> 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?
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);
+}
+
static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
{
int ret;
Thanks,
Stefano
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 <sgarzare@redhat.com>
> > > ---
> > >
> > > 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
On Wed, May 3, 2023 at 5:57 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> 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 <sgarzare@redhat.com>
> > > > ---
> > > >
> > > > 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.
Yep, I think this is the case.
>
> >
> > 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.
It seems to work, and I think it's easier than introducing the
dependency between the blkio module and monitor/ code.
Something similar has already been done with monitor_get_fd().
Anyway I'll send v2 by adding that patch and we can discuss it there.
Thanks for the advice,
Stefano
On Thu, May 4, 2023 at 9:38 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, May 3, 2023 at 5:57 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > 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 <sgarzare@redhat.com>
> > > > > ---
> > > > >
> > > > > 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.
>
> Yep, I think this is the case.
>
> >
> > >
> > > 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.
>
> It seems to work, and I think it's easier than introducing the
> dependency between the blkio module and monitor/ code.
> Something similar has already been done with monitor_get_fd().
> Anyway I'll send v2 by adding that patch and we can discuss it there.
Aaargs, the linker issue was only in some tests. Adding just
monitor_fd_param() in the stubs fixed the problem.
Thanks,
Stefano
© 2016 - 2024 Red Hat, Inc.