Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa
Stefano Garzarella writes: > On Sat, May 27, 2023 at 07:56:13AM +0200, Markus Armbruster wrote: >>Stefano Garzarella writes: >> >>> The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd >>> passing through the new 'fd' property. >>> >>> Since now we are using qemu_open() on '@path' if the virtio-blk driver >>> supports the fd passing, let's announce it. >>> In this way, 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. >>> >>> Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver >>> in libblkio supports it. >>> >>> Suggested-by: Markus Armbruster >>> Signed-off-by: Stefano Garzarella [...] >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 98d9116dae..1538d84ef4 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -3955,10 +3955,16 @@ >>> # >>> # @path: path to the vhost-vdpa character device. >>> # >>> +# Features: >>> +# @fdset: Member @path supports the special "/dev/fdset/N" path (since 8.1) >> >>Slightly long line, break it like this: >> >> # @fdset: Member @path supports the special "/dev/fdset/N" path >> # since 8.1) >> > > Sure, I'll fix it! > For the future, what is the maximum column? 70. Going over is okay if it improves legibility. However, when I reformatted the doc comments, I did not need to even once. [...]
Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa
On Fri, May 26, 2023 at 05:03:04PM +0200, Stefano Garzarella wrote: > The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd > passing through the new 'fd' property. > > Since now we are using qemu_open() on '@path' if the virtio-blk driver > supports the fd passing, let's announce it. > In this way, 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. > > Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver > in libblkio supports it. > > Suggested-by: Markus Armbruster > Signed-off-by: Stefano Garzarella > --- > > Notes: > v4: > - added this patch to allow libvirt to discover we support fdset [Markus] > > meson.build | 4 > qapi/block-core.json | 8 +++- > 2 files changed, 11 insertions(+), 1 deletion(-) Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa
On Sat, May 27, 2023 at 07:56:13AM +0200, Markus Armbruster wrote:
Stefano Garzarella writes:
The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd
passing through the new 'fd' property.
Since now we are using qemu_open() on '@path' if the virtio-blk driver
supports the fd passing, let's announce it.
In this way, 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.
Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver
in libblkio supports it.
Suggested-by: Markus Armbruster
Signed-off-by: Stefano Garzarella
---
Notes:
v4:
- added this patch to allow libvirt to discover we support fdset [Markus]
meson.build | 4
qapi/block-core.json | 8 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 78890f0155..8ea911f7b4 100644
--- a/meson.build
+++ b/meson.build
@@ -2108,6 +2108,10 @@ config_host_data.set('CONFIG_LZO', lzo.found())
config_host_data.set('CONFIG_MPATH', mpathpersist.found())
config_host_data.set('CONFIG_MPATH_NEW_API', mpathpersist_new_api)
config_host_data.set('CONFIG_BLKIO', blkio.found())
+if blkio.found()
+ config_host_data.set('CONFIG_BLKIO_VHOST_VDPA_FD',
+ blkio.version().version_compare('>=1.3.0'))
+endif
config_host_data.set('CONFIG_CURL', curl.found())
config_host_data.set('CONFIG_CURSES', curses.found())
config_host_data.set('CONFIG_GBM', gbm.found())
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98d9116dae..1538d84ef4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3955,10 +3955,16 @@
#
# @path: path to the vhost-vdpa character device.
#
+# Features:
+# @fdset: Member @path supports the special "/dev/fdset/N" path (since 8.1)
Slightly long line, break it like this:
# @fdset: Member @path supports the special "/dev/fdset/N" path
# since 8.1)
Sure, I'll fix it!
For the future, what is the maximum column?
+#
# Since: 7.2
##
{ 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
- 'data': { 'path': 'str' },
+ 'data': { 'path': { 'type': 'str',
+ 'features': [ { 'name' :'fdset',
+ 'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
+} },
'if': 'CONFIG_BLKIO' }
##
Tacking the feature to the member works.
For what it's worth, the existing features serving similar purposes are
all tacked to the command or type.
Do libvirt developers have a preference?
Yep, Jonathon pointed out that for libvirt it's better to move it to the
object level, so I'll do that in the next version.
Thanks,
Stefano
Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa
On Fri, May 26, 2023 at 04:20:14PM -0500, Jonathon Jongsma wrote:
On 5/26/23 10:03 AM, Stefano Garzarella wrote:
The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd
passing through the new 'fd' property.
Since now we are using qemu_open() on '@path' if the virtio-blk driver
supports the fd passing, let's announce it.
In this way, 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.
Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver
in libblkio supports it.
Suggested-by: Markus Armbruster
Signed-off-by: Stefano Garzarella
---
Notes:
v4:
- added this patch to allow libvirt to discover we support fdset [Markus]
meson.build | 4
qapi/block-core.json | 8 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 78890f0155..8ea911f7b4 100644
--- a/meson.build
+++ b/meson.build
@@ -2108,6 +2108,10 @@ config_host_data.set('CONFIG_LZO', lzo.found())
config_host_data.set('CONFIG_MPATH', mpathpersist.found())
config_host_data.set('CONFIG_MPATH_NEW_API', mpathpersist_new_api)
config_host_data.set('CONFIG_BLKIO', blkio.found())
+if blkio.found()
+ config_host_data.set('CONFIG_BLKIO_VHOST_VDPA_FD',
+ blkio.version().version_compare('>=1.3.0'))
+endif
config_host_data.set('CONFIG_CURL', curl.found())
config_host_data.set('CONFIG_CURSES', curses.found())
config_host_data.set('CONFIG_GBM', gbm.found())
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98d9116dae..1538d84ef4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3955,10 +3955,16 @@
#
# @path: path to the vhost-vdpa character device.
#
+# Features:
+# @fdset: Member @path supports the special "/dev/fdset/N" path (since 8.1)
+#
# Since: 7.2
##
{ 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
- 'data': { 'path': 'str' },
+ 'data': { 'path': { 'type': 'str',
+ 'features': [ { 'name' :'fdset',
+ 'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
+} },
'if': 'CONFIG_BLKIO' }
##
Take this for what it's worth and do what's best for qemu, but... It's
easier for libvirt if the 'features' are on the object rather than on
the 'path' member. Our schema parsing code already supports object
features but does not yet support features on builtin types.
I had done it that way in the first instance :-), then I saw that the
members themselves could have their own functionality, and it seemed
better.
However I agree that if it's easier for libvirt, then better to move the
feature to the whole object.
I'll change that in v5.
Thanks,
Stefano
i.e. something like this just works without any refactoring in libvirt:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1538d84ef4..78cfd10cdb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3961,11 +3961,11 @@
# Since: 7.2
##
{ 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
- 'data': { 'path': { 'type': 'str',
- 'features': [ { 'name' :'fdset',
- 'if':
'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
-} },
- 'if': 'CONFIG_BLKIO' }
+ 'data': { 'path': 'str' },
+ 'features': [ { 'name' :'fdset',
+'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ],
+ 'if': 'CONFIG_BLKIO'
+ }
##
# @IscsiTransport:
Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa
Stefano Garzarella writes:
> The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd
> passing through the new 'fd' property.
>
> Since now we are using qemu_open() on '@path' if the virtio-blk driver
> supports the fd passing, let's announce it.
> In this way, 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.
>
> Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver
> in libblkio supports it.
>
> Suggested-by: Markus Armbruster
> Signed-off-by: Stefano Garzarella
> ---
>
> Notes:
> v4:
> - added this patch to allow libvirt to discover we support fdset [Markus]
>
> meson.build | 4
> qapi/block-core.json | 8 +++-
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/meson.build b/meson.build
> index 78890f0155..8ea911f7b4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -2108,6 +2108,10 @@ config_host_data.set('CONFIG_LZO', lzo.found())
> config_host_data.set('CONFIG_MPATH', mpathpersist.found())
> config_host_data.set('CONFIG_MPATH_NEW_API', mpathpersist_new_api)
> config_host_data.set('CONFIG_BLKIO', blkio.found())
> +if blkio.found()
> + config_host_data.set('CONFIG_BLKIO_VHOST_VDPA_FD',
> + blkio.version().version_compare('>=1.3.0'))
> +endif
> config_host_data.set('CONFIG_CURL', curl.found())
> config_host_data.set('CONFIG_CURSES', curses.found())
> config_host_data.set('CONFIG_GBM', gbm.found())
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 98d9116dae..1538d84ef4 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3955,10 +3955,16 @@
> #
> # @path: path to the vhost-vdpa character device.
> #
> +# Features:
> +# @fdset: Member @path supports the special "/dev/fdset/N" path (since 8.1)
Slightly long line, break it like this:
# @fdset: Member @path supports the special "/dev/fdset/N" path
# since 8.1)
> +#
> # Since: 7.2
> ##
> { 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
> - 'data': { 'path': 'str' },
> + 'data': { 'path': { 'type': 'str',
> + 'features': [ { 'name' :'fdset',
> + 'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
> +} },
>'if': 'CONFIG_BLKIO' }
>
> ##
Tacking the feature to the member works.
For what it's worth, the existing features serving similar purposes are
all tacked to the command or type.
Do libvirt developers have a preference?
Re: [PATCH v4 2/2] qapi: add '@fdset' feature for BlockdevOptionsVirtioBlkVhostVdpa
On 5/26/23 10:03 AM, Stefano Garzarella wrote:
The virtio-blk-vhost-vdpa driver in libblkio 1.3.0 supports the fd
passing through the new 'fd' property.
Since now we are using qemu_open() on '@path' if the virtio-blk driver
supports the fd passing, let's announce it.
In this way, 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.
Add the '@fdset' feature only when the virtio-blk-vhost-vdpa driver
in libblkio supports it.
Suggested-by: Markus Armbruster
Signed-off-by: Stefano Garzarella
---
Notes:
v4:
- added this patch to allow libvirt to discover we support fdset [Markus]
meson.build | 4
qapi/block-core.json | 8 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/meson.build b/meson.build
index 78890f0155..8ea911f7b4 100644
--- a/meson.build
+++ b/meson.build
@@ -2108,6 +2108,10 @@ config_host_data.set('CONFIG_LZO', lzo.found())
config_host_data.set('CONFIG_MPATH', mpathpersist.found())
config_host_data.set('CONFIG_MPATH_NEW_API', mpathpersist_new_api)
config_host_data.set('CONFIG_BLKIO', blkio.found())
+if blkio.found()
+ config_host_data.set('CONFIG_BLKIO_VHOST_VDPA_FD',
+ blkio.version().version_compare('>=1.3.0'))
+endif
config_host_data.set('CONFIG_CURL', curl.found())
config_host_data.set('CONFIG_CURSES', curses.found())
config_host_data.set('CONFIG_GBM', gbm.found())
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98d9116dae..1538d84ef4 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3955,10 +3955,16 @@
#
# @path: path to the vhost-vdpa character device.
#
+# Features:
+# @fdset: Member @path supports the special "/dev/fdset/N" path (since 8.1)
+#
# Since: 7.2
##
{ 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
- 'data': { 'path': 'str' },
+ 'data': { 'path': { 'type': 'str',
+ 'features': [ { 'name' :'fdset',
+ 'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
+} },
'if': 'CONFIG_BLKIO' }
##
Take this for what it's worth and do what's best for qemu, but... It's
easier for libvirt if the 'features' are on the object rather than on
the 'path' member. Our schema parsing code already supports object
features but does not yet support features on builtin types.
i.e. something like this just works without any refactoring in libvirt:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1538d84ef4..78cfd10cdb 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3961,11 +3961,11 @@
# Since: 7.2
##
{ 'struct': 'BlockdevOptionsVirtioBlkVhostVdpa',
- 'data': { 'path': { 'type': 'str',
- 'features': [ { 'name' :'fdset',
- 'if':
'CONFIG_BLKIO_VHOST_VDPA_FD' } ]
-} },
- 'if': 'CONFIG_BLKIO' }
+ 'data': { 'path': 'str' },
+ 'features': [ { 'name' :'fdset',
+'if': 'CONFIG_BLKIO_VHOST_VDPA_FD' } ],
+ 'if': 'CONFIG_BLKIO'
+ }
##
# @IscsiTransport:
