[PATCH v3 0/1] block/blkio: support 'fd' option for virtio-blk-vhost-vdpa driver

[PATCH v3 0/1] block/blkio: support 'fd' option for virtio-blk-vhost-vdpa driver

[Stefano Garzarella]

v3:
- use qemu_open() on `path` to simplify libvirt code [Jonathon]
- remove patch 01 since we are not using monitor_fd_param() anymore

v2: https://lore.kernel.org/qemu-devel/20230504092843.62493-1-sgarzare@redhat.com/
- added patch 01 to use monitor_fd_param() in the blkio module
- use monitor_fd_param() to parse the fd like vhost devices [Stefan]

v1: https://lore.kernel.org/qemu-devel/20230502145050.224615-1-sgarzare@redhat.com/

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.

Stefano Garzarella (1):
  block/blkio: use qemu_open() to support fd passing for virtio-blk

 block/blkio.c | 53 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 9 deletions(-)

-- 
2.40.1

[PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefano Garzarella]

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.

If the libblkio virtio-blk driver supports fd passing, let's always
use qemu_open() to open the `path`, so we can handle fd passing
from the management layer through the "/dev/fdset/N" special path.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---

Notes:
    v3:
    - use qemu_open() on `path` to simplify libvirt code [Jonathon]

 block/blkio.c | 53 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/block/blkio.c b/block/blkio.c
index 0cdc99a729..6a6f20f923 100644
--- a/block/blkio.c
+++ b/block/blkio.c
@@ -672,25 +672,60 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,
 {
     const char *path = qdict_get_try_str(options, "path");
     BDRVBlkioState *s = bs->opaque;
-    int ret;
+    bool fd_supported = false;
+    int fd, ret;
 
     if (!path) {
         error_setg(errp, "missing 'path' option");
         return -EINVAL;
     }
 
-    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;
-    }
-
     if (!(flags & BDRV_O_NOCACHE)) {
         error_setg(errp, "cache.direct=off is not supported");
         return -EINVAL;
     }
+
+    if (blkio_get_int(s->blkio, "fd", &fd) == 0) {
+        fd_supported = true;
+    }
+
+    /*
+     * If the libblkio driver supports fd passing, let's always use qemu_open()
+     * to open the `path`, so we can handle fd passing from the management
+     * layer through the "/dev/fdset/N" special path.
+     */
+    if (fd_supported) {
+        int open_flags;
+
+        if (flags & BDRV_O_RDWR) {
+            open_flags = O_RDWR;
+        } else {
+            open_flags = O_RDONLY;
+        }
+
+        fd = qemu_open(path, open_flags, errp);
+        if (fd < 0) {
+            return -EINVAL;
+        }
+
+        ret = blkio_set_int(s->blkio, "fd", fd);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "failed to set fd: %s",
+                             blkio_get_error_msg());
+            qemu_close(fd);
+            return ret;
+        }
+    } else {
+        ret = blkio_set_str(s->blkio, "path", path);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "failed to set path: %s",
+                             blkio_get_error_msg());
+            return ret;
+        }
+    }
+
+    qdict_del(options, "path");
+
     return 0;
 }
 
-- 
2.40.1

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Jonathon Jongsma]

On 5/11/23 4:15 AM, 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.
> 
> If the libblkio virtio-blk driver supports fd passing, let's always
> use qemu_open() to open the `path`, so we can handle fd passing
> from the management layer through the "/dev/fdset/N" special path.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> 
> Notes:
>      v3:
>      - use qemu_open() on `path` to simplify libvirt code [Jonathon]


Thanks

The one drawback now is that it doesn't seem possible for libvirt to 
introspect whether or not qemu supports passing an fd to the driver or 
not. When I was writing my initial patch (before I realized that it was 
missing fd-passing), I just checked for the existence of the 
virtio-blk-vhost-vdpa device. But we actually need to know both that 
this device exists and supports fd passing. As far as I can tell, 
versions 7.2.0 and 8.0.0 include this device but won't accept fds.

Jonathon


