Michael S. Tsirkin
2021-Oct-03 07:26 UTC
[RFC PATCH 1/1] virtio: write back features before verify
On Sun, Oct 03, 2021 at 02:42:30AM -0400, Michael S. Tsirkin wrote:> On Sun, Oct 03, 2021 at 07:00:30AM +0200, Halil Pasic wrote: > > On Sat, 2 Oct 2021 14:20:47 -0400 > > "Michael S. Tsirkin" <mst at redhat.com> wrote: > > > > > > >From my perspective the problem is that the version of the device > > > > remains in limbo as long as the features have not yet been finalized, > > > > which means that the endianness of the config space remains in limbo as > > > > well. Both device and driver might come to different conclusions. > > > > > > Version === legacy versus modern? > > > It is true that feature negotiation can not be used by device to decide that > > > question simply because it happens too late. > > > So let's not use it for that then ;) > > > > > > Yes we have VERSION_1 which looks like it should allow this, but > > > unfortunately it only helps with that for the driver, not the device. > > > > > > In practice legacy versus modern has to be determined by > > > transport specific versioning, luckily we have that for all > > > specified transports (can't say what happens with rproc). > > > > So if we look at ccw, you say that the revision negotiation already > > determines whether VERSION_1 is negotiated or not, and the > > feature bit VERSION_1 is superfluous? > > > > That would also imply, that > > 1) if revision > 0 was negotiated then the device must offer VERSION_1 > > 2) if revision > 0 was negotiated and the driver cleared VERSION_1 > > the device must refuse to operate. > > 3) if revision > 0 was negotiated then the driver should reject > > to drive a device if it does not offer VERSION_1 > > 4) if revision > 0 was negotiated the driver must accept VERSION_1 > > 5) if revision > 0 was *not* negotiated then the device should not offer > > VERSION_1 because at this point it is already certain that the device > > can not act in accordance to the virtio 1.0 or higher interface. > > > > Does that sound about right? > > To me, it does. > > > IMHO we should also change > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-160003 > > and the definition of VERSION_1 because both sides have to know what is > > going on before features are fully negotiated. Or? > > > > Regards, > > Halil > > > > I guess so. And I guess we need transport-specific sections > describing this behaviour for each transport. > > So something like this, for starters?Sent too early. So here's what I propose. Could you pls take a look and if you like this, post a ccw section? There's also an attempt to prevent fallback from modern to legacy here since if driver does fallback then failing FEATURES_OK can't work properly. That's a separate issue, will be a separate patch when I post this for consideration by the TC. diff --git a/content.tex b/content.tex index 1398390..06271f4 100644 --- a/content.tex +++ b/content.tex @@ -140,10 +140,13 @@ \subsection{Legacy Interface: A Note on Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits / Legacy Interface: A Note on Feature Bits} -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 drivers MAY support operating legacy devices. +Transitional devices MAY support operation by legacy drivers. + +Transitional drivers MUST detect legacy devices in a way that is +transport specific. +Transitional devices MUST detect legacy drivers in a way that +is transport specific. In this case device is used through the legacy interface. @@ -160,6 +163,33 @@ \subsection{Legacy Interface: A Note on Feature Specification text within these sections generally does not apply to non-transitional devices. +\begin{note} +The device offers different features when used through +the legacy interface and when operated in accordance with this +specification. +\end{note} + +Transitional drivers MUST use Devices only through the legacy interface +if the feature bit VIRTIO_F_VERSION_1 is not offered. +Transitional devices MUST NOT offer VIRTIO_F_VERSION_1 when used through +the legacy interface. + +When the driver uses a device through the legacy interface, then it +MUST only accept the features the device offered through the +legacy interface. + +When used through the legacy interface, the device SHOULD +validate that the driver only accepted the features it +offered through the legacy interface. + +When operating a transitional device, a transitional driver +SHOULD NOT use the device through the legacy interface if +operation through the modern interface has failed. +In particular, a transitional driver +SHOULD NOT fall back to using the device through the +legacy interface if feature negotiation failed +(since that would defeat the purpose of the FEATURES_OK bit). + \section{Notifications}\label{sec:Basic Facilities of a Virtio Device / Notifications} @@ -1003,6 +1033,12 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport The driver MUST NOT write a 0 to \field{queue_enable}. +\paragraph}{Legacy Interface: Common configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interface: Common configuration structure layout} +Transitional drivers SHOULD detect legacy devices by detecting +that the device has the Transitional PCI Device ID in +the range 0x1000 to 0x103f and lacks a VIRTIO_PCI_CAP_COMMON_CFG +capability specifying the location of a common configuration structure. + \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG @@ -1288,6 +1324,10 @@ \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transitional devices MUST present part of configuration registers in a legacy configuration structure in BAR0 in the first I/O region of the PCI device, as documented below. + +Transitional devices SHOULD detect legacy drivers by detecting +access to the legacy configuration structure. + When using the legacy interface, transitional drivers MUST use the legacy configuration structure in BAR0 in the first I/O region of the PCI device, as documented below.
Cornelia Huck
2021-Oct-04 12:01 UTC
[RFC PATCH 1/1] virtio: write back features before verify
On Sun, Oct 03 2021, "Michael S. Tsirkin" <mst at redhat.com> wrote:> Sent too early. So here's what I propose. Could you pls take a look > and if you like this, post a ccw section?We have not talked about the mmio transport so far, but I guess it should be fine as legacy and standard are separated.> There's also an attempt to prevent fallback from modern to legacy > here since if driver does fallback then failing FEATURES_OK can't work > properly. > That's a separate issue, will be a separate patch when I post > this for consideration by the TC. > > > diff --git a/content.tex b/content.tex > index 1398390..06271f4 100644 > --- a/content.tex > +++ b/content.tex > @@ -140,10 +140,13 @@ \subsection{Legacy Interface: A Note on Feature > Bits}\label{sec:Basic Facilities of a Virtio Device / Feature > Bits / Legacy Interface: A Note on Feature Bits} > > -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 drivers MAY support operating legacy devices. > +Transitional devices MAY support operation by legacy drivers.Why 'MAY'? Isn't the whole point of transitional that it can deal with both?> + > +Transitional drivers MUST detect legacy devices in a way that is > +transport specific. > +Transitional devices MUST detect legacy drivers in a way that > +is transport specific. > > In this case device is used through the legacy interface. > > @@ -160,6 +163,33 @@ \subsection{Legacy Interface: A Note on Feature > Specification text within these sections generally does not apply > to non-transitional devices. > > +\begin{note} > +The device offers different features when used through > +the legacy interface and when operated in accordance with this > +specification. > +\end{note} > + > +Transitional drivers MUST use Devices only through the legacy interfaces/Devices only through the legacy interface/devices through the legacy interface only/ ?> +if the feature bit VIRTIO_F_VERSION_1 is not offered. > +Transitional devices MUST NOT offer VIRTIO_F_VERSION_1 when used through > +the legacy interface. > + > +When the driver uses a device through the legacy interface, then it > +MUST only accept the features the device offered through the > +legacy interface. > + > +When used through the legacy interface, the device SHOULD > +validate that the driver only accepted the features it > +offered through the legacy interface. > + > +When operating a transitional device, a transitional driver > +SHOULD NOT use the device through the legacy interface if > +operation through the modern interface has failed. > +In particular, a transitional driver > +SHOULD NOT fall back to using the device through the > +legacy interface if feature negotiation failed > +(since that would defeat the purpose of the FEATURES_OK bit). > + > \section{Notifications}\label{sec:Basic Facilities of a Virtio Device > / Notifications} > > @@ -1003,6 +1033,12 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport > > The driver MUST NOT write a 0 to \field{queue_enable}. > > +\paragraph}{Legacy Interface: Common configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interface: Common configuration structure layout} > +Transitional drivers SHOULD detect legacy devices by detecting > +that the device has the Transitional PCI Device ID in > +the range 0x1000 to 0x103f and lacks a VIRTIO_PCI_CAP_COMMON_CFG > +capability specifying the location of a common configuration structure. > + > \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} > > The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG > @@ -1288,6 +1324,10 @@ \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio > Transitional devices MUST present part of configuration > registers in a legacy configuration structure in BAR0 in the first I/O > region of the PCI device, as documented below. > + > +Transitional devices SHOULD detect legacy drivers by detecting > +access to the legacy configuration structure. > + > When using the legacy interface, transitional drivers > MUST use the legacy configuration structure in BAR0 in the first > I/O region of the PCI device, as documented below.Generally, looks good to me.