[virtio-dev] [PATCH] virtio: clarify feature negotiation

[virtio-dev] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

We state that drivers can access config fields before FEATURES_OK. We do
however need them to write the features before accessing config
otherwise our forward compatibility won't work.

We require devices to allow access to config space if they
offer a feature bit as long as that has been offered, but
this can't work of course since we don't know what value
does driver expect. What we should have said is "as long
as it has been acknowledged".

While at it, clarify that drivers can write features
repeatedly as long as FEATURES_OK have note been
acknowledged.

Note: if device denies FEATURES_OK then there's no reason
not to allow the driver to try with another set of features.
Current drivers do not need such a capability, so leave this
idea for another day.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/content.tex b/content.tex
index 8439cc1..4f46ce9 100644
--- a/content.tex
+++ b/content.tex
@@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
 \end{note}
 
 \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
-The device MUST allow reading of any device-specific configuration
+The device SHOULD allow reading of any device-specific configuration
 field before FEATURES_OK is set by the driver.  This includes fields which are
-conditional on feature bits, as long as those feature bits are offered
-by the device.
+conditional on feature bits, as long as those feature bits have
+been acknowledged by the driver.
 
 \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
 
@@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
 
 \item\label{itm:General Initialization And Device Operation /
 Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
-   understood by the OS and driver to the device.  During this step the
-   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
+   understood by the OS and driver to the device.  During this
+   step, after writing the subset of feature bits to the device the
+   driver MAY read (but MUST NOT write) the device-specific
+   configuration fields to validate that it can support the device
+   before accepting it. The driver MAY then repeatedly modify the
+   subset as appropriate, write the new subset and repeat the
+   validation, any number of times.
 
 \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
    new feature bits after this step.
@@ -3296,6 +3301,12 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
 \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
 the \field{status}.
 
+If the device offers VIRTIO_NET_F_MTU, the device SHOULD allow
+reading of \field{mtu} before FEATURES_OK is set by the driver
+even if VIRTIO_NET_F_MTU has not been acknowledged by the driver.
+This is to support existing drivers which access this field
+before acknowledging features.
+
 \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
 
 A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
-- 
MST


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> We state that drivers can access config fields before FEATURES_OK. We do
> however need them to write the features before accessing config
> otherwise our forward compatibility won't work.
>
> We require devices to allow access to config space if they
> offer a feature bit as long as that has been offered, but
> this can't work of course since we don't know what value
> does driver expect. What we should have said is "as long
> as it has been acknowledged".
>
> While at it, clarify that drivers can write features
> repeatedly as long as FEATURES_OK have note been
> acknowledged.
>
> Note: if device denies FEATURES_OK then there's no reason
> not to allow the driver to try with another set of features.
> Current drivers do not need such a capability, so leave this
> idea for another day.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  content.tex | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 8439cc1..4f46ce9 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
>  \end{note}
>  
>  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> -The device MUST allow reading of any device-specific configuration
> +The device SHOULD allow reading of any device-specific configuration

Why 'SHOULD'? I think the device MUST allow it for features that have
already been written by the driver, given that is always a subset of the
features that have been offered by the device.

Maybe "The device MUST allow reading of any device-specific
configuration field that is not depending on a feature bit, and any
device-specific configuration field that is conditional on a feature bit
that has been written back by the driver, before FEATURES_OK is set by
the driver. It MAY allow reading of any other configuration field."

>  field before FEATURES_OK is set by the driver.  This includes fields which are
> -conditional on feature bits, as long as those feature bits are offered
> -by the device.
> +conditional on feature bits, as long as those feature bits have
> +been acknowledged by the driver.

Better 'written back'? To me, 'acknowledged' carries overtones of
'negotiated', and that is not meaningful before FEATURES_OK.

>  
>  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
>  
> @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>  
>  \item\label{itm:General Initialization And Device Operation /
>  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> -   understood by the OS and driver to the device.  During this step the
> -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> +   understood by the OS and driver to the device.  During this
> +   step, after writing the subset of feature bits to the device the
> +   driver MAY read (but MUST NOT write) the device-specific
> +   configuration fields to validate that it can support the device
> +   before accepting it. The driver MAY then repeatedly modify the
> +   subset as appropriate, write the new subset and repeat the
> +   validation, any number of times.
>  
>  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
>     new feature bits after this step.
> @@ -3296,6 +3301,12 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
>  the \field{status}.
>  
> +If the device offers VIRTIO_NET_F_MTU, the device SHOULD allow
> +reading of \field{mtu} before FEATURES_OK is set by the driver
> +even if VIRTIO_NET_F_MTU has not been acknowledged by the driver.
> +This is to support existing drivers which access this field
> +before acknowledging features.

Maybe better 'written back', see my comment above.

Also note this in the patch description?


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
> On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > We state that drivers can access config fields before FEATURES_OK. We do
> > however need them to write the features before accessing config
> > otherwise our forward compatibility won't work.
> >
> > We require devices to allow access to config space if they
> > offer a feature bit as long as that has been offered, but
> > this can't work of course since we don't know what value
> > does driver expect. What we should have said is "as long
> > as it has been acknowledged".
> >
> > While at it, clarify that drivers can write features
> > repeatedly as long as FEATURES_OK have note been
> > acknowledged.
> >
> > Note: if device denies FEATURES_OK then there's no reason
> > not to allow the driver to try with another set of features.
> > Current drivers do not need such a capability, so leave this
> > idea for another day.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  content.tex | 21 ++++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 8439cc1..4f46ce9 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
> >  \end{note}
> >  
> >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> > -The device MUST allow reading of any device-specific configuration
> > +The device SHOULD allow reading of any device-specific configuration
> 
> Why 'SHOULD'? I think the device MUST allow it for features that have
> already been written by the driver, given that is always a subset of the
> features that have been offered by the device.
> 
> Maybe "The device MUST allow reading of any device-specific
> configuration field that is not depending on a feature bit, and any
> device-specific configuration field that is conditional on a feature bit
> that has been written back by the driver, before FEATURES_OK is set by
> the driver. It MAY allow reading of any other configuration field."


Like that.

> >  field before FEATURES_OK is set by the driver.  This includes fields which are
> > -conditional on feature bits, as long as those feature bits are offered
> > -by the device.
> > +conditional on feature bits, as long as those feature bits have
> > +been acknowledged by the driver.
> 
> Better 'written back'? To me, 'acknowledged' carries overtones of
> 'negotiated', and that is not meaningful before FEATURES_OK.

OK.
In that case we should also change this:

Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
   understood by the OS and driver to the device.

from "write" to "write back"




> >  
> >  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
> >  
> > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
> >  
> >  \item\label{itm:General Initialization And Device Operation /
> >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> > -   understood by the OS and driver to the device.  During this step the
> > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > +   understood by the OS and driver to the device.  During this
> > +   step, after writing the subset of feature bits to the device the
> > +   driver MAY read (but MUST NOT write) the device-specific
> > +   configuration fields to validate that it can support the device
> > +   before accepting it. The driver MAY then repeatedly modify the
> > +   subset as appropriate, write the new subset and repeat the
> > +   validation, any number of times.
> >  
> >  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
> >     new feature bits after this step.
> > @@ -3296,6 +3301,12 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
> >  \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
> >  the \field{status}.
> >  
> > +If the device offers VIRTIO_NET_F_MTU, the device SHOULD allow
> > +reading of \field{mtu} before FEATURES_OK is set by the driver
> > +even if VIRTIO_NET_F_MTU has not been acknowledged by the driver.
> > +This is to support existing drivers which access this field
> > +before acknowledging features.
> 
> Maybe better 'written back', see my comment above.
> 
> Also note this in the patch description?

thanks

-- 
MST

[virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Fri, 14 Jan 2022 17:50:51 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Subject: virtio: clarify feature negotiation

This is in my opinion more of an incompatible change that a
clarification, but since the commit message does not constitute a part
of the standard, it does not matter that much.
[..]

> @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
>  \end{note}
>  
>  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> -The device MUST allow reading of any device-specific configuration
> +The device SHOULD allow reading of any device-specific configuration
>  field before FEATURES_OK is set by the driver.  This includes fields which are
> -conditional on feature bits, as long as those feature bits are offered
> -by the device.
> +conditional on feature bits, as long as those feature bits have
> +been acknowledged by the driver.

"Those feature bits have been acknowledged by the driver" is new
terminology. IMHO we should have a precise definition
of "feature bit is acknowledged".

Currently "acknowledge features" means "write features".


>  
>  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
>  
> @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>  
>  \item\label{itm:General Initialization And Device Operation /
>  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> -   understood by the OS and driver to the device.  During this step the
> -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> +   understood by the OS and driver to the device.  During this
> +   step, after writing the subset of feature bits to the device the
> +   driver MAY read (but MUST NOT write) the device-specific
> +   configuration fields to validate that it can support the device
> +   before accepting it. The driver MAY then repeatedly modify the
> +   subset as appropriate, write the new subset and repeat the
> +   validation, any number of times.

IMHO don't have an exact definition of what "MAY read device-specific
configuration field means". To me, "may read" does not automatically
imply that what was read can be interpreted, is valid and meaningful.

For example there was no harm in reading MTU without having the
F_VERSION_1 feature acknowledged before. We did not end up in undefined
behavior land, the read_config operation did not blow up in our face,
and the field was also "there" if the corresponding feature was offered.
The only trouble was that one could not interpret the value of the field,
because one could not tell is it big or little endian.

IMHO, we want to accomplish, readable, interpretible, valid and
meaningful, before the feature has been negotiated by setting
FEATURES_OK.

I see a couple of potential problems with this:
1) The set of acknowledged features may be invalid! For example may end
up with feature B acknowledged and feature B not acknowledged, when
B depends on A. Before "acknowledge" we used to be safe, because the
device would reject FEATURES_OK.
2) In theory, we could have union-type conditional fields. To demonstrate
what I mean here is a stupid example:
struct ThermometherConfig {
union {
u16 temp_in_kelvin;
u16 temp_in_celsius;
u16 temp_in_farenheit;
} temp;
};
where kelvin is the default, but you can get Celsius or Fahrenheit
depending on mutex feature bits.
This also used to be and still is safe after the features have been
negotiated, but with "acknowledged" there is trouble when both the
features are set.
3) Features are written in 32 bit chunks. My guess is that what we
want for "features acknowledged is": driver has written each chunk it
knows about at least once. Of course, the question arises, what should
the device do if config is read before that happens.
4) It is pretty obvious to me, that after "features have been
acknowledged", we want the driver to read the very same config as
if we were in "features have been negotiated" state. The only  difference
we want is, that the features must not be "frozen", i.e. the driver
should still be able to modify the features.
5) It ain't clear to me when is the driver supposed to assume that
"the features have been acknowledged". I.e. that the expected changes
have taken effect. For PCI, as far as I remember, they have this write it
and then read it back drill to force side-effects. Would we have to do
something like this. I think for FEATURES_OK we have that stuff clearly
specified.

I don't know if that would be viable, but introducing a FEATURES_ACK
status bit would IMHO resolve a bunch of these potential problems, and
would also serve as a clear trigger for stuff like vhost (to flush
features)

The only problem, that would remain is that IMHO this is still
fundamentally a change that effectively breaks compatibility. IMHO
"features acknowledged" is essentially a new feature, which enables
a sane use of the config space before FEATURES_OK. But we can't guard it
with a feature bit, because the snake would just bite its tail. I'm
fine with going down this route. It is just that I would like to be
completely transparent about this.

Please also see my other mail!

Thank you  for tackling this! I highly appreciate it!

Regards,
Halil

[..]

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Mon, 17 Jan 2022 17:26:10 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
> > On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > We state that drivers can access config fields before FEATURES_OK. We do
> > > however need them to write the features before accessing config
> > > otherwise our forward compatibility won't work.
> > >
> > > We require devices to allow access to config space if they
> > > offer a feature bit as long as that has been offered, but
> > > this can't work of course since we don't know what value
> > > does driver expect. What we should have said is "as long
> > > as it has been acknowledged".
> > >
> > > While at it, clarify that drivers can write features
> > > repeatedly as long as FEATURES_OK have note been
> > > acknowledged.
> > >
> > > Note: if device denies FEATURES_OK then there's no reason
> > > not to allow the driver to try with another set of features.
> > > Current drivers do not need such a capability, so leave this
> > > idea for another day.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  content.tex | 21 ++++++++++++++++-----
> > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 8439cc1..4f46ce9 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
> > >  \end{note}
> > >  
> > >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> > > -The device MUST allow reading of any device-specific configuration
> > > +The device SHOULD allow reading of any device-specific configuration  
> > 
> > Why 'SHOULD'? I think the device MUST allow it for features that have
> > already been written by the driver, given that is always a subset of the
> > features that have been offered by the device.
> > 
> > Maybe "The device MUST allow reading of any device-specific
> > configuration field that is not depending on a feature bit, and any
> > device-specific configuration field that is conditional on a feature bit
> > that has been written back by the driver, before FEATURES_OK is set by
> > the driver. It MAY allow reading of any other configuration field."  
> 
> 
> Like that.
> 

I like Michaels original better for the following reason. To me, it is
much clearer, that one first SHOULD to do this new "acknowledge" step,
and only than is allowed to read the device-specific config. The
device-specific config in the most general consists of fields that are
conditional on feature bits, and fields that are not conditional but
always provided. IMHO Connie's version can be read as: the unconditional
ones you can read even before "acknowledging some of the features
offered", but for the conditional fields you have to do the
"acknowledge" first.

> > >  field before FEATURES_OK is set by the driver.  This includes fields which are
> > > -conditional on feature bits, as long as those feature bits are offered
> > > -by the device.
> > > +conditional on feature bits, as long as those feature bits have
> > > +been acknowledged by the driver.  
> > 
> > Better 'written back'? To me, 'acknowledged' carries overtones of
> > 'negotiated', and that is not meaningful before FEATURES_OK.  
> 
> OK.
> In that case we should also change this:
> 
> Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
>    understood by the OS and driver to the device.
> 
> from "write" to "write back"
> 

I believe we should define "acknowledge features" once, and then just
use the term consistently.

Please see also my other mail.

Regards,
Halil

