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