virtio: clarify feature negotiation

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 <[email protected]>

---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

content.tex +0 -0

index 8439cc1..4f46ce9 100644

content.tex +7 -2

 \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.

Quoted 29 lines · 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 <[email protected]>

---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

content.tex +0 -0

index 8439cc1..4f46ce9 100644

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 301

Quoted 5 lines

content.tex +1 -1

@@ -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."

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 5 lines

content.tex +2 -2

 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.

Quoted 20 lines

content.tex +7 -2

 \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.

content.tex \subsection{Device configuration layout}\label{sec:Device Types / Network Device · line 3308

Quoted 8 lines

content.tex +5 -0

@@ -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?

Quoted 41 lines · Cornelia Huck

Quoted 29 lines · 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 <[email protected]>

---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

content.tex +0 -0

index 8439cc1..4f46ce9 100644

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 301

Quoted 5 lines

content.tex +1 -1

@@ -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.

Quoted 5 lines

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.

Quoted 8 lines

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 5 lines

content.tex +2 -2

 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"

Quoted 34 lines

Quoted 23 lines

content.tex +7 -2

 \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.

content.tex \subsection{Device configuration layout}\label{sec:Device Types / Network Device · line 3308

Quoted 8 lines

content.tex +5 -0

@@ -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.

Quoted 1 line

Also note this in the patch description?

thanks

Quoted 42 lines · On Mon, 17 Jan 2022 17:26:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 41 lines · Cornelia Huck

Quoted 29 lines · 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 <[email protected]>

---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

content.tex +0 -0

index 8439cc1..4f46ce9 100644

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 301

Quoted 5 lines

content.tex +1 -1

@@ -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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 12 lines

Quoted 8 lines

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 5 lines

content.tex +2 -2

 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:

Quoted 5 lines

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

Quoted 38 lines

Quoted 34 lines

Quoted 23 lines

content.tex +7 -2

 \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.

content.tex \subsection{Device configuration layout}\label{sec:Device Types / Network Device · line 3308

Quoted 8 lines

content.tex +5 -0

@@ -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.

Quoted 1 line

Also note this in the patch description?

thanks

Quoted 55 lines · Halil Pasic

Quoted 42 lines · On Mon, 17 Jan 2022 17:26:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 41 lines · Cornelia Huck

Quoted 29 lines · 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 <[email protected]>

---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

content.tex +0 -0

index 8439cc1..4f46ce9 100644

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 301

Quoted 5 lines

content.tex +1 -1

@@ -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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 9 lines

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.

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 7 lines

content.tex +2 -2

 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:

Quoted 8 lines

Quoted 5 lines

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.

Quoted 66 lines · Michael S. Tsirkin

Quoted 64 lines · Halil Pasic

Quoted 42 lines · On Mon, 17 Jan 2022 17:26:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 41 lines · Cornelia Huck

Quoted 29 lines · 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 <[email protected]>

---
 content.tex | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

content.tex +0 -0

index 8439cc1..4f46ce9 100644

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 301

Quoted 5 lines

content.tex +1 -1

@@ -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.

Quoted 9 lines

Quoted 5 lines

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.

Quoted 3 lines

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?

Quoted 30 lines

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 7 lines

content.tex +2 -2

 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:

Quoted 8 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

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".

Quoted 13 lines

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.

Quoted 5 lines

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...

Quoted 2 lines

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 :(

Quoted 36 lines · Cornelia Huck

Quoted 34 lines · Michael S. Tsirkin

Quoted 17 lines · Halil Pasic

Quoted 14 lines · On Mon, 17 Jan 2022 17:26:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 9 lines · Cornelia Huck

Review

Quoted 5 lines · Michael S. Tsirkin

field before FEATURES_OK is set by the driver. This includes fields which are

(quoted diff) +2 -2

-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:

Quoted 8 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

and

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 13 lines

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.

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 15 lines

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?)

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 46 lines · Michael S. Tsirkin

Quoted 36 lines · Cornelia Huck

Quoted 34 lines · Michael S. Tsirkin

Quoted 17 lines · Halil Pasic

Quoted 14 lines · On Mon, 17 Jan 2022 17:26:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 9 lines · Cornelia Huck

Review

Quoted 5 lines · Michael S. Tsirkin

field before FEATURES_OK is set by the driver. This includes fields which are

(quoted diff) +2 -2

-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:

Quoted 8 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 51 lines

Quoted 6 lines

Quoted 6 lines

and

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 25 lines

Quoted 13 lines

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.

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 23 lines

Quoted 15 lines

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?)

Quoted 6 lines

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...

Quoted 3 lines

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."

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

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 :(

Quoted 4 lines

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.

Quoted 1 line · Cornelia Huck

On Wed, Jan 19 2022, "Michael S. Tsirkin" <[email protected]> wrote:

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 13 lines

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?)

Quoted 6 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Subject: virtio: clarify feature negotiation

content.tex +0 -0

 \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 /

Quoted 1 line · On Fri, 14 Jan 2022 17:50:51 -0500 "Michael S. Tsirkin" <[email protected]> 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.
[..]

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 11 lines

content.tex +3 -3

@@ -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".

Quoted 5 lines

content.tex +0 -0

 \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
 

content.tex \section{Device Initialization}\label{sec:General Initialization And Device Oper · line 509

Quoted 11 lines

content.tex +7 -2

 \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

[..]

Quoted 5 lines · Halil Pasic

Quoted 3 lines · On Fri, 14 Jan 2022 17:50:51 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Subject: virtio: clarify feature negotiation

(quoted diff) +0 -0

@@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
 

Quoted 15 lines

content.tex \section{Device Initialization}\label{sec:General Initialization And Device Oper · line 509

Quoted 11 lines

content.tex +7 -2

 \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?

Quoted 7 lines

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.

Quoted 5 lines

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

Quoted 5 lines

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.

Quoted 15 lines

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?

Quoted 5 lines

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 :).

Quoted 7 lines

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.

Quoted 7 lines

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.

Quoted 4 lines

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 ...

Quoted 7 lines

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?

Quoted 22 lines · Michael S. Tsirkin

Quoted 5 lines · Halil Pasic

Quoted 3 lines · On Fri, 14 Jan 2022 17:50:51 -0500 "Michael S. Tsirkin" <[email protected]> wrote:

Subject: virtio: clarify feature negotiation

(quoted diff) +0 -0

@@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General Initialization And Device Oper
 

Quoted 15 lines

content.tex \section{Device Initialization}\label{sec:General Initialization And Device Oper · line 509

Quoted 11 lines

content.tex +7 -2

 \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.

Quoted 1 line

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).

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines

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

Quoted 7 lines

Quoted 5 lines

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.

Quoted 16 lines

Quoted 15 lines

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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 5 lines

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.

Quoted 1 line

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 :)

Quoted 4 lines · On Thu, 20 Jan 2022 14:05:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 1 line · Michael S. Tsirkin

On Wed, Jan 19, 2022 at 01:23:19PM +0100, Halil Pasic wrote:

Quoted 12 lines

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.

Quoted 5 lines

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.

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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.

Quoted 9 lines

Quoted 1 line

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.

Quoted 20 lines · Halil Pasic

Quoted 16 lines · On Thu, 20 Jan 2022 14:05:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 1 line · Michael S. Tsirkin

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.

Quoted 6 lines

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.

Quoted 13 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 23 lines

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.

Quoted 15 lines

Quoted 7 lines

Quoted 5 lines

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 :)

Quoted 5 lines

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.

Quoted 22 lines · On Fri, 21 Jan 2022 17:05:07 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 20 lines · Halil Pasic

Quoted 16 lines · On Thu, 20 Jan 2022 14:05:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 1 line · Michael S. Tsirkin

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.

Quoted 8 lines

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 13 lines

Quoted 13 lines

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.

Quoted 5 lines

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.

Quoted 9 lines

Quoted 4 lines

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?

Quoted 30 lines

Quoted 22 lines

Quoted 15 lines

Quoted 15 lines

Quoted 14 lines

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 4 lines

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"?

Quoted 23 lines

Quoted 23 lines

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.

Quoted 15 lines

Quoted 7 lines

Quoted 5 lines

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 :)