> 
>   block/blkio.c | 53 ++++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/block/blkio.c b/block/blkio.c
> index 0cdc99a729..6a6f20f923 100644
> --- a/block/blkio.c
> +++ b/block/blkio.c
> @@ -672,25 +672,60 @@ static int blkio_virtio_blk_common_open(BlockDriverState *bs,
>   {
>       const char *path = qdict_get_try_str(options, "path");
>       BDRVBlkioState *s = bs->opaque;
> -    int ret;
> +    bool fd_supported = false;
> +    int fd, ret;
>   
>       if (!path) {
>           error_setg(errp, "missing 'path' option");
>           return -EINVAL;
>       }
>   
> -    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;
> -    }
> -
>       if (!(flags & BDRV_O_NOCACHE)) {
>           error_setg(errp, "cache.direct=off is not supported");
>           return -EINVAL;
>       }
> +
> +    if (blkio_get_int(s->blkio, "fd", &fd) == 0) {
> +        fd_supported = true;
> +    }
> +
> +    /*
> +     * If the libblkio driver supports fd passing, let's always use qemu_open()
> +     * to open the `path`, so we can handle fd passing from the management
> +     * layer through the "/dev/fdset/N" special path.
> +     */
> +    if (fd_supported) {
> +        int open_flags;
> +
> +        if (flags & BDRV_O_RDWR) {
> +            open_flags = O_RDWR;
> +        } else {
> +            open_flags = O_RDONLY;
> +        }
> +
> +        fd = qemu_open(path, open_flags, errp);
> +        if (fd < 0) {
> +            return -EINVAL;
> +        }
> +
> +        ret = blkio_set_int(s->blkio, "fd", fd);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "failed to set fd: %s",
> +                             blkio_get_error_msg());
> +            qemu_close(fd);
> +            return ret;
> +        }
> +    } else {
> +        ret = blkio_set_str(s->blkio, "path", path);
> +        if (ret < 0) {
> +            error_setg_errno(errp, -ret, "failed to set path: %s",
> +                             blkio_get_error_msg());
> +            return ret;
> +        }
> +    }
> +
> +    qdict_del(options, "path");
> +
>       return 0;
>   }
>   

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefano Garzarella]

On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
>On 5/11/23 4:15 AM, 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.
>>
>>If the libblkio virtio-blk driver supports fd passing, let's always
>>use qemu_open() to open the `path`, so we can handle fd passing
>>from the management layer through the "/dev/fdset/N" special path.
>>
>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>---
>>
>>Notes:
>>     v3:
>>     - use qemu_open() on `path` to simplify libvirt code [Jonathon]
>
>
>Thanks
>
>The one drawback now is that it doesn't seem possible for libvirt to 
>introspect whether or not qemu supports passing an fd to the driver or 
>not.

Yep, this was because the libblkio library did not support this new way.

>When I was writing my initial patch (before I realized that it was 
>missing fd-passing), I just checked for the existence of the 
>virtio-blk-vhost-vdpa device. But we actually need to know both that 
>this device exists and supports fd passing.

Yep, this was one of the advantages of using the new `fd` parameter.
Can't libvirt handle the later failure?

>As far as I can tell, versions 7.2.0 and 8.0.0 include this device but 
>won't accept fds.

Right.

How do you suggest to proceed?

Thanks,
Stefano

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Jonathon Jongsma]

On 5/15/23 5:10 AM, Stefano Garzarella wrote:
> On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
>> On 5/11/23 4:15 AM, 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.
>>>
>>> If the libblkio virtio-blk driver supports fd passing, let's always
>>> use qemu_open() to open the `path`, so we can handle fd passing
>>> from the management layer through the "/dev/fdset/N" special path.
>>>
>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     v3:
>>>     - use qemu_open() on `path` to simplify libvirt code [Jonathon]
>>
>>
>> Thanks
>>
>> The one drawback now is that it doesn't seem possible for libvirt to 
>> introspect whether or not qemu supports passing an fd to the driver or 
>> not.
> 
> Yep, this was because the libblkio library did not support this new way.
> 
>> When I was writing my initial patch (before I realized that it was 
>> missing fd-passing), I just checked for the existence of the 
>> virtio-blk-vhost-vdpa device. But we actually need to know both that 
>> this device exists and supports fd passing.
> 
> Yep, this was one of the advantages of using the new `fd` parameter.
> Can't libvirt handle the later failure?

Not very well. libvirt tries to provide useful errors to the user. So 
for example if the qemu executable doesn't support a device, we would 
want to provide an error indicating that the device is not supported 
rather than a possibly-inscrutable qemu error.

For example, in this scenario, we would want an error such as:

error: unsupported configuration: vhostvdpa disk is not supported with 
this QEMU binary

Instead of:

error: internal error: qemu unexpectedly closed the monitor: 
2023-05-16T15:17:36.666129Z qemu-system-x86_64: -blockdev 
{"driver":"virtio-blk-vhost-vdpa","path":"/dev/fdset/0","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}: 
blkio_connect failed: Failed to connect to vDPA device: Input/output error

And we can only do that if we can determine that the binary has the 
proper support for fds.

> 
>> As far as I can tell, versions 7.2.0 and 8.0.0 include this device but 
>> won't accept fds.
> 
> Right.
> 
> How do you suggest to proceed?

I need some way to determine that the particular qemu binary can accept 
a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of 
methods to determine capabilities for a given qemu binary, including 
querying the qmp schema, commands, object types, specific device/object 
properties, etc. For example, right now I can determine (via querying 
the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the 
blockdev-add command by querying the qmp schema. I need something more 
than that but I'm not sure how to do it without introducing a separate 
'fd' parameter. Any ideas?

Thanks,
Jonathon

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefano Garzarella]

CCing Markus for some advice.

On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
>On 5/15/23 5:10 AM, Stefano Garzarella wrote:
>>On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
>>>On 5/11/23 4:15 AM, 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.
>>>>
>>>>If the libblkio virtio-blk driver supports fd passing, let's always
>>>>use qemu_open() to open the `path`, so we can handle fd passing
>>>>from the management layer through the "/dev/fdset/N" special path.
>>>>
>>>>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>>>---
>>>>
>>>>Notes:
>>>>    v3:
>>>>    - use qemu_open() on `path` to simplify libvirt code [Jonathon]
>>>
>>>
>>>Thanks
>>>
>>>The one drawback now is that it doesn't seem possible for libvirt 
>>>to introspect whether or not qemu supports passing an fd to the 
>>>driver or not.
>>
>>Yep, this was because the libblkio library did not support this new way.
>>
>>>When I was writing my initial patch (before I realized that it was 
>>>missing fd-passing), I just checked for the existence of the 
>>>virtio-blk-vhost-vdpa device. But we actually need to know both 
>>>that this device exists and supports fd passing.
>>
>>Yep, this was one of the advantages of using the new `fd` parameter.
>>Can't libvirt handle the later failure?
>
>Not very well. libvirt tries to provide useful errors to the user. So 
>for example if the qemu executable doesn't support a device, we would 
>want to provide an error indicating that the device is not supported 
>rather than a possibly-inscrutable qemu error.
>
>For example, in this scenario, we would want an error such as:
>
>error: unsupported configuration: vhostvdpa disk is not supported with 
>this QEMU binary
>
>Instead of:
>
>error: internal error: qemu unexpectedly closed the monitor: 
>2023-05-16T15:17:36.666129Z qemu-system-x86_64: -blockdev {"driver":"virtio-blk-vhost-vdpa","path":"/dev/fdset/0","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}: 
>blkio_connect failed: Failed to connect to vDPA device: Input/output 
>error
>
>And we can only do that if we can determine that the binary has the 
>proper support for fds.

I see the problem, thanks for explaining this!

>
>>
>>>As far as I can tell, versions 7.2.0 and 8.0.0 include this device 
>>>but won't accept fds.
>>
>>Right.
>>
>>How do you suggest to proceed?
>
>I need some way to determine that the particular qemu binary can 
>accept a /dev/fdset/ path for vdpa block devices. libvirt uses a 
>variety of methods to determine capabilities for a given qemu binary, 
>including querying the qmp schema, commands, object types, specific 
>device/object properties, etc. For example, right now I can determine 
>(via querying the qmp schema) whether virtio-blk-vhost-vdpa is a valid 
>type for the blockdev-add command by querying the qmp schema. I need 
>something more than that but I'm not sure how to do it without 
>introducing a separate 'fd' parameter. Any ideas?

The only thing I can think of is to make a mix between v2 and v3. I mean 
add both the new `fd` parameter, and support qemu_open() on `path`.

That way libvirt (or other users) can check that fd passing is supported 
and use `fd` or fdset with `path`.

Obviously I would have liked to implement only one of the two methods, 
but if this helps, maybe it makes sense to support both.

What do you think?

Thanks,
Stefano

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4429 bytes --]

On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
> CCing Markus for some advice.
> 
> On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
> > On 5/15/23 5:10 AM, Stefano Garzarella wrote:
> > > On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
> > > > On 5/11/23 4:15 AM, 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.
> > > > > 
> > > > > If the libblkio virtio-blk driver supports fd passing, let's always
> > > > > use qemu_open() to open the `path`, so we can handle fd passing
> > > > > from the management layer through the "/dev/fdset/N" special path.
> > > > > 
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > ---
> > > > > 
> > > > > Notes:
> > > > >     v3:
> > > > >     - use qemu_open() on `path` to simplify libvirt code [Jonathon]
> > > > 
> > > > 
> > > > Thanks
> > > > 
> > > > The one drawback now is that it doesn't seem possible for
> > > > libvirt to introspect whether or not qemu supports passing an fd
> > > > to the driver or not.
> > > 
> > > Yep, this was because the libblkio library did not support this new way.
> > > 
> > > > When I was writing my initial patch (before I realized that it
> > > > was missing fd-passing), I just checked for the existence of the
> > > > virtio-blk-vhost-vdpa device. But we actually need to know both
> > > > that this device exists and supports fd passing.
> > > 
> > > Yep, this was one of the advantages of using the new `fd` parameter.
> > > Can't libvirt handle the later failure?
> > 
> > Not very well. libvirt tries to provide useful errors to the user. So
> > for example if the qemu executable doesn't support a device, we would
> > want to provide an error indicating that the device is not supported
> > rather than a possibly-inscrutable qemu error.
> > 
> > For example, in this scenario, we would want an error such as:
> > 
> > error: unsupported configuration: vhostvdpa disk is not supported with
> > this QEMU binary
> > 
> > Instead of:
> > 
> > error: internal error: qemu unexpectedly closed the monitor:
> > 2023-05-16T15:17:36.666129Z qemu-system-x86_64: -blockdev {"driver":"virtio-blk-vhost-vdpa","path":"/dev/fdset/0","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}:
> > blkio_connect failed: Failed to connect to vDPA device: Input/output
> > error
> > 
> > And we can only do that if we can determine that the binary has the
> > proper support for fds.
> 
> I see the problem, thanks for explaining this!
> 
> > 
> > > 
> > > > As far as I can tell, versions 7.2.0 and 8.0.0 include this
> > > > device but won't accept fds.
> > > 
> > > Right.
> > > 
> > > How do you suggest to proceed?
> > 
> > I need some way to determine that the particular qemu binary can accept
> > a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
> > methods to determine capabilities for a given qemu binary, including
> > querying the qmp schema, commands, object types, specific device/object
> > properties, etc. For example, right now I can determine (via querying
> > the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
> > blockdev-add command by querying the qmp schema. I need something more
> > than that but I'm not sure how to do it without introducing a separate
> > 'fd' parameter. Any ideas?
> 
> The only thing I can think of is to make a mix between v2 and v3. I mean add
> both the new `fd` parameter, and support qemu_open() on `path`.
> 
> That way libvirt (or other users) can check that fd passing is supported and
> use `fd` or fdset with `path`.
> 
> Obviously I would have liked to implement only one of the two methods, but
> if this helps, maybe it makes sense to support both.
> 
> What do you think?

Markus: Is a preferred way to make this new path handling behavior
introspectable? I vaguely remember a way for QMP clients to query
strings that describe QMP behavior that's not otherwise
introspectable...

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefano Garzarella]

Gentle ping :-)

On Wed, May 17, 2023 at 4:30 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
> > CCing Markus for some advice.
> >
> > On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
> > > On 5/15/23 5:10 AM, Stefano Garzarella wrote:
> > > > On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
> > > > > On 5/11/23 4:15 AM, 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.
> > > > > >
> > > > > > If the libblkio virtio-blk driver supports fd passing, let's always
> > > > > > use qemu_open() to open the `path`, so we can handle fd passing
> > > > > > from the management layer through the "/dev/fdset/N" special path.
> > > > > >
> > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > ---
> > > > > >
> > > > > > Notes:
> > > > > >     v3:
> > > > > >     - use qemu_open() on `path` to simplify libvirt code [Jonathon]
> > > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > The one drawback now is that it doesn't seem possible for
> > > > > libvirt to introspect whether or not qemu supports passing an fd
> > > > > to the driver or not.
> > > >
> > > > Yep, this was because the libblkio library did not support this new way.
> > > >
> > > > > When I was writing my initial patch (before I realized that it
> > > > > was missing fd-passing), I just checked for the existence of the
> > > > > virtio-blk-vhost-vdpa device. But we actually need to know both
> > > > > that this device exists and supports fd passing.
> > > >
> > > > Yep, this was one of the advantages of using the new `fd` parameter.
> > > > Can't libvirt handle the later failure?
> > >
> > > Not very well. libvirt tries to provide useful errors to the user. So
> > > for example if the qemu executable doesn't support a device, we would
> > > want to provide an error indicating that the device is not supported
> > > rather than a possibly-inscrutable qemu error.
> > >
> > > For example, in this scenario, we would want an error such as:
> > >
> > > error: unsupported configuration: vhostvdpa disk is not supported with
> > > this QEMU binary
> > >
> > > Instead of:
> > >
> > > error: internal error: qemu unexpectedly closed the monitor:
> > > 2023-05-16T15:17:36.666129Z qemu-system-x86_64: -blockdev {"driver":"virtio-blk-vhost-vdpa","path":"/dev/fdset/0","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}:
> > > blkio_connect failed: Failed to connect to vDPA device: Input/output
> > > error
> > >
> > > And we can only do that if we can determine that the binary has the
> > > proper support for fds.
> >
> > I see the problem, thanks for explaining this!
> >
> > >
> > > >
> > > > > As far as I can tell, versions 7.2.0 and 8.0.0 include this
> > > > > device but won't accept fds.
> > > >
> > > > Right.
> > > >
> > > > How do you suggest to proceed?
> > >
> > > I need some way to determine that the particular qemu binary can accept
> > > a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
> > > methods to determine capabilities for a given qemu binary, including
> > > querying the qmp schema, commands, object types, specific device/object
> > > properties, etc. For example, right now I can determine (via querying
> > > the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
> > > blockdev-add command by querying the qmp schema. I need something more
> > > than that but I'm not sure how to do it without introducing a separate
> > > 'fd' parameter. Any ideas?
> >
> > The only thing I can think of is to make a mix between v2 and v3. I mean add
> > both the new `fd` parameter, and support qemu_open() on `path`.
> >
> > That way libvirt (or other users) can check that fd passing is supported and
> > use `fd` or fdset with `path`.
> >
> > Obviously I would have liked to implement only one of the two methods, but
> > if this helps, maybe it makes sense to support both.
> >
> > What do you think?
>
> Markus: Is a preferred way to make this new path handling behavior
> introspectable? I vaguely remember a way for QMP clients to query
> strings that describe QMP behavior that's not otherwise
> introspectable...

If there is no other way for introspection with QMP, I think adding
`fd` and supporting `qemu_open()` on `path` is the easiest.

Thanks,
Stefano

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5352 bytes --]

On Wed, May 24, 2023 at 11:05:38AM +0200, Stefano Garzarella wrote:
> Gentle ping :-)
> 
> On Wed, May 17, 2023 at 4:30 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
> > > CCing Markus for some advice.
> > >
> > > On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
> > > > On 5/15/23 5:10 AM, Stefano Garzarella wrote:
> > > > > On Thu, May 11, 2023 at 11:03:22AM -0500, Jonathon Jongsma wrote:
> > > > > > On 5/11/23 4:15 AM, 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.
> > > > > > >
> > > > > > > If the libblkio virtio-blk driver supports fd passing, let's always
> > > > > > > use qemu_open() to open the `path`, so we can handle fd passing
> > > > > > > from the management layer through the "/dev/fdset/N" special path.
> > > > > > >
> > > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Notes:
> > > > > > >     v3:
> > > > > > >     - use qemu_open() on `path` to simplify libvirt code [Jonathon]
> > > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > The one drawback now is that it doesn't seem possible for
> > > > > > libvirt to introspect whether or not qemu supports passing an fd
> > > > > > to the driver or not.
> > > > >
> > > > > Yep, this was because the libblkio library did not support this new way.
> > > > >
> > > > > > When I was writing my initial patch (before I realized that it
> > > > > > was missing fd-passing), I just checked for the existence of the
> > > > > > virtio-blk-vhost-vdpa device. But we actually need to know both
> > > > > > that this device exists and supports fd passing.
> > > > >
> > > > > Yep, this was one of the advantages of using the new `fd` parameter.
> > > > > Can't libvirt handle the later failure?
> > > >
> > > > Not very well. libvirt tries to provide useful errors to the user. So
> > > > for example if the qemu executable doesn't support a device, we would
> > > > want to provide an error indicating that the device is not supported
> > > > rather than a possibly-inscrutable qemu error.
> > > >
> > > > For example, in this scenario, we would want an error such as:
> > > >
> > > > error: unsupported configuration: vhostvdpa disk is not supported with
> > > > this QEMU binary
> > > >
> > > > Instead of:
> > > >
> > > > error: internal error: qemu unexpectedly closed the monitor:
> > > > 2023-05-16T15:17:36.666129Z qemu-system-x86_64: -blockdev {"driver":"virtio-blk-vhost-vdpa","path":"/dev/fdset/0","node-name":"libvirt-1-storage","cache":{"direct":true,"no-flush":false},"auto-read-only":true,"discard":"unmap"}:
> > > > blkio_connect failed: Failed to connect to vDPA device: Input/output
> > > > error
> > > >
> > > > And we can only do that if we can determine that the binary has the
> > > > proper support for fds.
> > >
> > > I see the problem, thanks for explaining this!
> > >
> > > >
> > > > >
> > > > > > As far as I can tell, versions 7.2.0 and 8.0.0 include this
> > > > > > device but won't accept fds.
> > > > >
> > > > > Right.
> > > > >
> > > > > How do you suggest to proceed?
> > > >
> > > > I need some way to determine that the particular qemu binary can accept
> > > > a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
> > > > methods to determine capabilities for a given qemu binary, including
> > > > querying the qmp schema, commands, object types, specific device/object
> > > > properties, etc. For example, right now I can determine (via querying
> > > > the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
> > > > blockdev-add command by querying the qmp schema. I need something more
> > > > than that but I'm not sure how to do it without introducing a separate
> > > > 'fd' parameter. Any ideas?
> > >
> > > The only thing I can think of is to make a mix between v2 and v3. I mean add
> > > both the new `fd` parameter, and support qemu_open() on `path`.
> > >
> > > That way libvirt (or other users) can check that fd passing is supported and
> > > use `fd` or fdset with `path`.
> > >
> > > Obviously I would have liked to implement only one of the two methods, but
> > > if this helps, maybe it makes sense to support both.
> > >
> > > What do you think?
> >
> > Markus: Is a preferred way to make this new path handling behavior
> > introspectable? I vaguely remember a way for QMP clients to query
> > strings that describe QMP behavior that's not otherwise
> > introspectable...
> 
> If there is no other way for introspection with QMP, I think adding
> `fd` and supporting `qemu_open()` on `path` is the easiest.

It doesn't hurt to go ahead with that approach. If Markus prefers a
different approach, he'll let us know.

To be clear: the presence of 'fd' indicates that 'path' now supports
qemu_open() semantics and libvirt will still only use 'path'?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
>> CCing Markus for some advice.
>> 
>> On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:

[...]

>> > I need some way to determine that the particular qemu binary can accept
>> > a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
>> > methods to determine capabilities for a given qemu binary, including
>> > querying the qmp schema, commands, object types, specific device/object
>> > properties, etc. For example, right now I can determine (via querying
>> > the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
>> > blockdev-add command by querying the qmp schema. I need something more
>> > than that but I'm not sure how to do it without introducing a separate
>> > 'fd' parameter. Any ideas?
>> 
>> The only thing I can think of is to make a mix between v2 and v3. I mean add
>> both the new `fd` parameter, and support qemu_open() on `path`.
>> 
>> That way libvirt (or other users) can check that fd passing is supported and
>> use `fd` or fdset with `path`.
>> 
>> Obviously I would have liked to implement only one of the two methods, but
>> if this helps, maybe it makes sense to support both.
>> 
>> What do you think?
>
> Markus: Is a preferred way to make this new path handling behavior
> introspectable? I vaguely remember a way for QMP clients to query
> strings that describe QMP behavior that's not otherwise
> introspectable...

Let me try to answer this without first reading the entire thread.

QMP introspection lets you find out things like whether a command is
there, or whether an an argument is there, and what its type is.
Suffices most of the time.

However, behavior can certainly change while the introspection data
remains the same.  When a management application needs to know about the
change, we better expose the change in introspection somehow.

The "obvious" way to do that would be some arbitrary change that *is*
visible in introspection.  Meh.

The modern way is to add a suitable "feature".
docs/devel/qapi-code-gen.rst:

    Features
    --------

    Syntax::

        FEATURES = [ FEATURE, ... ]
        FEATURE = STRING
                | { 'name': STRING, '*if': COND }

    Sometimes, the behaviour of QEMU changes compatibly, but without a
    change in the QMP syntax (usually by allowing values or operations
    that previously resulted in an error).  QMP clients may still need to
    know whether the extension is available.

    For this purpose, a list of features can be specified for definitions,
    enumeration values, and struct members.  Each feature list member can
    either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is
    shorthand for ``{ 'name': STRING }``.

    The optional 'if' member specifies a conditional.  See `Configuring
    the schema`_ below for more on this.

    Example::

     { 'struct': 'TestType',
       'data': { 'number': 'int' },
       'features': [ 'allow-negative-numbers' ] }

    The feature strings are exposed to clients in introspection, as
    explained in section `Client JSON Protocol introspection`_.

    Intended use is to have each feature string signal that this build of
    QEMU shows a certain behaviour.

For a real example, see commit c6bdc312f30 (qapi: Add
'@allow-write-only-overlay' feature for 'blockdev-snapshot').

Does this answer your question?

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Jonathon Jongsma]

On 5/25/23 1:30 PM, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
>> On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
>>> CCing Markus for some advice.
>>>
>>> On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
> 
> [...]
> 
>>>> I need some way to determine that the particular qemu binary can accept
>>>> a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
>>>> methods to determine capabilities for a given qemu binary, including
>>>> querying the qmp schema, commands, object types, specific device/object
>>>> properties, etc. For example, right now I can determine (via querying
>>>> the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
>>>> blockdev-add command by querying the qmp schema. I need something more
>>>> than that but I'm not sure how to do it without introducing a separate
>>>> 'fd' parameter. Any ideas?
>>>
>>> The only thing I can think of is to make a mix between v2 and v3. I mean add
>>> both the new `fd` parameter, and support qemu_open() on `path`.
>>>
>>> That way libvirt (or other users) can check that fd passing is supported and
>>> use `fd` or fdset with `path`.
>>>
>>> Obviously I would have liked to implement only one of the two methods, but
>>> if this helps, maybe it makes sense to support both.
>>>
>>> What do you think?
>>
>> Markus: Is a preferred way to make this new path handling behavior
>> introspectable? I vaguely remember a way for QMP clients to query
>> strings that describe QMP behavior that's not otherwise
>> introspectable...
> 
> Let me try to answer this without first reading the entire thread.
> 
> QMP introspection lets you find out things like whether a command is
> there, or whether an an argument is there, and what its type is.
> Suffices most of the time.
> 
> However, behavior can certainly change while the introspection data
> remains the same.  When a management application needs to know about the
> change, we better expose the change in introspection somehow.
> 
> The "obvious" way to do that would be some arbitrary change that *is*
> visible in introspection.  Meh.
> 
> The modern way is to add a suitable "feature".
> docs/devel/qapi-code-gen.rst:
> 
>      Features
>      --------
> 
>      Syntax::
> 
>          FEATURES = [ FEATURE, ... ]
>          FEATURE = STRING
>                  | { 'name': STRING, '*if': COND }
> 
>      Sometimes, the behaviour of QEMU changes compatibly, but without a
>      change in the QMP syntax (usually by allowing values or operations
>      that previously resulted in an error).  QMP clients may still need to
>      know whether the extension is available.
> 
>      For this purpose, a list of features can be specified for definitions,
>      enumeration values, and struct members.  Each feature list member can
>      either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is
>      shorthand for ``{ 'name': STRING }``.
> 
>      The optional 'if' member specifies a conditional.  See `Configuring
>      the schema`_ below for more on this.
> 
>      Example::
> 
>       { 'struct': 'TestType',
>         'data': { 'number': 'int' },
>         'features': [ 'allow-negative-numbers' ] }
> 
>      The feature strings are exposed to clients in introspection, as
>      explained in section `Client JSON Protocol introspection`_.
> 
>      Intended use is to have each feature string signal that this build of
>      QEMU shows a certain behaviour.
> 
> For a real example, see commit c6bdc312f30 (qapi: Add
> '@allow-write-only-overlay' feature for 'blockdev-snapshot').
> 
> Does this answer your question?
> 

That sounds perfect for libvirt's needs.

Jonathon

Re: [PATCH v3 1/1] block/blkio: use qemu_open() to support fd passing for virtio-blk

[Stefano Garzarella]

On Thu, May 25, 2023 at 08:30:03PM +0200, Markus Armbruster wrote:
>Stefan Hajnoczi <stefanha@redhat.com> writes:
>
>> On Wed, May 17, 2023 at 09:19:26AM +0200, Stefano Garzarella wrote:
>>> CCing Markus for some advice.
>>>
>>> On Tue, May 16, 2023 at 11:04:21AM -0500, Jonathon Jongsma wrote:
>
>[...]
>
>>> > I need some way to determine that the particular qemu binary can accept
>>> > a /dev/fdset/ path for vdpa block devices. libvirt uses a variety of
>>> > methods to determine capabilities for a given qemu binary, including
>>> > querying the qmp schema, commands, object types, specific device/object
>>> > properties, etc. For example, right now I can determine (via querying
>>> > the qmp schema) whether virtio-blk-vhost-vdpa is a valid type for the
>>> > blockdev-add command by querying the qmp schema. I need something more
>>> > than that but I'm not sure how to do it without introducing a separate
>>> > 'fd' parameter. Any ideas?
>>>
>>> The only thing I can think of is to make a mix between v2 and v3. I mean add
>>> both the new `fd` parameter, and support qemu_open() on `path`.
>>>
>>> That way libvirt (or other users) can check that fd passing is supported and
>>> use `fd` or fdset with `path`.
>>>
>>> Obviously I would have liked to implement only one of the two methods, but
>>> if this helps, maybe it makes sense to support both.
>>>
>>> What do you think?
>>
>> Markus: Is a preferred way to make this new path handling behavior
>> introspectable? I vaguely remember a way for QMP clients to query
>> strings that describe QMP behavior that's not otherwise
>> introspectable...
>
>Let me try to answer this without first reading the entire thread.
>
>QMP introspection lets you find out things like whether a command is
>there, or whether an an argument is there, and what its type is.
>Suffices most of the time.
>
>However, behavior can certainly change while the introspection data
>remains the same.  When a management application needs to know about the
>change, we better expose the change in introspection somehow.
>
>The "obvious" way to do that would be some arbitrary change that *is*
>visible in introspection.  Meh.
>
>The modern way is to add a suitable "feature".
>docs/devel/qapi-code-gen.rst:
>
>    Features
>    --------
>
>    Syntax::
>
>        FEATURES = [ FEATURE, ... ]
>        FEATURE = STRING
>                | { 'name': STRING, '*if': COND }
>
>    Sometimes, the behaviour of QEMU changes compatibly, but without a
>    change in the QMP syntax (usually by allowing values or operations
>    that previously resulted in an error).  QMP clients may still need to
>    know whether the extension is available.
>
>    For this purpose, a list of features can be specified for definitions,
>    enumeration values, and struct members.  Each feature list member can
>    either be ``{ 'name': STRING, '*if': COND }``, or STRING, which is
>    shorthand for ``{ 'name': STRING }``.
>
>    The optional 'if' member specifies a conditional.  See `Configuring
>    the schema`_ below for more on this.
>
>    Example::
>
>     { 'struct': 'TestType',
>       'data': { 'number': 'int' },
>       'features': [ 'allow-negative-numbers' ] }
>
>    The feature strings are exposed to clients in introspection, as
>    explained in section `Client JSON Protocol introspection`_.
>
>    Intended use is to have each feature string signal that this build of
>    QEMU shows a certain behaviour.
>
>For a real example, see commit c6bdc312f30 (qapi: Add
>'@allow-write-only-overlay' feature for 'blockdev-snapshot').
>
>Does this answer your question?
>

Yep, this is exactly what we needed. Thank you very much!

I'll send v4 adding `features` to the `path` member, and I'll check the
version library in meson.build to enable the feature only when the
vhost-vdpa driver in libblkio really supports it, so libvirt can be sure
that if the feature is there, the fd passing will work.

Stefano