Re: [PATCH v2] content: clarify feature negotiation terminology and init sequence

On Fri, Apr 24, 2026 at 12:34=E2=80=AFAM Michael S. Tsirkin <[email protected]=
> wrote:
>
> Make several clarifications to the init sequence documentation:
>
> The Linux virtio core (drivers/virtio/virtio.c) initializes devices
> as follows:
>   1. Intersect driver and device feature bits
>   2. finalize_features() - write accepted features to the device
>   3. drv->validate() - read config space, may clear feature bits
>      (e.g. virtio-net clears VIRTIO_NET_F_MTU if mtu < MIN_MTU,
>      balloon clears PAGE_POISON if guest does not init pages)
>   4. If validate changed any features, finalize_features() again
>   5. virtio_features_ok() - set FEATURES_OK, confirm with device
>
> this allows the device to know which fields will be read:
> recommend this in the spec.
>
> Legacy driver detection is specified using a mechanism that
> does not work on all transports. Make it clear that it's an
> example: what matters is that devices do detection in some way
> and are compatible with legacy drivers.
>
> Define "negotiated" for features confirmed via FEATURES_OK.
>
> "acknowledged" is used as a synonym for "accepted", but only in two
> places. Just use "accepted" consistently.
>
> Spec describes multiple moving pieces then ends with "before accepting
> it" - vague, and is overloading "accept". Replace with a reference to
> FEATURES_OK.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/241
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> ---
>
> changes:
> address review comments by Stefan
>
>  content.tex | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index 5de811f..0f4d662 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -39,7 +39,7 @@ \section{\field{Device Status} Field}\label{sec:Basic F=
acilities of a Virtio Dev
>  \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
>    drive the device.
>
> -\item[FEATURES_OK (8)] Indicates that the driver has acknowledged all th=
e
> +\item[FEATURES_OK (8)] Indicates that the driver has accepted all the
>    features it understands, and feature negotiation is complete.
>
>  \item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that =
the
> @@ -89,13 +89,16 @@ \section{Feature Bits}\label{sec:Basic Facilities of =
a Virtio Device / Feature B
>
>  Each virtio device offers all the features it understands.  During
>  device initialization, the driver reads this and tells the device the
> -subset that it accepts.  The only way to renegotiate is to reset
> -the device.
> +subset that it accepts.  The device validates this subset and
> +either completes the negotiation successfully (the last subset of featur=
es
> +that the driver accepted is considered negotiated then) or fails,
> +leaving the feature negotiation incomplete. Once the negotiation is
> +complete, the only way to renegotiate is to reset the device.
>
>  This allows for forwards and backwards compatibility: if the device is
>  enhanced with a new feature bit, older drivers will not write that
>  feature bit back to the device.  Similarly, if a driver is enhanced with=
 a feature
> -that the device doesn't support, it see the new feature is not offered.
> +that the device doesn't support, it will see that the new feature is not=
 offered.
>
>  Feature bits are allocated as follows:
>
> @@ -189,8 +192,8 @@ \subsection{Legacy Interface: A Note on Feature
>
>  Transitional Drivers MUST detect Legacy Devices by detecting that
>  the feature bit VIRTIO_F_VERSION_1 is not offered.
> -Transitional devices MUST detect Legacy drivers by detecting that
> -VIRTIO_F_VERSION_1 has not been acknowledged by the driver.
> +Transitional devices MUST detect Legacy drivers, e.g. by detecting that
> +VIRTIO_F_VERSION_1 has not been accepted by the driver.
>
>  In this case device is used through the legacy interface.
>
> @@ -314,6 +317,11 @@ \section{Device Configuration Space}\label{sec:Basic=
 Facilities of a Virtio Devi
>  greater than the specified 8-bit size.
>  \end{note}
>
> +\drivernormative{\subsection}{Device Configuration Space}{Basic Faciliti=
es of a Virtio Device / Device Configuration Space}
> +Before reading a device-specific configuration field that is
> +conditional on a feature bit, the driver SHOULD first accept
> +that feature bit.
> +
>  \devicenormative{\subsection}{Device Configuration Space}{Basic Faciliti=
es of a Virtio Device / Device Configuration Space}
>  The device MUST allow reading of any device-specific configuration
>  field before FEATURES_OK is set by the driver.  This includes fields whi=
ch are
> @@ -530,7 +538,13 @@ \section{Device Initialization}\label{sec:General In=
itialization 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 configuratio=
n fields to check that it can support the device before accepting it.
> +   driver MAY read (but MUST NOT write) the device-specific configuratio=
n
> +   fields to check that it can support the device before setting FEATURE=
S_OK.
> +   The driver SHOULD accept feature bits before reading configuration

As I understand it, this should be SHALL (mandatory requirement) -- or
MUST, not SHOULD (which reads as a recommendation). Otherwise it would
violate the spec, and probably cause a hard BUG() panic in the kernel
if the driver queries a feature it never declared in its
feature_table, or result in silent data corruption if it bypasses
feature checks entirely and reads conditional config fields directly.

> +   fields conditional on them.  The driver MAY then accept a different
> +   subset of feature bits (e.g., deciding, based on the configuration
> +   fields, not to use a certain feature), tell the device about the
> +   updated subset, and repeat this process.
>
>  \item\label{itm:General Initialization And Device Operation / Device Ini=
tialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver =
MUST NOT accept
>     new feature bits after this step.
> --
> MST
>
>