Regards,
Halil
> 
> 
> 
> > >  
> > >  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
> > >  
> > > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
> > >  
> > >  \item\label{itm:General Initialization And Device Operation /
> > >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> > > -   understood by the OS and driver to the device.  During this step the
> > > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > > +   understood by the OS and driver to the device.  During this
> > > +   step, after writing the subset of feature bits to the device the
> > > +   driver MAY read (but MUST NOT write) the device-specific
> > > +   configuration fields to validate that it can support the device
> > > +   before accepting it. The driver MAY then repeatedly modify the
> > > +   subset as appropriate, write the new subset and repeat the
> > > +   validation, any number of times.
> > >  
> > >  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
> > >     new feature bits after this step.
> > > @@ -3296,6 +3301,12 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
> > >  \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
> > >  the \field{status}.
> > >  
> > > +If the device offers VIRTIO_NET_F_MTU, the device SHOULD allow
> > > +reading of \field{mtu} before FEATURES_OK is set by the driver
> > > +even if VIRTIO_NET_F_MTU has not been acknowledged by the driver.
> > > +This is to support existing drivers which access this field
> > > +before acknowledging features.  
> > 
> > Maybe better 'written back', see my comment above.
> > 
> > Also note this in the patch description?  
> 
> thanks
> 

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Wed, Jan 19, 2022 at 01:23:43PM +0100, Halil Pasic wrote:
> On Mon, 17 Jan 2022 17:26:10 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
> > > On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > We state that drivers can access config fields before FEATURES_OK. We do
> > > > however need them to write the features before accessing config
> > > > otherwise our forward compatibility won't work.
> > > >
> > > > We require devices to allow access to config space if they
> > > > offer a feature bit as long as that has been offered, but
> > > > this can't work of course since we don't know what value
> > > > does driver expect. What we should have said is "as long
> > > > as it has been acknowledged".
> > > >
> > > > While at it, clarify that drivers can write features
> > > > repeatedly as long as FEATURES_OK have note been
> > > > acknowledged.
> > > >
> > > > Note: if device denies FEATURES_OK then there's no reason
> > > > not to allow the driver to try with another set of features.
> > > > Current drivers do not need such a capability, so leave this
> > > > idea for another day.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  content.tex | 21 ++++++++++++++++-----
> > > >  1 file changed, 16 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/content.tex b/content.tex
> > > > index 8439cc1..4f46ce9 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
> > > >  \end{note}
> > > >  
> > > >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> > > > -The device MUST allow reading of any device-specific configuration
> > > > +The device SHOULD allow reading of any device-specific configuration  
> > > 
> > > Why 'SHOULD'? I think the device MUST allow it for features that have
> > > already been written by the driver, given that is always a subset of the
> > > features that have been offered by the device.
> > > 
> > > Maybe "The device MUST allow reading of any device-specific
> > > configuration field that is not depending on a feature bit, and any
> > > device-specific configuration field that is conditional on a feature bit
> > > that has been written back by the driver, before FEATURES_OK is set by
> > > the driver. It MAY allow reading of any other configuration field."  
> > 
> > 
> > Like that.
> > 
> 
> I like Michaels original better for the following reason. To me, it is
> much clearer, that one first SHOULD to do this new "acknowledge" step,
> and only than is allowed to read the device-specific config. The
> device-specific config in the most general consists of fields that are
> conditional on feature bits, and fields that are not conditional but
> always provided. IMHO Connie's version can be read as: the unconditional
> ones you can read even before "acknowledging some of the features
> offered", but for the conditional fields you have to do the
> "acknowledge" first.

I think I agree, and the problem with that is the transitional
devices with VERSION_1. If we always make drivers ack features
first, then devices can rely on VERSION_1 for all of config space.


> > > >  field before FEATURES_OK is set by the driver.  This includes fields which are
> > > > -conditional on feature bits, as long as those feature bits are offered
> > > > -by the device.
> > > > +conditional on feature bits, as long as those feature bits have
> > > > +been acknowledged by the driver.  
> > > 
> > > Better 'written back'? To me, 'acknowledged' carries overtones of
> > > 'negotiated', and that is not meaningful before FEATURES_OK.  
> > 
> > OK.
> > In that case we should also change this:
> > 
> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> >    understood by the OS and driver to the device.
> > 
> > from "write" to "write back"
> > 
> 
> I believe we should define "acknowledge features" once, and then just
> use the term consistently.

Right, and we use acknowledge features in this sense already.

E.g.
  If VIRTIO_CONSOLE_F_EMERG_WRITE is set then the driver can use emergency write
  to output a single character without initializing virtio queues, or even
  acknowledging the feature.

By the way this one is an exception to the "reads but not writes before
FEATURES_OK" rule ;) Might be a good idea to call this out here.

and

\item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
  features it understands, and feature negotiation is complete.

this meaning actually matches "acknowledge" just meaning "write".


I do however have a small problem with this terminology, in that
what happens if I set a bit and later clear it. I guess when
I set it I acknowledged it, and the feature is acknowledged. But what if I cleared it?
Did I unacknowledge it? Is it unacknowledged? I don't think it's a verb even.
And it's not terribly clear that the last action is what
matter and previous acknowledgements are ignored.
We can just say it has not been acknowledged but it's a bit confusing
in that the feature has been acknowledged, the acknowledgement just
has been withdrawn.


I am not sure I have a better idea for a term though, set and clear
are confusing since they do not relay the fact that both
device and driver have to set a bit. negotiated is already
used for after FEATURES_OK.

We could just add a paragraph explaining the terminology, I just
wish we could be more concise.


> Please see also my other mail.
> 
> Regards,
> Halil
> 
> Regards,
> Halil
> > 
> > 
> > 
> > > >  
> > > >  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
> > > >  
> > > > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
> > > >  
> > > >  \item\label{itm:General Initialization And Device Operation /
> > > >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> > > > -   understood by the OS and driver to the device.  During this step the
> > > > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > > > +   understood by the OS and driver to the device.  During this
> > > > +   step, after writing the subset of feature bits to the device the
> > > > +   driver MAY read (but MUST NOT write) the device-specific
> > > > +   configuration fields to validate that it can support the device
> > > > +   before accepting it. The driver MAY then repeatedly modify the
> > > > +   subset as appropriate, write the new subset and repeat the
> > > > +   validation, any number of times.
> > > >  
> > > >  \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
> > > >     new feature bits after this step.
> > > > @@ -3296,6 +3301,12 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
> > > >  \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
> > > >  the \field{status}.
> > > >  
> > > > +If the device offers VIRTIO_NET_F_MTU, the device SHOULD allow
> > > > +reading of \field{mtu} before FEATURES_OK is set by the driver
> > > > +even if VIRTIO_NET_F_MTU has not been acknowledged by the driver.
> > > > +This is to support existing drivers which access this field
> > > > +before acknowledging features.  
> > > 
> > > Maybe better 'written back', see my comment above.
> > > 
> > > Also note this in the patch description?  
> > 
> > thanks
> > 

[virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:
> On Fri, 14 Jan 2022 17:50:51 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Subject: virtio: clarify feature negotiation
> 
> This is in my opinion more of an incompatible change that a
> clarification, but since the commit message does not constitute a part
> of the standard, it does not matter that much.
> [..]
> 
> > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
> >  \end{note}
> >  
> >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> > -The device MUST allow reading of any device-specific configuration
> > +The device SHOULD allow reading of any device-specific configuration
> >  field before FEATURES_OK is set by the driver.  This includes fields which are
> > -conditional on feature bits, as long as those feature bits are offered
> > -by the device.
> > +conditional on feature bits, as long as those feature bits have
> > +been acknowledged by the driver.
> 
> "Those feature bits have been acknowledged by the driver" is new
> terminology. IMHO we should have a precise definition
> of "feature bit is acknowledged".
> 
> Currently "acknowledge features" means "write features".
> 
> 
> >  
> >  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
> >  
> > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
> >  
> >  \item\label{itm:General Initialization And Device Operation /
> >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> > -   understood by the OS and driver to the device.  During this step the
> > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > +   understood by the OS and driver to the device.  During this
> > +   step, after writing the subset of feature bits to the device the
> > +   driver MAY read (but MUST NOT write) the device-specific
> > +   configuration fields to validate that it can support the device
> > +   before accepting it. The driver MAY then repeatedly modify the
> > +   subset as appropriate, write the new subset and repeat the
> > +   validation, any number of times.
> 
> IMHO don't have an exact definition of what "MAY read device-specific
> configuration field means". To me, "may read" does not automatically
> imply that what was read can be interpreted, is valid and meaningful.

Let's say "MAY read (and use)" - is that clear enough?

> For example there was no harm in reading MTU without having the
> F_VERSION_1 feature acknowledged before. We did not end up in undefined
> behavior land, the read_config operation did not blow up in our face,
> and the field was also "there" if the corresponding feature was offered.
> The only trouble was that one could not interpret the value of the field,
> because one could not tell is it big or little endian.
> 
> IMHO, we want to accomplish, readable, interpretible, valid and
> meaningful, before the feature has been negotiated by setting
> FEATURES_OK.
> 
> I see a couple of potential problems with this:
> 1) The set of acknowledged features may be invalid! For example may end
> up with feature B acknowledged and feature B not acknowledged, when
> B depends on A. Before "acknowledge" we used to be safe, because the
> device would reject FEATURES_OK.

right. but for reading config fields it's probably enough in practice.

> 2) In theory, we could have union-type conditional fields. To demonstrate
> what I mean here is a stupid example:
> struct ThermometherConfig {
> union {
> u16 temp_in_kelvin;
> u16 temp_in_celsius;
> u16 temp_in_farenheit;
> } temp;
> };
> where kelvin is the default, but you can get Celsius or Fahrenheit
> depending on mutex feature bits.
> This also used to be and still is safe after the features have been
> negotiated, but with "acknowledged" there is trouble when both the
> features are set.

something to worry about in the future but are there cases like
this in present? should we worry?

> 3) Features are written in 32 bit chunks. My guess is that what we
> want for "features acknowledged is": driver has written each chunk it
> knows about at least once. Of course, the question arises, what should
> the device do if config is read before that happens.

we are lucky that at present it's just mtu of the net device :).


> 4) It is pretty obvious to me, that after "features have been
> acknowledged", we want the driver to read the very same config as
> if we were in "features have been negotiated" state. The only  difference
> we want is, that the features must not be "frozen", i.e. the driver
> should still be able to modify the features.

I can add that, ok.

> 5) It ain't clear to me when is the driver supposed to assume that
> "the features have been acknowledged". I.e. that the expected changes
> have taken effect. For PCI, as far as I remember, they have this write it
> and then read it back drill to force side-effects. Would we have to do
> something like this. I think for FEATURES_OK we have that stuff clearly
> specified.

Hmm good point.  normally reads do not bypass writes, so you can write
features and then read config, should work.  only exception is when they
are different types e.g.  in theory features could be in IO and config
in memory.  however I think reads from different types of memory can be
reordered anyway, so that config isn't really legal.  I'll poke at the
pci spec some more to refresh my memory on this. 

> I don't know if that would be viable, but introducing a FEATURES_ACK
> status bit would IMHO resolve a bunch of these potential problems, and
> would also serve as a clear trigger for stuff like vhost (to flush
> features)

Right. With this proposal I think basically a config read
flushes features. Which isn't too bad ...

> The only problem, that would remain is that IMHO this is still
> fundamentally a change that effectively breaks compatibility. IMHO
> "features acknowledged" is essentially a new feature, which enables
> a sane use of the config space before FEATURES_OK. But we can't guard it
> with a feature bit, because the snake would just bite its tail. I'm
> fine with going down this route. It is just that I would like to be
> completely transparent about this.

well kind of. But I think the nice thing about it and the reason
I structured it this way and not as a complete new status bit
is that it really matches pretty well what the existing
body of code does, e.g. we don't need any qemu changes at all,
and most linux and windows drivers except for network with
the MTU thing are also good to go.

> Please also see my other mail!
> 
> Thank you  for tackling this! I highly appreciate it!
> 
> Regards,
> Halil
> 
> [..]


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio] Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 19, 2022 at 01:23:43PM +0100, Halil Pasic wrote:
>> On Mon, 17 Jan 2022 17:26:10 -0500
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
>> > > On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > >   
>> > > > We state that drivers can access config fields before FEATURES_OK. We do
>> > > > however need them to write the features before accessing config
>> > > > otherwise our forward compatibility won't work.
>> > > >
>> > > > We require devices to allow access to config space if they
>> > > > offer a feature bit as long as that has been offered, but
>> > > > this can't work of course since we don't know what value
>> > > > does driver expect. What we should have said is "as long
>> > > > as it has been acknowledged".
>> > > >
>> > > > While at it, clarify that drivers can write features
>> > > > repeatedly as long as FEATURES_OK have note been
>> > > > acknowledged.
>> > > >
>> > > > Note: if device denies FEATURES_OK then there's no reason
>> > > > not to allow the driver to try with another set of features.
>> > > > Current drivers do not need such a capability, so leave this
>> > > > idea for another day.
>> > > >
>> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > > > ---
>> > > >  content.tex | 21 ++++++++++++++++-----
>> > > >  1 file changed, 16 insertions(+), 5 deletions(-)
>> > > >
>> > > > diff --git a/content.tex b/content.tex
>> > > > index 8439cc1..4f46ce9 100644
>> > > > --- a/content.tex
>> > > > +++ b/content.tex
>> > > > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
>> > > >  \end{note}
>> > > >  
>> > > >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
>> > > > -The device MUST allow reading of any device-specific configuration
>> > > > +The device SHOULD allow reading of any device-specific configuration  
>> > > 
>> > > Why 'SHOULD'? I think the device MUST allow it for features that have
>> > > already been written by the driver, given that is always a subset of the
>> > > features that have been offered by the device.
>> > > 
>> > > Maybe "The device MUST allow reading of any device-specific
>> > > configuration field that is not depending on a feature bit, and any
>> > > device-specific configuration field that is conditional on a feature bit
>> > > that has been written back by the driver, before FEATURES_OK is set by
>> > > the driver. It MAY allow reading of any other configuration field."  
>> > 
>> > 
>> > Like that.
>> > 
>> 
>> I like Michaels original better for the following reason. To me, it is
>> much clearer, that one first SHOULD to do this new "acknowledge" step,
>> and only than is allowed to read the device-specific config. The
>> device-specific config in the most general consists of fields that are
>> conditional on feature bits, and fields that are not conditional but
>> always provided. IMHO Connie's version can be read as: the unconditional
>> ones you can read even before "acknowledging some of the features
>> offered", but for the conditional fields you have to do the
>> "acknowledge" first.
>
> I think I agree, and the problem with that is the transitional
> devices with VERSION_1. If we always make drivers ack features
> first, then devices can rely on VERSION_1 for all of config space.

Do we have any bit other than VERSION_1 that changes the layout for
non-optional config space fields?

>
>
>> > > >  field before FEATURES_OK is set by the driver.  This includes fields which are
>> > > > -conditional on feature bits, as long as those feature bits are offered
>> > > > -by the device.
>> > > > +conditional on feature bits, as long as those feature bits have
>> > > > +been acknowledged by the driver.  
>> > > 
>> > > Better 'written back'? To me, 'acknowledged' carries overtones of
>> > > 'negotiated', and that is not meaningful before FEATURES_OK.  
>> > 
>> > OK.
>> > In that case we should also change this:
>> > 
>> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
>> >    understood by the OS and driver to the device.
>> > 
>> > from "write" to "write back"
>> > 
>> 
>> I believe we should define "acknowledge features" once, and then just
>> use the term consistently.
>
> Right, and we use acknowledge features in this sense already.
>
> E.g.
>   If VIRTIO_CONSOLE_F_EMERG_WRITE is set then the driver can use emergency write
>   to output a single character without initializing virtio queues, or even
>   acknowledging the feature.
>
> By the way this one is an exception to the "reads but not writes before
> FEATURES_OK" rule ;) Might be a good idea to call this out here.

