Introduce device group and device management

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented. Do it now.

Suggested-by Halil Pasic <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

content.tex +0 -0

index 3051399..1e91b1d 100644
@@ -114,21 +114,58 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 In particular, new fields in the device configuration space are
 indicated by offering a new feature bit.
 

In particular, this is

content.tex +7 -0

+especially important for features limited to specific transports,
+as enabling these for more transports in future versions of the
+specification is highly likely to require changing the behaviour
+from drivers and devices.  Drivers and devices supporting
+multiple transports need to carefully maintain per-transport
+lists of allowed features.
+

content.tex +5 -0

+
 \devicenormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The device MUST NOT offer a feature which requires another feature
 which was not offered.  The device SHOULD accept any valid subset
 of features the driver accepts, otherwise it MUST fail to set the
 FEATURES_OK \field{device status} bit when the driver writes it.
 
+The device MUST NOT offer feature bits corresponding to features
+it would not support if accepted by the driver (even if the
+driver is prohibited from accepting the feature bits by the
+specification); for the sake of clarity, this refers to feature

Quoted 18 lines · Michael S. Tsirkin

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented. Do it now.

Suggested-by Halil Pasic <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

content.tex +0 -0

index 3051399..1e91b1d 100644
@@ -114,21 +114,58 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 In particular, new fields in the device configuration space are
 indicated by offering a new feature bit.
 

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 126

Quoted 10 lines

content.tex +10 -0

+To keep the feature negotiation mechanism extensible, it is
+important that devices \em{do not} offer feature bits they would
+not be able to handle if accepted by the driver (even though
+according to this version of the specification even if offered
+drivers are not supposed to accept them in the first place),
+while drivers \em{do not} accept feature bits they do not know
+how to handle (even though according to this version of the
+specification devices are not supposed to offer them in the first
+place).  Any reserved and unexpected features are to be by
+preference ignored by the driver.

I think readability would benefit from splitting this up a bit:

"To keep te feature negotiation mechanism extensible, it is important that devices \em{do not} offer any feature bits that they would not be able to handle if the driver accepted them (even though drivers are not supposed to accept them in the first place even if offered, according to this version of the specification.) Likewise, it is important that drivers \em{do not} accept feature bits they do not know how to handle (even though devices are not supposed to offer them in the first place, according to this version of the specification.) The preferred way for handling reserved and unexpected features is that the driver ignores them."

Quoted 13 lines

In particular, this is

content.tex +7 -0

+especially important for features limited to specific transports,
+as enabling these for more transports in future versions of the
+specification is highly likely to require changing the behaviour
+from drivers and devices.  Drivers and devices supporting
+multiple transports need to carefully maintain per-transport
+lists of allowed features.
+
 \drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The driver MUST NOT accept a feature which the device did not offer,
 and MUST NOT accept a feature which requires another feature which was
 not accepted.

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 143

Quoted 5 lines

content.tex +5 -0

+The driver MUST validate the feature bits offered by the device.
+The driver MUST ignore and MUST NOT accept feature bits not
+described in this specification, reserved feature bits and
+feature bits reserved or not supported for the specific transport
+or the specific device type.

I think we can mandate that the driver MUST NOT accept these feature bits; but can we mandate that it MUST ignore them? Alternatively, it could also set the FAILED status bit.

Also, what does "specific device type" mean in this case? Does it refer to a driver that supports devices virtio-foo, virtio-bar, and virtio-baz, and so needs to ignore a feature that is only valid for virtio-foo, but not for virtio-bar or virtio-baz, if it is offered for anything that is not virtio-foo?

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 150

Quoted 8 lines

content.tex +3 -0

+
 The driver SHOULD go into backwards compatibility mode
 if the device does not offer a feature it understands, otherwise MUST
 set the FAILED \field{device status} bit and cease initialization.
 
+By contrast, the driver MUST NOT fail solely because a feature
+it does not understand has been offered by the device.

See above; I'm not sure that is a good requirement. Maybe SHOULD NOT?

Quoted 21 lines

content.tex +12 -0

+
 \devicenormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The device MUST NOT offer a feature which requires another feature
 which was not offered.  The device SHOULD accept any valid subset
 of features the driver accepts, otherwise it MUST fail to set the
 FEATURES_OK \field{device status} bit when the driver writes it.
 
+The device MUST NOT offer feature bits corresponding to features
+it would not support if accepted by the driver (even if the
+driver is prohibited from accepting the feature bits by the
+specification); for the sake of clarity, this refers to feature
+bits not described in this specification, reserved feature bits
+and feature bits reserved or not supported for the specific
+transport or the specific device type, but this does not preclude
+devices written to a future version of this specification from
+offering such feature bits should such a specification have a
+provision for devices to support the corresponding features.
+
 If a device has successfully negotiated a set of features
 at least once (by accepting the FEATURES_OK \field{device
 status} bit during device initialization), then it SHOULD

Quoted 6 lines · Cornelia Huck

Quoted 5 lines · Michael S. Tsirkin

\drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits} The driver MUST NOT accept a feature which the device did not offer,
and MUST NOT accept a feature which requires another feature which was
not accepted.

Quoted 9 lines

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 143

Quoted 5 lines

content.tex +5 -0

+The driver MUST validate the feature bits offered by the device.
+The driver MUST ignore and MUST NOT accept feature bits not
+described in this specification, reserved feature bits and
+feature bits reserved or not supported for the specific transport
+or the specific device type.

I think we can mandate that the driver MUST NOT accept these feature bits; but can we mandate that it MUST ignore them? Alternatively, it could also set the FAILED status bit.

Well if drivers assume there are no
new features then we can't add new ones and forward compatibility breaks. We always assumed drivers ignore features they don't understand. This is why I propose mandating this strongly and I feel it's okay to add such a requirement even at this late stage anything violating this would have been broken many times already. No?

Quoted 6 lines

Also, what does "specific device type" mean in this case? Does it refer to a driver that supports devices virtio-foo, virtio-bar, and virtio-baz, and so needs to ignore a feature that is only valid for virtio-foo, but not for virtio-bar or virtio-baz, if it is offered for anything that is not virtio-foo?

For example, virtio-rng has no features, driver must ignore all device specific feature bits and only negotiate generic and transport specific feature bits.

How would you put it better?

Quoted 17 lines · Michael S. Tsirkin

Quoted 6 lines · Cornelia Huck

Quoted 5 lines · Michael S. Tsirkin

\drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits} The driver MUST NOT accept a feature which the device did not offer,
and MUST NOT accept a feature which requires another feature which was
not accepted.

Quoted 9 lines

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 143

Quoted 5 lines

content.tex +5 -0

+The driver MUST validate the feature bits offered by the device.
+The driver MUST ignore and MUST NOT accept feature bits not
+described in this specification, reserved feature bits and
+feature bits reserved or not supported for the specific transport
+or the specific device type.

I think we can mandate that the driver MUST NOT accept these feature bits; but can we mandate that it MUST ignore them? Alternatively, it could also set the FAILED status bit.

Quoted 7 lines

Well if drivers assume there are no
new features then we can't add new ones and forward compatibility breaks. We always assumed drivers ignore features they don't understand. This is why I propose mandating this strongly and I feel it's okay to add such a requirement even at this late stage anything violating this would have been broken many times already. No?

Hm, right. Let's keep this as MUST.

Quoted 10 lines

Quoted 6 lines

Also, what does "specific device type" mean in this case? Does it refer to a driver that supports devices virtio-foo, virtio-bar, and virtio-baz, and so needs to ignore a feature that is only valid for virtio-foo, but not for virtio-bar or virtio-baz, if it is offered for anything that is not virtio-foo?

For example, virtio-rng has no features, driver must ignore all device specific feature bits and only negotiate generic and transport specific feature bits.

Isn't that the same as a feature bit that the driver does not know? Feature bit 0 means different things for different device types, and for virtio-rng, it's simply an undefined feature (as of now).

Quoted 1 line

How would you put it better?

Let me try:

The driver MUST ignore and MUST NOT accept any feature bit that is \begin{itemize}
\item not described in this specification,
\item marked as reserved,
\item not supported for the specific transport,
\item not defined for the device type.
\end{itemize}

Each device group has a type. For now, define one initial group:

SR-IOV type - PCI SR-IOV virtual functions (VFs) of a given PCI SR-IOV physical function (PF). This group may contain one or more virtio devices.

Each device within a group has a unique identifier. This identifier is the group member identifier.

Note: one can argue both ways whether the new device group handling functionality (this and following patches) is closer
to a new device type or a new transport type.

However, I expect that we will add more features in the near future. To facilitate this as much as possible of the text is located in the new admin chapter.

I did my best to minimize transport-specific text.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 52 insertions(+)
 create mode 100644 admin.tex

admin.tex new file +2 -0

new file mode 100644
index 0000000..4337db0
@@ -0,0 +1,50 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
+

admin.tex new file +6 -0

+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as the owner and includes
+all its SR-IOV virtual functions (VFs) as members (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+The PF device itself is not a member of the group.

Quoted 35 lines · Michael S. Tsirkin

Each device group has a type. For now, define one initial group:

SR-IOV type - PCI SR-IOV virtual functions (VFs) of a given PCI SR-IOV physical function (PF). This group may contain one or more virtio devices.

Each device within a group has a unique identifier. This identifier is the group member identifier.

Note: one can argue both ways whether the new device group handling functionality (this and following patches) is closer
to a new device type or a new transport type.

However, I expect that we will add more features in the near future. To facilitate this as much as possible of the text is located in the new admin chapter.

I did my best to minimize transport-specific text.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 52 insertions(+)
 create mode 100644 admin.tex

admin.tex new file +2 -0

new file mode 100644
index 0000000..4337db0
@@ -0,0 +1,50 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
+

admin.tex line 12

Quoted 10 lines

admin.tex new file +10 -0

+It is occasionally useful to have a device control a group of
+other devices. Terminology used in such cases:
+
+\begin{description}
+\item[Device group]
+        or just group, includes zero or more devices.
+\item[Owner device]
+        or owner, the device controlling the group.
+\item[Member device]
+        a device within a group. Owner device itself is not

s/Owner/The owner/

admin.tex line 15

Quoted 3 lines

admin.tex new file +3 -0

+	a member of the group. In the future it is envisoned that
+	new group types may be introduced where the owner
+	device is a member of the group.

So, shouldn't it rather be: "Whether the owner device itself is a member of the group depends on the type of the group." ?
Or do we want to
prefer the owner _not_ being a member of the group?

admin.tex line 21

Quoted 6 lines

admin.tex new file +6 -0

+\item[Member identifier]
+        each member has this identifier, unique within the group
+	and used to address it through the owner device.
+\item[Group type identifier]
+	specifies what kind of member devices there are in a
+	group, how is the member identifier interpreted

"how the member indentifier is interpreted, ..."

admin.tex line 22

Quoted 1 line

admin.tex new file +1 -0

+	and what kind of control does the owner have.

s/does the owner have/the owner has/

admin.tex line 27

Quoted 5 lines

admin.tex new file +5 -0

+	At the moment, a given owner can control
+	a single group of a given type, thus the type and
+	the owner together identify the group.
+	It is envisioned that this last restriction might be relaxed in the future,
+	with multiple groups of the same type for a given owner.

Hm...

"A given owner may control a single group of a given type (which means that the type and the owner together identify the group), or multiple groups of the same type. Currently, only a single group per owner is supported." ?

Basically, I'd prefer if we spelled out what is possible in general, and then add a comment that only a subset of the possibilities is currently implemented.

admin.tex line 30

Quoted 3 lines

admin.tex new file +3 -0

+\end{description}
+
+A single group type is currently specified:

"The following group types are currently specified:" ?

Less editing once we add a second one :)

admin.tex line 32

Quoted 2 lines

admin.tex new file +2 -0

+\begin{description}
+\item[SR-IOV group type]

Maybe \item[SR-IOV group type (1)] ? It's nice to have the identifier in the title already (like we do for features.)

Quoted 6 lines

admin.tex new file +6 -0

+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as the owner and includes
+all its SR-IOV virtual functions (VFs) as members (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+The PF device itself is not a member of the group.

admin.tex line 48

Quoted 10 lines

admin.tex new file +10 -0

+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value 0x1 to 0xFFFF
+and equals the SR-IOV VF number of the member device (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+Both owner and member devices for this group type use the Virtio
+PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
+\end{description}

Quoted 6 lines · Cornelia Huck

Quoted 2 lines · Michael S. Tsirkin

+\item[Member device]
+ a device within a group. Owner device itself is not

s/Owner/The owner/

Quoted 8 lines

admin.tex line 15

Quoted 3 lines

admin.tex new file +3 -0

+	a member of the group. In the future it is envisoned that
+	new group types may be introduced where the owner
+	device is a member of the group.

So, shouldn't it rather be: "Whether the owner device itself is a member of the group depends on the type of the group." ?
Or do we want to
prefer the owner _not_ being a member of the group?

Maybe I will drop this "In the future" thing. We'll cross that bridge when we get to it.

Quoted 27 lines

admin.tex line 21

Quoted 7 lines

+\item[Member identifier]

admin.tex new file +5 -0

+        each member has this identifier, unique within the group
+	and used to address it through the owner device.
+\item[Group type identifier]
+	specifies what kind of member devices there are in a
+	group, how is the member identifier interpreted

"how the member indentifier is interpreted, ..."

admin.tex line 22

Quoted 1 line

admin.tex new file +1 -0

+	and what kind of control does the owner have.

s/does the owner have/the owner has/

admin.tex line 27

Quoted 5 lines

admin.tex new file +5 -0

+	At the moment, a given owner can control
+	a single group of a given type, thus the type and
+	the owner together identify the group.
+	It is envisioned that this last restriction might be relaxed in the future,
+	with multiple groups of the same type for a given owner.

Hm...

"A given owner may control a single group of a given type (which means that the type and the owner together identify the group), or multiple groups of the same type. Currently, only a single group per owner is supported." ?

Quoted 3 lines

Basically, I'd prefer if we spelled out what is possible in general, and then add a comment that only a subset of the possibilities is currently implemented.

I feel I went too broad here. Let's just drop the "It is envisioned" for now.

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

+to simplify common implementations, these are intentionally

+\hline
+01h & VIRTIO_ADMIN_STATUS_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_INVALID_FIELD & invalid field within command specific data \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_INVALID_GROUP & invalid group identifier was set \\

+\hline
+04h & VIRTIO_ADMIN_STATUS_INVALID_MEM & invalid group member identifier was set \\

+\hline
+other & - & group administration command error \\
+\hline
+\end{tabular}

The admin virtqueues will be the first interface to issue admin commands.

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

We also support more than one admin virtqueue, for QoS and scalability requirements.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  6 ++++--
 2 files changed, 57 insertions(+), 2 deletions(-)

admin.tex +1 -0

index 507e188..bfa63a2 100644
@@ -128,3 +128,56 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 other   & -    & group administration command error  \\
 \hline
 \end{tabular}
+

Device silently ignores any data falling outside

admin.tex +1 -0

+for versioning.

content.tex +2 -2

index 9deacbf..d2fd1de 100644
@@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 \begin{description}
 \item[0 to 23, and 50 to 127] Feature bits for the specific device type
 
-\item[24 to 40] Feature bits reserved for extensions to the queue and
+\item[24 to 41] Feature bits reserved for extensions to the queue and
   feature negotiation mechanisms
 
-\item[41 to 49, and 128 and above] Feature bits reserved for future extensions.
+\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}

Quoted 34 lines · Michael S. Tsirkin

The admin virtqueues will be the first interface to issue admin commands.

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

We also support more than one admin virtqueue, for QoS and scalability requirements.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  6 ++++--
 2 files changed, 57 insertions(+), 2 deletions(-)

admin.tex +6 -0

index 507e188..bfa63a2 100644
@@ -128,3 +128,56 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 other   & -    & group administration command error  \\
 \hline
 \end{tabular}
+
+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
+
+An administration virtqueue of an owner device is used to submit
+group administration commands. An owner device can have more
+than one administration virtqueue.

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 141

Quoted 5 lines

admin.tex +5 -0

+
+Administration virtqueues exists for a certain owner device if
+VIRTIO_F_ADMIN_VQ feature has been negotiated. The index of the
+first administration virtqueue and their number is exposed by the
+owner device in a transport specific manner.

(Do we always expect admin virtqueues to use a range of indices, i.e. no holes? That seems a bit limiting.)

What about

"If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one or more adminstration virtqueues. The number and locations of the administration virtqueues is exposed by the owner device in a transport specific manner."

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 148

Quoted 7 lines

admin.tex +7 -0

+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands
+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Are all admin vqs supposed to be equal? Could a device expose e.g. a high prio admin vq and a normal prio admin vq?

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 152

Quoted 4 lines

admin.tex +4 -0

+
+Administration virtqueues are used as follows:
+\begin{itemize}
+\item Driver submits the command using the \field{struct virtio_admin_cmd}

s/Driver/The driver/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 165

Quoted 13 lines

admin.tex +13 -0

+structure using two buffers: a device-readable one followed by a
+device-writable one.
+\item the device-readable buffer includes fields from \field{opcode}
+through \field{command_specific_data}.
+\item the device-writeable buffer includes fields from \field{status}
+through \field{command_specific_result} inclusive.
+\end{itemize}
+
+It is legal for the driver to submit commands with
+device-writeable and device-readable structures with buffer
+length both shorter or longer than the length of the
+\field{command_specific_data} structure described in this
+specification. Device silently truncates the structures to the

s/Device/The device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 167

Quoted 2 lines

admin.tex +2 -0

+shorter of the two lengths (submitted by driver and described in this
+specification).

"...to either the length submitted by the driver, or the length described in this specification, whichever is shorter."

Quoted 1 line

Device silently ignores any data falling outside

s/Device/The device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 174

Quoted 7 lines

admin.tex +7 -0

+the shorter of the two lengths. Any missing fields are assumed to be
+set to zero.
+
+Similarly, it is legal for the device to use, for a specific
+command, a shorter part of the \field{command_specific_result}
+buffer than the length of the structure described in this
+specification. Driver silently ignores any data falling outside

s/Driver/The driver/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 182

Quoted 8 lines

admin.tex +8 -0

+the used buffer length reported by the device.  Any missing
+fields are assumed to be set to zero.
+
+This simplifies driver implementations since the driver can
+simply maintain a single large C structure for a command and its
+result. As new versions of the specification are designed,
+new fields can be added to the tail of a structure,
+with driver using the full structure without concern

s/driver/the driver/

Quoted 22 lines

admin.tex +1 -0

+for versioning.

content.tex +2 -2

index 9deacbf..d2fd1de 100644
@@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 \begin{description}
 \item[0 to 23, and 50 to 127] Feature bits for the specific device type
 
-\item[24 to 40] Feature bits reserved for extensions to the queue and
+\item[24 to 41] Feature bits reserved for extensions to the queue and
   feature negotiation mechanisms
 
-\item[41 to 49, and 128 and above] Feature bits reserved for future extensions.
+\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}

