| Message ID | 20231016083201.23736-1-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vhost-user: Fix protocol feature bit conflict | expand |
On Mon, 16 Oct 2023 11:32, Hanna Czenczek <hreitz@redhat.com> wrote: >diff --git a/include/hw/virtio/vhost-user.h >b/include/hw/virtio/vhost-user.h >index 9f9ddf878d..1d4121431b 100644 >--- a/include/hw/virtio/vhost-user.h >+++ b/include/hw/virtio/vhost-user.h >@@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > VHOST_USER_PROTOCOL_F_STATUS = 16, >- VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, >+ /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ >+ VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > VHOST_USER_PROTOCOL_F_MAX > }; May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well instead of a comment mention? Otherwise: Reviewed-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org>
On Mon, Oct 16, 2023 at 10:32:01AM +0200, Hanna Czenczek wrote: >The VHOST_USER_PROTOCOL_F_XEN_MMAP feature bit was defined in >f21e95ee97d, which has been part of qemu's 8.1.0 release. However, it >seems it was never added to qemu's code, but it is well possible that it >is already used by different front-ends outside of qemu (i.e., Xen). Yep, and also some backends (e.g. we released rust-vmm/vhost v0.8.0 with F_XEN_MMAP = 17 defined). > >VHOST_USER_PROTOCOL_F_SHARED_OBJECT in contrast was added to qemu's >code >in 16094766627, but never defined in the vhost-user specification. As a >consequence, both bits were defined to be 17, which cannot work. > >Regardless of whether actual code or the specification should take >precedence, F_XEN_MMAP is already part of a qemu release, while >F_SHARED_OBJECT is not. Therefore, bump the latter to take number 18 >instead of 17, and add this to the specification. > >Take the opportunity to add at least a little note on the >VhostUserShared structure to the specification. This structure is >referenced by the new commands introduced in 16094766627, but was not >defined. > >Fixes: 160947666276c5b7f6bca4d746bcac2966635d79 > ("vhost-user: add shared_object msg") >Signed-off-by: Hanna Czenczek <hreitz@redhat.com> >--- > docs/interop/vhost-user.rst | 11 +++++++++++ > include/hw/virtio/vhost-user.h | 3 ++- > subprojects/libvhost-user/libvhost-user.h | 3 ++- > 3 files changed, 15 insertions(+), 2 deletions(-) Thanks for fixinig this! Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > >diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >index 415bb47a19..768fb5c28c 100644 >--- a/docs/interop/vhost-user.rst >+++ b/docs/interop/vhost-user.rst >@@ -275,6 +275,16 @@ Inflight description > > :queue size: a 16-bit size of virtqueues > >+VhostUserShared >+^^^^^^^^^^^^^^^ >+ >++------+ >+| UUID | >++------+ >+ >+:UUID: 16 bytes UUID, whose first three components (a 32-bit value, then >+ two 16-bit values) are stored in big endian. >+ > C structure > ----------- > >@@ -885,6 +895,7 @@ Protocol features > #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 > #define VHOST_USER_PROTOCOL_F_STATUS 16 > #define VHOST_USER_PROTOCOL_F_XEN_MMAP 17 >+ #define VHOST_USER_PROTOCOL_F_SHARED_OBJECT 18 > > Front-end message types > ----------------------- >diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h >index 9f9ddf878d..1d4121431b 100644 >--- a/include/hw/virtio/vhost-user.h >+++ b/include/hw/virtio/vhost-user.h >@@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > VHOST_USER_PROTOCOL_F_STATUS = 16, >- VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, >+ /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ >+ VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > VHOST_USER_PROTOCOL_F_MAX > }; > >diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h >index b36a42a7ca..c2352904f0 100644 >--- a/subprojects/libvhost-user/libvhost-user.h >+++ b/subprojects/libvhost-user/libvhost-user.h >@@ -65,7 +65,8 @@ enum VhostUserProtocolFeature { > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */ >- VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, >+ /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ >+ VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > VHOST_USER_PROTOCOL_F_MAX > }; > >-- >2.41.0 > >
On 16-10-23, 11:45, Manos Pitsidianakis wrote: > On Mon, 16 Oct 2023 11:32, Hanna Czenczek <hreitz@redhat.com> wrote: > > diff --git a/include/hw/virtio/vhost-user.h > > b/include/hw/virtio/vhost-user.h > > index 9f9ddf878d..1d4121431b 100644 > > --- a/include/hw/virtio/vhost-user.h > > +++ b/include/hw/virtio/vhost-user.h > > @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { > > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > > VHOST_USER_PROTOCOL_F_STATUS = 16, > > - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, > > + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ > > + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > > VHOST_USER_PROTOCOL_F_MAX > > }; > > May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well instead of > a comment mention? Perhaps because we will never use it from Qemu code ? Anyway: Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
On 16.10.23 10:45, Manos Pitsidianakis wrote: > On Mon, 16 Oct 2023 11:32, Hanna Czenczek <hreitz@redhat.com> wrote: >> diff --git a/include/hw/virtio/vhost-user.h >> b/include/hw/virtio/vhost-user.h >> index 9f9ddf878d..1d4121431b 100644 >> --- a/include/hw/virtio/vhost-user.h >> +++ b/include/hw/virtio/vhost-user.h >> @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { >> VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, >> VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, >> VHOST_USER_PROTOCOL_F_STATUS = 16, >> - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, >> + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ >> + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, >> VHOST_USER_PROTOCOL_F_MAX >> }; > > May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well > instead of a comment mention? No particular reason, it’s just what I had planned to do for a while in other series that would introduce feature bits (e.g. https://lists.nongnu.org/archive/html/qemu-devel/2023-07/msg02452.html). I think I took that from libvhost-user, which does this for VHOST_USER_PROTOCOL_F_STATUS. > Otherwise: > > Reviewed-by: Emmanouil Pitsidianakis <manos.pitsidianakis@linaro.org> Thanks! Hanna
Viresh Kumar <viresh.kumar@linaro.org> writes: > On 16-10-23, 11:45, Manos Pitsidianakis wrote: >> On Mon, 16 Oct 2023 11:32, Hanna Czenczek <hreitz@redhat.com> wrote: >> > diff --git a/include/hw/virtio/vhost-user.h >> > b/include/hw/virtio/vhost-user.h >> > index 9f9ddf878d..1d4121431b 100644 >> > --- a/include/hw/virtio/vhost-user.h >> > +++ b/include/hw/virtio/vhost-user.h >> > @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { >> > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, >> > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, >> > VHOST_USER_PROTOCOL_F_STATUS = 16, >> > - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, >> > + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ >> > + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, >> > VHOST_USER_PROTOCOL_F_MAX >> > }; >> >> May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well instead of >> a comment mention? > > Perhaps because we will never use it from Qemu code ? Vikram's work on enabling xenpvh support will mean enabling grant support and while I suspect most VirtIO backends will be within QEMU itself if it ever want to off-load something to a vhost-user backend it will need to ensure this flag is set. > > Anyway: > > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
On Mon, 16 Oct 2023 13:32, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>Perhaps because we will never use it from Qemu code ?
It'd be nice if downstream users of the vhost-user protocol can fetch
and use the header verbatim. For example, for automatically generating
bindings.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> On Mon, 16 Oct 2023 at 04:33, Hanna Czenczek <hreitz@redhat.com> wrote: > > The VHOST_USER_PROTOCOL_F_XEN_MMAP feature bit was defined in > f21e95ee97d, which has been part of qemu's 8.1.0 release. However, it > seems it was never added to qemu's code, but it is well possible that it > is already used by different front-ends outside of qemu (i.e., Xen). > > VHOST_USER_PROTOCOL_F_SHARED_OBJECT in contrast was added to qemu's code > in 16094766627, but never defined in the vhost-user specification. As a > consequence, both bits were defined to be 17, which cannot work. > > Regardless of whether actual code or the specification should take > precedence, F_XEN_MMAP is already part of a qemu release, while > F_SHARED_OBJECT is not. Therefore, bump the latter to take number 18 > instead of 17, and add this to the specification. > > Take the opportunity to add at least a little note on the > VhostUserShared structure to the specification. This structure is > referenced by the new commands introduced in 16094766627, but was not > defined. > > Fixes: 160947666276c5b7f6bca4d746bcac2966635d79 > ("vhost-user: add shared_object msg") > Signed-off-by: Hanna Czenczek <hreitz@redhat.com> > --- > docs/interop/vhost-user.rst | 11 +++++++++++ > include/hw/virtio/vhost-user.h | 3 ++- > subprojects/libvhost-user/libvhost-user.h | 3 ++- > 3 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst > index 415bb47a19..768fb5c28c 100644 > --- a/docs/interop/vhost-user.rst > +++ b/docs/interop/vhost-user.rst > @@ -275,6 +275,16 @@ Inflight description > > :queue size: a 16-bit size of virtqueues > > +VhostUserShared > +^^^^^^^^^^^^^^^ > + > ++------+ > +| UUID | > ++------+ > + > +:UUID: 16 bytes UUID, whose first three components (a 32-bit value, then > + two 16-bit values) are stored in big endian. > + > C structure > ----------- > > @@ -885,6 +895,7 @@ Protocol features > #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 > #define VHOST_USER_PROTOCOL_F_STATUS 16 > #define VHOST_USER_PROTOCOL_F_XEN_MMAP 17 > + #define VHOST_USER_PROTOCOL_F_SHARED_OBJECT 18 > > Front-end message types > ----------------------- > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h > index 9f9ddf878d..1d4121431b 100644 > --- a/include/hw/virtio/vhost-user.h > +++ b/include/hw/virtio/vhost-user.h > @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > VHOST_USER_PROTOCOL_F_STATUS = 16, > - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, > + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ > + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > VHOST_USER_PROTOCOL_F_MAX > }; > > diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h > index b36a42a7ca..c2352904f0 100644 > --- a/subprojects/libvhost-user/libvhost-user.h > +++ b/subprojects/libvhost-user/libvhost-user.h > @@ -65,7 +65,8 @@ enum VhostUserProtocolFeature { > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */ > - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, > + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ > + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > VHOST_USER_PROTOCOL_F_MAX > }; > > -- > 2.41.0 > >
On 16-10-23, 12:40, Alex Bennée wrote: > > Viresh Kumar <viresh.kumar@linaro.org> writes: > > > On 16-10-23, 11:45, Manos Pitsidianakis wrote: > >> On Mon, 16 Oct 2023 11:32, Hanna Czenczek <hreitz@redhat.com> wrote: > >> > diff --git a/include/hw/virtio/vhost-user.h > >> > b/include/hw/virtio/vhost-user.h > >> > index 9f9ddf878d..1d4121431b 100644 > >> > --- a/include/hw/virtio/vhost-user.h > >> > +++ b/include/hw/virtio/vhost-user.h > >> > @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { > >> > VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, > >> > VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, > >> > VHOST_USER_PROTOCOL_F_STATUS = 16, > >> > - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, > >> > + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ > >> > + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, > >> > VHOST_USER_PROTOCOL_F_MAX > >> > }; > >> > >> May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well instead of > >> a comment mention? > > > > Perhaps because we will never use it from Qemu code ? > > Vikram's work on enabling xenpvh support will mean enabling grant > support and while I suspect most VirtIO backends will be within QEMU > itself if it ever want to off-load something to a vhost-user backend it > will need to ensure this flag is set. Hanna, It would be good to define it then in the current commit itself.
On 17.10.23 07:36, Viresh Kumar wrote: > On 16-10-23, 12:40, Alex Bennée wrote: >> Viresh Kumar <viresh.kumar@linaro.org> writes: >> >>> On 16-10-23, 11:45, Manos Pitsidianakis wrote: >>>> On Mon, 16 Oct 2023 11:32, Hanna Czenczek <hreitz@redhat.com> wrote: >>>>> diff --git a/include/hw/virtio/vhost-user.h >>>>> b/include/hw/virtio/vhost-user.h >>>>> index 9f9ddf878d..1d4121431b 100644 >>>>> --- a/include/hw/virtio/vhost-user.h >>>>> +++ b/include/hw/virtio/vhost-user.h >>>>> @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { >>>>> VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, >>>>> VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, >>>>> VHOST_USER_PROTOCOL_F_STATUS = 16, >>>>> - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, >>>>> + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ >>>>> + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, >>>>> VHOST_USER_PROTOCOL_F_MAX >>>>> }; >>>> May I ask, why not define VHOST_USER_PROTOCOL_F_XEN_MMAP as well instead of >>>> a comment mention? >>> Perhaps because we will never use it from Qemu code ? >> Vikram's work on enabling xenpvh support will mean enabling grant >> support and while I suspect most VirtIO backends will be within QEMU >> itself if it ever want to off-load something to a vhost-user backend it >> will need to ensure this flag is set. > Hanna, > > It would be good to define it then in the current commit itself. Not that I’m really opposed to that, but I don’t see the problem with just doing that in the same work that makes qemu actually use this flag, exactly because it’s just a -1/+1 change. I can send a v2, but should I do the same for libvhost-user and define the flag there? Do I have to add a patch to do the same for F_STATUS, which so far only got a placeholder comment? Hanna
On 17-10-23, 09:51, Hanna Czenczek wrote: > Not that I’m really opposed to that, but I don’t see the problem with just > doing that in the same work that makes qemu actually use this flag, exactly > because it’s just a -1/+1 change. > > I can send a v2, but should I do the same for libvhost-user and define the > flag there? Do I have to add a patch to do the same for F_STATUS, which so > far only got a placeholder comment? Sure, that's fine too.
On 17.10.23 09:53, Viresh Kumar wrote: > On 17-10-23, 09:51, Hanna Czenczek wrote: >> Not that I’m really opposed to that, but I don’t see the problem with just >> doing that in the same work that makes qemu actually use this flag, exactly >> because it’s just a -1/+1 change. >> >> I can send a v2, but should I do the same for libvhost-user and define the >> flag there? Do I have to add a patch to do the same for F_STATUS, which so >> far only got a placeholder comment? > Sure, that's fine too. I would rather not, though, and don’t see a tangible benefit in doing so.
On 17-10-23, 10:14, Hanna Czenczek wrote: > On 17.10.23 09:53, Viresh Kumar wrote: > > On 17-10-23, 09:51, Hanna Czenczek wrote: > > > Not that I’m really opposed to that, but I don’t see the problem with just > > > doing that in the same work that makes qemu actually use this flag, exactly > > > because it’s just a -1/+1 change. I should have replied here :) > > > > > > I can send a v2, but should I do the same for libvhost-user and define the > > > flag there? Do I have to add a patch to do the same for F_STATUS, which so > > > far only got a placeholder comment? > > Sure, that's fine too. > > I would rather not, though, and don’t see a tangible benefit in doing so. Sorry for the miscommunication, I meant we can leave it as is for now and let it be handled by the commit that uses it.
Hi! Thanks for the patch, and sorry for not noticing the flag had already been assigned. The patch looks good to me! BR, Albert On Tue, Oct 17, 2023 at 9:54 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 17-10-23, 09:51, Hanna Czenczek wrote: > > Not that I’m really opposed to that, but I don’t see the problem with > just > > doing that in the same work that makes qemu actually use this flag, > exactly > > because it’s just a -1/+1 change. > > > > I can send a v2, but should I do the same for libvhost-user and define > the > > flag there? Do I have to add a patch to do the same for F_STATUS, which > so > > far only got a placeholder comment? > > Sure, that's fine too. > > -- > viresh > >
On Tue, 17 Oct 2023 at 04:26, Albert Esteve <aesteve@redhat.com> wrote: > > Hi! > > Thanks for the patch, and sorry for not noticing the flag had already been assigned. The patch looks good to me! Hi Albert, I looked at the shared object code for the first time: - There are memory leaks in virtio_add_dmabuf() and virtio_add_vhost_device() when the UUID was added previously. - The hash table is global and there is no spoofing protection, so vhost-user devices can hijack known UUIDs. Is it possible to associate a vhost_dev owner with each shared object and only allow the owner to remove it? - Is there cleanup code that removes shared objects from the hash table when a vhost_dev is destroyed? Otherwise TYPE_VHOST_DEV shared objects are leaked and their stale vhost_dev pointers could be dereferenced. - virtio-dmabuf.h API naming suggests this is a core VirtIODevice API: virtio_free_resources(), virtio_add_vhost_device(), etc rather than an API for VirtioSharedObject. Can the names be made more specific: virtio_dmabuf_*() or virtio_shobj_*() so it's clear these APIs are related to the dmabufs/shared objects? Thanks, Stefan
On Tue, Oct 17, 2023 at 12:57 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Tue, 17 Oct 2023 at 04:26, Albert Esteve <aesteve@redhat.com> wrote: > > > > Hi! > > > > Thanks for the patch, and sorry for not noticing the flag had already > been assigned. The patch looks good to me! > > Hi Albert, > I looked at the shared object code for the first time: > - There are memory leaks in virtio_add_dmabuf() and > virtio_add_vhost_device() when the UUID was added previously. > There is a patch already for fixing the leak: https://patchew.org/QEMU/c61c13f9a0c67dec473bdbfc8789c29ef26c900b.1696624734.git.quic._5Fmathbern@quicinc.com/ > - The hash table is global and there is no spoofing protection, so > vhost-user devices can hijack known UUIDs. Is it possible to associate > a vhost_dev owner with each shared object and only allow the owner to > remove it? > True, it is unprotected from another backend to remove an entry without ownership. It sounds like a nice addition, I will post a patch. Thanks! > - Is there cleanup code that removes shared objects from the hash > table when a vhost_dev is destroyed? Otherwise TYPE_VHOST_DEV shared > objects are leaked and their stale vhost_dev pointers could be > dereferenced. > There is not. In a first thought, I assumed that the backends will be in charge of cleaning their entries from the shared hash table when they are destroyed (prematurely or no). I will look into occurrences of vhost_dev getting destroyed that may need explicit handling of the leftover entries. > - virtio-dmabuf.h API naming suggests this is a core VirtIODevice API: > virtio_free_resources(), virtio_add_vhost_device(), etc rather than an > API for VirtioSharedObject. Can the names be made more specific: > virtio_dmabuf_*() or virtio_shobj_*() so it's clear these APIs are > related to the dmabufs/shared objects? > Improving the names with what you suggested sounds good to me. I guess I'll go with virtio_dmabuf_*, for consistency with the file name. But `shobj` would be ok too. > > Thanks, > Stefan > >
On Tue, 17 Oct 2023 at 10:38, Albert Esteve <aesteve@redhat.com> wrote: > On Tue, Oct 17, 2023 at 12:57 PM Stefan Hajnoczi <stefanha@gmail.com> wrote: >> >> On Tue, 17 Oct 2023 at 04:26, Albert Esteve <aesteve@redhat.com> wrote: Thanks for considering my feedback! > There is not. In a first thought, I assumed that the backends will be in charge > of cleaning their entries from the shared hash table when they are destroyed > (prematurely or no). I will look into occurrences of vhost_dev getting destroyed > that may need explicit handling of the leftover entries. QEMU supports hot (un)plug of vhost-user devices. vhost-user backends may also crash. QEMU cannot rely solely on the backend to take any cleanup action because a backend may be buggy or misbehave. Stefan
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 415bb47a19..768fb5c28c 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -275,6 +275,16 @@ Inflight description :queue size: a 16-bit size of virtqueues +VhostUserShared +^^^^^^^^^^^^^^^ + ++------+ +| UUID | ++------+ + +:UUID: 16 bytes UUID, whose first three components (a 32-bit value, then + two 16-bit values) are stored in big endian. + C structure ----------- @@ -885,6 +895,7 @@ Protocol features #define VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS 15 #define VHOST_USER_PROTOCOL_F_STATUS 16 #define VHOST_USER_PROTOCOL_F_XEN_MMAP 17 + #define VHOST_USER_PROTOCOL_F_SHARED_OBJECT 18 Front-end message types ----------------------- diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h index 9f9ddf878d..1d4121431b 100644 --- a/include/hw/virtio/vhost-user.h +++ b/include/hw/virtio/vhost-user.h @@ -29,7 +29,8 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, VHOST_USER_PROTOCOL_F_STATUS = 16, - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, VHOST_USER_PROTOCOL_F_MAX }; diff --git a/subprojects/libvhost-user/libvhost-user.h b/subprojects/libvhost-user/libvhost-user.h index b36a42a7ca..c2352904f0 100644 --- a/subprojects/libvhost-user/libvhost-user.h +++ b/subprojects/libvhost-user/libvhost-user.h @@ -65,7 +65,8 @@ enum VhostUserProtocolFeature { VHOST_USER_PROTOCOL_F_INBAND_NOTIFICATIONS = 14, VHOST_USER_PROTOCOL_F_CONFIGURE_MEM_SLOTS = 15, /* Feature 16 is reserved for VHOST_USER_PROTOCOL_F_STATUS. */ - VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 17, + /* Feature 17 reserved for VHOST_USER_PROTOCOL_F_XEN_MMAP. */ + VHOST_USER_PROTOCOL_F_SHARED_OBJECT = 18, VHOST_USER_PROTOCOL_F_MAX };
The VHOST_USER_PROTOCOL_F_XEN_MMAP feature bit was defined in f21e95ee97d, which has been part of qemu's 8.1.0 release. However, it seems it was never added to qemu's code, but it is well possible that it is already used by different front-ends outside of qemu (i.e., Xen). VHOST_USER_PROTOCOL_F_SHARED_OBJECT in contrast was added to qemu's code in 16094766627, but never defined in the vhost-user specification. As a consequence, both bits were defined to be 17, which cannot work. Regardless of whether actual code or the specification should take precedence, F_XEN_MMAP is already part of a qemu release, while F_SHARED_OBJECT is not. Therefore, bump the latter to take number 18 instead of 17, and add this to the specification. Take the opportunity to add at least a little note on the VhostUserShared structure to the specification. This structure is referenced by the new commands introduced in 16094766627, but was not defined. Fixes: 160947666276c5b7f6bca4d746bcac2966635d79 ("vhost-user: add shared_object msg") Signed-off-by: Hanna Czenczek <hreitz@redhat.com> --- docs/interop/vhost-user.rst | 11 +++++++++++ include/hw/virtio/vhost-user.h | 3 ++- subprojects/libvhost-user/libvhost-user.h | 3 ++- 3 files changed, 15 insertions(+), 2 deletions(-)