Introduce device group and device management

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

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

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, it's expected 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.

Effort was made to minimize transport-specific text.

There's a bit of duplication with 0x1 repeated twice and no special section for group type identifiers.
It seems ok to defer adding these until we have more group types.

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

Reviewed-by Stefan Hajnoczi <[email protected]>

---
changes in v12:
stefan's ack
minor commit log tweaks, Parav
use neutral tone, Parav

changes in v11:
dropped Max's S.O.B.
dropped an unmatched ) as reported by Parav
document that member id is 1 to numvfs
document that vf enable must be set (will also be reflected in
the conformance section)
document that we deferred generalizing text as requested by Parav - I think we can do it later
minor wording fixes to address comments by David
add concepts of owner and member driver. unused currently but will come in handy later, as suggested by Stefan

---
  admin.tex   | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  content.tex |  2 ++
  2 files changed, 65 insertions(+)

create mode 100644 admin.tex

admin.tex new file +63 -0

new file mode 100644
index 0000000..05d506a
@@ -0,0 +1,63 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
+
+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. The owner device itself is not
+	a member of the group.
+\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 the member identifier is interpreted
+	and what kind of control the owner has.
+	A given owner can control multiple groups
+	of different types but only a single group of a given type,
+	thus the type and the owner together identify the group.
+	\footnote{Even though some group types only support
+			specific transports, group type identifiers
+			are global rather than transport-specific -
+			a flood of new group types is not expected.}
+\end{description}
+
+\begin{note}
+Each device only has a single driver, thus for the purposes of
+this section, "the driver" is usually unambiguous and refers to
+the driver of the owner device.  When there's ambiguity, "owner
+driver" refers to the driver of the owner device, while "member
+driver" refers to the driver of a member device.
+\end{note}
+
+The following group types, and their identifiers, are currently specified:
+\begin{description}
+\item[SR-IOV group type (0x1)]
+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.
+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value from 0x1 to
+\field{NumVFs} as specified in the
+SR-IOV Extended Capability of the owner device
+and equals the SR-IOV VF number of the member device;
+the group only exists when the \field{VF Enable} bit
+in the SR-IOV Control Register within the
+SR-IOV Extended Capability of the owner device is set
+(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}
+
+

content.tex +0 -0

index 8098988..04592fb 100644

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.

Note that the commands are focused on controlling device groups:
this is why group related fields are in the generic part of the structure.
Without this the admin vq would become a "whatever" vq which does not do anything specific at all, just a general transport like thing. I feel going this way opens the design space to the point where we no longer know what belongs in e.g. config space
what in the control q and what in the admin q.
As it is, whatever deals with groups is in the admin q; other things not in the admin q.

There are specific exceptions such as query but that's an exception that proves the rule ;)

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

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

---

changes since v11:
acks by Stefan and Lingshan

changes since v10:
explain the role of status vs status_qualifier, addresses
multiple comments
add more status values and appropriate qualifiers,
as suggested by Parav
clarify reserved command opcodes as suggested by Stefan better formatting for ranges, comment by Jiri make sure command-specific-data is a multiple of qword, so things are aligned, suggested by Jiri
add Q_OK qualifier for success

---
  admin.tex        | 121 +++++++++++++++++++++++++++++++++++++++++++++++
  introduction.tex |   3 ++
  2 files changed, 124 insertions(+)

admin.tex +81 -0

index 05d506a..d6042e4 100644
@@ -60,4 +60,125 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
  PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
  \end{description}
 
+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of
+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1       - SR-IOV
+         * 2-65535 - reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        le64 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+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
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
+\hline
+\end{tabular}
+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The \field{status} describes the command result and possibly
+failure reason at an abstract level, this is appropriate for
+forwarding to applications. The \field{status_qualifier} describes
+failures at a low virtio specific level, as appropriate for debugging.
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\
+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+11   & VIRTIO_ADMIN_STATUS_EAGAIN    & try again \\
+\hline
+12   & VIRTIO_ADMIN_STATUS_ENOMEM    & insufficient resources \\

admin.tex +14 -0

+The following table describes possible \field{status_qualifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_OK               & used with VIRTIO_ADMIN_STATUS_OK  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND  & command error: no additional information  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE   & unsupported or invalid \field{opcode}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\

admin.tex +6 -0

+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

introduction.tex +0 -0

index 287c5fc..b7155bf 100644

Quoted 131 lines · Michael S. Tsirkin

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.

Note that the commands are focused on controlling device groups:
this is why group related fields are in the generic part of the structure.
Without this the admin vq would become a "whatever" vq which does not do anything specific at all, just a general transport like thing. I feel going this way opens the design space to the point where we no longer know what belongs in e.g. config space
what in the control q and what in the admin q.
As it is, whatever deals with groups is in the admin q; other things not in the admin q.

There are specific exceptions such as query but that's an exception that proves the rule ;)

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

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

---

changes since v11:
acks by Stefan and Lingshan

changes since v10:
explain the role of status vs status_qualifier, addresses
multiple comments
add more status values and appropriate qualifiers,
as suggested by Parav
clarify reserved command opcodes as suggested by Stefan better formatting for ranges, comment by Jiri make sure command-specific-data is a multiple of qword, so things are aligned, suggested by Jiri
add Q_OK qualifier for success

---
  admin.tex        | 121 +++++++++++++++++++++++++++++++++++++++++++++++
  introduction.tex |   3 ++
  2 files changed, 124 insertions(+)

admin.tex +81 -0

index 05d506a..d6042e4 100644
@@ -60,4 +60,125 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
  PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
  \end{description}
+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+The driver sends group administration commands to the owner device of
+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1       - SR-IOV
+         * 2-65535 - reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        le64 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+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
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
+\hline
+\end{tabular}
+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The \field{status} describes the command result and possibly
+failure reason at an abstract level, this is appropriate for
+forwarding to applications. The \field{status_qualifier} describes
+failures at a low virtio specific level, as appropriate for debugging.
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\
+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+11   & VIRTIO_ADMIN_STATUS_EAGAIN    & try again \\
+\hline
+12   & VIRTIO_ADMIN_STATUS_ENOMEM    & insufficient resources \\

admin.tex \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g · line 152

Quoted 10 lines

admin.tex +10 -0

+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qualifier}
+is reserved and set to zero by the device.
+

