On Mon, Jan 17, 2022 at 04:39:32PM +0100, Cornelia Huck wrote:
> On Fri, Jan 14 2022, "Michael S. Tsirkin" <[email protected]> wrote:
>
> > We state that drivers can access config fields before FEATURES_OK. We do
> > however need them to write the features before accessing config
> > otherwise our forward compatibility won't work.
> >
> > We require devices to allow access to config space if they
> > offer a feature bit as long as that has been offered, but
> > this can't work of course since we don't know what value
> > does driver expect. What we should have said is "as long
> > as it has been acknowledged".
> >
> > While at it, clarify that drivers can write features
> > repeatedly as long as FEATURES_OK have note been
> > acknowledged.
> >
> > Note: if device denies FEATURES_OK then there's no reason
> > not to allow the driver to try with another set of features.
> > Current drivers do not need such a capability, so leave this
> > idea for another day.
> >
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> > content.tex | 21 ++++++++++++++++-----
> > 1 file changed, 16 insertions(+), 5 deletions(-)
> >
> > diff --git a/content.tex b/content.tex
> > index 8439cc1..4f46ce9 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -298,10 +298,10 @@ \section{Device Configuration Space}\label{sec:Basic
> > Facilities of a Virtio Devi
> > \end{note}
> >
> > \devicenormative{\subsection}{Device Configuration Space}{Basic Facilities
> > of a Virtio Device / Device Configuration Space}
> > -The device MUST allow reading of any device-specific configuration
> > +The device SHOULD allow reading of any device-specific configuration
>
> Why 'SHOULD'? I think the device MUST allow it for features that have
> already been written by the driver, given that is always a subset of the
> features that have been offered by the device.
>
> Maybe "The device MUST allow reading of any device-specific
> configuration field that is not depending on a feature bit, and any
> device-specific configuration field that is conditional on a feature bit
> that has been written back by the driver, before FEATURES_OK is set by
> the driver. It MAY allow reading of any other configuration field."
Like that.
> > field before FEATURES_OK is set by the driver. This includes fields which
> > are
> > -conditional on feature bits, as long as those feature bits are offered
> > -by the device.
> > +conditional on feature bits, as long as those feature bits have
> > +been acknowledged by the driver.
>
> Better 'written back'? To me, 'acknowledged' carries overtones of
> 'negotiated', and that is not meaningful before FEATURES_OK.
OK.
In that case we should also change this:
Device Initialization / Read feature bits} Read device feature bits, and write
the subset of feature bits
understood by the OS and driver to the device.
from "write" to "write back"
> >
> > \subsection{Legacy Interface: A Note on Device Configuration Space
> > endian-ness}\label{sec:Basic Facilities of a Virtio Device / Device
> > Configuration Space / Legacy Interface: A Note on Configuration Space
> > endian-ness}
> >
> > @@ -500,8 +500,13 @@ \section{Device Initialization}\label{sec:General
> > Initialization And Device Oper
> >
> > \item\label{itm:General Initialization And Device Operation /
> > Device Initialization / Read feature bits} Read device feature bits, and
> > write the subset of feature bits
> > - understood by the OS and driver to the device. During this step the
> > - driver MAY read (but MUST NOT write) the device-specific configuration
> > fields to check that it can support the device before accepting it.
> > + understood by the OS and driver to the device. During this
> > + step, after writing the subset of feature bits to the device the
> > + driver MAY read (but MUST NOT write) the device-specific
> > + configuration fields to validate that it can support the device
> > + before accepting it. The driver MAY then repeatedly modify the
> > + subset as appropriate, write the new subset and repeat the
> > + validation, any number of times.
> >
> > \item\label{itm:General Initialization And Device Operation / Device
> > Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit. The
> > driver MUST NOT accept
> > new feature bits after this step.
> > @@ -3296,6 +3301,12 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device
> > \field{duplex} fields as long as VIRTIO_NET_S_LINK_UP is set in
> > the \field{status}.
> >
> > +If the device offers VIRTIO_NET_F_MTU, the device SHOULD allow
> > +reading of \field{mtu} before FEATURES_OK is set by the driver
> > +even if VIRTIO_NET_F_MTU has not been acknowledged by the driver.
> > +This is to support existing drivers which access this field
> > +before acknowledging features.
>
> Maybe better 'written back', see my comment above.
>
> Also note this in the patch description?
thanks
--
MST
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