Yes, that's a really odd one. But if we take "acknowledging the feature"
to mean "the driver has written the feature bit, and set FEATURES_OK",
it still makes sense, as it is the only time we allow a config space
write before FEATURES_OK.

> and
>
> \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
>   features it understands, and feature negotiation is complete.
>
> this meaning actually matches "acknowledge" just meaning "write".

Hm, I would rather interpret the act of "setting FEATURES_OK" as
"acknowledging the features it wrote to the device", as in not only "I
know those features, and can use them in principle", but in "this is
actually a feature combination I can use".

>
>
> I do however have a small problem with this terminology, in that
> what happens if I set a bit and later clear it. I guess when
> I set it I acknowledged it, and the feature is acknowledged. But what if I cleared it?
> Did I unacknowledge it? Is it unacknowledged? I don't think it's a verb even.
> And it's not terribly clear that the last action is what
> matter and previous acknowledgements are ignored.
> We can just say it has not been acknowledged but it's a bit confusing
> in that the feature has been acknowledged, the acknowledgement just
> has been withdrawn.
>
>
> I am not sure I have a better idea for a term though, set and clear
> are confusing since they do not relay the fact that both
> device and driver have to set a bit. negotiated is already
> used for after FEATURES_OK.
>

My personal understanding of the terminology is:
- acknowledged: the driver has written a feature to the device, and set
  FEATURES_OK
- negotiated: the bit is acknowledged, and the device has actually
  accepted FEATURES_OK

Generally, "acknowledging" a bit does not really carry more weight than
simply writing the bit to the device, the big distinction is whether the
device accepted FEATURES_OK.

So, the things that really matter are, I think,
- whether the driver has written VERSION_1, which means that the config
  space is supposed to be litte-endian
- whether the driver has written any bit that governs an optional field,
  which means that the driver may read that field
- that weird special case above
- whether the device has accepted FEATURES_OK, which means that the
  config space layout is fixed and the driver may write to it

That still leaves the problem to find a good term for clearing the bit
again... unless we simply talk about the driver setting and clearing it
(but how do we express that it needs to make the device aware of it?)

The other big problem is of course legacy... the spec talks of
"acknowledging" there, but in the legacy case, it means simply that the
driver wrote the bit, as we do not really have an explicit point where
the bit has actually been negotiated, it's only implicit. I think
phrasing matters for transitional devices, VIRTIO_1, and config space
endianness here...

> We could just add a paragraph explaining the terminology, I just
> wish we could be more concise.

I'm unfortunately struggling to come up with good, waterproof, and
understandable terminology covering all cases :(


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Wed, Jan 19, 2022 at 06:50:18PM +0100, Cornelia Huck wrote:
> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 19, 2022 at 01:23:43PM +0100, Halil Pasic wrote:
> >> On Mon, 17 Jan 2022 17:26:10 -0500
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
> >> > > On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > >   
> >> > > > We state that drivers can access config fields before FEATURES_OK. We do
> >> > > > however need them to write the features before accessing config
> >> > > > otherwise our forward compatibility won't work.
> >> > > >
> >> > > > We require devices to allow access to config space if they
> >> > > > offer a feature bit as long as that has been offered, but
> >> > > > this can't work of course since we don't know what value
> >> > > > does driver expect. What we should have said is "as long
> >> > > > as it has been acknowledged".
> >> > > >
> >> > > > While at it, clarify that drivers can write features
> >> > > > repeatedly as long as FEATURES_OK have note been
> >> > > > acknowledged.
> >> > > >
> >> > > > Note: if device denies FEATURES_OK then there's no reason
> >> > > > not to allow the driver to try with another set of features.
> >> > > > Current drivers do not need such a capability, so leave this
> >> > > > idea for another day.
> >> > > >
> >> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > > > ---
> >> > > >  content.tex | 21 ++++++++++++++++-----
> >> > > >  1 file changed, 16 insertions(+), 5 deletions(-)
> >> > > >
> >> > > > diff --git a/content.tex b/content.tex
> >> > > > index 8439cc1..4f46ce9 100644
> >> > > > --- a/content.tex
> >> > > > +++ b/content.tex
> >> > > > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
> >> > > >  \end{note}
> >> > > >  
> >> > > >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> >> > > > -The device MUST allow reading of any device-specific configuration
> >> > > > +The device SHOULD allow reading of any device-specific configuration  
> >> > > 
> >> > > Why 'SHOULD'? I think the device MUST allow it for features that have
> >> > > already been written by the driver, given that is always a subset of the
> >> > > features that have been offered by the device.
> >> > > 
> >> > > Maybe "The device MUST allow reading of any device-specific
> >> > > configuration field that is not depending on a feature bit, and any
> >> > > device-specific configuration field that is conditional on a feature bit
> >> > > that has been written back by the driver, before FEATURES_OK is set by
> >> > > the driver. It MAY allow reading of any other configuration field."  
> >> > 
> >> > 
> >> > Like that.
> >> > 
> >> 
> >> I like Michaels original better for the following reason. To me, it is
> >> much clearer, that one first SHOULD to do this new "acknowledge" step,
> >> and only than is allowed to read the device-specific config. The
> >> device-specific config in the most general consists of fields that are
> >> conditional on feature bits, and fields that are not conditional but
> >> always provided. IMHO Connie's version can be read as: the unconditional
> >> ones you can read even before "acknowledging some of the features
> >> offered", but for the conditional fields you have to do the
> >> "acknowledge" first.
> >
> > I think I agree, and the problem with that is the transitional
> > devices with VERSION_1. If we always make drivers ack features
> > first, then devices can rely on VERSION_1 for all of config space.
> 
> Do we have any bit other than VERSION_1 that changes the layout for
> non-optional config space fields?

Not ATM

> >
> >
> >> > > >  field before FEATURES_OK is set by the driver.  This includes fields which are
> >> > > > -conditional on feature bits, as long as those feature bits are offered
> >> > > > -by the device.
> >> > > > +conditional on feature bits, as long as those feature bits have
> >> > > > +been acknowledged by the driver.  
> >> > > 
> >> > > Better 'written back'? To me, 'acknowledged' carries overtones of
> >> > > 'negotiated', and that is not meaningful before FEATURES_OK.  
> >> > 
> >> > OK.
> >> > In that case we should also change this:
> >> > 
> >> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> >> >    understood by the OS and driver to the device.
> >> > 
> >> > from "write" to "write back"
> >> > 
> >> 
> >> I believe we should define "acknowledge features" once, and then just
> >> use the term consistently.
> >
> > Right, and we use acknowledge features in this sense already.
> >
> > E.g.
> >   If VIRTIO_CONSOLE_F_EMERG_WRITE is set then the driver can use emergency write
> >   to output a single character without initializing virtio queues, or even
> >   acknowledging the feature.
> >
> > By the way this one is an exception to the "reads but not writes before
> > FEATURES_OK" rule ;) Might be a good idea to call this out here.
> 
> Yes, that's a really odd one. But if we take "acknowledging the feature"
> to mean "the driver has written the feature bit, and set FEATURES_OK",
> it still makes sense, as it is the only time we allow a config space
> write before FEATURES_OK.


If you look at qemu it's actually the new meaning, not FEATURES_OK.
And it's actually useful like this - e.g. driver can oops
before FEATURES_OK.

> > and
> >
> > \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
> >   features it understands, and feature negotiation is complete.
> >
> > this meaning actually matches "acknowledge" just meaning "write".
> 
> Hm, I would rather interpret the act of "setting FEATURES_OK" as
> "acknowledging the features it wrote to the device", as in not only "I
> know those features, and can use them in principle", but in "this is
> actually a feature combination I can use".

that's the feature negotiation is complete part I think.


> >
> >
> > I do however have a small problem with this terminology, in that
> > what happens if I set a bit and later clear it. I guess when
> > I set it I acknowledged it, and the feature is acknowledged. But what if I cleared it?
> > Did I unacknowledge it? Is it unacknowledged? I don't think it's a verb even.
> > And it's not terribly clear that the last action is what
> > matter and previous acknowledgements are ignored.
> > We can just say it has not been acknowledged but it's a bit confusing
> > in that the feature has been acknowledged, the acknowledgement just
> > has been withdrawn.
> >
> >
> > I am not sure I have a better idea for a term though, set and clear
> > are confusing since they do not relay the fact that both
> > device and driver have to set a bit. negotiated is already
> > used for after FEATURES_OK.
> >
> 
> My personal understanding of the terminology is:
> - acknowledged: the driver has written a feature to the device, and set
>   FEATURES_OK
> - negotiated: the bit is acknowledged, and the device has actually
>   accepted FEATURES_OK
> 
> Generally, "acknowledging" a bit does not really carry more weight than
> simply writing the bit to the device, the big distinction is whether the
> device accepted FEATURES_OK.

Hmm I don't think this is the current reading of the spec. Do you?
acknowledged is really something we inherited from 0.X
and never changed, so it always meant "written to device as 1".
Nothing to do with FEATURES_OK.
1.0 added negotiated to deal with FEATURES_OK.

And I don't really like moving to this variant since acknowledged is then
a very short transient state. We would be burning a very useful
term on something that's barely there.


> So, the things that really matter are, I think,
> - whether the driver has written VERSION_1, which means that the config
>   space is supposed to be litte-endian
> - whether the driver has written any bit that governs an optional field,
>   which means that the driver may read that field
> - that weird special case above
> - whether the device has accepted FEATURES_OK, which means that the
>   config space layout is fixed and the driver may write to it
> 
> That still leaves the problem to find a good term for clearing the bit
> again... unless we simply talk about the driver setting and clearing it
> (but how do we express that it needs to make the device aware of it?)
> 
> The other big problem is of course legacy... the spec talks of
> "acknowledging" there, but in the legacy case, it means simply that the
> driver wrote the bit, as we do not really have an explicit point where
> the bit has actually been negotiated, it's only implicit. I think
> phrasing matters for transitional devices, VIRTIO_1, and config space
> endianness here...


So for all of the above reasons I think we are better off
with keeping the way acknowledging is currently understood in
spec - really the pre-1.0 meaning.

> > We could just add a paragraph explaining the terminology, I just
> > wish we could be more concise.
> 
> I'm unfortunately struggling to come up with good, waterproof, and
> understandable terminology covering all cases :(

I think if you recall what terminology used to be pre-1.0 things
will become clearer. And I think the requirement to allow
early config space access and the idea of detecting legacy
based on features all predate FEATURES_OK.

-- 
MST

[virtio-comment] Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 19, 2022 at 06:50:18PM +0100, Cornelia Huck wrote:
>> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Wed, Jan 19, 2022 at 01:23:43PM +0100, Halil Pasic wrote:
>> >> On Mon, 17 Jan 2022 17:26:10 -0500
>> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> 
>> >> > On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
>> >> > > On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> > > >  field before FEATURES_OK is set by the driver.  This includes fields which are
>> >> > > > -conditional on feature bits, as long as those feature bits are offered
>> >> > > > -by the device.
>> >> > > > +conditional on feature bits, as long as those feature bits have
>> >> > > > +been acknowledged by the driver.  
>> >> > > 
>> >> > > Better 'written back'? To me, 'acknowledged' carries overtones of
>> >> > > 'negotiated', and that is not meaningful before FEATURES_OK.  
>> >> > 
>> >> > OK.
>> >> > In that case we should also change this:
>> >> > 
>> >> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
>> >> >    understood by the OS and driver to the device.
>> >> > 
>> >> > from "write" to "write back"
>> >> > 
>> >> 
>> >> I believe we should define "acknowledge features" once, and then just
>> >> use the term consistently.
>> >
>> > Right, and we use acknowledge features in this sense already.
>> >
>> > E.g.
>> >   If VIRTIO_CONSOLE_F_EMERG_WRITE is set then the driver can use emergency write
>> >   to output a single character without initializing virtio queues, or even
>> >   acknowledging the feature.
>> >
>> > By the way this one is an exception to the "reads but not writes before
>> > FEATURES_OK" rule ;) Might be a good idea to call this out here.
>> 
>> Yes, that's a really odd one. But if we take "acknowledging the feature"
>> to mean "the driver has written the feature bit, and set FEATURES_OK",
>> it still makes sense, as it is the only time we allow a config space
>> write before FEATURES_OK.
>
>
> If you look at qemu it's actually the new meaning, not FEATURES_OK.
> And it's actually useful like this - e.g. driver can oops
> before FEATURES_OK.

I think actually either is fine, as it's some kind of negated
statement.

>
>> > and
>> >
>> > \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
>> >   features it understands, and feature negotiation is complete.
>> >
>> > this meaning actually matches "acknowledge" just meaning "write".
>> 
>> Hm, I would rather interpret the act of "setting FEATURES_OK" as
>> "acknowledging the features it wrote to the device", as in not only "I
>> know those features, and can use them in principle", but in "this is
>> actually a feature combination I can use".
>
> that's the feature negotiation is complete part I think.
>
>
>> >
>> >
>> > I do however have a small problem with this terminology, in that
>> > what happens if I set a bit and later clear it. I guess when
>> > I set it I acknowledged it, and the feature is acknowledged. But what if I cleared it?
>> > Did I unacknowledge it? Is it unacknowledged? I don't think it's a verb even.
>> > And it's not terribly clear that the last action is what
>> > matter and previous acknowledgements are ignored.
>> > We can just say it has not been acknowledged but it's a bit confusing
>> > in that the feature has been acknowledged, the acknowledgement just
>> > has been withdrawn.
>> >
>> >
>> > I am not sure I have a better idea for a term though, set and clear
>> > are confusing since they do not relay the fact that both
>> > device and driver have to set a bit. negotiated is already
>> > used for after FEATURES_OK.
>> >
>> 
>> My personal understanding of the terminology is:
>> - acknowledged: the driver has written a feature to the device, and set
>>   FEATURES_OK
>> - negotiated: the bit is acknowledged, and the device has actually
>>   accepted FEATURES_OK
>> 
>> Generally, "acknowledging" a bit does not really carry more weight than
>> simply writing the bit to the device, the big distinction is whether the
>> device accepted FEATURES_OK.
>
> Hmm I don't think this is the current reading of the spec. Do you?
> acknowledged is really something we inherited from 0.X
> and never changed, so it always meant "written to device as 1".
> Nothing to do with FEATURES_OK.
> 1.0 added negotiated to deal with FEATURES_OK.
>
> And I don't really like moving to this variant since acknowledged is then
> a very short transient state. We would be burning a very useful
> term on something that's barely there.

Hm... so it seems "acknowledged" is really legacy from, well, legacy,
and we should not go ahead and change its meaning.

>
>
>> So, the things that really matter are, I think,
>> - whether the driver has written VERSION_1, which means that the config
>>   space is supposed to be litte-endian
>> - whether the driver has written any bit that governs an optional field,
>>   which means that the driver may read that field
>> - that weird special case above
>> - whether the device has accepted FEATURES_OK, which means that the
>>   config space layout is fixed and the driver may write to it
>> 
>> That still leaves the problem to find a good term for clearing the bit
>> again... unless we simply talk about the driver setting and clearing it
>> (but how do we express that it needs to make the device aware of it?)
>> 
>> The other big problem is of course legacy... the spec talks of
>> "acknowledging" there, but in the legacy case, it means simply that the
>> driver wrote the bit, as we do not really have an explicit point where
>> the bit has actually been negotiated, it's only implicit. I think
>> phrasing matters for transitional devices, VIRTIO_1, and config space
>> endianness here...
>
>
> So for all of the above reasons I think we are better off
> with keeping the way acknowledging is currently understood in
> spec - really the pre-1.0 meaning.

Yeah. We should probably spell it out, though.

"A feature bit is said to have been acknowledged if the driver has
written the bit to the device."

...which leads us back to the problem that "unacknowledge" is not really
a word (or at least not a well-defined one.)

Maybe we can use something like:

"A feature bit is said to have been unacknowledged if the driver has
cleared a previously acknowledged bit and written the resulting set of
feature bits to the device."

>
>> > We could just add a paragraph explaining the terminology, I just
>> > wish we could be more concise.
>> 
>> I'm unfortunately struggling to come up with good, waterproof, and
>> understandable terminology covering all cases :(
>
> I think if you recall what terminology used to be pre-1.0 things
> will become clearer. And I think the requirement to allow
> early config space access and the idea of detecting legacy
> based on features all predate FEATURES_OK.

It seems the "early config space access" thing only made it into the
spec after FEATURES_OK had already been introduced... but I think the
idea that VERSION_1 set denotes non-legacy was there from the start.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-dev] Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:
>> On Fri, 14 Jan 2022 17:50:51 -0500
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > Subject: virtio: clarify feature negotiation
>> > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
>> >  
>> >  \item\label{itm:General Initialization And Device Operation /
>> >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
>> > -   understood by the OS and driver to the device.  During this step the
>> > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
>> > +   understood by the OS and driver to the device.  During this
>> > +   step, after writing the subset of feature bits to the device the
>> > +   driver MAY read (but MUST NOT write) the device-specific
>> > +   configuration fields to validate that it can support the device
>> > +   before accepting it. The driver MAY then repeatedly modify the
>> > +   subset as appropriate, write the new subset and repeat the
>> > +   validation, any number of times.
>> 
>> IMHO don't have an exact definition of what "MAY read device-specific
>> configuration field means". To me, "may read" does not automatically
>> imply that what was read can be interpreted, is valid and meaningful.
>
> Let's say "MAY read (and use)" - is that clear enough?

That would work for me. We just need to make sure that the device
actually MUST present something valid if the feature bit is set
(acknowledged).

>
>> For example there was no harm in reading MTU without having the
>> F_VERSION_1 feature acknowledged before. We did not end up in undefined
>> behavior land, the read_config operation did not blow up in our face,
>> and the field was also "there" if the corresponding feature was offered.
>> The only trouble was that one could not interpret the value of the field,
>> because one could not tell is it big or little endian.

I think there's an elephant in the room: which bits can the device
actually assume to be set or unset?

My take would be:
- If the driver wrote a set of feature bits that contain MTU but not
  VERSION_1, the device MUST provide a valid mtu value in platform
  endian.
- If the driver wrote a set of feature bits that contain MTU and
  VERSION_1, the device MUST provide a valid mtu value in little
  endian.
- If the driver has not written the MTU bit, the device MAY provide
  anything (but it could make an educated guess and provide a valid mtu
  value in any endianness it thinks the driver might be expecting.)
  If that is not something that the driver can use, it's really the
  driver's problem for not setting the feature bit.

We just need to make sure stating that setting relevant feature bits is
needed if the driver expects to get anything useful back, especially if
you consider endianness. That problem just does not show up on
little-endian platforms.

>> 
>> IMHO, we want to accomplish, readable, interpretible, valid and
>> meaningful, before the feature has been negotiated by setting
>> FEATURES_OK.
>> 
>> I see a couple of potential problems with this:
>> 1) The set of acknowledged features may be invalid! For example may end
>> up with feature B acknowledged and feature B not acknowledged, when
>> B depends on A. Before "acknowledge" we used to be safe, because the
>> device would reject FEATURES_OK.
>
> right. but for reading config fields it's probably enough in practice.

It's probably ok to defer actually validating the bits to
FEATURES_OK. If an inconsistent set of feature bits leads to a
poorly-defined value, the device can really provide anything, and the
driver just has to deal with trying to do something that is not valid
anyway. But probably not a problem in practice, I agree.

>
>> 2) In theory, we could have union-type conditional fields. To demonstrate
>> what I mean here is a stupid example:
>> struct ThermometherConfig {
>> union {
>> u16 temp_in_kelvin;
>> u16 temp_in_celsius;
>> u16 temp_in_farenheit;
>> } temp;
>> };
>> where kelvin is the default, but you can get Celsius or Fahrenheit
>> depending on mutex feature bits.
>> This also used to be and still is safe after the features have been
>> negotiated, but with "acknowledged" there is trouble when both the
>> features are set.
>
> something to worry about in the future but are there cases like
> this in present? should we worry?

I would say that this is simply the driver triggering undefined
behaviour from the device.

>
>> 3) Features are written in 32 bit chunks. My guess is that what we
>> want for "features acknowledged is": driver has written each chunk it
>> knows about at least once. Of course, the question arises, what should
>> the device do if config is read before that happens.
>
> we are lucky that at present it's just mtu of the net device :).