actually it is:

When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qualifier} can be ignored by driver and set to VIRTIO_ADMIN_STATUS_Q_OK by the device.

Can be moved after the status_qualifier values paragraph.

Quoted 19 lines

admin.tex +19 -0

+The following table describes possible \field{status_qualifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_OK               & used with VIRTIO_ADMIN_STATUS_OK  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND  & command error: no additional information  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE   & unsupported or invalid \field{opcode}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x05   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER   & unsupported or invalid \field{group_member_id} \\
+\hline
+0x06   & VIRTIO_ADMIN_STATUS_Q_NORESOURCE       & out of internal resources: ok to retry \\
+\hline

admin.tex \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g · line 178

Quoted 5 lines

admin.tex +5 -0

+0x07   & VIRTIO_ADMIN_STATUS_Q_TRYAGAIN         & command blocks for too long: should retry \\
+\hline
+0x08-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}

should we mention which status_qualifier can match to which status (as we did for STATUS_OK) ?

Quoted 12 lines

admin.tex +6 -0

+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

introduction.tex +0 -0

index 287c5fc..b7155bf 100644
@@ -68,6 +68,9 @@ \section{Normative References}\label{sec:Normative References}
  	\phantomsection\label{intro:FUSE}\textbf{[FUSE]} &

introduction.tex \section{Normative References}\label{sec:Normative References} · line 76

Quoted 8 lines

introduction.tex +3 -0

  	Linux FUSE interface,
  	\newline\url{https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fuse.h}\\
+	\phantomsection\label{intro:errno}\textbf{[errno]} &
+	Linux error names and numbers,
+	\newline\url{https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/errno-base.h}\\
          \phantomsection\label{intro:eMMC}\textbf{[eMMC]} &
          eMMC Electrical Standard (5.1), JESD84-B51,
          \newline\url{http://www.jedec.org/sites/default/files/docs/JESD84-B51.pdf}\\

comments on previous one have all been minor, so I hope this means we are close to merging this.

Change log:

since 11:
one "breaking" change in response to stefan's comment:
in patch 5, num_queues has been specified not to include admin
queues: just regular ones.

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

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

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, it's expected 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.

Effort was made to minimize transport-specific text.

There's a bit of duplication with 0x1 repeated twice and no special section for group type identifiers.
It seems ok to defer adding these until we have more group types.

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

Reviewed-by Stefan Hajnoczi <[email protected]>

---
changes in v12:
stefan's ack
minor commit log tweaks, Parav
use neutral tone, Parav

changes in v11:
dropped Max's S.O.B.
dropped an unmatched ) as reported by Parav
document that member id is 1 to numvfs
document that vf enable must be set (will also be reflected in
the conformance section)
document that we deferred generalizing text as requested by Parav - I think we can do it later
minor wording fixes to address comments by David
add concepts of owner and member driver. unused currently but will come in handy later, as suggested by Stefan

---
  admin.tex   | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  content.tex |  2 ++
  2 files changed, 65 insertions(+)

create mode 100644 admin.tex

admin.tex new file +53 -0

new file mode 100644
index 0000000..05d506a
@@ -0,0 +1,63 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
+
+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. The owner device itself is not
+	a member of the group.
+\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 the member identifier is interpreted
+	and what kind of control the owner has.
+	A given owner can control multiple groups
+	of different types but only a single group of a given type,
+	thus the type and the owner together identify the group.
+	\footnote{Even though some group types only support
+			specific transports, group type identifiers
+			are global rather than transport-specific -
+			a flood of new group types is not expected.}
+\end{description}
+
+\begin{note}
+Each device only has a single driver, thus for the purposes of
+this section, "the driver" is usually unambiguous and refers to
+the driver of the owner device.  When there's ambiguity, "owner
+driver" refers to the driver of the owner device, while "member
+driver" refers to the driver of a member device.
+\end{note}
+
+The following group types, and their identifiers, are currently specified:
+\begin{description}
+\item[SR-IOV group type (0x1)]
+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.
+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value from 0x1 to
+\field{NumVFs} as specified in the
+SR-IOV Extended Capability of the owner device
+and equals the SR-IOV VF number of the member device;

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 Michael S. Tsirkin <[email protected]>

---
dropped Max's S.O.B
make queue_num not 0 based

squash! pci: add admin vq registers to virtio over pci

since v11:
document that admin vqs are not counted with regular vqs

Reviewed-by Parav Pandit <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

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

content.tex +18 -0

index 2eb15fa..bcbc06d 100644
@@ -948,6 +948,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
          le64 queue_device;              /* read-write */
          le16 queue_notify_data;         /* read-only for driver */
          le16 queue_reset;               /* read-write */
+
+        /* About the administration virtqueue. */
+        le16 admin_queue_index;         /* read-only for driver */
+        le16 admin_queue_num;         /* read-only for driver */
  };
  \end{lstlisting}
 

@@ -974,6 +978,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 
  \item[\field{num_queues}]
          The device specifies the maximum number of virtqueues supported here.
+        This excludes administration virtqueues if any are supported.
 
  \item[\field{device_status}]
          The driver writes the device status here (see \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}). Writing 0 into this

