Re: [PATCH] vhost-user.rst: Clarify enabling/disabling vrings

[Hanna Czenczek <hreitz@redhat.com>]

On 12.07.23 13:17, Michael S. Tsirkin wrote:
> On Wed, Jul 12, 2023 at 11:17:04AM +0200, Hanna Czenczek wrote:
>> Currently, the vhost-user documentation says that rings are to be
>> initialized in a disabled state when VHOST_USER_F_PROTOCOL_FEATURES is
>> negotiated.  However, by the time of feature negotiation, all rings have
>> already been initialized, so it is not entirely clear what this means.
>>
>> At least the vhost-user-backend Rust crate's implementation interpreted
>> it to mean that whenever this feature is negotiated, all rings are to be
>> put into a disabled state, which means that every SET_FEATURES call
>> would disable all rings, effectively halting the device.  This is
>> problematic because the VHOST_F_LOG_ALL feature is also set or cleared
>> this way, which happens during migration.  Doing so should not halt the
>> device.
>>
>> Other implementations have interpreted this to mean that the device is
>> to be initialized with all rings disabled, and a subsequent SET_FEATURES
>> call that does not set VHOST_USER_F_PROTOCOL_FEATURES will enable all of
>> them.  Here, SET_FEATURES will never disable any ring.
> Huh. I don't know why we don't set VHOST_USER_F_PROTOCOL_FEATURES
> on all calls though. I think it's a bug. Let's fix that first of all?
> Then we can still document behaviour of existing buggy QEMU.

To my knowledge we (i.e. qemu) do.  I think we’d only not set it if the 
back-end just doesn’t support it.

In the above paragraph, I just meant to describe how back-end 
implementations other than the Rust one behave (when they support 
F_PROTOCOL_FEATURES): They disable all vrings from the start.  If 
SET_FEATURES is called without F_PROTOCOL_FEATURES (which qemu won’t 
do), they’ll enable them.  But outside of SET_VRING_ENABLE, they’ll 
never disable them after initialization.

I.e. the case where a back-end supports F_PROTOCOL_FEATURES but the 
front-end doesn’t set it is just hypothetical, and not meant to describe 
the behavior of any current qemu version.

Hanna

>> This other interpretation does not suffer the problem of unintentionally
>> halting the device whenever features are set or cleared, so it seems
>> better and more reasonable.
>>
>> We should clarify this in the documentation.
>>
>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com>
>> ---
>>   docs/interop/vhost-user.rst | 23 +++++++++++++++++------
>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
>> index 5a070adbc1..ca0e899765 100644
>> --- a/docs/interop/vhost-user.rst
>> +++ b/docs/interop/vhost-user.rst
>> @@ -383,12 +383,23 @@ and stop ring upon receiving ``VHOST_USER_GET_VRING_BASE``.
>>   
>>   Rings can be enabled or disabled by ``VHOST_USER_SET_VRING_ENABLE``.
>>   
>> -If ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been negotiated, the
>> -ring starts directly in the enabled state.
>> -
>> -If ``VHOST_USER_F_PROTOCOL_FEATURES`` has been negotiated, the ring is
>> -initialized in a disabled state and is enabled by
>> -``VHOST_USER_SET_VRING_ENABLE`` with parameter 1.
>> +Between initialization and the first ``VHOST_USER_SET_FEATURES`` call, it
>> +is implementation-defined whether each ring is enabled or disabled.
>> +
>> +If ``VHOST_USER_SET_FEATURES`` does not negotiate
>> +``VHOST_USER_F_PROTOCOL_FEATURES``, each ring, when started, will be
>> +enabled immediately.
>> +
>> +If ``VHOST_USER_SET_FEATURES`` does negotiate
>> +``VHOST_USER_F_PROTOCOL_FEATURES``, each ring will remain in the disabled
>> +state until ``VHOST_USER_SET_VRING_ENABLE`` enables it with parameter 1.
>> +
>> +Back-end implementations that support ``VHOST_USER_F_PROTOCOL_FEATURES``
>> +should implement this by initializing each ring in a disabled state, and
>> +enabling them when ``VHOST_USER_SET_FEATURES`` is used without
>> +negotiating ``VHOST_USER_F_PROTOCOL_FEATURES``.  Other than that, rings
>> +should only be enabled and disabled through
>> +``VHOST_USER_SET_VRING_ENABLE``.
>>   
>>   While processing the rings (whether they are enabled or not), the back-end
>>   must support changing some configuration aspects on the fly.
>> -- 
>> 2.41.0