[Rust-VMM] [virtio-dev] [VHOST USER SPEC PATCH] vhost-user.rst: add clarifying language about protocol negotiation
alex.bennee at linaro.org
Mon Mar 1 11:38:47 UTC 2021
Stefan Hajnoczi <stefanha at 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 at linaro.org>
>> Cc: Jiang Liu <gerry at 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
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:
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:
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
> 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
OK - so does that mean that feature bit will remain UNUSED for ever
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
> The only order I found was:
> 1. VHOST_USER_GET_FEATURES to determine whether protocol features are
> 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
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
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
> "the feature" == VHOST_USER_F_PROTOCOL_FEATURES?
More information about the Rust-vmm