@@ -1033,6 +1038,19 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
          This field exists only if VIRTIO_F_RING_RESET has been
          negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 
+\item[\field{admin_queue_index}]
+        The device uses this to report the index of the first administration virtqueue.
+        This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
+\item[\field{admin_queue_num}]
+	The device uses this to report the number of the
+	supported administration virtqueues.
+	Virtqueues with index
+	between \field{admin_queue_index} and (\field{admin_queue_index} +
+	\field{admin_queue_num} - 1) inclusive serve as administration
+	virtqueues.
+	The value 0 indicates no supported administration virtqueues.
+	This field is valid only if VIRTIO_F_ADMIN_VQ has been
+	negotiated.
  \end{description}
 
  \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}

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

@@ -7686,6 +7712,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
    \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
    administration virtqueues.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI

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.

Note that the commands are focused on controlling device groups:
this is why group related fields are in the generic part of the structure.

Without this the admin vq would become a "whatever" vq which does not do anything specific at all, just a general transport like thing. I feel going this way opens the design space to the point where we no longer know what belongs in e.g. config space
what in the control q and what in the admin q.
As it is, whatever deals with groups is in the admin q; other things not in the admin q.

There are specific exceptions such as query but that's an exception that proves the rule ;)

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

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

---

changes since v11:
acks by Stefan and Lingshan

changes since v10:
explain the role of status vs status_qualifier, addresses
multiple comments
add more status values and appropriate qualifiers,
as suggested by Parav
clarify reserved command opcodes as suggested by Stefan better formatting for ranges, comment by Jiri make sure command-specific-data is a multiple of qword, so things are aligned, suggested by Jiri
add Q_OK qualifier for success

---
  admin.tex        | 121 +++++++++++++++++++++++++++++++++++++++++++++++
  introduction.tex |   3 ++
  2 files changed, 124 insertions(+)

admin.tex +121 -0

index 05d506a..d6042e4 100644
@@ -60,4 +60,125 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
  PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
  \end{description}
 
+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of
+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1       - SR-IOV
+         * 2-65535 - reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        le64 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+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
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
+\hline
+\end{tabular}
+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The \field{status} describes the command result and possibly
+failure reason at an abstract level, this is appropriate for
+forwarding to applications. The \field{status_qualifier} describes
+failures at a low virtio specific level, as appropriate for debugging.
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\
+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+11   & VIRTIO_ADMIN_STATUS_EAGAIN    & try again \\
+\hline
+12   & VIRTIO_ADMIN_STATUS_ENOMEM    & insufficient resources \\
+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qualifier}
+is reserved and set to zero by the device.
+
+The following table describes possible \field{status_qualifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_OK               & used with VIRTIO_ADMIN_STATUS_OK  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND  & command error: no additional information  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE   & unsupported or invalid \field{opcode}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x05   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER   & unsupported or invalid \field{group_member_id} \\
+\hline
+0x06   & VIRTIO_ADMIN_STATUS_Q_NORESOURCE       & out of internal resources: ok to retry \\
+\hline
+0x07   & VIRTIO_ADMIN_STATUS_Q_TRYAGAIN         & command blocks for too long: should retry \\
+\hline
+0x08-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}
+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

introduction.tex +0 -0

index 287c5fc..b7155bf 100644

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]>

Reviewed-by Parav Pandit <[email protected]>

Reviewed-by Stefan Hajnoczi <[email protected]>

---

changes in v12:
drop "as of now" to address comment by parav

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

content.tex +5 -0

index bcbc06d..7d6d9c4 100644
@@ -2365,6 +2365,18 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
 
  Notification mechanisms did not change.
 
+\subsection{Features reserved for future use}\label{sec:Virtio Transport Options / Virtio Over MMIO / Features reserved for future use}
+
+Devices and drivers utilizing Virtio Over MMIO
+do not support the following features:
+\begin{itemize}

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.

Note: there are more features to add to this list. Will be done later with a patch on top.

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

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Parav Pandit <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

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

content.tex +5 -0

index 7d6d9c4..07be8cb 100644
@@ -2979,6 +2979,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 channel I/O / Features reserved for future use}
+
+At this time, devices and drivers utilizing Virtio over channel I/O
+do not support the following features:
+\begin{itemize}

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

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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes.
For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

can we address this ?

vq reset and shared memory are unsupported, too.

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

Fixes https://github.com/oasis-tcs/virtio-spec/issues/160

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

---

changes in v12:
address comment by parav

---
  content.tex | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

content.tex +0 -0

index 1bc9af0..fe032ac 100644
@@ -2988,11 +2988,13 @@ \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov
 

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.

An alternative is per-type VQs. This is possible but will require more per-transport work. Discovery through the vq helps keep things contained.

e.g. lack of support for some command can switch to a legacy mode

note that commands are expected to be avolved by adding new

fields to command specific data at the tail, so we generally do not need feature bits for compatibility.

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

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

---

changes in v12:

devices->device address comment by parav

changes in v11:

dropped S.O.B by Max
explain in commit log how commands will evolve, comment by Stefan replace will use with is capable of use, comment by Stefan typo fix reported by David and Lingshan
rename cmds to cmd_opcodes suggested by Jiri
list group_type explicitly in command description suggested by Jiri clarify how can device not support some commands
always say group administration commands, comment by Jiri

---
  admin.tex | 102 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 101 insertions(+), 1 deletion(-)

admin.tex +94 -1

index 91e0cba..68ee7e5 100644
@@ -113,7 +113,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
  \hline
  opcode & Name & Command Description \\
  \hline \hline
-0x0000 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
+0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
  \hline
  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
  \hline

@@ -183,6 +185,104 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
  depends on these structures and is described separately or is
  implicit in the structure description.
 
