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