If, say, the driver writes the first chunk with the MTU bit, then reads
the mtu config field, then writes the second chunk with VERSION_1, it
should get the mtu in platform-endian (and on the next read after the
second chunk has been written, in little endian.) I'd say that this is
simply an issue of the driver not serializing its own reads/writes
correctly :)

>
>
>> 4) It is pretty obvious to me, that after "features have been
>> acknowledged", we want the driver to read the very same config as
>> if we were in "features have been negotiated" state. The only  difference
>> we want is, that the features must not be "frozen", i.e. the driver
>> should still be able to modify the features.
>
> I can add that, ok.

Ack.

>
>> 5) It ain't clear to me when is the driver supposed to assume that
>> "the features have been acknowledged". I.e. that the expected changes
>> have taken effect. For PCI, as far as I remember, they have this write it
>> and then read it back drill to force side-effects. Would we have to do
>> something like this. I think for FEATURES_OK we have that stuff clearly
>> specified.
>
> Hmm good point.  normally reads do not bypass writes, so you can write
> features and then read config, should work.  only exception is when they
> are different types e.g.  in theory features could be in IO and config
> in memory.  however I think reads from different types of memory can be
> reordered anyway, so that config isn't really legal.  I'll poke at the
> pci spec some more to refresh my memory on this. 
>
>> I don't know if that would be viable, but introducing a FEATURES_ACK
>> status bit would IMHO resolve a bunch of these potential problems, and
>> would also serve as a clear trigger for stuff like vhost (to flush
>> features)
>
> Right. With this proposal I think basically a config read
> flushes features. Which isn't too bad ...
>
>> The only problem, that would remain is that IMHO this is still
>> fundamentally a change that effectively breaks compatibility. IMHO
>> "features acknowledged" is essentially a new feature, which enables
>> a sane use of the config space before FEATURES_OK. But we can't guard it
>> with a feature bit, because the snake would just bite its tail. I'm
>> fine with going down this route. It is just that I would like to be
>> completely transparent about this.
>
> well kind of. But I think the nice thing about it and the reason
> I structured it this way and not as a complete new status bit
> is that it really matches pretty well what the existing
> body of code does, e.g. we don't need any qemu changes at all,
> and most linux and windows drivers except for network with
> the MTU thing are also good to go.

Do we know anything about the state of e.g. hardware implementations?


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Thu, Jan 20, 2022 at 01:39:48PM +0100, Cornelia Huck wrote:
> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 19, 2022 at 06:50:18PM +0100, Cornelia Huck wrote:
> >> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > On Wed, Jan 19, 2022 at 01:23:43PM +0100, Halil Pasic wrote:
> >> >> On Mon, 17 Jan 2022 17:26:10 -0500
> >> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> 
> >> >> > On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
> >> >> > > On Fri, Jan 14 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> > > >  field before FEATURES_OK is set by the driver.  This includes fields which are
> >> >> > > > -conditional on feature bits, as long as those feature bits are offered
> >> >> > > > -by the device.
> >> >> > > > +conditional on feature bits, as long as those feature bits have
> >> >> > > > +been acknowledged by the driver.  
> >> >> > > 
> >> >> > > Better 'written back'? To me, 'acknowledged' carries overtones of
> >> >> > > 'negotiated', and that is not meaningful before FEATURES_OK.  
> >> >> > 
> >> >> > OK.
> >> >> > In that case we should also change this:
> >> >> > 
> >> >> > Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> >> >> >    understood by the OS and driver to the device.
> >> >> > 
> >> >> > from "write" to "write back"
> >> >> > 
> >> >> 
> >> >> I believe we should define "acknowledge features" once, and then just
> >> >> use the term consistently.
> >> >
> >> > Right, and we use acknowledge features in this sense already.
> >> >
> >> > E.g.
> >> >   If VIRTIO_CONSOLE_F_EMERG_WRITE is set then the driver can use emergency write
> >> >   to output a single character without initializing virtio queues, or even
> >> >   acknowledging the feature.
> >> >
> >> > By the way this one is an exception to the "reads but not writes before
> >> > FEATURES_OK" rule ;) Might be a good idea to call this out here.
> >> 
> >> Yes, that's a really odd one. But if we take "acknowledging the feature"
> >> to mean "the driver has written the feature bit, and set FEATURES_OK",
> >> it still makes sense, as it is the only time we allow a config space
> >> write before FEATURES_OK.
> >
> >
> > If you look at qemu it's actually the new meaning, not FEATURES_OK.
> > And it's actually useful like this - e.g. driver can oops
> > before FEATURES_OK.
> 
> I think actually either is fine, as it's some kind of negated
> statement.
> 
> >
> >> > and
> >> >
> >> > \item[FEATURES_OK (8)] Indicates that the driver has acknowledged all the
> >> >   features it understands, and feature negotiation is complete.
> >> >
> >> > this meaning actually matches "acknowledge" just meaning "write".
> >> 
> >> Hm, I would rather interpret the act of "setting FEATURES_OK" as
> >> "acknowledging the features it wrote to the device", as in not only "I
> >> know those features, and can use them in principle", but in "this is
> >> actually a feature combination I can use".
> >
> > that's the feature negotiation is complete part I think.
> >
> >
> >> >
> >> >
> >> > I do however have a small problem with this terminology, in that
> >> > what happens if I set a bit and later clear it. I guess when
> >> > I set it I acknowledged it, and the feature is acknowledged. But what if I cleared it?
> >> > Did I unacknowledge it? Is it unacknowledged? I don't think it's a verb even.
> >> > And it's not terribly clear that the last action is what
> >> > matter and previous acknowledgements are ignored.
> >> > We can just say it has not been acknowledged but it's a bit confusing
> >> > in that the feature has been acknowledged, the acknowledgement just
> >> > has been withdrawn.
> >> >
> >> >
> >> > I am not sure I have a better idea for a term though, set and clear
> >> > are confusing since they do not relay the fact that both
> >> > device and driver have to set a bit. negotiated is already
> >> > used for after FEATURES_OK.
> >> >
> >> 
> >> My personal understanding of the terminology is:
> >> - acknowledged: the driver has written a feature to the device, and set
> >>   FEATURES_OK
> >> - negotiated: the bit is acknowledged, and the device has actually
> >>   accepted FEATURES_OK
> >> 
> >> Generally, "acknowledging" a bit does not really carry more weight than
> >> simply writing the bit to the device, the big distinction is whether the
> >> device accepted FEATURES_OK.
> >
> > Hmm I don't think this is the current reading of the spec. Do you?
> > acknowledged is really something we inherited from 0.X
> > and never changed, so it always meant "written to device as 1".
> > Nothing to do with FEATURES_OK.
> > 1.0 added negotiated to deal with FEATURES_OK.
> >
> > And I don't really like moving to this variant since acknowledged is then
> > a very short transient state. We would be burning a very useful
> > term on something that's barely there.
> 
> Hm... so it seems "acknowledged" is really legacy from, well, legacy,
> and we should not go ahead and change its meaning.
> 
> >
> >
> >> So, the things that really matter are, I think,
> >> - whether the driver has written VERSION_1, which means that the config
> >>   space is supposed to be litte-endian
> >> - whether the driver has written any bit that governs an optional field,
> >>   which means that the driver may read that field
> >> - that weird special case above
> >> - whether the device has accepted FEATURES_OK, which means that the
> >>   config space layout is fixed and the driver may write to it
> >> 
> >> That still leaves the problem to find a good term for clearing the bit
> >> again... unless we simply talk about the driver setting and clearing it
> >> (but how do we express that it needs to make the device aware of it?)
> >> 
> >> The other big problem is of course legacy... the spec talks of
> >> "acknowledging" there, but in the legacy case, it means simply that the
> >> driver wrote the bit, as we do not really have an explicit point where
> >> the bit has actually been negotiated, it's only implicit. I think
> >> phrasing matters for transitional devices, VIRTIO_1, and config space
> >> endianness here...
> >
> >
> > So for all of the above reasons I think we are better off
> > with keeping the way acknowledging is currently understood in
> > spec - really the pre-1.0 meaning.
> 
> Yeah. We should probably spell it out, though.
> 
> "A feature bit is said to have been acknowledged if the driver has
> written the bit to the device."
> 
> ...which leads us back to the problem that "unacknowledge" is not really
> a word (or at least not a well-defined one.)
> 
> Maybe we can use something like:
> 
> "A feature bit is said to have been unacknowledged if the driver has
> cleared a previously acknowledged bit and written the resulting set of
> feature bits to the device."

I guess we can just say "not acknowledged". More verbose but oh well.

