| Message ID | 20230526150304.158206-3-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/blkio: support fd passing for virtio-blk-vhost-vdpa driver | expand |
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 <armbru@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > 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:
Stefano Garzarella <sgarzare@redhat.com> 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 <armbru@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > 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?
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 <armbru@redhat.com> >>Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>--- >> >>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: > >
On Sat, May 27, 2023 at 07:56:13AM +0200, Markus Armbruster wrote: >Stefano Garzarella <sgarzare@redhat.com> 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 <armbru@redhat.com> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> >> 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
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 <armbru@redhat.com> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > > 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 <stefanha@redhat.com>
Stefano Garzarella <sgarzare@redhat.com> writes: > On Sat, May 27, 2023 at 07:56:13AM +0200, Markus Armbruster wrote: >>Stefano Garzarella <sgarzare@redhat.com> 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 <armbru@redhat.com> >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> [...] >>> 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. [...]
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' } ##
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 <armbru@redhat.com> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- 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(-)