@@ -6985,6 +6985,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   that the driver can reset a queue individually.
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 

content.tex \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} · line 6988

Quoted 1 line

content.tex +1 -0

+  \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.

"This feature indicates that the device exposes one or more administration virtqueues." ?

Quoted 4 lines

content.tex +1 -0

+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

Quoted 8 lines · Cornelia Huck

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 141

Quoted 4 lines · Michael S. Tsirkin

admin.tex +4 -0

+Administration virtqueues exists for a certain owner device if
+VIRTIO_F_ADMIN_VQ feature has been negotiated. The index of the
+first administration virtqueue and their number is exposed by the
+owner device in a transport specific manner.

(Do we always expect admin virtqueues to use a range of indices, i.e. no holes? That seems a bit limiting.)

For the device?
I frankly feel it's enough, yea.

Quoted 11 lines

What about

"If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one or more adminstration virtqueues. The number and locations of the administration virtqueues is exposed by the owner device in a transport specific manner."

Quoted 10 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 148

Quoted 7 lines

admin.tex +7 -0

+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands
+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Are all admin vqs supposed to be equal? Could a device expose e.g. a high prio admin vq and a normal prio admin vq?

ATM yes, for priority we'll need a separate capability. Work on top?

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Cornelia Huck

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 141

Quoted 4 lines · Michael S. Tsirkin

admin.tex +4 -0

+Administration virtqueues exists for a certain owner device if
+VIRTIO_F_ADMIN_VQ feature has been negotiated. The index of the
+first administration virtqueue and their number is exposed by the
+owner device in a transport specific manner.

(Do we always expect admin virtqueues to use a range of indices, i.e. no holes? That seems a bit limiting.)

Quoted 2 lines

For the device?
I frankly feel it's enough, yea.

About how many admin virtqueues per device are we thinking for current use cases, anyway? If it's usually just one or two, the range would not really be limiting.

Quoted 23 lines

Quoted 11 lines

What about

"If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one or more adminstration virtqueues. The number and locations of the administration virtqueues is exposed by the owner device in a transport specific manner."

Quoted 10 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 148

Quoted 7 lines

admin.tex +7 -0

+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands
+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Are all admin vqs supposed to be equal? Could a device expose e.g. a high prio admin vq and a normal prio admin vq?

Quoted 1 line

ATM yes, for priority we'll need a separate capability. Work on top?

Ok, if we don't need it now, we can add it later.

Quoted 15 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Quoted 8 lines · Cornelia Huck

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 141

Quoted 4 lines · Michael S. Tsirkin

admin.tex +4 -0

+Administration virtqueues exists for a certain owner device if
+VIRTIO_F_ADMIN_VQ feature has been negotiated. The index of the
+first administration virtqueue and their number is exposed by the
+owner device in a transport specific manner.

(Do we always expect admin virtqueues to use a range of indices, i.e. no holes? That seems a bit limiting.)

For the device?
I frankly feel it's enough, yea.

Quoted 3 lines

About how many admin virtqueues per device are we thinking for current use cases, anyway? If it's usually just one or two, the range would not really be limiting.

I think it's one or two for now, yes. E.g. we could use one for SRIOV and one for the (TBD) SIOV.

Add new registers to the PCI common configuration structure.

These registers will be used for querying the indices of the admin virtqueues of the owner device. To configure, reset or enable the admin virtqueues, the driver should follow existing queue configuration/setup sequence.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

content.tex +3 -0

+
 \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

@@ -6986,6 +7011,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
   \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI

Quoted 12 lines · Michael S. Tsirkin

Add new registers to the PCI common configuration structure.

These registers will be used for querying the indices of the admin virtqueues of the owner device. To configure, reset or enable the admin virtqueues, the driver should follow existing queue configuration/setup sequence.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

(...)

Quoted 4 lines

content.tex +0 -0

@@ -1112,6 +1129,14 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 were used before the queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 1138

Quoted 7 lines

content.tex +7 -0

+If VIRTIO_F_ADMIN_VQ has been negotiated, and if the driver
+configures any administration virtqueues, the driver MUST
+configure the administration virtqueues using the index
+in the range \field{admin_queue_index} to
+\field{admin_queue_index} + \field{admin_queue_num} inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

Is the driver allowed to pick any admin queue from within the range, e.g. queues 2 and 5, and leave the rest?

Quoted 18 lines

content.tex +10 -0

+
 \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

@@ -6986,6 +7011,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
   \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI
+	  Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
+	  as the transport and is reserved for future use for
+	  devices using other transports (see
+	  \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
+	and
+	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
+	handling features reserved for future use.
 

Review \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} · line 7025

Quoted 2 lines

(quoted diff) +0 -0

 \end{description}
 

We don't say for any other feature which transports support it; do we really need to state it here explicitly if we have the rules for reserved feature bits in place? It simply will be neither offered nor accepted if the device and driver use an unsupported transport.

Quoted 21 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Add new registers to the PCI common configuration structure.

These registers will be used for querying the indices of the admin virtqueues of the owner device. To configure, reset or enable the admin virtqueues, the driver should follow existing queue configuration/setup sequence.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

(...)

Quoted 4 lines

content.tex +0 -0

@@ -1112,6 +1129,14 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 were used before the queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 

Quoted 10 lines

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 1138

Quoted 7 lines

content.tex +7 -0