> >
> >> > We could just add a paragraph explaining the terminology, I just
> >> > wish we could be more concise.
> >> 
> >> I'm unfortunately struggling to come up with good, waterproof, and
> >> understandable terminology covering all cases :(
> >
> > I think if you recall what terminology used to be pre-1.0 things
> > will become clearer. And I think the requirement to allow
> > early config space access and the idea of detecting legacy
> > based on features all predate FEATURES_OK.
> 
> It seems the "early config space access" thing only made it into the
> spec after FEATURES_OK had already been introduced... but I think the
> idea that VERSION_1 set denotes non-legacy was there from the start.


Found more btw

Each virtio device offers all the features it understands.  During
device initialization, the driver reads this and tells the device the
subset that it accepts.  The only way to renegotiate is to reset
the device.

This allows for forwards and backwards compatibility: if the device is
enhanced with a new feature bit, older drivers will not write that
feature bit back to the device.  Similarly, if a driver is enhanced with a feature
that the device doesn't support, it see the new feature is not offered.


this will become "acknowledge" I guess.



-- 
MST

Re: [virtio-dev] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Wed, 19 Jan 2022 18:48:04 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 19, 2022 at 06:50:18PM +0100, Cornelia Huck wrote:
> > On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
[..] 
> > Hm, I would rather interpret the act of "setting FEATURES_OK" as
> > "acknowledging the features it wrote to the device", as in not only "I
> > know those features, and can use them in principle", but in "this is
> > actually a feature combination I can use".  
> 
> that's the feature negotiation is complete part I think.
> 
> 

I agree.

[..]
> > My personal understanding of the terminology is:
> > - acknowledged: the driver has written a feature to the device, and set
> >   FEATURES_OK
> > - negotiated: the bit is acknowledged, and the device has actually
> >   accepted FEATURES_OK
> > 
> > Generally, "acknowledging" a bit does not really carry more weight than
> > simply writing the bit to the device, the big distinction is whether the
> > device accepted FEATURES_OK.  
> 
> Hmm I don't think this is the current reading of the spec. Do you?
> acknowledged is really something we inherited from 0.X
> and never changed, so it always meant "written to device as 1".
> Nothing to do with FEATURES_OK.
> 1.0 added negotiated to deal with FEATURES_OK.
> 

IMHO, the fact that Connie got this wrong is a reason enough to clarify
this thoroughly. Connie isn't exactly new to Virtio, and people with
less exposure are even likelier to get it wrong.

I wasn't aware either that "acknowledged" is a thing for modern devices,
or that it is terminology inherited form 0.X.

So let us say what is in 0.X. I'm quoting form
https://ozlabs.org/~rusty/virtio-spec/virtio-0.9.5.pdf

"The feature bits are negotiated: the device lists all the features it
understands in the Device Features field, and the guest writes the
subset that it understands into the Guest Features field. The only way
to renegotiate is to reset the device."

"Extensible: Virtio PCI devices contain feature bits which are
acknowledged by the guest operating system during device setup. This allows forwards
and backwards compatibility: the device offers all the features it knows
ab out, and the driver acknowledges those it understands and wishes to
use."

So at least 0.9.5 defines "features negotiated" and defines it well, in
a sense that there is no ambiguity about it (when are the features
considered to be negotiated, and how is this done). The only problem
with the definition is, that it ain't extensible! But let us get
back to well defined. In this document, the the "Guest Features" are
defined as single 32 bit field, which may be written (by the driver)
exactly once. And the act of writing "Guest Features" which is an atomic
operation concludes the feature negotiation. The only way to
re-negotiate, i.e. write again to "Guest Features" is to reset and then
re-init the device.

IMHO in this document, there is term "acknowledged" in relation to
feature bits is not defined. "Acknowledge" is rather used as a synonym
for "negotiated" in the second quote. And that is the only feature bits
related occurrence of "acknowledge I could find. The meaning of
"acknowledge" we want here, i.e. the state where the features have been
written, but not yet negotiated (finalized) is not something one can find
in v0.9.5.

BTW one thing I like about v0.9.5 is that it makes very clear, that
"Device Features" and "Guest Features" are two pairs of shoes.


> And I don't really like moving to this variant since acknowledged is then
> a very short transient state. We would be burning a very useful
> term on something that's barely there.
> 
> 
> > So, the things that really matter are, I think,
> > - whether the driver has written VERSION_1, which means that the config
> >   space is supposed to be litte-endian
> > - whether the driver has written any bit that governs an optional field,
> >   which means that the driver may read that field
> > - that weird special case above
> > - whether the device has accepted FEATURES_OK, which means that the
> >   config space layout is fixed and the driver may write to it
> > 
> > That still leaves the problem to find a good term for clearing the bit
> > again... unless we simply talk about the driver setting and clearing it
> > (but how do we express that it needs to make the device aware of it?)
> > 
> > The other big problem is of course legacy... the spec talks of
> > "acknowledging" there, but in the legacy case, it means simply that the
> > driver wrote the bit, as we do not really have an explicit point where
> > the bit has actually been negotiated, it's only implicit. I think
> > phrasing matters for transitional devices, VIRTIO_1, and config space
> > endianness here...  
> 
> 
> So for all of the above reasons I think we are better off
> with keeping the way acknowledging is currently understood in
> spec - really the pre-1.0 meaning.

See my comment above.

> 
> > > We could just add a paragraph explaining the terminology, I just
> > > wish we could be more concise.  
> > 
> > I'm unfortunately struggling to come up with good, waterproof, and
> > understandable terminology covering all cases :(  
> 
> I think if you recall what terminology used to be pre-1.0 things
> will become clearer. 

I disagree.

> And I think the requirement to allow
> early config space access and the idea of detecting legacy
> based on features all predate FEATURES_OK.
> 

I agree with this part.

Regards,
Halil

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Thu, Jan 20, 2022 at 02:05:37PM +0100, Cornelia Huck wrote:
> Do we know anything about the state of e.g. hardware implementations?

One I'm aware of is LE and just does not work on BE
platforms in legacy mode. But generally hardware has a standard MTU
so does not need it ...

-- 
MST

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Thu, 20 Jan 2022 14:05:37 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:  
> >> On Fri, 14 Jan 2022 17:50:51 -0500
> >> "Michael S. Tsirkin" <mst@redhat.com> wrote:  
> >> > Subject: virtio: clarify feature negotiation
> >> > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
> >> >  
> >> >  \item\label{itm:General Initialization And Device Operation /
> >> >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> >> > -   understood by the OS and driver to the device.  During this step the
> >> > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> >> > +   understood by the OS and driver to the device.  During this
> >> > +   step, after writing the subset of feature bits to the device the
> >> > +   driver MAY read (but MUST NOT write) the device-specific
> >> > +   configuration fields to validate that it can support the device
> >> > +   before accepting it. The driver MAY then repeatedly modify the
> >> > +   subset as appropriate, write the new subset and repeat the
> >> > +   validation, any number of times.  
> >> 
> >> IMHO don't have an exact definition of what "MAY read device-specific
> >> configuration field means". To me, "may read" does not automatically
> >> imply that what was read can be interpreted, is valid and meaningful.  
> >
> > Let's say "MAY read (and use)" - is that clear enough?  
> 
> That would work for me. We just need to make sure that the device
> actually MUST present something valid if the feature bit is set
> (acknowledged).
> 
> >  
> >> For example there was no harm in reading MTU without having the
> >> F_VERSION_1 feature acknowledged before. We did not end up in undefined
> >> behavior land, the read_config operation did not blow up in our face,
> >> and the field was also "there" if the corresponding feature was offered.
> >> The only trouble was that one could not interpret the value of the field,
> >> because one could not tell is it big or little endian.  
> 
> I think there's an elephant in the room: which bits can the device
> actually assume to be set or unset?

I really don't understand what is the issue here. Please have a look
at
https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg08187.html
and compare the part about "guest features" and "device features" being
distinct "device registers" (or a set of registers for modern) with the
QEMU implementation.

I don't think the initial value of bits in "guest features"
is specified, in the spec. But I may be wrong. If it is unspecified, this
is a little tricky. In practice, I think initially all bits are not set,
at least with QEMU, but I'm not 100% sure. In any case, that is the
most reasonable thing to assume anyway.

The device should consider the bits set currently have the value 1. Bits
are manipulated using the features_write operation, and immediately
after the successful execution of such an op by the device,
the register should have the value that was written, modulo filtering
away features that were not offered in the first place. I think that is
the maximum allowed amount of filtering, although this does not seem to
be specified. 

When is the driver safe to assume that the device has performed the
operation is an entirely different issue. If the filtering confined to
what I described (features not offered in the first place) after writing
the features, the guest knows which feature bits are set. The problem is
only it does not necessarily know when.

> 
> My take would be:
> - If the driver wrote a set of feature bits that contain MTU but not
>   VERSION_1, the device MUST provide a valid mtu value in platform
>   endian.
> - If the driver wrote a set of feature bits that contain MTU and
>   VERSION_1, the device MUST provide a valid mtu value in little
>   endian.
> - If the driver has not written the MTU bit, the device MAY provide
>   anything (but it could make an educated guess and provide a valid mtu
>   value in any endianness it thinks the driver might be expecting.)
>   If that is not something that the driver can use, it's really the
>   driver's problem for not setting the feature bit.

We actually don't have to take any educated guesses for endinannes. We
can do the transport specific detection.

The problem with serving whatever is, that we are declaring a bunch of
drivers non-spec-conform with this. Can you name me one driver, that did
the right thing?

We can pretend that the spec was always clear, and all the implementers
were stupid, and neglected to do the right thing. And this may be the
best course of action. I just want us to speak the truth during review
at least.

> 
> We just need to make sure stating that setting relevant feature bits is
> needed if the driver expects to get anything useful back, especially if
> you consider endianness. That problem just does not show up on
> little-endian platforms.

Is that relevant form spec perspective? I understand the practical side
of it, but still it feels wrong to use that as an argument. The spec is
not supposed to favor little-endina platforms over big-big endian
platforms in my opinion.

> 
> >> 
> >> IMHO, we want to accomplish, readable, interpretible, valid and
> >> meaningful, before the feature has been negotiated by setting
> >> FEATURES_OK.
> >> 
> >> I see a couple of potential problems with this:
> >> 1) The set of acknowledged features may be invalid! For example may end
> >> up with feature B acknowledged and feature B not acknowledged, when
> >> B depends on A. Before "acknowledge" we used to be safe, because the
> >> device would reject FEATURES_OK.  
> >
> > right. but for reading config fields it's probably enough in practice.  
> 
> It's probably ok to defer actually validating the bits to
> FEATURES_OK. If an inconsistent set of feature bits leads to a
> poorly-defined value, the device can really provide anything, and the
> driver just has to deal with trying to do something that is not valid
> anyway. But probably not a problem in practice, I agree.
> 
> >  
> >> 2) In theory, we could have union-type conditional fields. To demonstrate
> >> what I mean here is a stupid example:
> >> struct ThermometherConfig {
> >> union {
> >> u16 temp_in_kelvin;
> >> u16 temp_in_celsius;
> >> u16 temp_in_farenheit;
> >> } temp;
> >> };
> >> where kelvin is the default, but you can get Celsius or Fahrenheit
> >> depending on mutex feature bits.
> >> This also used to be and still is safe after the features have been
> >> negotiated, but with "acknowledged" there is trouble when both the
> >> features are set.  
> >
> > something to worry about in the future but are there cases like
> > this in present? should we worry?  
> 
> I would say that this is simply the driver triggering undefined
> behaviour from the device.
> 

That is the problem. In this paragraph we are trying to make sure that
the value of the field is valid and meaningful. You say, the driver
messed up. And I agree. But it is hard to pinpoint the requirement
which was violated. Can you help me do that? To be safe, we would
have to stipulate that the "guest features" are valid and acceptable.
I.e. that a FEATURES_OK would succeed. And that is implementation
specific, i.e. determined by the spec.

What bails us out is "can read" which is vague, and does allow for the
field containing garbage. But the problem with that is, that this
non-determinism makes the interface awful to program against: because it
may or may not work.

> >  
> >> 3) Features are written in 32 bit chunks. My guess is that what we
> >> want for "features acknowledged is": driver has written each chunk it
> >> knows about at least once. Of course, the question arises, what should
> >> the device do if config is read before that happens.  
> >
> > we are lucky that at present it's just mtu of the net device :).  
> 
> If, say, the driver writes the first chunk with the MTU bit, then reads
> the mtu config field, then writes the second chunk with VERSION_1, it
> should get the mtu in platform-endian (and on the next read after the
> second chunk has been written, in little endian.) I'd say that this is
> simply an issue of the driver not serializing its own reads/writes
> correctly :)
> 

Point me to the sentence that says the driver did not serialize its own
reads/writes correctly please!

We can't just come up with requirements when it suits us. Well, we can
but I argue, that should be the last resort.

[..]

Regards,
Halil

[virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Wed, 19 Jan 2022 11:34:35 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:
> > On Fri, 14 Jan 2022 17:50:51 -0500
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:  
> > > Subject: virtio: clarify feature negotiation  
> > 
> > This is in my opinion more of an incompatible change that a
> > clarification, but since the commit message does not constitute a part
> > of the standard, it does not matter that much.
> > [..]
> >   
> > > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi
> > >  \end{note}
> > >  
> > >  \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities of a Virtio Device / Device Configuration Space}
> > > -The device MUST allow reading of any device-specific configuration
> > > +The device SHOULD allow reading of any device-specific configuration
> > >  field before FEATURES_OK is set by the driver.  This includes fields which are
> > > -conditional on feature bits, as long as those feature bits are offered
> > > -by the device.
> > > +conditional on feature bits, as long as those feature bits have
> > > +been acknowledged by the driver.  
> > 
> > "Those feature bits have been acknowledged by the driver" is new
> > terminology. IMHO we should have a precise definition
> > of "feature bit is acknowledged".
> > 
> > Currently "acknowledge features" means "write features".
> > 
> >   
> > >  
> > >  \subsection{Legacy Interface: A Note on Device Configuration Space endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space / Legacy Interface: A Note on Configuration Space endian-ness}
> > >  
> > > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
> > >  
> > >  \item\label{itm:General Initialization And Device Operation /
> > >  Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
> > > -   understood by the OS and driver to the device.  During this step the
> > > -   driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
> > > +   understood by the OS and driver to the device.  During this
> > > +   step, after writing the subset of feature bits to the device the
> > > +   driver MAY read (but MUST NOT write) the device-specific
> > > +   configuration fields to validate that it can support the device
> > > +   before accepting it. The driver MAY then repeatedly modify the
> > > +   subset as appropriate, write the new subset and repeat the
> > > +   validation, any number of times.  
> > 
> > IMHO don't have an exact definition of what "MAY read device-specific
> > configuration field means". To me, "may read" does not automatically
> > imply that what was read can be interpreted, is valid and meaningful.  
> 
> Let's say "MAY read (and use)" - is that clear enough?

I would prefer "MAY read, and the value read  is valid".

> 
> > For example there was no harm in reading MTU without having the
> > F_VERSION_1 feature acknowledged before. We did not end up in undefined
> > behavior land, the read_config operation did not blow up in our face,
> > and the field was also "there" if the corresponding feature was offered.
> > The only trouble was that one could not interpret the value of the field,
> > because one could not tell is it big or little endian.
> > 
> > IMHO, we want to accomplish, readable, interpretible, valid and
> > meaningful, before the feature has been negotiated by setting
> > FEATURES_OK.
> > 
> > I see a couple of potential problems with this:
> > 1) The set of acknowledged features may be invalid! For example may end
> > up with feature B acknowledged and feature B not acknowledged, when
> > B depends on A. Before "acknowledge" we used to be safe, because the
> > device would reject FEATURES_OK.  
> 
> right. but for reading config fields it's probably enough in practice.
> 

