Michael S. Tsirkin <m...@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.ben...@linaro.org> >> Cc: Jiang Liu <ge...@linux.alibaba.com> >> Message-Id: <20210226111619.21178-1-alex.ben...@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 -- Alex Bennée
