| Message ID | 20210325144846.17520-1-hi@alyssa.is (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] docs: clarify absence of set_features in vhost-user | expand |
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
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
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
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``. ``VHOST_USER_SET_OWNER`` :id: 3
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/> 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(-)