+Before sending any group 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 is
+capable of using via VIRTIO_ADMIN_CMD_LIST_USE.
+
+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
+       le64 device_admin_cmd_opcodes[];
+};
+\end{lstlisting}
+
+This structure is an array of 64 bit values in little-endian byte
+order, in which a bit is set if the specific command opcode
+is supported. Thus, \field{device_admin_cmd_opcodes[0]} refers to the
+first 64-bit value in this array corresponding to opcodes 0 to
+63, \field{device_admin_cmd_opcodes[1]} is the second 64-bit value
+corresponding to opcodes 64 to 127, etc.
+For example, the array of size 2 including
+the values 0x3 in \field{device_admin_cmd_opcodes[0]}
+and 0x1 in \field{device_admin_cmd_opcodes[1]} indicates that only
+opcodes 0, 1 and 64 are supported.
+The length of the array depends on the supported opcodes - it is
+large enough to include bits set for all supported opcodes,
+that is the length can be calculated by starting with the largest
+supported opcode adding one, dividing by 64 and rounding up.
+In other words, for
+VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE the
+length of \field{command_specific_result} and
+\field{command_specific_data} respectively will be
+$DIV_ROUND_UP(max_cmd, 64) * 8$ where DIV_ROUND_UP is integer division
+with round up and \field{max_cmd} is the largest available command opcode.
+
+The array is also allowed to be larger and to additionally
+include an arbitrary number of all-zero entries.
+
+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_cmd_opcodes[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 group administration commands supported for the group type
+specified by \field{group_type}.
+
+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 group administration commands used by the driver
+with the group type specified by \field{group_type}.
+
+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
+(for example, if the driver is not capable to use
+some required commands), the device
+completes the command with status
+VIRTIO_ADMIN_STATUS_INVALID_FIELD.
+
+Note: the driver is assumed not to set bits in
+device_admin_cmd_opcodes
+if it is not familiar with how the command opcode
+is used, since the device could have dependencies between

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 Michael S. Tsirkin <[email protected]>

---
dropped Max's S.O.B
make queue_num not 0 based

squash! pci: add admin vq registers to virtio over pci

since v11:
document that admin vqs are not counted with regular vqs

Reviewed-by Parav Pandit <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

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

content.tex +28 -0

index 2eb15fa..bcbc06d 100644
@@ -948,6 +948,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
          le64 queue_device;              /* read-write */
          le16 queue_notify_data;         /* read-only for driver */
          le16 queue_reset;               /* read-write */
+
+        /* About the administration virtqueue. */
+        le16 admin_queue_index;         /* read-only for driver */
+        le16 admin_queue_num;         /* read-only for driver */
  };
  \end{lstlisting}
 

@@ -974,6 +978,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 
  \item[\field{num_queues}]
          The device specifies the maximum number of virtqueues supported here.
+        This excludes administration virtqueues if any are supported.
 
  \item[\field{device_status}]
          The driver writes the device status here (see \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}). Writing 0 into this

@@ -1033,6 +1038,19 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
          This field exists only if VIRTIO_F_RING_RESET has been
          negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 
+\item[\field{admin_queue_index}]
+        The device uses this to report the index of the first administration virtqueue.
+        This field is valid only if VIRTIO_F_ADMIN_VQ has been negotiated.
+\item[\field{admin_queue_num}]
+	The device uses this to report the number of the
+	supported administration virtqueues.
+	Virtqueues with index
+	between \field{admin_queue_index} and (\field{admin_queue_index} +
+	\field{admin_queue_num} - 1) inclusive serve as administration
+	virtqueues.
+	The value 0 indicates no supported administration virtqueues.
+	This field is valid only if VIRTIO_F_ADMIN_VQ has been
+	negotiated.
  \end{description}
 
  \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}

@@ -1119,6 +1137,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}).
 
+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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.
+
  \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

@@ -7686,6 +7712,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
    \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
    administration virtqueues.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI

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

Currently the virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on:
virtio-net, virtio-scsi, etc all have existing control virtqueues. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

Keeping the device-specific virtqueue separate from the admin virtqueue is simpler and has fewer potential problems. I don't think creating common infrastructure for device-specific control virtqueues across device types worthwhile or within the scope of this patch series.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.
The admin virtqueue can be seen as the virtqueue analog to a transport. The admin queue thus does nothing device type-specific (net, scsi, etc) and instead focuses on transporting the admin commands.

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

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

Reviewed-by Stefan Hajnoczi <[email protected]>

---

changes since v11:

	ack by stefan
	queues->enqueues to address comment by parav

changes since v10:

explain ordering of commands as suggested by Stefan dropped Max's S.O.B
reword commit log as suggested by David minor wording fixes suggested by David

---
  admin.tex   | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  content.tex |  7 +++--
  2 files changed, 80 insertions(+), 2 deletions(-)

admin.tex +75 -0

index d6042e4..91e0cba 100644
@@ -182,3 +182,78 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
  \field{command_specific_data} and \field{command_specific_result}
  depends on these structures and is described separately or is
  implicit in the structure description.
