On Wed, 19 Jul 2023 at 09:34, Hanna Czenczek <hre...@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 <hre...@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