+If VIRTIO_F_ADMIN_VQ has been negotiated, and if the driver
+configures any administration virtqueues, the driver MUST
+configure the administration virtqueues using the index
+in the range \field{admin_queue_index} to
+\field{admin_queue_index} + \field{admin_queue_num} inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

Is the driver allowed to pick any admin queue from within the range, e.g. queues 2 and 5, and leave the rest?

I was split on this. In the end I don't see why not. Do you feel we need to document this?

Quoted 19 lines

Quoted 19 lines

content.tex +10 -0

+
 \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

@@ -6986,6 +7011,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
   \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI
+	  Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
+	  as the transport and is reserved for future use for
+	  devices using other transports (see
+	  \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
+	and
+	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
+	handling features reserved for future use.
 

Quoted 7 lines

Review \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} · line 7025

Quoted 2 lines

(quoted diff) +0 -0

 \end{description}
 

We don't say for any other feature which transports support it; do we really need to state it here explicitly if we have the rules for reserved feature bits in place? It simply will be neither offered nor accepted if the device and driver use an unsupported transport.

It's just easy for someone to add code for feature in transport agnostic part and then it will be negotiated mistakenly when we add it for a new transport.
Potential for such a bug is what worries me and this is why I add this in so many places. Harmless no?

Quoted 33 lines · Michael S. Tsirkin

Quoted 21 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Add new registers to the PCI common configuration structure.

These registers will be used for querying the indices of the admin virtqueues of the owner device. To configure, reset or enable the admin virtqueues, the driver should follow existing queue configuration/setup sequence.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

(...)

Quoted 4 lines

content.tex +0 -0

@@ -1112,6 +1129,14 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 were used before the queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 

Quoted 10 lines

content.tex \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport · line 1138

Quoted 7 lines

content.tex +7 -0

+If VIRTIO_F_ADMIN_VQ has been negotiated, and if the driver
+configures any administration virtqueues, the driver MUST
+configure the administration virtqueues using the index
+in the range \field{admin_queue_index} to
+\field{admin_queue_index} + \field{admin_queue_num} inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

Is the driver allowed to pick any admin queue from within the range, e.g. queues 2 and 5, and leave the rest?

Quoted 2 lines

I was split on this. In the end I don't see why not. Do you feel we need to document this?

It should work fine, I guess; probably no need to spell it out explicitly.

Quoted 27 lines

Quoted 26 lines

Quoted 11 lines

content.tex +3 -0

+
 \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

@@ -6986,6 +7011,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
   \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI

content.tex \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits} · line 7025

Quoted 10 lines

content.tex +7 -0