In practice yes. But we can do better, e.g. if we mandate that all
features need to be written before early config space access. And
that is actually what your Linux patch does anyway.

> > 2) In theory, we could have union-type conditional fields. To demonstrate
> > what I mean here is a stupid example:
> > struct ThermometherConfig {
> > union {
> > u16 temp_in_kelvin;
> > u16 temp_in_celsius;
> > u16 temp_in_farenheit;
> > } temp;
> > };
> > where kelvin is the default, but you can get Celsius or Fahrenheit
> > depending on mutex feature bits.
> > This also used to be and still is safe after the features have been
> > negotiated, but with "acknowledged" there is trouble when both the
> > features are set.  
> 
> something to worry about in the future but are there cases like
> this in present? should we worry?

I will have to rescann the spec. In practice, it ain't a huge issue,
if we mandate that "right before FEATURES_OK", because that way
we would only have a problem in the cases where we would have had
a problem if we did set  FEATURES_OK.

If we want to go down this route, it probably wouldn't hurt to watch
out for unions in config space, when the spec is extended.


> 
> > 3) Features are written in 32 bit chunks. My guess is that what we
> > want for "features acknowledged is": driver has written each chunk it
> > knows about at least once. Of course, the question arises, what should
> > the device do if config is read before that happens.  
> 
> we are lucky that at present it's just mtu of the net device :).
> 

Yes, I was trying to showcase the properties of the interface.

> 
> > 4) It is pretty obvious to me, that after "features have been
> > acknowledged", we want the driver to read the very same config as
> > if we were in "features have been negotiated" state. The only  difference
> > we want is, that the features must not be "frozen", i.e. the driver
> > should still be able to modify the features.  
> 
> I can add that, ok.

I think that would also help with several of these, admittedly rather
theoretical issues. My approach to this was really to take a step back,
and examine the interface. I like nicely designed interfaces. :D


> 
> > 5) It ain't clear to me when is the driver supposed to assume that
> > "the features have been acknowledged". I.e. that the expected changes
> > have taken effect. For PCI, as far as I remember, they have this write it
> > and then read it back drill to force side-effects. Would we have to do
> > something like this. I think for FEATURES_OK we have that stuff clearly
> > specified.  
> 
> Hmm good point.  normally reads do not bypass writes, so you can write
> features and then read config, should work.  only exception is when they
> are different types e.g.  in theory features could be in IO and config
> in memory.  however I think reads from different types of memory can be
> reordered anyway, so that config isn't really legal.  I'll poke at the
> pci spec some more to refresh my memory on this. 

Thx!

> 
> > I don't know if that would be viable, but introducing a FEATURES_ACK
> > status bit would IMHO resolve a bunch of these potential problems, and
> > would also serve as a clear trigger for stuff like vhost (to flush
> > features)  
> 
> Right. With this proposal I think basically a config read
> flushes features. Which isn't too bad ...

No it is not, but we would have to implement it. And we would have
to ether do dirty tracking, or flush on each config read if we aren't
FEATURES_OK yet. And we would also require an awareness of config reads
in the part that does the features, if the config space is handled in
a different part. In practice, it is probably a non-issue. But I wanted
to mention it.

> 
> > The only problem, that would remain is that IMHO this is still
> > fundamentally a change that effectively breaks compatibility. IMHO
> > "features acknowledged" is essentially a new feature, which enables
> > a sane use of the config space before FEATURES_OK. But we can't guard it
> > with a feature bit, because the snake would just bite its tail. I'm
> > fine with going down this route. It is just that I would like to be
> > completely transparent about this.  
> 
> well kind of. But I think the nice thing about it and the reason
> I structured it this way and not as a complete new status bit
> is that it really matches pretty well what the existing
> body of code does, e.g. we don't need any qemu changes at all,

Wouldn't we need to change QEMU to flush features to vhost backends?

Would we want to make QEMU tolerate drivers that do not play by the
new rules. I'm asking about that RFC of mine which does the transport
specific detection of legacy/modern?

> and most linux and windows drivers except for network with
> the MTU thing are also good to go.

Yes, going with a new satus bit would use up a status bit, and 
result in more code, at least on the Linux side of things. On the
other hand, the new status bit approach would result in a cleaner
and easier to understand interface.

I think, one key point we have to clarify is: did we really have
this acknowledged but not yet negotiated state in pre-1.0 virtio. If we
did, then it is much more plausible to say, that doing it like this
is a clarification, and doing it with a new status bit would be a new
feature of virtio.

On this question please see my other email:
https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg08187.html


Regards,
Halil

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-dev] Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Fri, Jan 21 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 20 Jan 2022 14:05:37 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:  

>> My take would be:
>> - If the driver wrote a set of feature bits that contain MTU but not
>>   VERSION_1, the device MUST provide a valid mtu value in platform
>>   endian.
>> - If the driver wrote a set of feature bits that contain MTU and
>>   VERSION_1, the device MUST provide a valid mtu value in little
>>   endian.
>> - If the driver has not written the MTU bit, the device MAY provide
>>   anything (but it could make an educated guess and provide a valid mtu
>>   value in any endianness it thinks the driver might be expecting.)
>>   If that is not something that the driver can use, it's really the
>>   driver's problem for not setting the feature bit.
>
> We actually don't have to take any educated guesses for endinannes. We
> can do the transport specific detection.
>
> The problem with serving whatever is, that we are declaring a bunch of
> drivers non-spec-conform with this. Can you name me one driver, that did
> the right thing?

I don't understand how that makes any driver non-conformant... we can
say that the device should check for the expected endianness via a
transport-specific method, but in fact, the guarantee has never been
there; it just happened to work in most cases, except the problems on
s390x we saw recently. It is reasonable to ask the device to try to use
the correct endianness, but it was never a requirement, it all hinged
upon the VERSION_1 bit.

>
> We can pretend that the spec was always clear, and all the implementers
> were stupid, and neglected to do the right thing. And this may be the
> best course of action. I just want us to speak the truth during review
> at least.
>
>> 
>> We just need to make sure stating that setting relevant feature bits is
>> needed if the driver expects to get anything useful back, especially if
>> you consider endianness. That problem just does not show up on
>> little-endian platforms.
>
> Is that relevant form spec perspective? I understand the practical side
> of it, but still it feels wrong to use that as an argument. The spec is
> not supposed to favor little-endina platforms over big-big endian
> platforms in my opinion.

How would adding some words of guidance favour one endianness over the
other? The device should make an effort to get it right; but that did
not happen historically anyway.

>> >> 2) In theory, we could have union-type conditional fields. To demonstrate
>> >> what I mean here is a stupid example:
>> >> struct ThermometherConfig {
>> >> union {
>> >> u16 temp_in_kelvin;
>> >> u16 temp_in_celsius;
>> >> u16 temp_in_farenheit;
>> >> } temp;
>> >> };
>> >> where kelvin is the default, but you can get Celsius or Fahrenheit
>> >> depending on mutex feature bits.
>> >> This also used to be and still is safe after the features have been
>> >> negotiated, but with "acknowledged" there is trouble when both the
>> >> features are set.  
>> >
>> > something to worry about in the future but are there cases like
>> > this in present? should we worry?  
>> 
>> I would say that this is simply the driver triggering undefined
>> behaviour from the device.
>> 
>
> That is the problem. In this paragraph we are trying to make sure that
> the value of the field is valid and meaningful. You say, the driver
> messed up. And I agree. But it is hard to pinpoint the requirement
> which was violated. Can you help me do that? To be safe, we would
> have to stipulate that the "guest features" are valid and acceptable.
> I.e. that a FEATURES_OK would succeed. And that is implementation
> specific, i.e. determined by the spec.

Do we really have to specify what happens when the driver tries to make
actual use of an invalid feature set? If yes, then we need to be clear
that the driver must care to never write a feature set with conflicting
features, which is a superset of the feature combinations that the
device would reject during FEATURES_OK.

>
> What bails us out is "can read" which is vague, and does allow for the
> field containing garbage. But the problem with that is, that this
> non-determinism makes the interface awful to program against: because it
> may or may not work.
>
>> >  
>> >> 3) Features are written in 32 bit chunks. My guess is that what we
>> >> want for "features acknowledged is": driver has written each chunk it
>> >> knows about at least once. Of course, the question arises, what should
>> >> the device do if config is read before that happens.  
>> >
>> > we are lucky that at present it's just mtu of the net device :).  
>> 
>> If, say, the driver writes the first chunk with the MTU bit, then reads
>> the mtu config field, then writes the second chunk with VERSION_1, it
>> should get the mtu in platform-endian (and on the next read after the
>> second chunk has been written, in little endian.) I'd say that this is
>> simply an issue of the driver not serializing its own reads/writes
>> correctly :)
>> 
>
> Point me to the sentence that says the driver did not serialize its own
> reads/writes correctly please!
>
> We can't just come up with requirements when it suits us. Well, we can
> but I argue, that should be the last resort.

What I say is that the driver simply gets things back from the device
that are consistent from the device's point of view. If it cannot write
a set of bits atomically, that it simply needs to expect that it can get
different values at different times during the writing process. If it
wants consistent values, it must not mix up non-atomic writes and
reads -- but that's nothing special.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Fri, 21 Jan 2022 17:05:07 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, Jan 21 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Thu, 20 Jan 2022 14:05:37 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>   
> >> > On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:    
> 
> >> My take would be:
> >> - If the driver wrote a set of feature bits that contain MTU but not
> >>   VERSION_1, the device MUST provide a valid mtu value in platform
> >>   endian.
> >> - If the driver wrote a set of feature bits that contain MTU and
> >>   VERSION_1, the device MUST provide a valid mtu value in little
> >>   endian.
> >> - If the driver has not written the MTU bit, the device MAY provide
> >>   anything (but it could make an educated guess and provide a valid mtu
> >>   value in any endianness it thinks the driver might be expecting.)
> >>   If that is not something that the driver can use, it's really the
> >>   driver's problem for not setting the feature bit.  
> >
> > We actually don't have to take any educated guesses for endinannes. We
> > can do the transport specific detection.
> >
> > The problem with serving whatever is, that we are declaring a bunch of
> > drivers non-spec-conform with this. Can you name me one driver, that did
> > the right thing?  
> 
> I don't understand how that makes any driver non-conformant...

In my reading, before this patch the the spec implies, that the driver
is fine to read and use device-specific config fields to figure out which
features to write. I'm quoting form the spec "Read device feature bits,
and write the subset of feature bits understood by the OS and driver to
the device. During this step the driver MAY read (but MUST NOT write) the
device-specific configuration fields to check that it can support the
device before accepting it." (3.1.1 Driver Requirements: Device
Initialization).

With this change we basically introduce a new step, which the driver
is supposed to do in order to get sane values. Instead of
[read features, read config, write supported features] we say the
driver should do [read features, write features which could be supported
(modulo what the config says), read config, write actually supported
features].

So the drivers currently out there, that do the early config read to
validate if they support some of the features don't fulfill the new
driver requirements. In my reading of the spec, a driver that does
not some fulfill the driver requirements is non-conform.

Is there a somewhere a hole in my reasoning which makes me reach the
wrong conclusion? If yes, please point it out.

> we can
> say that the device should check for the expected endianness via a
> transport-specific method, but in fact, the guarantee has never been
> there; it just happened to work in most cases, except the problems on
> s390x we saw recently. It is reasonable to ask the device to try to use
> the correct endianness, but it was never a requirement, it all hinged
> upon the VERSION_1 bit.

I read this paragraph as: transport specific "modern-legacy" detection
was never a requirement. I tend to agree with that statement. Although,
AFAIR Michael at least used to think differently.

To be more precise, IMHO it was all hinged upon the answer to the
question: was VERSION_1 _negotiated_ or not? But that is just not good
enough for _early_ config access.

> 
> >
> > We can pretend that the spec was always clear, and all the implementers
> > were stupid, and neglected to do the right thing. And this may be the
> > best course of action. I just want us to speak the truth during review
> > at least.
> >  
> >> 
> >> We just need to make sure stating that setting relevant feature bits is
> >> needed if the driver expects to get anything useful back, especially if
> >> you consider endianness. That problem just does not show up on
> >> little-endian platforms.  
> >
> > Is that relevant form spec perspective? I understand the practical side
> > of it, but still it feels wrong to use that as an argument. The spec is
> > not supposed to favor little-endina platforms over big-big endian
> > platforms in my opinion.  
> 
> How would adding some words of guidance favour one endianness over the
> other? The device should make an effort to get it right; but that did
> not happen historically anyway.
> 

I'm a bit puzzled. I assumed, you were trying to say something like: the
problem ain't such a big deal because in practice it affects only
big-endian platforms. I probably misunderstood you. What is the point you
were trying to make with that paragraph?

> >> >> 2) In theory, we could have union-type conditional fields. To demonstrate
> >> >> what I mean here is a stupid example:
> >> >> struct ThermometherConfig {
> >> >> union {
> >> >> u16 temp_in_kelvin;
> >> >> u16 temp_in_celsius;
> >> >> u16 temp_in_farenheit;
> >> >> } temp;
> >> >> };
> >> >> where kelvin is the default, but you can get Celsius or Fahrenheit
> >> >> depending on mutex feature bits.
> >> >> This also used to be and still is safe after the features have been
> >> >> negotiated, but with "acknowledged" there is trouble when both the
> >> >> features are set.    
> >> >
> >> > something to worry about in the future but are there cases like
> >> > this in present? should we worry?    
> >> 
> >> I would say that this is simply the driver triggering undefined
> >> behaviour from the device.
> >>   
> >
> > That is the problem. In this paragraph we are trying to make sure that
> > the value of the field is valid and meaningful. You say, the driver
> > messed up. And I agree. But it is hard to pinpoint the requirement
> > which was violated. Can you help me do that? To be safe, we would
> > have to stipulate that the "guest features" are valid and acceptable.
> > I.e. that a FEATURES_OK would succeed. And that is implementation
> > specific, i.e. determined by the spec.  
> 
> Do we really have to specify what happens when the driver tries to make
> actual use of an invalid feature set? 

The point is, the driver does not know if the feature set it has written
is valid or not at the point when early config space access is
happening. At least not with the current proposal. 

> If yes, then we need to be clear
> that the driver must care to never write a feature set with conflicting
> features, which is a superset of the feature combinations that the
> device would reject during FEATURES_OK.

I don't understand this part. Did you mean "subset" in stead of
"superset"?