Quoted 8 lines

Quoted 5 lines

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.

Quoted 5 lines

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

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".

Quoted 22 lines · Michael S. Tsirkin

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.

Quoted 6 lines

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?

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?

Quoted 7 lines · Michael S. Tsirkin

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".

Quoted 3 lines

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.

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

Quoted 8 lines · Jean-Philippe Brucker

Quoted 7 lines · Michael S. Tsirkin

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".

Quoted 8 lines

Quoted 3 lines

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.

On Mon, Jan 24, 2022 at 05:40:52PM +0100, Halil Pasic wrote:

Quoted 5 lines

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".

Quoted 23 lines · On Fri, 21 Jan 2022 17:05:07 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 21 lines · Halil Pasic

Quoted 17 lines · On Thu, 20 Jan 2022 14:05:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines · Halil Pasic

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 :)

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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?

Quoted 35 lines · Halil Pasic

Quoted 23 lines · On Fri, 21 Jan 2022 17:05:07 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 21 lines · Halil Pasic

Quoted 17 lines · On Thu, 20 Jan 2022 14:05:37 +0100 Cornelia Huck <[email protected]> wrote:

Quoted 7 lines · Michael S. Tsirkin

Quoted 4 lines · Halil Pasic

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 :)

Quoted 8 lines

Quoted 5 lines

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.

Quoted 11 lines

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?

Quoted 18 lines

Quoted 6 lines

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?

Quoted 2 lines

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.

Quoted 33 lines · Halil Pasic

Quoted 1 line · On Fri, 14 Jan 2022 17:50:51 -0500 "Michael S. Tsirkin" <[email protected]> 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.
[..]

content.tex \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Devi · line 304

Quoted 11 lines

content.tex +3 -3

@@ -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".

Quoted 5 lines

content.tex +0 -0

 \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
 

Quoted 15 lines

content.tex \section{Device Initialization}\label{sec:General Initialization And Device Oper · line 509

Quoted 11 lines

content.tex +7 -2

 \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?

Quoted 12 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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;

Quoted 8 lines

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?

Quoted 5 lines

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 :).

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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 ...

Quoted 8 lines

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.