+
+\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.
+
+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 are exposed by the owner device in a transport
+specific manner.
+
+The driver enqueues requests to an arbitrary administration
+virtqueue, and they are used by the device on that same
+virtqueue. It is the responsibility of the driver to ensure
+strict request ordering for commands, because they will be
+consumed with no order constraints.  For example, if consistency
+is required then the driver can wait for the processing of a
+first command by the device to be completed before submitting
+another command depending on the first one.
+
+Administration virtqueues are used as follows:
+\begin{itemize}
+\item The driver submits the command using the \field{struct virtio_admin_cmd}
+structure using a buffer consisting of two parts: a device-readable one followed by a
+device-writable one.
+\item the device-readable part 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}
+
+For each command, this specification describes a distinct
+format structure used for \field{command_specific_data} and
+\field{command_specific_result}, the length of these fields
+depends on the command.
+
+However, to ensure forward compatibility
+\begin{itemize}
+\item drivers are allowed to submit buffers that are longer
+than the device expects
+(that is, longer than the length of
+\field{opcode} through \field{command_specific_data}).
+This allows the driver to maintain
+a single format structure even if some structure fields are
+unused by the device.
+\item drivers are allowed to submit buffers that are shorter
+than what the device expects
+(that is, shorter than the length of \field{status} through
+\field{command_specific_result}). This allows the device to maintain
+a single format structure even if some structure fields are
+unused by the driver.
+\end{itemize}
+
+The device compares the length of each part (device-readable and
+device-writeable) of the buffer as submitted by driver to what it
+expects and then silently truncates the structures to either the
+length submitted by the driver, or the length described in this
+specification, whichever is shorter.  The device silently ignores
+any data falling outside the shorter of the two lengths. Any
+missing fields are interpreted as set to zero.
+
+Similarly, the driver compares the used buffer length
+of the buffer to what it expects and then silently
+truncates the structure to the used buffer length.
+The driver silently ignores any data falling outside
+the used buffer length reported by the device.  Any missing
+fields are interpreted as set to zero.
+
+This simplifies driver and device implementations since the
+driver/device can simply maintain a single large structure (such
+as a 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 the driver/device using the full
+structure without concern for versioning.

content.tex +2 -2

index 04592fb..2eb15fa 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 6 lines · Michael S. Tsirkin

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.

Quoted 3 lines

Note that the commands are focused on controlling device groups:
this is why group related fields are in the generic part of the structure.

Below paragraph is no longer applies as we already discussed more use cases of the AQ such as accessing the PCI VF's legacy config space registers. Please drop below paragraph.

Quoted 8 lines

Without this the admin vq would become a "whatever" vq which does not do anything specific at all, just a general transport like thing. I feel going this way opens the design space to the point where we no longer know what belongs in e.g. config space
what in the control q and what in the admin q.
As it is, whatever deals with groups is in the admin q; other things not in the admin q.

A PF can use same or different AQ with a new struct virtio_legacy_register_access with a different opcode range.

AQ doesn't replace the control vq, so that part description is fine.

Quoted 158 lines

There are specific exceptions such as query but that's an exception that proves the rule ;)

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

Reviewed-by Stefan Hajnoczi <[email protected]>

Reviewed-by Zhu Lingshan <[email protected]>

---

changes since v11:
acks by Stefan and Lingshan

changes since v10:
explain the role of status vs status_qualifier, addresses
multiple comments
add more status values and appropriate qualifiers,
as suggested by Parav
clarify reserved command opcodes as suggested by Stefan better formatting for ranges, comment by Jiri make sure command-specific-data is a multiple of qword, so things are aligned, suggested by Jiri
add Q_OK qualifier for success

---
  admin.tex        | 121 +++++++++++++++++++++++++++++++++++++++++++++++
  introduction.tex |   3 ++
  2 files changed, 124 insertions(+)

admin.tex +121 -0

index 05d506a..d6042e4 100644
@@ -60,4 +60,125 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
  PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
  \end{description}
+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+The driver sends group administration commands to the owner device of
+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1       - SR-IOV
+         * 2-65535 - reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        le64 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+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
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
+\hline
+\end{tabular}
+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The \field{status} describes the command result and possibly
+failure reason at an abstract level, this is appropriate for
+forwarding to applications. The \field{status_qualifier} describes
+failures at a low virtio specific level, as appropriate for debugging.
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\
+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+11   & VIRTIO_ADMIN_STATUS_EAGAIN    & try again \\
+\hline
+12   & VIRTIO_ADMIN_STATUS_ENOMEM    & insufficient resources \\
+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qualifier}
+is reserved and set to zero by the device.
+
+The following table describes possible \field{status_qualifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_OK               & used with VIRTIO_ADMIN_STATUS_OK  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND  & command error: no additional information  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE   & unsupported or invalid \field{opcode}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x05   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER   & unsupported or invalid \field{group_member_id} \\
+\hline
+0x06   & VIRTIO_ADMIN_STATUS_Q_NORESOURCE       & out of internal resources: ok to retry \\
+\hline
+0x07   & VIRTIO_ADMIN_STATUS_Q_TRYAGAIN         & command blocks for too long: should retry \\
+\hline
+0x08-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}
+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

introduction.tex +0 -0

index 287c5fc..b7155bf 100644

introduction.tex \section{Normative References}\label{sec:Normative References} · line 73

Quoted 7 lines

introduction.tex +3 -0

@@ -68,6 +68,9 @@ \section{Normative References}\label{sec:Normative References}
  	\phantomsection\label{intro:FUSE}\textbf{[FUSE]} &
  	Linux FUSE interface,
  	\newline\url{https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fuse.h}\\
+	\phantomsection\label{intro:errno}\textbf{[errno]} &
+	Linux error names and numbers,
+	\newline\url{https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/asm-generic/errno-base.h}\\

Thanks for fixing this link. Missing in commit log changes for v12.

Hopefully after b4 integration, this will be smooth.

introduction.tex \section{Normative References}\label{sec:Normative References} · line 76

Quoted 3 lines