+	  Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
+	  as the transport and is reserved for future use for
+	  devices using other transports (see
+	  \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
+	and
+	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
+	handling features reserved for future use.
 
 \end{description}
 

We don't say for any other feature which transports support it; do we really need to state it here explicitly if we have the rules for reserved feature bits in place? It simply will be neither offered nor accepted if the device and driver use an unsupported transport.

Quoted 5 lines

It's just easy for someone to add code for feature in transport agnostic part and then it will be negotiated mistakenly when we add it for a new transport.
Potential for such a bug is what worries me and this is why I add this in so many places. Harmless no?

By this reasoning, we probably should also add a comment for NOTIFICATION_DATA and RING_RESET? (On top, of course.)

Adding relevant registers needs more work and it's not clear what the use-case will be as currently only the PCI transport is supported. But let's keep the door open on this.
We already say it's reserved in a central place, but it does not hurt to remind implementers to mask it.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 12 ++++++++++++
 1 file changed, 12 insertions(+)

content.tex +0 -0

index 9c74fa9..7111929 100644

Quoted 17 lines · Michael S. Tsirkin

Adding relevant registers needs more work and it's not clear what the use-case will be as currently only the PCI transport is supported. But let's keep the door open on this.
We already say it's reserved in a central place, but it does not hurt to remind implementers to mask it.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 12 ++++++++++++
 1 file changed, 12 insertions(+)

content.tex +0 -0

index 9c74fa9..7111929 100644

content.tex \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov · line 2974

Quoted 4 lines

content.tex +1 -0

@@ -2971,6 +2971,18 @@ \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov
 MAY also choose to verify reset completion by reading \field{device status} via
 CCW_CMD_READ_STATUS and checking whether it is 0 afterwards.
 
+\subsection{Features reserved for future use}\label{sec:Virtio Transport Options / Virtio Over MMIO / Features reserved for future use}

s/MMIO/Channel I/O/

content.tex \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov · line 2980

Quoted 6 lines

content.tex +6 -0

+
+At this time, devices and drivers utilizing Virtio over channel I/O
+do not support the following features:
+\begin{itemize}
+
+\item VIRTIO_F_ADMIN_VQ

There are already a few features not yet supported for ccw (queue reset, shared memory, ...) -- would it make sense to add a separate patch listing all of the features not yet supported prior to adding all of the admin vq infrastructure? Otherwise, this section is a bit misleading.

(What is the situation for MMIO?)

Quoted 8 lines

content.tex +5 -0

+
+\end{itemize}
+
+These features are reserved for future use.
+
 \chapter{Device Types}\label{sec:Device Types}
 
 On top of the queues, config space and feature negotiation facilities

Quoted 17 lines · Michael S. Tsirkin

Adding relevant registers needs more work and it's not clear what the use-case will be as currently only the PCI transport is supported. But let's keep the door open on this.
We already say it's reserved in a central place, but it does not hurt to remind implementers to mask it.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 content.tex | 12 ++++++++++++
 1 file changed, 12 insertions(+)

content.tex +0 -0

index 9c74fa9..7111929 100644

content.tex \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov · line 2974

Quoted 4 lines

content.tex +1 -0

@@ -2971,6 +2971,18 @@ \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov
 MAY also choose to verify reset completion by reading \field{device status} via
 CCW_CMD_READ_STATUS and checking whether it is 0 afterwards.
 
+\subsection{Features reserved for future use}\label{sec:Virtio Transport Options / Virtio Over MMIO / Features reserved for future use}

s/MMIO/Channel I/O/

content.tex \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov · line 2980

Quoted 6 lines

content.tex +6 -0

+
+At this time, devices and drivers utilizing Virtio over channel I/O
+do not support the following features:
+\begin{itemize}
+
+\item VIRTIO_F_ADMIN_VQ

There are already a few features not yet supported for ccw (queue reset, shared memory, ...) -- would it make sense to add a separate patch listing all of the features not yet supported prior to adding all of the admin vq infrastructure? Otherwise, this section is a bit misleading.

(What is the situation for MMIO?)

content.tex \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov · line 2988

Quoted 8 lines

content.tex +5 -0

+
+\end{itemize}
+
+These features are reserved for future use.
+
 \chapter{Device Types}\label{sec:Device Types}
 
 On top of the queues, config space and feature negotiation facilities

OK I think for now the following is sufficient, right? Separately, we should make an effort to document that these features are transport specific and not supported on all transports:
happily VIRTIO_F_RING_RESET is only used in drivers/virtio/virtio_pci_modern.c for now, and VIRTIO_PMEM_F_SHMEM_REGION seems to be unused by Linux.

ccw: document more reserved features

vq reset and shared memory are unsupported, too.

Signed-off-by Michael S. Tsirkin <[email protected]>

content.tex +0 -0

index 30f8efb..776fc56 100644

Quoted 17 lines · Michael S. Tsirkin

OK I think for now the following is sufficient, right? Separately, we should make an effort to document that these features are transport specific and not supported on all transports:
happily VIRTIO_F_RING_RESET is only used in drivers/virtio/virtio_pci_modern.c for now, and VIRTIO_PMEM_F_SHMEM_REGION seems to be unused by Linux.

ccw: document more reserved features

vq reset and shared memory are unsupported, too.

Signed-off-by Michael S. Tsirkin <[email protected]>

content.tex +0 -0

index 30f8efb..776fc56 100644

content.tex \subsection{Features reserved for future use}\label{sec:Virtio Transport Options · line 2982

Quoted 5 lines

content.tex +2 -0

@@ -2978,6 +2978,8 @@ \subsection{Features reserved for future use}\label{sec:Virtio Transport Options
 \begin{itemize}
 
 \item VIRTIO_F_ADMIN_VQ
+\item VIRTIO_F_RING_RESET
+\item Shared memory regions including VIRTIO_PMEM_F_SHMEM_REGION

\item VIRTIO_F_SR_IOV (not applicable for this transport) \item VIRTIO_F_NOTIFICATION_DATA
\item VIRTIO_F_NOTIF_CONFIG_DATA

I think that would be all AFAICS.

What about doing a separate github issue for patch 1 + the list of reserved features for ccw as of now? We should be able to get that in independently of the admin vq work.

Instead of a bit-coded list of supported commands, can we just use an array of supported command values? It allows us to use non-contiguous command coding and checking how big is an array: le32 device_admin_cmds[];

From: Michael S. Tsirkin <[email protected]>

Quoted 1 line · Piotr

Instead of a bit-coded list of supported commands, can we just use an

array of supported command values? It allows us to use non-contiguous
command coding and checking how big is an array: le32
device_admin_cmds[];

Interesting, what's the usecase exactly?

I don't really expect more than order or 200 commands - about 8 dwords.

And the operation we normally need is checking whether a given command is supported, with a list that will be a slow lookup.

Besides with your idea driver first needs to check how many commands are supported, then do the query - two roundtrips to device.

With the proposed interface, driver just allocates a buffer for commands it knows about. Device truncates the buffer to the length supplied by driver, so any commands outside the buffer are ignored.

--
MST

Quoted 9 lines

From: Michael S. Tsirkin <[email protected]>

Quoted 1 line · Piotr

Instead of a bit-coded list of supported commands, can we just use an

array of supported command values? It allows us to use non-contiguous
command coding and checking how big is an array: le32
device_admin_cmds[];

Interesting, what's the usecase exactly?

In some protocols command IDs are grouped, for example:
Commands 0-99: standard device management
Commands 100 - 199: vendor-specific device management
Commands 200 - 299: standard queue management
Commands 300 - 399: vendor-specific queue management
Initially in each group only some command IDs are used. It makes it easier to add new commands later.

Quoted 1 line

I don't really expect more than order or 200 commands - about 8 dwords.

Agree that the number of commands will not be big. However if we define a command #1 (say standard one) and command #100 (say vendor-specific), we need to use 4 dwords for 2 commands anyway.

Quoted 2 lines

And the operation we normally need is checking whether a given command is supported, with a list that will be a slow lookup.

I don't think that is a significant difference - this is once-per-initialization operation.

Quoted 2 lines

Besides with your idea driver first needs to check how many commands are supported, then do the query - two roundtrips to device.

Right - this is the main disadvantage of my proposal.

Quoted 6 lines

With the proposed interface, driver just allocates a buffer for commands it knows about. Device truncates the buffer to the length supplied by driver, so any commands outside the buffer are ignored.

--
MST

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN. Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.

Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione. This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.

This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting.

Subscribe: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Add commands to find out which commands does each group support, as well as enable their use by driver.
This will be especially useful once we have multiple group types.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 114 insertions(+), 16 deletions(-)

admin.tex +0 -0

index bfa63a2..589e06a 100644
@@ -72,13 +72,14 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 
         /* Device-writable part */
         le16 status;

admin.tex +58 -0

+
+Commands VIRTIO_ADMIN_CMD_LIST_QUERY and
+VIRTIO_ADMIN_CMD_LIST_USE
+both use the following structure describing the
+command opcodes:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_list {
+       /* Indicates which of the below fields were returned
+       le32 device_admin_cmds[];
+};
+\end{lstlisting}
+
+This structure is an array of 32 bit values in little-endian byte
+order, in which a bit is set if the specific command opcode
+is supported. Thus, \field{device_admin_cmds[0]} refers to the first 32-bit value
+in this array corresponding to opcodes 0 to 31,
+\field{device_admin_cmds[1]} is the second 32-bit value
+corresponding to opcodes 32 to 63, etc.
+For example, the array of size 2 including
+the values 0x3 in \field{device_admin_cmds[0]}
+and 0x1 in \field{device_admin_cmds[1]} indicates that only opcodes 0, 1 and 32
+are supported.
+
+Accordingly, bits 0 and 1 corresponding to opcode 0
+(VIRTIO_ADMIN_CMD_LIST_QUERY) and 1
+(VIRTIO_ADMIN_CMD_LIST_USE) are
+always set in \field{device_admin_cmds[0]} returned by VIRTIO_ADMIN_CMD_LIST_QUERY.
+
+For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
+The \field{group_member_id} is unused. It is set to zero by driver.
+This command has no command specific data.
+The device, upon success, returns a result in
+\field{command_specific_result} in the format
+\field{struct virtio_admin_cmd_list} describing the
+list of administration commands supported for the given group.
+
+
+For the command VIRTIO_ADMIN_CMD_LIST_USE, \field{opcode}
+is set to 0x1.
+The \field{group_member_id} is unused. It is set to zero by driver.
+The \field{command_specific_data} is in the format
+\field{struct virtio_admin_cmd_list} describing the
+list of administration commands used by the driver.
+This command has no command specific result.
+
+The driver issues the command VIRTIO_ADMIN_CMD_LIST_QUERY to
+query the list of commands valid for this group and before sending
+any commands for any member of a group.
+
+The driver then enables use of some of the opcodes by sending to
+the device the command VIRTIO_ADMIN_CMD_LIST_USE with a subset
+of the list returned by VIRTIO_ADMIN_CMD_LIST_QUERY that is
+both understood and used by the driver.
+
+If the device supports the command list used by the driver, the
+device completes the command with status VIRTIO_ADMIN_STATUS_OK.
+If the device does not support the command list, the device

Quoted 26 lines · Michael S. Tsirkin

Add commands to find out which commands does each group support, as well as enable their use by driver.
This will be especially useful once we have multiple group types.

Signed-off-by Max Gurtovoy <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 114 insertions(+), 16 deletions(-)

admin.tex +2 -1

index bfa63a2..589e06a 100644
@@ -72,13 +72,14 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 
         /* Device-writable part */
         le16 status;
+        le16 status_qualifier;
         /* unused, reserved for future extensions */
-        u8 reserved2[6];
+        u8 reserved2[4];
         u8 command_specific_result[];
 };
 \end{lstlisting}
 

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 82

Quoted 2 lines

admin.tex +1 -1

-For all commands, \field{opcode}, \field{group_id} and if
+For all commands, \field{opcode}, \field{group_type} and if

Hm, this change probably belongs to a different patch?

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 97

Quoted 9 lines

admin.tex +1 -1

 necessary \field{group_member_id} and \field{command_specific_data} are
 set by the driver, and the owner device sets \field{status} and if
 needed \field{status_qualifier} and

@@ -93,18 +94,20 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 
 \begin{tabular}{|l|l|}
 \hline
-opcode & Command Description \\
+opcode & Name & Command Description \\

Maybe use this table heading from the start?

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 99

Quoted 3 lines

admin.tex +1 -1

 \hline \hline
-0000h - 7FFFh   & Group administration commands    \\
+0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\

0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Query the device about the list of commands supported for this group type \\

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 100

Quoted 1 line

admin.tex +1 -0

+0001h & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\

0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Inform the device about the list of commands used for this group type \\

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 101

Quoted 1 line

admin.tex +1 -0

+0002h - 7FFFh & - &  Group administration commands    \\

0x0002 - 0x7FFF & - & Further group administration commands \\

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 103

Quoted 2 lines

admin.tex +0 -0

 \hline
 8000h - FFFFh   & Reserved    \\

0x8000 - 0xFFFF & - & Reserved \\

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 108

Quoted 7 lines

admin.tex +2 -2

 \hline
 \end{tabular}
 
-The \field{group_id} specifies the device group.
-The \field{group_member_id} specifies the member device within the group.
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.

This likely should go into the patch initially introducing those lines.

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 113

Quoted 6 lines

admin.tex +1 -1

 See section \ref{sec:Introduction / Terminology / Device group}
-for the definition of the group identifier and group member
+for the definition of the group type identifier and group member
 identifier.
 
 The following table describes possible \field{status} values,

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 119

Quoted 6 lines

admin.tex +1 -1

@@ -113,22 +116,117 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 
 \begin{tabular}{|l|l|l|}
 \hline
-Status & Name & Description \\
+Status (decimal) & Name & Description \\

Here as well. Although I think we can simply omit the "(decimal)" when we drop the "h".

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 123

Quoted 12 lines

admin.tex +2 -8

 \hline \hline
-00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
 \hline
-01h   & VIRTIO_ADMIN_STATUS_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
-\hline
-02h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
-\hline
-03h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
-\hline
-04h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\

This also looks like it is in the wrong patch?

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 129

Quoted 6 lines

admin.tex +1 -0

 \hline
 other   & -    & group administration command error  \\
 \hline
 \end{tabular}
 
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}

status_qualifier

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 138

Quoted 9 lines

admin.tex +9 -0

+is reserved and set to zero by the device.
+
+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 144

Quoted 6 lines

admin.tex +6 -0

+\hline
+01h   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+02h   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+03h   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} was set  \\

s/was set//

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 146

Quoted 2 lines

admin.tex +2 -0

+\hline
+04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\

s/was set//

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 148

Quoted 2 lines

admin.tex +2 -0

+\hline
+other   & -    & group administration command error  \\

Again the question whether this is something that can be defined per group type.

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 160

Quoted 12 lines

admin.tex +12 -0

+\hline
+\end{tabular}
+
+Before sending any administration commands to the device, the
+commands in question are reported to the device as used by the driver.
+Initially (after reset), only two commands are assumed to be used:
+VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
+
+Accordingly,
+before sending any other commands for any member of a specific
+group to the device, the driver issues the commands
+VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.

What about

"Before sending any administration commands to the device, the driver needs to communicate to the device which commands it is going to use. Initially (after reset), only two commands are assumed to be used:
VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.

Before sending any other commands for any member of a specific group to the device, the driver queries the supported commands via VIRTIO_ADMIN_CMD_LIST_QUERY and sends the commands it will use via VIRTIO_ADMIN_CMD_LIST_USE."

Quoted 64 lines

admin.tex +64 -0

+
+Commands VIRTIO_ADMIN_CMD_LIST_QUERY and
+VIRTIO_ADMIN_CMD_LIST_USE
+both use the following structure describing the
+command opcodes:
+
+\begin{lstlisting}
+struct virtio_admin_cmd_list {
+       /* Indicates which of the below fields were returned
+       le32 device_admin_cmds[];
+};
+\end{lstlisting}
+
+This structure is an array of 32 bit values in little-endian byte
+order, in which a bit is set if the specific command opcode
+is supported. Thus, \field{device_admin_cmds[0]} refers to the first 32-bit value
+in this array corresponding to opcodes 0 to 31,
+\field{device_admin_cmds[1]} is the second 32-bit value
+corresponding to opcodes 32 to 63, etc.
+For example, the array of size 2 including
+the values 0x3 in \field{device_admin_cmds[0]}
+and 0x1 in \field{device_admin_cmds[1]} indicates that only opcodes 0, 1 and 32
+are supported.
+
+Accordingly, bits 0 and 1 corresponding to opcode 0
+(VIRTIO_ADMIN_CMD_LIST_QUERY) and 1
+(VIRTIO_ADMIN_CMD_LIST_USE) are
+always set in \field{device_admin_cmds[0]} returned by VIRTIO_ADMIN_CMD_LIST_QUERY.
+
+For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
+The \field{group_member_id} is unused. It is set to zero by driver.
+This command has no command specific data.
+The device, upon success, returns a result in
+\field{command_specific_result} in the format
+\field{struct virtio_admin_cmd_list} describing the
+list of administration commands supported for the given group.
+
+
+For the command VIRTIO_ADMIN_CMD_LIST_USE, \field{opcode}
+is set to 0x1.
+The \field{group_member_id} is unused. It is set to zero by driver.
+The \field{command_specific_data} is in the format
+\field{struct virtio_admin_cmd_list} describing the
+list of administration commands used by the driver.
+This command has no command specific result.
+
+The driver issues the command VIRTIO_ADMIN_CMD_LIST_QUERY to
+query the list of commands valid for this group and before sending
+any commands for any member of a group.
+
+The driver then enables use of some of the opcodes by sending to
+the device the command VIRTIO_ADMIN_CMD_LIST_USE with a subset
+of the list returned by VIRTIO_ADMIN_CMD_LIST_QUERY that is
+both understood and used by the driver.
+
+If the device supports the command list used by the driver, the
+device completes the command with status VIRTIO_ADMIN_STATUS_OK.
+If the device does not support the command list, the device
+completes the command with status
+VIRTIO_ADMIN_STATUS_INVALID_FIELD.
+
+Note: drivers are assumed not to set bits in device_admin_cmds
+if they are not familiar with how the command opcode
+is used, since devices could have dependencies between

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 228

Quoted 4 lines

admin.tex +4 -0

+command opcodes.
+
+It is assumed that all members in a group support and are used
+with the same list of commands.

Can the driver later change its mind, i.e. use VIRTIO_ADMIN_CMD_LIST_USE a second time with a different set of commands? If yes, can it add commands, or only withdraw them?

What happens if a driver tries to send a command that it had not included in the list?

Quoted 1 line · Cornelia Huck

On Sun, Nov 20 2022, "Michael S. Tsirkin" <[email protected]> wrote:

Using 0x instead of h suffix? Sure.

We should probably document the syntax in introduction.tex though this seems low priority.

Quoted 9 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 138

Quoted 7 lines

admin.tex +7 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

I think I prefer 0x here. And maybe I will add status values in both hex and decimal (I used decimal to be consistent with linux headers but fundamentally what we use most of the time is hex).

Quoted 15 lines

Quoted 7 lines

+\hline
+01h & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or invalid field within \field{command_specific_data} \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or invalid \field{group_type} was set \\

s/was set//

Quoted 2 lines

+\hline
+04h & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM & unsupported or invalid \field{group_member_id} was set \\

s/was set//

Quoted 5 lines

Quoted 2 lines

+\hline
+other & - & group administration command error \\

Again the question whether this is something that can be defined per group type.

probably - above ones are generic, let's see if we need specific ones. if yes will be easy to add.

Quoted 14 lines

Quoted 14 lines

admin.tex +14 -0

+The driver then enables use of some of the opcodes by sending to
+the device the command VIRTIO_ADMIN_CMD_LIST_USE with a subset
+of the list returned by VIRTIO_ADMIN_CMD_LIST_QUERY that is
+both understood and used by the driver.
+
+If the device supports the command list used by the driver, the
+device completes the command with status VIRTIO_ADMIN_STATUS_OK.
+If the device does not support the command list, the device
+completes the command with status
+VIRTIO_ADMIN_STATUS_INVALID_FIELD.
+
+Note: drivers are assumed not to set bits in device_admin_cmds
+if they are not familiar with how the command opcode
+is used, since devices could have dependencies between

Quoted 8 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 228

Quoted 4 lines

admin.tex +4 -0

+command opcodes.
+
+It is assumed that all members in a group support and are used
+with the same list of commands.

Can the driver later change its mind, i.e. use VIRTIO_ADMIN_CMD_LIST_USE a second time with a different set of commands? If yes, can it add commands, or only withdraw them?

I think it's ok to allow changing them arbitrarily at any time. If driver wants to "lock down" the list then all it has to do is send a list with VIRTIO_ADMIN_CMD_LIST_USE cleared.

It seemed clear along the lines of since it's not prohibited it's allowed but since the question arose I will add a conformance clause for this.

Quoted 4 lines

What happens if a driver tries to send a command that it had not included in the list?

This is covered in conformance statements in the next patch.

Quoted 6 lines · Michael S. Tsirkin

Quoted 1 line · Cornelia Huck

On Sun, Nov 20 2022, "Michael S. Tsirkin" <[email protected]> wrote:

Using 0x instead of h suffix? Sure.

We should probably document the syntax in introduction.tex though this seems low priority.

Nod.

Quoted 10 lines

Quoted 9 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 138

Quoted 7 lines

admin.tex +7 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

Quoted 3 lines

I think I prefer 0x here. And maybe I will add status values in both hex and decimal (I used decimal to be consistent with linux headers but fundamentally what we use most of the time is hex).

Ok.

Quoted 15 lines

Quoted 15 lines

Quoted 7 lines

+\hline
+01h & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or invalid field within \field{command_specific_data} \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or invalid \field{group_type} was set \\

s/was set//

Quoted 2 lines

+\hline
+04h & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM & unsupported or invalid \field{group_member_id} was set \\

s/was set//

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+\hline
+other & - & group administration command error \\

Again the question whether this is something that can be defined per group type.

probably - above ones are generic, let's see if we need specific ones. if yes will be easy to add.

I think we want to distinguish between "reserved" (not defined yet, may get a meaning later on) and "group type specific" (a group type may use it, don't reuse for generic stuff). If we need group type specific errors (and don't want a free-for-all), we could go with eg.

0x05 & VIRTIO_ADMIN_STATUS_Q_GROUP_ERR_0 & group type specific error \\

or so? Do we see any need for that yet?

Quoted 27 lines

Quoted 14 lines

Quoted 14 lines

admin.tex +14 -0

+The driver then enables use of some of the opcodes by sending to
+the device the command VIRTIO_ADMIN_CMD_LIST_USE with a subset
+of the list returned by VIRTIO_ADMIN_CMD_LIST_QUERY that is
+both understood and used by the driver.
+
+If the device supports the command list used by the driver, the
+device completes the command with status VIRTIO_ADMIN_STATUS_OK.
+If the device does not support the command list, the device
+completes the command with status
+VIRTIO_ADMIN_STATUS_INVALID_FIELD.
+
+Note: drivers are assumed not to set bits in device_admin_cmds
+if they are not familiar with how the command opcode
+is used, since devices could have dependencies between

Quoted 8 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 228

Quoted 4 lines

admin.tex +4 -0

+command opcodes.
+
+It is assumed that all members in a group support and are used
+with the same list of commands.

Can the driver later change its mind, i.e. use VIRTIO_ADMIN_CMD_LIST_USE a second time with a different set of commands? If yes, can it add commands, or only withdraw them?

I think it's ok to allow changing them arbitrarily at any time. If driver wants to "lock down" the list then all it has to do is send a list with VIRTIO_ADMIN_CMD_LIST_USE cleared.

Quoted 3 lines

It seemed clear along the lines of since it's not prohibited it's allowed but since the question arose I will add a conformance clause for this.

So if the driver sends LIST_USE with an additional command, and the device rejects that list (because of dependencies or whatever), is the list of commands supposed to stay whatever it was prior to that?

Quoted 8 lines

Quoted 4 lines

What happens if a driver tries to send a command that it had not included in the list?

This is covered in conformance statements in the next patch.

Review

Quoted 6 lines

(quoted diff) +6 -0

+
+Device MUST validate commands against the list used by
+driver and MUST fail any commands not in the list with
+\field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
+and \field{status_qualifier} set to
+VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE.

Ok.

Quoted 49 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Quoted 10 lines · Cornelia Huck

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 138

Quoted 7 lines · Michael S. Tsirkin

admin.tex +7 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

Quoted 3 lines

I think I prefer 0x here. And maybe I will add status values in both hex and decimal (I used decimal to be consistent with linux headers but fundamentally what we use most of the time is hex).

Ok.

Quoted 15 lines

Quoted 15 lines

Quoted 7 lines

+\hline
+01h & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or invalid field within \field{command_specific_data} \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or invalid \field{group_type} was set \\

s/was set//

Quoted 2 lines

+\hline
+04h & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM & unsupported or invalid \field{group_member_id} was set \\

s/was set//

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+\hline
+other & - & group administration command error \\

Again the question whether this is something that can be defined per group type.

probably - above ones are generic, let's see if we need specific ones. if yes will be easy to add.

I think we want to distinguish between "reserved" (not defined yet, may get a meaning later on) and "group type specific" (a group type may use it, don't reuse for generic stuff). If we need group type specific errors (and don't want a free-for-all), we could go with eg.

Quoted 3 lines

0x05 & VIRTIO_ADMIN_STATUS_Q_GROUP_ERR_0 & group type specific error \\

or so? Do we see any need for that yet?

Not yet.

Quoted 54 lines · Michael S. Tsirkin

Quoted 52 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Quoted 10 lines · Cornelia Huck

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 138

Quoted 7 lines · Michael S. Tsirkin

admin.tex +7 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

Quoted 3 lines

I think I prefer 0x here. And maybe I will add status values in both hex and decimal (I used decimal to be consistent with linux headers but fundamentally what we use most of the time is hex).

Ok.

Quoted 15 lines

Quoted 15 lines

Quoted 7 lines

+\hline
+01h & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or invalid field within \field{command_specific_data} \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or invalid \field{group_type} was set \\

s/was set//

Quoted 2 lines

+\hline
+04h & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM & unsupported or invalid \field{group_member_id} was set \\

s/was set//

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+\hline
+other & - & group administration command error \\

Again the question whether this is something that can be defined per group type.

probably - above ones are generic, let's see if we need specific ones. if yes will be easy to add.

I think we want to distinguish between "reserved" (not defined yet, may get a meaning later on) and "group type specific" (a group type may use it, don't reuse for generic stuff). If we need group type specific errors (and don't want a free-for-all), we could go with eg.

0x05 & VIRTIO_ADMIN_STATUS_Q_GROUP_ERR_0 & group type specific error \\

or so? Do we see any need for that yet?

Quoted 1 line

Not yet.

Then maybe let's make the last line

0x05 - & - & reserved for future use \\

?

Quoted 60 lines · Cornelia Huck

Quoted 50 lines · Michael S. Tsirkin

Quoted 49 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Quoted 10 lines · Cornelia Huck

Review

Quoted 7 lines · Michael S. Tsirkin

(quoted diff) +7 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

Quoted 3 lines

I think I prefer 0x here. And maybe I will add status values in both hex and decimal (I used decimal to be consistent with linux headers but fundamentally what we use most of the time is hex).

Ok.

Quoted 15 lines

Quoted 15 lines

Quoted 7 lines

+\hline
+01h & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or invalid field within \field{command_specific_data} \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or invalid \field{group_type} was set \\

s/was set//

Quoted 2 lines

+\hline
+04h & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM & unsupported or invalid \field{group_member_id} was set \\

s/was set//

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+\hline
+other & - & group administration command error \\

Again the question whether this is something that can be defined per group type.

probably - above ones are generic, let's see if we need specific ones. if yes will be easy to add.

I think we want to distinguish between "reserved" (not defined yet, may get a meaning later on) and "group type specific" (a group type may use it, don't reuse for generic stuff). If we need group type specific errors (and don't want a free-for-all), we could go with eg.

Quoted 5 lines

Quoted 3 lines

0x05 & VIRTIO_ADMIN_STATUS_Q_GROUP_ERR_0 & group type specific error \\

or so? Do we see any need for that yet?

Not yet.

Then maybe let's make the last line

Quoted 3 lines

0x05 - & - & reserved for future use \\

?

Hmm 5 is reserved but anything else is a generic error. I'm not sure what the difference is. Could you clarify? E.g. how will driver handle such an error if it gets it? Is it an error to get a reserved error value?

Quoted 61 lines · Michael S. Tsirkin

Quoted 60 lines · Cornelia Huck

Quoted 50 lines · Michael S. Tsirkin

Quoted 49 lines · Cornelia Huck

Quoted 12 lines · Michael S. Tsirkin

Quoted 10 lines · Cornelia Huck

Review

Quoted 7 lines · Michael S. Tsirkin

(quoted diff) +7 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\

Either 0x00, or decimal (which one is better?)

Quoted 3 lines

I think I prefer 0x here. And maybe I will add status values in both hex and decimal (I used decimal to be consistent with linux headers but fundamentally what we use most of the time is hex).

Ok.

Quoted 15 lines

Quoted 15 lines

Quoted 7 lines

+\hline
+01h & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or invalid \field{opcode} \\
+\hline
+02h & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or invalid field within \field{command_specific_data} \\
+\hline
+03h & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or invalid \field{group_type} was set \\

s/was set//

Quoted 2 lines

+\hline
+04h & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM & unsupported or invalid \field{group_member_id} was set \\

s/was set//

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

+\hline
+other & - & group administration command error \\

Again the question whether this is something that can be defined per group type.

probably - above ones are generic, let's see if we need specific ones. if yes will be easy to add.

I think we want to distinguish between "reserved" (not defined yet, may get a meaning later on) and "group type specific" (a group type may use it, don't reuse for generic stuff). If we need group type specific errors (and don't want a free-for-all), we could go with eg.

Quoted 5 lines

Quoted 3 lines

0x05 & VIRTIO_ADMIN_STATUS_Q_GROUP_ERR_0 & group type specific error \\

or so? Do we see any need for that yet?

Not yet.

Then maybe let's make the last line

Quoted 8 lines

Quoted 3 lines

0x05 - & - & reserved for future use \\

?

Hmm 5 is reserved but anything else is a generic error. I'm not sure what the difference is. Could you clarify? E.g. how will driver handle such an error if it gets it? Is it an error to get a reserved error value?

Oh, I meant 0x05 - ... (i.e. 0x05 or higher)

Getting a reserved error code basically means that either (a) the device implements a newer version of the standard, or (b) the device is buggy and doesn't conform to the standard. It's probably best to handle that the same as error 0x00 (no additional information.)

Add conformance clauses for admin commands and admin virtqueues.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

admin.tex +0 -0

index 589e06a..290cbbc 100644

admin.tex +3 -0

+before sending VIRTIO_ADMIN_CMD_LIST_USE with the correct
+list of command opcodes and group type.
+
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
 
 An administration virtqueue of an owner device is used to submit

Quoted 12 lines · Michael S. Tsirkin

Add conformance clauses for admin commands and admin virtqueues.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 admin.tex | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

admin.tex +0 -0

index 589e06a..290cbbc 100644

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 232

Quoted 6 lines

admin.tex +3 -0

@@ -227,6 +227,71 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 It is assumed that all members in a group support and are used
 with the same list of commands.
 
+\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+
+Device MUST validate \field{opcode}, \field{group_type} and

s/Device/The device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 235

Quoted 3 lines

admin.tex +3 -0

+\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
+set \field{status_qualifier} accordingly if any of these has an
+invalid or unsupported value.

"...and if any of these has an invalid or unsupported value, set ..." ?

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 241

Quoted 6 lines

admin.tex +6 -0

+
+If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
+command MUST NOT have any side effects, in particular the device
+MUST not enter an error state as a result of this command.
+
+Device MAY enforce additional restrictions and dependencies on

s/Device/The device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 249

Quoted 8 lines

admin.tex +8 -0

+opcodes used by the driver and MAY fail the command
+VIRTIO_ADMIN_CMD_LIST_USE with \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
+and \field{status_qualifier} set to VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
+if the list of commands used violate internal device dependencies.
+
+If the device supports multiple group types, commands for each group
+type MUST operate independently of each other, in particular,
+device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY

s/device/the device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 253

Quoted 4 lines

admin.tex +4 -0

+for different group types.
+
+Before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
+device MUST assume

s/device/, the device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 254

Quoted 1 line

admin.tex +1 -0

+that the list of legal commands used by driver consists of two commands

s/driver/the driver/
s/two/the two/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 257

Quoted 3 lines

admin.tex +3 -0

+VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
+
+Device MUST set the list of legal commands used by driver

s/Device/The device/
s/driver/the driver/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 258

Quoted 1 line

admin.tex +1 -0

+with the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.

s/with/to/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 260

Quoted 2 lines

admin.tex +2 -0

+
+Device MUST validate commands against the list used by

s/Device/The device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 261

Quoted 1 line

admin.tex +1 -0

+driver and MUST fail any commands not in the list with

s/driver/the driver/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 266

Quoted 5 lines

admin.tex +5 -0

+\field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
+and \field{status_qualifier} set to
+VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE.
+
+List of supported commands MUST NOT change: after reporting a

s/List/The list/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 268

Quoted 2 lines

admin.tex +2 -0

+given command as supported through VIRTIO_ADMIN_CMD_LIST_QUERY
+device MUST NOT later report it as unsupported.

s/device/the device/

Is that for multiple queries while the device is alive? Is the device allowed to report a different list of commands after it has been reset? And is it allowed to _add_ commands?

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 272

Quoted 4 lines

admin.tex +4 -0

+
+\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+
+Driver MAY discover whether device supports a specific group type

s/Driver/The driver/
s/Device/the device/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 276

Quoted 4 lines

admin.tex +4 -0

+by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
+\field{group_type}.
+
+Driver MUST issue VIRTIO_ADMIN_CMD_LIST_USE

s/Driver/The driver/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 282

Quoted 6 lines

admin.tex +6 -0

+and wait for it to be completed with status
+VIRTIO_ADMIN_STATUS_OK before issuing any commands
+(except for the initial VIRTIO_ADMIN_CMD_LIST_QUERY
+and VIRTIO_ADMIN_CMD_LIST_USE).
+
+Driver SHOULD NOT set bits in device_admin_cmds

s/Driver/The driver/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 285

Quoted 3 lines

admin.tex +3 -0

+if it is not familiar with how the command opcode
+is used, since there could exist dependencies between
+command opcodes.

", as dependencies between command opcodes might exist."

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 287

Quoted 2 lines

admin.tex +2 -0

+
+Driver MUST NOT request (by VIRTIO_ADMIN_CMD_LIST_USE)

s/Driver/The driver/
s/by/via/

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 291

Quoted 4 lines

admin.tex +4 -0

+the use of any commands not previously reported as
+supported for the same group type by VIRTIO_ADMIN_CMD_LIST_QUERY.
+
+Driver MUST NOT use any commands for a given group type

s/Driver/The driver/

Quoted 7 lines

admin.tex +3 -0

+before sending VIRTIO_ADMIN_CMD_LIST_USE with the correct
+list of command opcodes and group type.
+
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
 
 An administration virtqueue of an owner device is used to submit

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 350

Quoted 7 lines

admin.tex +4 -0

@@ -279,3 +344,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
 new fields can be added to the tail of a structure,
 with driver using the full structure without concern
 for versioning.
+
+\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+
+Device MUST support device-readable and device-writeable buffers

s/Device/The device/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 359

Quoted 9 lines

admin.tex +9 -0

+shorter than described in this specification, by
+\begin{enumerate}
+\item assuming that any data that would be read outside the
+device-readable buffers is set to zero, and
+\item discarding data that would be written outside the
+specified device-writeable buffers.
+\end{enumerate}
+
+Device MUST support device-readable and device-writeable buffers

s/Device/The device/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 370

Quoted 11 lines

admin.tex +11 -0

+longer than described in this specification, by
+\begin{enumerate}
+\item ignoring any data in device-readable buffers outside
+the expected length, and
+\item only writing the expected structure to the device-writeable
+buffers, ignoring any extra buffers, and reporting the
+actual length of data written, in bytes,
+as buffer used length.
+\end{enumerate}
+
+Device SHOULD initialize the device-writeable buffer

s/Device/The device/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 373

Quoted 3 lines

admin.tex +3 -0

+up to the smaller of the structure described by
+this specification and the length of the buffer supplied by the
+driver (even if the buffer is all set to zero).

I had to read this one several times... maybe

"up to the length of the structure described by this specification or the length of the buffer supplied by the driver (even if the buffer is all set to zero), whichever is shorter."

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 375

Quoted 2 lines

admin.tex +2 -0

+
+Device MUST NOT fail a command solely because the buffers

s/Device/The device/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 379

Quoted 4 lines

admin.tex +4 -0

+provided are shorter or longer than described in this
+specification.
+
+Device MUST process commands on a given administration virtqueues

s/Device/The device/
s/virtqueues/virtqueue/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 388

Quoted 9 lines

admin.tex +9 -0

+in the order in which they are queued.
+
+If multiple administration virtqueues have been configured,
+device MAY process commands on distinct virtqueues with
+no order constraints.
+
+\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+
+Driver MAY supply device-readable and device-writeable buffers

s/Driver/The driver/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 391

Quoted 3 lines

admin.tex +3 -0

+longer than described in this specification.
+
+Driver SHOULD supply device-readable buffers at least as

s/Driver/The driver/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 395

Quoted 4 lines

admin.tex +4 -0

+large as the structure described by this specification
+(even if the buffer is all set to zero).
+
+Driver MUST NOT assume that device will initialize the whole

s/Driver/The driver/
s/device/the device/

admin.tex \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic · line 396

Quoted 1 line

admin.tex +1 -0

+structure as described in the specification, instead,

s/,/;/