> 
> >
> > What bails us out is "can read" which is vague, and does allow for the
> > field containing garbage. But the problem with that is, that this
> > non-determinism makes the interface awful to program against: because it
> > may or may not work.
> >  
> >> >    
> >> >> 3) Features are written in 32 bit chunks. My guess is that what we
> >> >> want for "features acknowledged is": driver has written each chunk it
> >> >> knows about at least once. Of course, the question arises, what should
> >> >> the device do if config is read before that happens.    
> >> >
> >> > we are lucky that at present it's just mtu of the net device :).    
> >> 
> >> If, say, the driver writes the first chunk with the MTU bit, then reads
> >> the mtu config field, then writes the second chunk with VERSION_1, it
> >> should get the mtu in platform-endian (and on the next read after the
> >> second chunk has been written, in little endian.) I'd say that this is
> >> simply an issue of the driver not serializing its own reads/writes
> >> correctly :)
> >>   
> >
> > Point me to the sentence that says the driver did not serialize its own
> > reads/writes correctly please!
> >
> > We can't just come up with requirements when it suits us. Well, we can
> > but I argue, that should be the last resort.  
> 
> What I say is that the driver simply gets things back from the device
> that are consistent from the device's point of view.

That is what I'm concerned about. The current proposal stipulates, that
the device needs to deliver a consistent config space, when it is in an
inconsistent state, in a sense that the feature set, that has been
written, is not supported. For example if I had a feature X2 that depends
on bit F2 which in turn depends on feature X1 and bit F1, and has a
config field C2 that is conditional on F2 and needs to be computed during
initialization of X1 and requires private values that were computed
during the initialization of the feature X1, I would refuse to
initialize X2 if bit F1 is not set, because there is no sane way to
do that without initializing X1 first. An thus there is no sane way
to compute C2.

> If it cannot write
> a set of bits atomically, that it simply needs to expect that it can get
> different values at different times during the writing process. If it
> wants consistent values, it must not mix up non-atomic writes and
> reads -- but that's nothing special.

IMHO if the spec promises consistent values under certain conditions,
then the device needs to deliver consistent values, at least under those
conditions. Are you telling me "after writing the subset of feature bits
to the device" somehow includes "in must not mix up ..."?

You said in the hypothesized scenario the driver does not keep it's end
of the bargain. I asked to point me to the exact points in the spec
which got violated. I don't see those pointers in your response. Sorry.
So I'm asking again, can you please tell me which section, which
paragraph of the spec, or some linked spec is violated?

Or did I misunderstand you? Were you not claiming that the given case
the driver would be in violation of the spec?

Regards,
Halil

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Mon, Jan 24, 2022 at 05:40:52PM +0100, Halil Pasic wrote:
> On Fri, 21 Jan 2022 17:05:07 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Fri, Jan 21 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> > 
> > > On Thu, 20 Jan 2022 14:05:37 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >  
> > >> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >>   
> > >> > On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:    
> > 
> > >> My take would be:
> > >> - If the driver wrote a set of feature bits that contain MTU but not
> > >>   VERSION_1, the device MUST provide a valid mtu value in platform
> > >>   endian.
> > >> - If the driver wrote a set of feature bits that contain MTU and
> > >>   VERSION_1, the device MUST provide a valid mtu value in little
> > >>   endian.
> > >> - If the driver has not written the MTU bit, the device MAY provide
> > >>   anything (but it could make an educated guess and provide a valid mtu
> > >>   value in any endianness it thinks the driver might be expecting.)
> > >>   If that is not something that the driver can use, it's really the
> > >>   driver's problem for not setting the feature bit.  
> > >
> > > We actually don't have to take any educated guesses for endinannes. We
> > > can do the transport specific detection.
> > >
> > > The problem with serving whatever is, that we are declaring a bunch of
> > > drivers non-spec-conform with this. Can you name me one driver, that did
> > > the right thing?  
> > 
> > I don't understand how that makes any driver non-conformant...
> 
> In my reading, before this patch the the spec implies, that the driver
> is fine to read and use device-specific config fields to figure out which
> features to write. I'm quoting form the spec "Read device feature bits,
> and write the subset of feature bits understood by the OS and driver to
> the device. During this step the driver MAY read (but MUST NOT write) the
> device-specific configuration fields to check that it can support the
> device before accepting it." (3.1.1 Driver Requirements: Device
> Initialization).
> 
> With this change we basically introduce a new step, which the driver
> is supposed to do in order to get sane values. Instead of
> [read features, read config, write supported features] we say the
> driver should do [read features, write features which could be supported
> (modulo what the config says), read config, write actually supported
> features].
> 
> So the drivers currently out there, that do the early config read to
> validate if they support some of the features don't fulfill the new
> driver requirements. In my reading of the spec, a driver that does
> not some fulfill the driver requirements is non-conform.

It's true. However, that's a generic rule and specific devices
can differ. and if you look at specific instances, you will
see that they typically require that the feature is negotiated.
For example:

\item If the VIRTIO_BLK_F_BLK_SIZE feature is negotiated,
  \field{blk_size} can be read to determine the optimal sector size
  for the driver to use.

so a driver reading blk_size before FEATURES_OK is actually using
an undocumented aspect of the behaviour.

compare this to MTU:

\item[VIRTIO_NET_F_MTU(3)] Device maximum MTU reporting is supported. If
    offered by the device, device advises driver about the value of
    its maximum MTU. If negotiated, the driver uses \field{mtu} as
    the maximum MTU value.

from which it's clear that it can be read even if not
negotiated.

So from that point of view, we are actually relaxing the requirements,
and yes we'll need to carefully go over each instance of
"offered".
I thought I did, but now I did a quick grep again and I found instances
in virtio-iommu.tex and virtio-gpio.tex
Both of these are new, so I think we can just fix them to "negotiated".

Jean-Philippe, Viresh, what do you think? Would it be a problem to
replace offered with negotiated, and restrict iommu and gpio drivers to
access config space after FEATURES_OK?




> Is there a somewhere a hole in my reasoning which makes me reach the
> wrong conclusion? If yes, please point it out.
>
> > we can
> > say that the device should check for the expected endianness via a
> > transport-specific method, but in fact, the guarantee has never been
> > there; it just happened to work in most cases, except the problems on
> > s390x we saw recently. It is reasonable to ask the device to try to use
> > the correct endianness, but it was never a requirement, it all hinged
> > upon the VERSION_1 bit.
> 
> I read this paragraph as: transport specific "modern-legacy" detection
> was never a requirement. I tend to agree with that statement. Although,
> AFAIR Michael at least used to think differently.
> 
> To be more precise, IMHO it was all hinged upon the answer to the
> question: was VERSION_1 _negotiated_ or not? But that is just not good
> enough for _early_ config access.
> 
> > 
> > >
> > > We can pretend that the spec was always clear, and all the implementers
> > > were stupid, and neglected to do the right thing. And this may be the
> > > best course of action. I just want us to speak the truth during review
> > > at least.
> > >  
> > >> 
> > >> We just need to make sure stating that setting relevant feature bits is
> > >> needed if the driver expects to get anything useful back, especially if
> > >> you consider endianness. That problem just does not show up on
> > >> little-endian platforms.  
> > >
> > > Is that relevant form spec perspective? I understand the practical side
> > > of it, but still it feels wrong to use that as an argument. The spec is
> > > not supposed to favor little-endina platforms over big-big endian
> > > platforms in my opinion.  
> > 
> > How would adding some words of guidance favour one endianness over the
> > other? The device should make an effort to get it right; but that did
> > not happen historically anyway.
> > 
> 
> I'm a bit puzzled. I assumed, you were trying to say something like: the
> problem ain't such a big deal because in practice it affects only
> big-endian platforms. I probably misunderstood you. What is the point you
> were trying to make with that paragraph?
> 
> > >> >> 2) In theory, we could have union-type conditional fields. To demonstrate
> > >> >> what I mean here is a stupid example:
> > >> >> struct ThermometherConfig {
> > >> >> union {
> > >> >> u16 temp_in_kelvin;
> > >> >> u16 temp_in_celsius;
> > >> >> u16 temp_in_farenheit;
> > >> >> } temp;
> > >> >> };
> > >> >> where kelvin is the default, but you can get Celsius or Fahrenheit
> > >> >> depending on mutex feature bits.
> > >> >> This also used to be and still is safe after the features have been
> > >> >> negotiated, but with "acknowledged" there is trouble when both the
> > >> >> features are set.    
> > >> >
> > >> > something to worry about in the future but are there cases like
> > >> > this in present? should we worry?    
> > >> 
> > >> I would say that this is simply the driver triggering undefined
> > >> behaviour from the device.
> > >>   
> > >
> > > That is the problem. In this paragraph we are trying to make sure that
> > > the value of the field is valid and meaningful. You say, the driver
> > > messed up. And I agree. But it is hard to pinpoint the requirement
> > > which was violated. Can you help me do that? To be safe, we would
> > > have to stipulate that the "guest features" are valid and acceptable.
> > > I.e. that a FEATURES_OK would succeed. And that is implementation
> > > specific, i.e. determined by the spec.  
> > 
> > Do we really have to specify what happens when the driver tries to make
> > actual use of an invalid feature set? 
> 
> The point is, the driver does not know if the feature set it has written
> is valid or not at the point when early config space access is
> happening. At least not with the current proposal. 
> 
> > If yes, then we need to be clear
> > that the driver must care to never write a feature set with conflicting
> > features, which is a superset of the feature combinations that the
> > device would reject during FEATURES_OK.
> 
> I don't understand this part. Did you mean "subset" in stead of
> "superset"?
> 
> > 
> > >
> > > What bails us out is "can read" which is vague, and does allow for the
> > > field containing garbage. But the problem with that is, that this
> > > non-determinism makes the interface awful to program against: because it
> > > may or may not work.
> > >  
> > >> >    
> > >> >> 3) Features are written in 32 bit chunks. My guess is that what we
> > >> >> want for "features acknowledged is": driver has written each chunk it
> > >> >> knows about at least once. Of course, the question arises, what should
> > >> >> the device do if config is read before that happens.    
> > >> >
> > >> > we are lucky that at present it's just mtu of the net device :).    
> > >> 
> > >> If, say, the driver writes the first chunk with the MTU bit, then reads
> > >> the mtu config field, then writes the second chunk with VERSION_1, it
> > >> should get the mtu in platform-endian (and on the next read after the
> > >> second chunk has been written, in little endian.) I'd say that this is
> > >> simply an issue of the driver not serializing its own reads/writes
> > >> correctly :)
> > >>   
> > >
> > > Point me to the sentence that says the driver did not serialize its own
> > > reads/writes correctly please!
> > >
> > > We can't just come up with requirements when it suits us. Well, we can
> > > but I argue, that should be the last resort.  
> > 
> > What I say is that the driver simply gets things back from the device
> > that are consistent from the device's point of view.
> 
> That is what I'm concerned about. The current proposal stipulates, that
> the device needs to deliver a consistent config space, when it is in an
> inconsistent state, in a sense that the feature set, that has been
> written, is not supported. For example if I had a feature X2 that depends
> on bit F2 which in turn depends on feature X1 and bit F1, and has a
> config field C2 that is conditional on F2 and needs to be computed during
> initialization of X1 and requires private values that were computed
> during the initialization of the feature X1, I would refuse to
> initialize X2 if bit F1 is not set, because there is no sane way to
> do that without initializing X1 first. An thus there is no sane way
> to compute C2.
> 
> > If it cannot write
> > a set of bits atomically, that it simply needs to expect that it can get
> > different values at different times during the writing process. If it
> > wants consistent values, it must not mix up non-atomic writes and
> > reads -- but that's nothing special.
> 
> IMHO if the spec promises consistent values under certain conditions,
> then the device needs to deliver consistent values, at least under those
> conditions. Are you telling me "after writing the subset of feature bits
> to the device" somehow includes "in must not mix up ..."?
> 
> You said in the hypothesized scenario the driver does not keep it's end
> of the bargain. I asked to point me to the exact points in the spec
> which got violated. I don't see those pointers in your response. Sorry.
> So I'm asking again, can you please tell me which section, which
> paragraph of the spec, or some linked spec is violated?
> 
> Or did I misunderstand you? Were you not claiming that the given case
> the driver would be in violation of the spec?
> 
> Regards,
> Halil

[virtio] Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Mon, Jan 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> It's true. However, that's a generic rule and specific devices
> can differ. and if you look at specific instances, you will
> see that they typically require that the feature is negotiated.
> For example:
>
> \item If the VIRTIO_BLK_F_BLK_SIZE feature is negotiated,
>   \field{blk_size} can be read to determine the optimal sector size
>   for the driver to use.
>
> so a driver reading blk_size before FEATURES_OK is actually using
> an undocumented aspect of the behaviour.
>
> compare this to MTU:
>
> \item[VIRTIO_NET_F_MTU(3)] Device maximum MTU reporting is supported. If
>     offered by the device, device advises driver about the value of
>     its maximum MTU. If negotiated, the driver uses \field{mtu} as
>     the maximum MTU value.
>
> from which it's clear that it can be read even if not
> negotiated.
>
> So from that point of view, we are actually relaxing the requirements,
> and yes we'll need to carefully go over each instance of
> "offered".
> I thought I did, but now I did a quick grep again and I found instances
> in virtio-iommu.tex and virtio-gpio.tex
> Both of these are new, so I think we can just fix them to "negotiated".

Considering this, should we actually add any requirements as to when
fields are valid? The device may present a value that is always valid if
it offers the corresponding feature, or that value may change depending
on what is actually negotiated by the driver. Maybe a statement that the
actual validity of a field's value before negotiation is device type
dependant?


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio-dev] Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

On Mon, Jan 24 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 21 Jan 2022 17:05:07 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Fri, Jan 21 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
>> 
>> > On Thu, 20 Jan 2022 14:05:37 +0100
>> > Cornelia Huck <cohuck@redhat.com> wrote:
>> >  
>> >> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >>   
>> >> > On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:    
>> >> >> 3) Features are written in 32 bit chunks. My guess is that what we
>> >> >> want for "features acknowledged is": driver has written each chunk it
>> >> >> knows about at least once. Of course, the question arises, what should
>> >> >> the device do if config is read before that happens.    
>> >> >
>> >> > we are lucky that at present it's just mtu of the net device :).    
>> >> 
>> >> If, say, the driver writes the first chunk with the MTU bit, then reads
>> >> the mtu config field, then writes the second chunk with VERSION_1, it
>> >> should get the mtu in platform-endian (and on the next read after the
>> >> second chunk has been written, in little endian.) I'd say that this is
>> >> simply an issue of the driver not serializing its own reads/writes
>> >> correctly :)
>> >>   
>> >
>> > Point me to the sentence that says the driver did not serialize its own
>> > reads/writes correctly please!
>> >
>> > We can't just come up with requirements when it suits us. Well, we can
>> > but I argue, that should be the last resort.  
>> 
>> What I say is that the driver simply gets things back from the device
>> that are consistent from the device's point of view.
>
> That is what I'm concerned about. The current proposal stipulates, that
> the device needs to deliver a consistent config space, when it is in an
> inconsistent state, in a sense that the feature set, that has been
> written, is not supported. For example if I had a feature X2 that depends
> on bit F2 which in turn depends on feature X1 and bit F1, and has a
> config field C2 that is conditional on F2 and needs to be computed during
> initialization of X1 and requires private values that were computed
> during the initialization of the feature X1, I would refuse to
> initialize X2 if bit F1 is not set, because there is no sane way to
> do that without initializing X1 first. An thus there is no sane way
> to compute C2.

