Re: [virtio-dev] [VHOST USER SPEC PATCH] vhost-user.rst: add clarifying language about protocol negotiation

[Alex BennÃe <alex.bennee@linaro.org>]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> On Fri, Feb 26, 2021 at 11:16:19AM +0000, Alex BennÃe wrote:
>> In practice the protocol negotiation between vhost master and slave
>> occurs before the final feature negotiation between backend and
>> frontend. This has lead to an inconsistency between the rust-vmm vhost
>> implementation and the libvhost-user library in their approaches to
>> checking if all the requirements for REPLY_ACK processing were met.
>> As this is purely a function of the protocol negotiation and not of
>> interest to the frontend lets make the language clearer about the
>> requirements for a successfully negotiated protocol feature.
>> 
>> Signed-off-by: Alex BennÃe <alex.bennee@linaro.org>
>> Cc: Jiang Liu <gerry@linux.alibaba.com>
>> ---
>>  docs/interop/vhost-user.rst | 14 ++++++++++++--
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> I had difficulty understanding this change and its purpose. I think it's
> emphasizing what the spec already says: VHOST_USER_SET_PROTOCOL_FEATURES
> can be sent after VHOST_USER_F_PROTOCOL_FEATURES was reported by
> VHOST_USER_GET_FEATURES.

Well and also the protocol feature is considered negotiated after that
sequence and doesn't require the feature bit to also be negotiated. I
think I read the spec properly when I submitted:

  https://github.com/rust-vmm/vhost/pull/24

however it was implied rather than explicit. I was hoping to make that
clearer but obviously I've failed with this iteration.

> BTW Paolo has just sent a patch here to use the terms "frontend" and
> "backend" with different meanings from how you are using them:
> https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg08347.html

Yeah we have mixed up terminology - the relationship between QEMU and a
vhost-user daemon is separate from the relationship between a VirtIO
device driver (in the guest) and the device implementation (as done by
the combination of QEMU and the vhost-user daemon).

I wish we had clearer terminology sections throughout both specs.

>
>> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
>> index d6085f7045..3ac221a8c7 100644
>> --- a/docs/interop/vhost-user.rst
>> +++ b/docs/interop/vhost-user.rst
>> @@ -301,12 +301,22 @@ If *slave* detects some error such as incompatible features, it may also
>>  close the connection. This should only happen in exceptional circumstances.
>>  
>>  Any protocol extensions are gated by protocol feature bits, which
>> -allows full backwards compatibility on both master and slave.  As
>> -older slaves don't support negotiating protocol features, a feature
>> +allows full backwards compatibility on both master and slave. As older
>> +slaves don't support negotiating protocol features, a device feature
>>  bit was dedicated for this purpose::
>>  
>>    #define VHOST_USER_F_PROTOCOL_FEATURES 30
>>  
>> +However as the protocol negotiation something that only occurs between
>
> Missing "is". Shortening the sentence fixes that without losing clarity:
> s/something that/negotiation/
>
>> +parts of the backend implementation it is permissible to for the master
>
> "vhost-user device backend" is often used to refer to the slave (to
> avoid saying the word "slave") but "backend" is being used in a
> different sense here. That is confusing.
>
>> +to mask the feature bit from the guest.
>
> I think this sentence effectively says "the master MAY mask the
> VHOST_USER_F_PROTOCOL_FEATURES bit from the VIRTIO feature bits". That
> is not really accurate since VIRTIO devices do not advertise this
> feature bit and so it can never be negotiated through the VIRTIO feature
> negotiation process.
>
> How about referring to the details from the VIRTIO 1.1 specification
> instead. Something like this:
>
>   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.

OK - so does that mean that feature bit will remain UNUSED for ever
more?

What about other feature bits? Is it permissible for the
master/requester/vhost-user front-end/QEMU to filter any other feature
bits the slave/vhost-user backend/daemon may offer from being read by
the guest driver when it reads the feature bits?

>
>> As noted for the
>> +``VHOST_USER_GET_PROTOCOL_FEATURES`` and
>> +``VHOST_USER_SET_PROTOCOL_FEATURES`` messages this occurs before a
>> +final ``VHOST_USER_SET_FEATURES`` comes from the guest.
>
> I couldn't find any place where vhost-user.rst states that
> VHOST_USER_SET_PROTOCOL_FEATURES has to come before
> VHOST_USER_SET_FEATURES?
>
> The only order I found was:
>
> 1. VHOST_USER_GET_FEATURES to determine whether protocol features are
>    supported.
> 2. VHOST_USER_GET_PROTOCOL_FEATURES to fetch available protocol feature bits.
> 3. VHOST_USER_SET_PROTOCOL_FEATURES to set protocol feature bits.
> 4. Using functionality that depends on enabled protocol feature bits.
>
> Is the purpose of this sentence to add a new requirement to the spec
> that "VHOST_USER_SET_PROTOCOL_FEATURES MUST be sent before
> VHOST_USER_SET_FEATURES"?

No I don't want to add a new sequence requirement. But if SET_FEATURES
doesn't acknowledge the VHOST_USER_F_PROTOCOL_FEATURES bit should that
stop the processing of
VHOST_USER_GET_PROTOCOL_FEATURES/VHOST_USER_SET_PROTOCOL_FEATURES
messages? AFAICT SET_FEATURES should be irrelevant to the negotiation of
the PROTOCOL_FEATURES right?

>> So the
>> +enabling of protocol features need only require the advertising of the
>> +feature by the slave and the successful get/set protocol features
>> +sequence.
>
> "the feature" == VHOST_USER_F_PROTOCOL_FEATURES?

yes.

>
> Stefan


-- 
Alex BennÃe