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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, 19 Jul 2023 at 09:34, Hanna Czenczek <hreitz@redhat.com> wrote:
>
> On 18.07.23 17:26, Stefan Hajnoczi 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.
> >>
> >> 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.
> > What is the purpose of this statement? Rings cannot be used before
> > feature negotiation (with the possible exception of legacy devices that
> > allowed this to accomodate buggy drivers).
>
> Perfect :)
>
> > To me this statement complicates things and raises more questions than
> > it answers.
>
> OK.  The context for the statement is as follows: When the back-end
> supports F_PROTOCOL_FEATURES, it is supposed to initialize all vrings in
> a disabled state, so that when the flag is indeed negotiated, that will
> be the state they’re in.  In contrast, older back-ends that don’t
> support that flag will initialize them in an enabled state (because they
> won’t have support for disabled vrings).
>
> The statement was intended to make it clear that this difference in
> behavior is OK, and that the front-end must not rely on either of the
> two.  Only after SET_FEATURES will and must the state be well-defined.
>
> But if you find it just confusing because enabled/disabled has no
> meaning before a virt queue is started anyway, and they mustn’t be
> started before negotiating features, I’m happy to drop it without
> replacement.

Yes, dropping this statement sounds good.

Stefan