[PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Alex Bennée]

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
-- 
2.20.1

Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Michael S. Tsirkin]

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

Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Alex Bennée]

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


-- 
Alex Bennée

Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Alex Bennée]

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?

-- 
Alex Bennée

Re: [PATCH v2] vhost-user.rst: add clarifying language about protocol negotiation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3733 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]