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

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Apr 24, 2026 at 02:05:46PM +0200, Albert Esteve wrote:
> On Fri, Apr 24, 2026 at 12:34 AM Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com>
> > ---
> >
> > 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 Facilities 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 the
> > +\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 features
> > +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 Facilities 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 Facilities 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 which are
> > @@ -530,7 +538,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.
> > +   driver MAY read (but MUST NOT write) the device-specific configuration
> > +   fields to check that it can support the device before setting FEATURES_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.


Unfortunately, we never required this in the past.

So we can recommend but we can not make it a MUST now. Devices have to
cope with existing drivers.



> > +   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 Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
> >     new feature bits after this step.
> > --
> > MST
> >
> >