Well, if the device can't compute it, it can't... this needs to be
specified on a per-device basis. But I don't see what that has to do
with the device returning something that makes sense from its point of
view? It simply needs a specification that defines a default value for
the field if it hasn't been initialized yet?

>
>> If it cannot write
>> a set of bits atomically, that it simply needs to expect that it can get
>> different values at different times during the writing process. If it
>> wants consistent values, it must not mix up non-atomic writes and
>> reads -- but that's nothing special.
>
> IMHO if the spec promises consistent values under certain conditions,
> then the device needs to deliver consistent values, at least under those
> conditions. Are you telling me "after writing the subset of feature bits
> to the device" somehow includes "in must not mix up ..."?
>
> You said in the hypothesized scenario the driver does not keep it's end
> of the bargain. I asked to point me to the exact points in the spec
> which got violated. I don't see those pointers in your response. Sorry.
> So I'm asking again, can you please tell me which section, which
> paragraph of the spec, or some linked spec is violated?
>
> Or did I misunderstand you? Were you not claiming that the given case
> the driver would be in violation of the spec?

No, neither the driver nor the device are in violation of the spec. The
driver gets what it is supposed to get; if it cannot deal with that,
then it is a bug within the driver.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Tue, Jan 25, 2022 at 03:57:30PM +0100, Cornelia Huck wrote:
> On Mon, Jan 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > It's true. However, that's a generic rule and specific devices
> > can differ. and if you look at specific instances, you will
> > see that they typically require that the feature is negotiated.
> > For example:
> >
> > \item If the VIRTIO_BLK_F_BLK_SIZE feature is negotiated,
> >   \field{blk_size} can be read to determine the optimal sector size
> >   for the driver to use.
> >
> > so a driver reading blk_size before FEATURES_OK is actually using
> > an undocumented aspect of the behaviour.
> >
> > compare this to MTU:
> >
> > \item[VIRTIO_NET_F_MTU(3)] Device maximum MTU reporting is supported. If
> >     offered by the device, device advises driver about the value of
> >     its maximum MTU. If negotiated, the driver uses \field{mtu} as
> >     the maximum MTU value.
> >
> > from which it's clear that it can be read even if not
> > negotiated.
> >
> > So from that point of view, we are actually relaxing the requirements,
> > and yes we'll need to carefully go over each instance of
> > "offered".
> > I thought I did, but now I did a quick grep again and I found instances
> > in virtio-iommu.tex and virtio-gpio.tex
> > Both of these are new, so I think we can just fix them to "negotiated".
> 
> Considering this, should we actually add any requirements as to when
> fields are valid? The device may present a value that is always valid if
> it offers the corresponding feature, or that value may change depending
> on what is actually negotiated by the driver. Maybe a statement that the
> actual validity of a field's value before negotiation is device type
> dependant?

Yes, that makes sense.

-- 
MST

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Jean-Philippe Brucker]

On Mon, Jan 24, 2022 at 04:42:35PM -0500, Michael S. Tsirkin wrote:
> So from that point of view, we are actually relaxing the requirements,
> and yes we'll need to carefully go over each instance of
> "offered".
> I thought I did, but now I did a quick grep again and I found instances
> in virtio-iommu.tex and virtio-gpio.tex
> Both of these are new, so I think we can just fix them to "negotiated".
> 
> Jean-Philippe, Viresh, what do you think? Would it be a problem to
> replace offered with negotiated, and restrict iommu and gpio drivers to
> access config space after FEATURES_OK?

No problem for iommu. The config space fields domain_range and input_range
are not useful to the driver before DRIVER_OK. Reading field bypass
earlier could be useful for debugging, but in that case going a few more
steps into device initialization wouldn't hurt.

Thanks,
Jean

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Tue, 25 Jan 2022 16:22:05 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Jan 24 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Fri, 21 Jan 2022 17:05:07 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Fri, Jan 21 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> >>   
> >> > On Thu, 20 Jan 2022 14:05:37 +0100
> >> > Cornelia Huck <cohuck@redhat.com> wrote:
> >> >    
> >> >> On Wed, Jan 19 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >>     
> >> >> > On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:      
> >> >> >> 3) Features are written in 32 bit chunks. My guess is that what we
> >> >> >> want for "features acknowledged is": driver has written each chunk it
> >> >> >> knows about at least once. Of course, the question arises, what should
> >> >> >> the device do if config is read before that happens.      
> >> >> >
> >> >> > we are lucky that at present it's just mtu of the net device :).      
> >> >> 
> >> >> If, say, the driver writes the first chunk with the MTU bit, then reads
> >> >> the mtu config field, then writes the second chunk with VERSION_1, it
> >> >> should get the mtu in platform-endian (and on the next read after the
> >> >> second chunk has been written, in little endian.) I'd say that this is
> >> >> simply an issue of the driver not serializing its own reads/writes
> >> >> correctly :)
> >> >>     
> >> >
> >> > Point me to the sentence that says the driver did not serialize its own
> >> > reads/writes correctly please!
> >> >
> >> > We can't just come up with requirements when it suits us. Well, we can
> >> > but I argue, that should be the last resort.    
> >> 
> >> What I say is that the driver simply gets things back from the device
> >> that are consistent from the device's point of view.  
> >
> > That is what I'm concerned about. The current proposal stipulates, that
> > the device needs to deliver a consistent config space, when it is in an
> > inconsistent state, in a sense that the feature set, that has been
> > written, is not supported. For example if I had a feature X2 that depends
> > on bit F2 which in turn depends on feature X1 and bit F1, and has a
> > config field C2 that is conditional on F2 and needs to be computed during
> > initialization of X1 and requires private values that were computed
> > during the initialization of the feature X1, I would refuse to
> > initialize X2 if bit F1 is not set, because there is no sane way to
> > do that without initializing X1 first. An thus there is no sane way
> > to compute C2.  
> 
> Well, if the device can't compute it, it can't... this needs to be
> specified on a per-device basis. But I don't see what that has to do
> with the device returning something that makes sense from its point of
> view? 

It is about interface design. We are introducing an ugly intermediate
state for which we mandate the device to provide useful config space
fields.

>It simply needs a specification that defines a default value for
> the field if it hasn't been initialized yet?
> 

I agree, one could introduce an extremal value, or if push comes to
shove a separate field that indicates the validity of the field of
interest, and provide the meta useful information that the actually
useful information is not available (yet, because the driver didn't
do the full dance, which we think it should do, but don't bother to
be explicit about that expectation of ours in the spec).

In practice the drivers we care about are going to write all the
feature chunks they know about anyway. I guess I should not hang myself
up too much on spec quality.

> >  
> >> If it cannot write
> >> a set of bits atomically, that it simply needs to expect that it can get
> >> different values at different times during the writing process. If it
> >> wants consistent values, it must not mix up non-atomic writes and
> >> reads -- but that's nothing special.  
> >
> > IMHO if the spec promises consistent values under certain conditions,
> > then the device needs to deliver consistent values, at least under those
> > conditions. Are you telling me "after writing the subset of feature bits
> > to the device" somehow includes "in must not mix up ..."?
> >
> > You said in the hypothesized scenario the driver does not keep it's end
> > of the bargain. I asked to point me to the exact points in the spec
> > which got violated. I don't see those pointers in your response. Sorry.
> > So I'm asking again, can you please tell me which section, which
> > paragraph of the spec, or some linked spec is violated?
> >
> > Or did I misunderstand you? Were you not claiming that the given case
> > the driver would be in violation of the spec?  
> 
> No, neither the driver nor the device are in violation of the spec. The
> driver gets what it is supposed to get; if it cannot deal with that,
> then it is a bug within the driver.
> 

The driver is supposed to get some parameters of certain features offered
by the device, based on which it should decide if it wants to keep or
fence some of these features. That is the point of early config access,
or? In my opinion, it would be advantageous to write the spec so, that
if both the driver and the device stick to the rules set by the spec, a
favorable outcome (interoperability) is guaranteed. 

Anyway, I have the feeling this discussion aren't very productive, so I
will just fold.

Regards,
Halil

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Halil Pasic]

On Mon, 24 Jan 2022 16:42:35 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Jan 24, 2022 at 05:40:52PM +0100, Halil Pasic wrote:
[..]
> > 
> > So the drivers currently out there, that do the early config read to
> > validate if they support some of the features don't fulfill the new
> > driver requirements. In my reading of the spec, a driver that does
> > not some fulfill the driver requirements is non-conform.  
> 
> It's true. However, that's a generic rule and specific devices
> can differ. and if you look at specific instances, you will
> see that they typically require that the feature is negotiated.

IMHO we should be careful about the spec contradicting itself, so I guess
where a generic rule is overruled by device specific rules, we should
use something like "unless stated otherwise in the device specific part
of the specification ..."

> For example:
> 
> \item If the VIRTIO_BLK_F_BLK_SIZE feature is negotiated,
>   \field{blk_size} can be read to determine the optimal sector size
>   for the driver to use.
> 

There are two ways to read this, and right now you are choosing the
first one:
1) This kind of implicitly tells us, that if VIRTIO_BLK_F_BLK_SIZE
feature not yet negotiated, the the driver can not read  and use
the field to determine the optimal sector size.
2) This is just a positive guarantee. This sentence does not tell us
anything about what happens if feature is not (yet) negotiated. And that
could compose with a generic guarantee in a sense, that this sentence
says nothing about our situation, but the generic rule does give us the
guarantee we need (field is readable and valid). 

> so a driver reading blk_size before FEATURES_OK is actually using
> an undocumented aspect of the behaviour.

Which actually does not even work for all supported platforms! 

According to that, commit 82e89ea077b9 ("virtio-blk: Add validation for
block size in config space") is simply bugous, right?

Should we change the linux code accordingly?

> 
> compare this to MTU:
> 
> \item[VIRTIO_NET_F_MTU(3)] Device maximum MTU reporting is supported. If
>     offered by the device, device advises driver about the value of
>     its maximum MTU. If negotiated, the driver uses \field{mtu} as
>     the maximum MTU value.
> 
> from which it's clear that it can be read even if not
> negotiated.

I agree, this was probably the intention, nevertheless we should be
careful to express our intention in a non-ambiguous way (see above).

So is the current QEMU implementation of virtio-net is violating the
spec under certain circumstances? I mean when \field{mtu} is read,
before F_VERSION_1was written, ,  F_VERSION_1 was offered and guest
endiannes is big-endian. 

> 
> So from that point of view, we are actually relaxing the requirements,
> and yes we'll need to carefully go over each instance of
> "offered".

Which requirements do you mean here? The device requirements or the
driver requirements?

> I thought I did, but now I did a quick grep again and I found instances
> in virtio-iommu.tex and virtio-gpio.tex
> Both of these are new, so I think we can just fix them to "negotiated".
> 

[..]

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Cornelia Huck]

[sorry about resurrecting an undead thread; I'm trying to collect some
editorial changes]

On Wed, Jan 26 2022, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:

> On Mon, Jan 24, 2022 at 04:42:35PM -0500, Michael S. Tsirkin wrote:
>> So from that point of view, we are actually relaxing the requirements,
>> and yes we'll need to carefully go over each instance of
>> "offered".
>> I thought I did, but now I did a quick grep again and I found instances
>> in virtio-iommu.tex and virtio-gpio.tex
>> Both of these are new, so I think we can just fix them to "negotiated".
>> 
>> Jean-Philippe, Viresh, what do you think? Would it be a problem to
>> replace offered with negotiated, and restrict iommu and gpio drivers to
>> access config space after FEATURES_OK?
>
> No problem for iommu. The config space fields domain_range and input_range
> are not useful to the driver before DRIVER_OK. Reading field bypass
> earlier could be useful for debugging, but in that case going a few more
> steps into device initialization wouldn't hurt.

For domain_range and input_range, offered->negotiated looks like the
right thing to do. However, I'm a bit unsure about bypass. We currently
have:

"
An endpoint is in bypass mode if:
\begin{itemize}
  \item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and:
    \begin{itemize}
      \item config field \field{bypass} is 1 and the endpoint is
        not attached to a domain. This applies even if the driver
        does not accept the VIRTIO_IOMMU_F_BYPASS_CONFIG feature
        and the device initializes field \field{bypass} to 1.

        or
      \item the endpoint is attached to a domain with
        VIRTIO_IOMMU_ATTACH_F_BYPASS.
    \end{itemize}
"

The "This applies even if..." clause refers to "offered, but not
accepted". I assume we want to do offered->negotiated even here, but
wanted to double-check.


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [virtio] [PATCH] virtio: clarify feature negotiation

[Michael S. Tsirkin]

On Thu, Apr 07, 2022 at 04:28:03PM +0200, Cornelia Huck wrote:
> [sorry about resurrecting an undead thread; I'm trying to collect some
> editorial changes]
> 
> On Wed, Jan 26 2022, Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> 
> > On Mon, Jan 24, 2022 at 04:42:35PM -0500, Michael S. Tsirkin wrote:
> >> So from that point of view, we are actually relaxing the requirements,
> >> and yes we'll need to carefully go over each instance of
> >> "offered".
> >> I thought I did, but now I did a quick grep again and I found instances
> >> in virtio-iommu.tex and virtio-gpio.tex
> >> Both of these are new, so I think we can just fix them to "negotiated".
> >> 
> >> Jean-Philippe, Viresh, what do you think? Would it be a problem to
> >> replace offered with negotiated, and restrict iommu and gpio drivers to
> >> access config space after FEATURES_OK?
> >
> > No problem for iommu. The config space fields domain_range and input_range
> > are not useful to the driver before DRIVER_OK. Reading field bypass
> > earlier could be useful for debugging, but in that case going a few more
> > steps into device initialization wouldn't hurt.
> 
> For domain_range and input_range, offered->negotiated looks like the
> right thing to do. However, I'm a bit unsure about bypass. We currently
> have:
> 
> "
> An endpoint is in bypass mode if:
> \begin{itemize}
>   \item the VIRTIO_IOMMU_F_BYPASS_CONFIG feature is offered and:
>     \begin{itemize}
>       \item config field \field{bypass} is 1 and the endpoint is
>         not attached to a domain. This applies even if the driver
>         does not accept the VIRTIO_IOMMU_F_BYPASS_CONFIG feature
>         and the device initializes field \field{bypass} to 1.
> 
>         or
>       \item the endpoint is attached to a domain with
>         VIRTIO_IOMMU_ATTACH_F_BYPASS.
>     \end{itemize}
> "
> 
> The "This applies even if..." clause refers to "offered, but not
> accepted". I assume we want to do offered->negotiated even here, but
> wanted to double-check.

Negotiated is same as accepted, I think it's good as is

-- 
MST