introduction.tex +0 -0

          \phantomsection\label{intro:eMMC}\textbf{[eMMC]} &
          eMMC Electrical Standard (5.1), JESD84-B51,
          \newline\url{http://www.jedec.org/sites/default/files/docs/JESD84-B51.pdf}\\

Other than commit log change,

Reviewed-by Parav Pandit <[email protected]>

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, April 25, 2023 2:20 AM

Quoted 3 lines

Below paragraph is no longer applies as we already discussed more use cases of the AQ such as accessing the PCI VF's legacy config space registers. Please drop below paragraph.

Look - at one point you want commit log to record design process, an another you turn around and ask me to drop it.
I feel like keeping this around frankly. Maybe I will add a sentence or two along the lines of "Note that nothing limits us from extending this down the road though - but let's focus on what we already know for now".

This extreme focus on commit log is poinless anyway - it makes much more sense for code since commit log describes the change in english. Here the whole change is english anyway.

Quoted 9 lines

Quoted 8 lines

Without this the admin vq would become a "whatever" vq which does not do anything specific at all, just a general transport like thing. I feel going this way opens the design space to the point where we no longer know what belongs in e.g. config space what in the control q and what in the admin q.
As it is, whatever deals with groups is in the admin q; other things not in the admin q.

Quoted 2 lines

A PF can use same or different AQ with a new struct virtio_legacy_register_access with a different opcode range.

We'll get to that bridge and we'll cross it, the proposal is not on list even yet.

I
actually think that yes, you need it in a separate function. If PF is passed through to guest then PF can not also safely DMA into host memory.

Alternatively, we could use an MMIO based mechanism for admin commands. And maybe that would be a good fit.

Quoted 8 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, April 25, 2023 2:20 AM

Quoted 3 lines

Below paragraph is no longer applies as we already discussed more use cases of the AQ such as accessing the PCI VF's legacy config space registers. Please drop below paragraph.

Quoted 6 lines

Look - at one point you want commit log to record design process, an another you turn around and ask me to drop it.
I feel like keeping this around frankly. Maybe I will add a sentence or two along the lines of "Note that nothing limits us from extending this down the road though - but let's focus on what we already know for now".

I am requesting to drop the commit point that limits the usage of the AQ. Do you agree that AQ can be extended for use beyond VF and SIOV group management commands? Or one should create a new VQ type?
If it is the later, I don't see the need of multiple AQ.

Quoted 13 lines

This extreme focus on commit log is poinless anyway - it makes much more sense for code since commit log describes the change in english. Here the whole change is english anyway.

Quoted 9 lines

Quoted 8 lines

Without this the admin vq would become a "whatever" vq which does not do anything specific at all, just a general transport like thing. I feel going this way opens the design space to the point where we no longer know what belongs in e.g. config space what in the control q and what in the admin q.
As it is, whatever deals with groups is in the admin q; other things not in the admin q.

Quoted 4 lines

Quoted 2 lines

A PF can use same or different AQ with a new struct virtio_legacy_register_access with a different opcode range.

We'll get to that bridge and we'll cross it, the proposal is not on list even yet.

It is at [1].
[1] https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html
I cannot draft the legacy command using AQ because AQ patch-5 in v12 fails to merge.

Before I draft it, I sent [1] on the design way forward.

Quoted 3 lines

I
actually think that yes, you need it in a separate function. If PF is passed through to guest then PF can not also safely DMA into host memory.

That is fine. Only some user tests would pass the PF and VF to the VMs.
One can create N combinations to make it not work.
In all practical purposes, PF is owned by the hypervisor, trusted sw as listed in the PCI spec.

Quoted 2 lines

Alternatively, we could use an MMIO based mechanism for admin commands. And maybe that would be a good fit.

We discussed that MMIO is not a good fit for the VFs at scale as listed in [1].

[1] https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, April 25, 2023 9:32 AM

Quoted 2 lines

I am requesting to drop the commit point that limits the usage of the AQ. Do you agree that AQ can be extended for use beyond VF and SIOV group

management commands?

Quoted 2 lines

Or one should create a new VQ type?
If it is the later, I don't see the need of multiple AQ.

not management. that is old term and indeed too narrow. administration as in hypervisor (admin) access through PF while guest has access through VF.

legacy access you link to below seems to fall within scope:
we access VF through a PF.

So what is the problem?

maybe we'll change the meaning down the road. I do not see the immediate need currently though.

Quoted 2 lines

[1]
https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html

Then I don't see a problem. It's just commit log, not spec. Let it slide is my suggestion, this is spec text not code.

Quoted 11 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, April 25, 2023 9:32 AM

Quoted 2 lines

I am requesting to drop the commit point that limits the usage of the AQ. Do you agree that AQ can be extended for use beyond VF and SIOV group

management commands?

Quoted 2 lines

Or one should create a new VQ type?
If it is the later, I don't see the need of multiple AQ.

Quoted 9 lines

not management. that is old term and indeed too narrow. administration as in hypervisor (admin) access through PF while guest has access through VF.

legacy access you link to below seems to fall within scope:
we access VF through a PF.

So what is the problem?

Commit log confused me. You clarified above.
It is clear now. Hence no problem.

Quoted 3 lines

maybe we'll change the meaning down the road. I do not see the immediate need currently though.

As long as we agree that it is open to widen the scope, it is acceptable.

Quoted 5 lines

Quoted 2 lines

[1]
https://lists.oasis-open.org/archives/virtio-dev/202304/msg00511.html

Then I don't see a problem. It's just commit log, not spec. Let it slide is my suggestion, this is spec text not code.

Ok. I feel it is still worth to have commit log updated. If you happen to revise v13, please do. Else its fine.

v12 surely doesn't merge cleanly. So v13 is needed anyway.

Can you please generate? Patch-5 of v12 fails to apply.

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, April 25, 2023 4:05 PM

Quoted 1 line

Ok. I feel it is still worth to have commit log updated. If you happen to revise

v13, please do.

Quoted 2 lines

Else its fine.

Quoted 3 lines

v12 surely doesn't merge cleanly. So v13 is needed anyway.

Can you please generate? Patch-5 of v12 fails to apply.

should apply on top of 985bbf397db4228faf3ec464e838687dc4cb4904. master will fail because of the file movement. I will need to rebase to apply on latest master, I will do it before we start voting.

Quoted 11 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, April 25, 2023 4:05 PM

Quoted 1 line

Ok. I feel it is still worth to have commit log updated. If you happen to revise

v13, please do.

Quoted 5 lines

Else its fine.

v12 surely doesn't merge cleanly. So v13 is needed anyway.

Can you please generate? Patch-5 of v12 fails to apply.

Quoted 3 lines

should apply on top of 985bbf397db4228faf3ec464e838687dc4cb4904. master will fail because of the file movement. I will need to rebase to apply on latest master, I will do it before we start voting.

Ok. sounds good. I assume you will post rebased v13 with commit log updated?
When do you plan to do?
I request that it is better to rebase now because legacy related patches need to touch the add the PCI extended capability on top of your changes.

This results in further manual merges for me.
Better to do the work once with one time rebase, instead of both of us.

Let me know if I should generate v13 fixing above two items as you have holiday this week. It helps me build on top of AQ patches with one time rebase.

From: [email protected] <[email protected] open.org> On Behalf Of Michael S. Tsirkin

I pushed the rebase to tag mst-admin-v13 on github.

Don't have time for tweaks it's a holiday.
I usually leave patches on list for a week or so. If no comments I will post v13 and we will roll a vote.

Quoted 11 lines · Max Gurtovoy

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

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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes.
For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit. Also the drivers with and without need a different VQ index handling.

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

This also aligns to the exclusion of aq count from num_queues field.

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 5:12 PM

Quoted 26 lines · Parav Pandit

Quoted 12 lines · Max Gurtovoy

Review

Quoted 7 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

Quoted 1 line

This also aligns to the exclusion of aq count from num_queues field.

The namespace of the aq and other vq index is the same.
And the configuration is done using the same queue_select and other registers. Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Quoted 33 lines

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 5:12 PM

Quoted 27 lines · Parav Pandit

Quoted 12 lines · Max Gurtovoy

Review

Quoted 7 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

This also aligns to the exclusion of aq count from num_queues field.

Quoted 4 lines

The namespace of the aq and other vq index is the same.
And the configuration is done using the same queue_select and other registers. Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Michael has added below line in this patch in num_queues description so it covers the exclusion part.

+ This excludes administration virtqueues if any are supported.

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 6:31 PM

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 13 lines

Review

Quoted 8 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

This also aligns to the exclusion of aq count from num_queues field.

The namespace of the aq and other vq index is the same. And the configuration is done using the same queue_select and other

registers.

Quoted 4 lines

Quoted 2 lines

Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Michael has added below line in this patch in num_queues description so it

covers the exclusion part.

Quoted 3 lines

+ This excludes administration virtqueues if any are supported.

This is not related to what I was talking about.

For example if vnet device:

0 receiveq1
1 transmitq1
...
2(N-1) receiveqN
2(N-1)+1 transmitqN
2N controlq
N=1 if neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are negotiated, otherwise N is set by max_- virtqueue_pairs.
controlq only exists if VIRTIO_NET_F_CTRL_VQ set

and in vfs device:

0 hiprio
1...n request queues

So if one device will expose adminq that will be with index 0 the above is wrong description.

Agree that this should be addressed ?

Quoted 33 lines · Parav Pandit

Quoted 33 lines

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 5:12 PM

Quoted 27 lines · Parav Pandit

Quoted 12 lines · Max Gurtovoy

Review

Quoted 7 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

This also aligns to the exclusion of aq count from num_queues field.

Quoted 9 lines

Quoted 4 lines

The namespace of the aq and other vq index is the same.
And the configuration is done using the same queue_select and other registers. Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Michael has added below line in this patch in num_queues description so it covers the exclusion part.

+ This excludes administration virtqueues if any are supported.

This is not related to what I was talking about.

For example if vnet device:

0 receiveq1
1 transmitq1
...
2(N-1) receiveqN
2(N-1)+1 transmitqN
2N controlq
N=1 if neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are negotiated, otherwise N is set by max_-
virtqueue_pairs.
controlq only exists if VIRTIO_NET_F_CTRL_VQ set

and in vfs device:

0 hiprio
1...n request queues

So if one device will expose adminq that will be with index 0 the above is wrong description.

Agree that this should be addressed ?

Quoted 63 lines · Max Gurtovoy

Quoted 33 lines · Parav Pandit

Quoted 33 lines

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 5:12 PM

Quoted 27 lines · Parav Pandit

Quoted 12 lines · Max Gurtovoy

Review

Quoted 7 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

This also aligns to the exclusion of aq count from num_queues field.

Quoted 9 lines

Quoted 4 lines

The namespace of the aq and other vq index is the same.
And the configuration is done using the same queue_select and other registers. Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Michael has added below line in this patch in num_queues description so it covers the exclusion part.

+ This excludes administration virtqueues if any are supported.

This is not related to what I was talking about.

For example if vnet device:

0 receiveq1
1 transmitq1
...
2(N-1) receiveqN
2(N-1)+1 transmitqN
2N controlq
N=1 if neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are negotiated, otherwise N is set by max_-
virtqueue_pairs.
controlq only exists if VIRTIO_NET_F_CTRL_VQ set

and in vfs device:

Quoted 7 lines

0 hiprio
1...n request queues

So if one device will expose adminq that will be with index 0 the above is wrong description.

Agree that this should be addressed ?

It's addressed by patch 9:

(quoted diff) +6 -0

+If VIRTIO_F_ADMIN_VQ has been negotiated, the value
+\field{admin_queue_index} MUST be equal to, or bigger than
+\field{num_queues}; also, \field{admin_queue_num} MUST be
+smaller than, or equal to 0x10000 - \field{admin_queue_index},
+to ensure that indices of valid admin queues fit into
+a 16 bit range beyond all other virtqueues.

Thus adminq can not have index 0.

comments on previous one have all been minor, so I hope this means we are close to merging this.

Quoted 56 lines

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 6:31 PM

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 13 lines

Review

Quoted 8 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

This also aligns to the exclusion of aq count from num_queues field.

The namespace of the aq and other vq index is the same. And the configuration is done using the same queue_select and other

registers.

Quoted 4 lines

Quoted 2 lines

Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Michael has added below line in this patch in num_queues description so it

covers the exclusion part.

Quoted 3 lines

+ This excludes administration virtqueues if any are supported.

This is not related to what I was talking about.

For example if vnet device:

0 receiveq1
1 transmitq1
...
2(N-1) receiveqN
2(N-1)+1 transmitqN
2N controlq
N=1 if neither VIRTIO_NET_F_MQ nor VIRTIO_NET_F_RSS are negotiated, otherwise N is set by max_- virtqueue_pairs.
controlq only exists if VIRTIO_NET_F_CTRL_VQ set

and in vfs device:

Quoted 7 lines

0 hiprio
1...n request queues

So if one device will expose adminq that will be with index 0 the above is wrong description.

Agree that this should be addressed ?

Oh yes, I agree.
In response [1] I acked it as,

"I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field."

So, admin_queue_index description needs to have description like below.

admin_queue_index must be greater than or equal to the value of num_queues.

Yes?

[1] https://lore.kernel.org/virtio-dev/[email protected]/

From: Michael S. Tsirkin <[email protected]>
Sent: Friday, May 5, 2023 11:12 AM

Quoted 1 line

"I am thinking that aq index should not overlap with the vq index in range

exposed by num_queues field."

Quoted 4 lines

So, admin_queue_index description needs to have description like below.

admin_queue_index must be greater than or equal to the value of

num_queues.

Quoted 6 lines

Yes?

[1]
https://lore.kernel.org/virtio-dev/a4b1596c-2132-714a-7557-fa5249a5594
[email protected]/

this is present in the conformance clause patch:

Quoted 31 lines

From: Max Gurtovoy <[email protected]>
Sent: Wednesday, April 26, 2023 5:12 PM

Quoted 26 lines · Parav Pandit

Quoted 12 lines · Max Gurtovoy

Review

Quoted 7 lines

(quoted diff) +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} - 1 inclusive.
+The driver MAY configure less administration virtqueues than
+supported by the device.

we need to say something about other virtq's indexes. For example, if aq index = 0 then for the vblk device the request queues would start from index = 1.

Quoted 1 line

can we address this ?

It may slightly complicate the device implementation when device dynamically changes the VQ indices based on the negotiation of the feature bit.
Also the drivers with and without need a different VQ index handling.

I am thinking that aq index should not overlap with the vq index in range exposed by num_queues field.

Quoted 6 lines

Quoted 1 line

This also aligns to the exclusion of aq count from num_queues field.

The namespace of the aq and other vq index is the same.
And the configuration is done using the same queue_select and other registers. Thus, we need to address the above comment otherwise all the device virtqueues chapters are wrong.

Michael has added below line in this patch in num_queues description so it covers the exclusion part.

+ This excludes administration virtqueues if any are supported.

I inspected all the devices.
Following devices which has multi queue supports are fine, net, console, scsi host, gpu, input, crypto, socket, rpmb, iommu, sound, scmi, gpio.

Following single q devices are fine too: entropy, mem ballon dev, mem, i2c, pmem. So mostly all chapters don’t look wrong.

Block device needs below change.
WDYT?

From 0592d167451280bc212df7322077f4c94b28c917 Mon Sep 17 00:00:00 2001
From: Parav Pandit <[email protected]>
Date: Thu, 27 Apr 2023 01:01:41 +0300
Subject: [PATCH] virtio-blk: Rename num_queues to num_req_queues

