Re: [PATCH RESEND] docs: clarify absence of set_features in vhost-user

[Stefan Hajnoczi]

On Thu, Jun 17, 2021 at 04:19:26PM +0100, Alex Bennée wrote:
> 
> Alex Bennée <alex.bennee@linaro.org> writes:
> 
> > Alyssa Ross <hi@alyssa.is> writes:
> >
> >> The previous wording was (at least to me) ambiguous about whether a
> >> backend should enable features immediately after they were set using
> >> VHOST_USER_SET_PROTOCOL_FEATURES, or wait for support for protocol
> >> features to be acknowledged if it hasn't been yet before enabling
> >> those features.
> >>
> >> This patch attempts to make it clearer that
> >> VHOST_USER_SET_PROTOCOL_FEATURES should immediately enable features,
> >> even if support for protocol features has not yet been acknowledged,
> >> while still also making clear that the frontend SHOULD acknowledge
> >> support for protocol features.
> >>
> >> Previous discussion begins here:
> >> <https://lore.kernel.org/qemu-devel/87sgd1ktx9.fsf@alyssa.is/>
> >
> > I totally missed this when I posted a similar attempt at clarification:
> >
> >   Subject: [PATCH v2] vhost-user.rst: add clarifying language about 
> > protocol negotiation
> >   Date: Wed,  3 Mar 2021 14:50:11 +0000
> >   Message-Id: <20210303145011.14547-1-alex.bennee@linaro.org>
> >
> >>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Alyssa Ross <hi@alyssa.is>
> >> ---
> >>  docs/interop/vhost-user.rst | 14 +++++++++-----
> >>  1 file changed, 9 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/docs/interop/vhost-user.rst b/docs/interop/vhost-user.rst
> >> index d6085f7045..c42150331d 100644
> >> --- a/docs/interop/vhost-user.rst
> >> +++ b/docs/interop/vhost-user.rst
> >> @@ -871,9 +871,9 @@ Master message types
> >>    ``VHOST_USER_GET_FEATURES``.
> >>  
> >>  .. Note::
> >> -   Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must
> >> -   support this message even before ``VHOST_USER_SET_FEATURES`` was
> >> -   called.
> >> +   While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a
> >> +   backend must allow ``VHOST_USER_GET_PROTOCOL_FEATURES`` even if
> >> +   ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet.
> >>  
> >>  ``VHOST_USER_SET_PROTOCOL_FEATURES``
> >>    :id: 16
> >> @@ -886,8 +886,12 @@ Master message types
> >>    ``VHOST_USER_GET_FEATURES``.
> >>  
> >>  .. Note::
> >> -   Slave that reported ``VHOST_USER_F_PROTOCOL_FEATURES`` must support
> >> -   this message even before ``VHOST_USER_SET_FEATURES`` was called.
> >> +   While QEMU should acknowledge ``VHOST_USER_F_PROTOCOL_FEATURES``, a
> >> +   backend must allow ``VHOST_USER_SET_PROTOCOL_FEATURES`` even if
> >> +   ``VHOST_USER_F_PROTOCOL_FEATURES`` has not been acknowledged yet.
> >> +   The backend must not wait for ``VHOST_USER_SET_FEATURES`` before
> >> +   enabling protocol features requested with
> >> +   ``VHOST_USER_SET_PROTOCOL_FEATURES``.
> >
> > I think this is perfectly fine clarification although I think there
> > might be a patch in flight which changes some of the master slave
> > terminology so with that resolved:
> >
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > However there is still the edge case of what happens after the vhost
> > pair have negotiated protocol features like REPLY_ACK should
> > VHOST_USER_F_PROTOCOL_FEATURES still be acknowledged by
> > VHOST_USER_SET_FEATURES.
> >
> > Stefan's proposed wording which I incorporated in my patch made it clear
> > that VHOST_USER_F_PROTOCOL_FEATURES is never exposed to the guest by the
> > VMM due to it's UNUSED status. I would just like it explicit if it needs
> > to be preserved between the two sides of the VHOST_USER_*_FEATURES for
> > the negotiated protocol features to remain valid.
> >
> > Perhaps you could incorporate some wording for that?
> >
> >>  
> >>  ``VHOST_USER_SET_OWNER``
> >>    :id: 3
> 
> General ping to the vhost-user spec maintainers. This was also mentioned
> while merging:
> 
>   https://github.com/rust-vmm/vhost/pull/24

Alyssa or Alex: Please send another revision rebased on qemu.git/master.
Michael Tsirkin is the vhost-user.rst maintainer but I can help with
reviewing an updated patch.

Stefan

signature.asc
Description: PGP signature