1. Patchew
  2. QEMU
  3. View series

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

Alyssa Ross posted 1 patch 3 years, 1 month ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210325144846.17520-1-hi@alyssa.is
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
docs/interop/vhost-user.rst | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
[PATCH RESEND] docs: clarify absence of set_features in vhost-user
Posted by Alyssa Ross 3 years, 1 month ago 
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(-)

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
-- 
2.30.0
Re: [PATCH RESEND] docs: clarify absence of set_features in vhost-user
Posted by Alex Bennée 3 years ago 
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
Re: [PATCH RESEND] docs: clarify absence of set_features in vhost-user
Posted by Alex Bennée 2 years, 10 months ago 
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

-- 
Alex Bennée
Re: [PATCH RESEND] docs: clarify absence of set_features in vhost-user
Posted by Stefan Hajnoczi 2 years, 10 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.