num_queues field represents number of blk specific request queues.

Renaming it to num_req_queues reflect its precise usage.
It also avoids confusion with PCI transport's generic num_queues field.

Signed-off-by Parav Pandit <[email protected]>

---
 device-types/blk/description.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

device-types/blk/description.tex +4 -5

index f04c932..5a27399 100644
@@ -16,7 +16,7 @@ \subsection{Virtqueues}\label{sec:Device Types / Block Device / Virtqueues}
 \end{description}
 
  N=1 if VIRTIO_BLK_F_MQ is not negotiated, otherwise N is set by
- \field{num_queues}.
+ \field{num_req_queues}.
 
 \subsection{Feature bits}\label{sec:Device Types / Block Device / Feature bits}
 

@@ -108,7 +108,7 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
         } topology;
         u8 writeback;
         u8 unused0;
-        u16 num_queues;
+        u16 num_req_queues;
         le32 max_discard_sectors;
         le32 max_discard_seg;
         le32 discard_sector_alignment;

@@ -135,8 +135,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Block Device /
 present. The availability of the others all depend on various feature
 bits as indicated above.
 
-The field \field{num_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
-the number of queues.
+The field \field{num_req_queues} only exists if VIRTIO_BLK_F_MQ is set. This field specifies
+the number of request queues.
 
 The parameters in the configuration space of the device \field{max_discard_sectors}
 \field{discard_sector_alignment} are expressed in 512-byte units if the
--

2.26.2

From: Michael S. Tsirkin <[email protected]>
Sent: Friday, May 5, 2023 11:22 AM

Quoted 2 lines

Renaming it to num_req_queues reflect its precise usage.
It also avoids confusion with PCI transport's generic num_queues field.

There's no confusion actually - both fields are exactly the same are they not? The one in blk just predates the one in pci.