| Message ID | 20210303145011.14547-1-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] vhost-user.rst: add clarifying language about protocol negotiation | expand |
On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: > Make the language about feature negotiation explicitly clear about the > handling of the VHOST_USER_F_PROTOCOL_FEATURES feature bit. Try and > avoid the sort of bug introduced in vhost.rs REPLY_ACK processing: > > https://github.com/rust-vmm/vhost/pull/24 > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Jiang Liu <gerry@linux.alibaba.com> > Message-Id: <20210226111619.21178-1-alex.bennee@linaro.org> > > --- > v2 > - use Stefan's suggested wording > - Be super explicit in the message descriptions > --- > docs/interop/vhost-user.rst | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: > Make the language about feature negotiation explicitly clear about the > handling of the VHOST_USER_F_PROTOCOL_FEATURES feature bit. Try and > avoid the sort of bug introduced in vhost.rs REPLY_ACK processing: > > https://github.com/rust-vmm/vhost/pull/24 > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Jiang Liu <gerry@linux.alibaba.com> > Message-Id: <20210226111619.21178-1-alex.bennee@linaro.org> > > --- > v2 > - use Stefan's suggested wording > - Be super explicit in the message descriptions > --- > docs/interop/vhost-user.rst | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst > index 2918d7c757..7c1fb8c209 100644 > --- a/docs/interop/vhost-user.rst > +++ b/docs/interop/vhost-user.rst > @@ -307,6 +307,18 @@ bit was dedicated for this purpose:: > > #define VHOST_USER_F_PROTOCOL_FEATURES 30 > > +Note that VHOST_USER_F_PROTOCOL_FEATURES is the UNUSED (30) feature > +bit defined in `VIRTIO 1.1 6.3 Legacy Interface: Reserved Feature Bits > +<https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4130003>`_. > +VIRTIO devices do not advertise this feature bit and therefore VIRTIO > +drivers cannot negotiate it. > + > +This reserved feature bit was reused by the vhost-user protocol to add > +vhost-user protocol feature negotiation in a backwards compatible > +fashion. Old vhost-user master and slave implementations continue to > +work even though they are not aware of vhost-user protocol feature > +negotiation. > + > Ring states > ----------- > > @@ -865,7 +877,8 @@ Front-end message types > Get the protocol feature bitmask from the underlying vhost > implementation. Only legal if feature bit > ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > - ``VHOST_USER_GET_FEATURES``. > + ``VHOST_USER_GET_FEATURES``. It does not need to be acknowledged by > + ``VHOST_USER_SET_FEATURES``. > > .. Note:: > Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must > @@ -881,7 +894,8 @@ Front-end message types > Enable protocol features in the underlying vhost implementation. > > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > - ``VHOST_USER_GET_FEATURES``. > + ``VHOST_USER_GET_FEATURES``. It does not need to be acknowledged by > + ``VHOST_USER_SET_FEATURES``. > > .. Note:: > Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must support Not really clear what does "It" refer to here. Also, are we sure it's ok to send the messages and then send VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear? Looks more like a violation to me ... How about: It -> this bit does not need to be -> before ... has been so: Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in - ``VHOST_USER_GET_FEATURES``, and even before this bit has been acknowledged by VHOST_USER_SET_FEATURES. > -- > 2.20.1
Michael S. Tsirkin <mst@redhat.com> writes: > On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: >> Make the language about feature negotiation explicitly clear about the >> handling of the VHOST_USER_F_PROTOCOL_FEATURES feature bit. Try and >> avoid the sort of bug introduced in vhost.rs REPLY_ACK processing: >> >> https://github.com/rust-vmm/vhost/pull/24 >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Cc: Jiang Liu <gerry@linux.alibaba.com> >> Message-Id: <20210226111619.21178-1-alex.bennee@linaro.org> >> >> --- >> v2 >> - use Stefan's suggested wording >> - Be super explicit in the message descriptions >> --- >> docs/interop/vhost-user.rst | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst >> index 2918d7c757..7c1fb8c209 100644 >> --- a/docs/interop/vhost-user.rst >> +++ b/docs/interop/vhost-user.rst >> @@ -307,6 +307,18 @@ bit was dedicated for this purpose:: >> >> #define VHOST_USER_F_PROTOCOL_FEATURES 30 >> >> +Note that VHOST_USER_F_PROTOCOL_FEATURES is the UNUSED (30) feature >> +bit defined in `VIRTIO 1.1 6.3 Legacy Interface: Reserved Feature Bits >> +<https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4130003>`_. >> +VIRTIO devices do not advertise this feature bit and therefore VIRTIO >> +drivers cannot negotiate it. >> + >> +This reserved feature bit was reused by the vhost-user protocol to add >> +vhost-user protocol feature negotiation in a backwards compatible >> +fashion. Old vhost-user master and slave implementations continue to >> +work even though they are not aware of vhost-user protocol feature >> +negotiation. >> + >> Ring states >> ----------- >> >> @@ -865,7 +877,8 @@ Front-end message types >> Get the protocol feature bitmask from the underlying vhost >> implementation. Only legal if feature bit >> ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in >> - ``VHOST_USER_GET_FEATURES``. >> + ``VHOST_USER_GET_FEATURES``. It does not need to be acknowledged by >> + ``VHOST_USER_SET_FEATURES``. >> >> .. Note:: >> Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must >> @@ -881,7 +894,8 @@ Front-end message types >> Enable protocol features in the underlying vhost implementation. >> >> Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in >> - ``VHOST_USER_GET_FEATURES``. >> + ``VHOST_USER_GET_FEATURES``. It does not need to be acknowledged by >> + ``VHOST_USER_SET_FEATURES``. >> >> .. Note:: >> Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must support > > > Not really clear what does "It" refer to here. > Also, are we sure it's ok to send the messages and then send > VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear? > Looks more like a violation to me ... So what behaviour are we looking for here? Should the vhost-user sender re-apply the VHOST_USER_F_PROTOCOL_FEATURES bit to the features when the guest does it SET_FEATURES during the negotiation? We will have already gone through the VHOST_USER_GET_PROTOCOL_FEATURES/VHOST_USER_SET_PROTOCOL_FEATURES dance at this point and have started passing messages. Should we stop at the point we finally process SET_FEATURES? > > > How about: It -> this bit > does not need to be -> before ... has been > > so: > > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > - ``VHOST_USER_GET_FEATURES``, and even before this bit has been > acknowledged by VHOST_USER_SET_FEATURES. That leaves open to interpretation what happens if SET_FEATURES clears the bit? > > > > >> -- >> 2.20.1
On Wed, Mar 03, 2021 at 05:01:05PM -0500, Michael S. Tsirkin wrote: > On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: > Also, are we sure it's ok to send the messages and then send > VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear? > Looks more like a violation to me ... Looking again I agree it would be a violation to omit VHOST_USER_F_PROTOCOL_FEATURES in VHOST_USER_SET_FEATURES. Previously I only looked at VHOST_USER_SET_PROTOCOL_FEATURES where the spec says: Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in ``VHOST_USER_GET_FEATURES``. So negotiation is *not* necessary for sending VHOST_USER_SET_PROTOCOL_FEATURES. However, I missed that other features *do* require negotiation of VHOST_USER_F_PROTOCOL_FEATURES according to the spec. For example: If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the ring is initialized in an enabled state. Now I think: 1. VHOST_USER_F_PROTOCOL_FEATURES *must* be included VHOST_USER_SET_FEATURES if the master supports it. 2. VHOST_USER_SET_PROTOCOL_FEATURES does not require negotiation, instead the master just needs to check that VHOST_USER_F_PROTOCOL_FEATURES is included in the VHOST_USER_GET_FEATURES reply. It's an exception. Stefan
Stefan Hajnoczi <stefanha@redhat.com> writes: > On Wed, Mar 03, 2021 at 05:01:05PM -0500, Michael S. Tsirkin wrote: >> On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: >> Also, are we sure it's ok to send the messages and then send >> VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear? >> Looks more like a violation to me ... > > Looking again I agree it would be a violation to omit > VHOST_USER_F_PROTOCOL_FEATURES in VHOST_USER_SET_FEATURES. > > Previously I only looked at VHOST_USER_SET_PROTOCOL_FEATURES where the > spec says: > > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > ``VHOST_USER_GET_FEATURES``. > > So negotiation is *not* necessary for sending > VHOST_USER_SET_PROTOCOL_FEATURES. > > However, I missed that other features *do* require negotiation of > VHOST_USER_F_PROTOCOL_FEATURES according to the spec. For example: > > If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the > ring is initialized in an enabled state. > > Now I think: > > 1. VHOST_USER_F_PROTOCOL_FEATURES *must* be included > VHOST_USER_SET_FEATURES if the master supports it. So added by the master - still invisible to the guest? > > 2. VHOST_USER_SET_PROTOCOL_FEATURES does not require negotiation, > instead the master just needs to check that > VHOST_USER_F_PROTOCOL_FEATURES is included in the > VHOST_USER_GET_FEATURES reply. It's an exception. OK I'm now thoroughly confused but I guess that's a good thing. However if we make the changes to QEMU to honour this won't we break with existing vhost-user receivers? We'll also need to track the state of a SET_FEATURES has happened and then gate the sending of things like reply_ack requests?
On Thu, Mar 04, 2021 at 06:11:26PM +0000, Alex Bennée wrote: > > Stefan Hajnoczi <stefanha@redhat.com> writes: > > > On Wed, Mar 03, 2021 at 05:01:05PM -0500, Michael S. Tsirkin wrote: > >> On Wed, Mar 03, 2021 at 02:50:11PM +0000, Alex Bennée wrote: > >> Also, are we sure it's ok to send the messages and then send > >> VHOST_USER_SET_FEATURES with VHOST_USER_F_PROTOCOL_FEATURES clear? > >> Looks more like a violation to me ... > > > > Looking again I agree it would be a violation to omit > > VHOST_USER_F_PROTOCOL_FEATURES in VHOST_USER_SET_FEATURES. > > > > Previously I only looked at VHOST_USER_SET_PROTOCOL_FEATURES where the > > spec says: > > > > Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in > > ``VHOST_USER_GET_FEATURES``. > > > > So negotiation is *not* necessary for sending > > VHOST_USER_SET_PROTOCOL_FEATURES. > > > > However, I missed that other features *do* require negotiation of > > VHOST_USER_F_PROTOCOL_FEATURES according to the spec. For example: > > > > If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the > > ring is initialized in an enabled state. > > > > Now I think: > > > > 1. VHOST_USER_F_PROTOCOL_FEATURES *must* be included > > VHOST_USER_SET_FEATURES if the master supports it. > > So added by the master - still invisible to the guest? The relationship between a guest-visible VIRTIO device and a vhost-user device is not covered by any specification AFAIK. My opinion is that VMMs should not expose VHOST_USER_F_PROTOCOL_FEATURES to the guest since that bit has a different meaning in VIRTIO. If VMMs do expose it to the guest then they'll probably get away with it since bit 30 is unused in VIRTIO. Drivers will probably not enable the bit, even if the device reports it. > > > > 2. VHOST_USER_SET_PROTOCOL_FEATURES does not require negotiation, > > instead the master just needs to check that > > VHOST_USER_F_PROTOCOL_FEATURES is included in the > > VHOST_USER_GET_FEATURES reply. It's an exception. > > OK I'm now thoroughly confused but I guess that's a good thing. However > if we make the changes to QEMU to honour this won't we break with > existing vhost-user receivers? We'll also need to track the state of a > SET_FEATURES has happened and then gate the sending of things like > reply_ack requests? (To avoid confusion below when I write about negotiating VHOST_USER_F_PROTOCOL_FEATURES it means sending VHOST_USER_SET_FEATURES with the feature bit enabled. It shouldn't be confused with sending VHOST_USER_SET_PROTOCOL_FEATURES to negotiate protocol features.) I went through the spec and checked where VHOST_USER_F_PROTOCOL_FEATURES must be negotiated. There is only one place: ring initialization state (enabled/disabled). This makes sense: for backwards compatibility the device backend needs to know whether or not the master supports VHOST_USER_F_PROTOCOL_FEATURES. There are also two messages where the spec says they "should" only be sent when VHOST_USER_F_PROTOCOL_FEATURES has been negotiated: VHOST_USER_SET_VRING_ENABLE and VHOST_USER_SET_SLAVE_REQ_FD. However, QEMU's vhost-user master implementation actually sends VHOST_USER_SET_SLAVE_REQ_FD before negotiating VHOST_USER_F_PROTOCOL_FEATURES. I *think* VHOST_USER_SET_VRING_ENABLE is really only sent after VHOST_USER_F_PROTOCOL_FEATURES has been negotiated but I haven't checked all code paths. All other mentions of VHOST_USER_F_PROTOCOL_FEATURES in the spec only require checking VHOST_USER_GET_FEATURES, not negotiation. So QEMU follows the current spec pretty closely. I don't anything needs to be changed in QEMU. Have you found something? Stefan
diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst index 2918d7c757..7c1fb8c209 100644 --- a/docs/interop/vhost-user.rst +++ b/docs/interop/vhost-user.rst @@ -307,6 +307,18 @@ bit was dedicated for this purpose:: #define VHOST_USER_F_PROTOCOL_FEATURES 30 +Note that VHOST_USER_F_PROTOCOL_FEATURES is the UNUSED (30) feature +bit defined in `VIRTIO 1.1 6.3 Legacy Interface: Reserved Feature Bits +<https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4130003>`_. +VIRTIO devices do not advertise this feature bit and therefore VIRTIO +drivers cannot negotiate it. + +This reserved feature bit was reused by the vhost-user protocol to add +vhost-user protocol feature negotiation in a backwards compatible +fashion. Old vhost-user master and slave implementations continue to +work even though they are not aware of vhost-user protocol feature +negotiation. + Ring states ----------- @@ -865,7 +877,8 @@ Front-end message types Get the protocol feature bitmask from the underlying vhost implementation. Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in - ``VHOST_USER_GET_FEATURES``. + ``VHOST_USER_GET_FEATURES``. It does not need to be acknowledged by + ``VHOST_USER_SET_FEATURES``. .. Note:: Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must @@ -881,7 +894,8 @@ Front-end message types Enable protocol features in the underlying vhost implementation. Only legal if feature bit ``VHOST_USER_F_PROTOCOL_FEATURES`` is present in - ``VHOST_USER_GET_FEATURES``. + ``VHOST_USER_GET_FEATURES``. It does not need to be acknowledged by + ``VHOST_USER_SET_FEATURES``. .. Note:: Back-ends that report ``VHOST_USER_F_PROTOCOL_FEATURES`` must support
Make the language about feature negotiation explicitly clear about the handling of the VHOST_USER_F_PROTOCOL_FEATURES feature bit. Try and avoid the sort of bug introduced in vhost.rs REPLY_ACK processing: https://github.com/rust-vmm/vhost/pull/24 Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Jiang Liu <gerry@linux.alibaba.com> Message-Id: <20210226111619.21178-1-alex.bennee@linaro.org> --- v2 - use Stefan's suggested wording - Be super explicit in the message descriptions --- docs/interop/vhost-user.rst | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)