[PATCH v8 0/9] Introduce device group and device management

[PATCH v8 0/9] Introduce device group and device management

[Michael S. Tsirkin]

This is out of RFC, since we have two commands which are useful
to discover supported group types (ATM can be none or SR-IOV).

Change log:

since v7:
	make high level error codes match linux, with virtio specific codes
		in a separate field
	renamed _ACCEPT to _USE since that's what it does
	clarified forward compatibility and non pci transports
	support multiple admin vqs
	conformance statements
	lots of changes all over the place to I changed author from Max
	to myself. Don't need to take credit but also don't want
	to blame Max for my mistakes.

since v6:

	- removed some extentions intended for future use.
	  We'll do them when we get there.

	- brought back command list query from v5 in a simplified form -
	  it's here to address the case where a single parent
	  can address multiple groups, such as PF addressing
	  transport vq and sriov vfs.

	- attempt to make terminology more formal.
	In particular a term for whoever controls the group.
 	I am still going back
	and forth between "parent" and "owner" - owner might
	be better after all since it will work if we ever
	have a self group. For now it's parent.

TODO (maybe?) - probably ok to defer until this part is upstream:

	Add "all members" member id.

	Add commands for MSI, feature discovery.

	Add commands for transport vq.


My intent is to try and support both SR-IOV and SIOV
usecases with the same structure and maybe even the same
VQ.

For example, it might make sense to split creating/destroying
SIOV devices from the transport passing data from the guest - the
driver would then not negotiate VIRTIO_F_SR_IOV (which
then means auto-provisioning).


Michael S. Tsirkin (9):
  virtio: document forward compatibility guarantees
  admin: introduce device group and related concepts
  admin: introduce group administration commands
  admin: introduce virtio admin virtqueues
  pci: add admin vq registers to virtio over pci
  mmio: document ADMIN_VQ as reserved
  ccw: document ADMIN_VQ as reserved
  admin: command list discovery
  admin: conformance clauses

 admin.tex   | 403 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex | 103 +++++++++++++-
 2 files changed, 504 insertions(+), 2 deletions(-)
 create mode 100644 admin.tex

-- 
MST

[PATCH v8 1/9] virtio: document forward compatibility guarantees

[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 <pasic@linux.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 content.tex | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/content.tex b/content.tex
index 3051399..1e91b1d 100644
--- a/content.tex
+++ b/content.tex
@@ -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.
 
+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.  In particular, this is
+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.
 
+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.
+
 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.
+
 \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
-- 
MST

[PATCH v8 2/9] admin: introduce device group and related concepts

[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 <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 52 insertions(+)
 create mode 100644 admin.tex

diff --git a/admin.tex b/admin.tex
new file mode 100644
index 0000000..4337db0
--- /dev/null
+++ b/admin.tex
@@ -0,0 +1,50 @@
+\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. Owner device itself is not
+	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.
+\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
+	and what kind of control does the owner have.
+	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.
+\end{description}
+
+A single group type is currently specified:
+\begin{description}
+\item[SR-IOV group type]
+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 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}
+
+
diff --git a/content.tex b/content.tex
index 1e91b1d..9deacbf 100644
--- a/content.tex
+++ b/content.tex
@@ -486,6 +486,8 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
 types. It is RECOMMENDED that devices generate version 4
 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
 
+\input{admin.tex}
+
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
 
 We start with an overview of device initialization, then expand on the
-- 
MST

[PATCH v8 3/9] admin: introduce group administration commands

[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.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/admin.tex b/admin.tex
index 4337db0..507e188 100644
--- a/admin.tex
+++ b/admin.tex
@@ -47,4 +47,84 @@ \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}
 
+Group administration commands can be issued through an owner
+device to control member devices of a group.  This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+
+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;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        /* unused, reserved for future extensions */
+        u8 reserved2[6];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_id} 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}.
+
+As a rule, 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 & Command Description \\
+\hline \hline
+0000h - 7FFFh   & Group administration commands    \\
+\hline
+8000h - FFFFh   & Reserved    \\
+\hline
+\end{tabular}
+
+The \field{group_id} specifies the device group.
+The \field{group_member_id} specifies the member device within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group identifier and group member
+identifier.
+
+The following table describes possible \field{status} values,
+to simplify common implementations, these are intentionally
+matching common Linux names and error numbers:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & 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  \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
-- 
MST

[PATCH v8 4/9] admin: introduce virtio admin virtqueues

[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 <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  6 ++++--
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/admin.tex b/admin.tex
index 507e188..bfa63a2 100644
--- a/admin.tex
+++ b/admin.tex
@@ -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.
+
+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.
+
+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.
+
+Administration virtqueues are used as follows:
+\begin{itemize}
+\item Driver submits the command using the \field{struct virtio_admin_cmd}
+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
+shorter of the two lengths (submitted by driver and described in this
+specification). Device silently ignores any data falling outside
+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
+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
+for versioning.
diff --git a/content.tex b/content.tex
index 9deacbf..d2fd1de 100644
--- a/content.tex
+++ b/content.tex
@@ -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}.
 
+  \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
MST

[PATCH v8 5/9] pci: add admin vq registers to virtio over pci

[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 <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 content.tex | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/content.tex b/content.tex
index d2fd1de..03cbd3f 100644
--- a/content.tex
+++ b/content.tex
@@ -941,6 +941,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}
 
@@ -1026,6 +1030,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}) inclusive serve as administration
+	virtqueues.
+	Thus the number of administration virtqueues equals
+	(\field{admin_queue_num} + 1).
+	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}
@@ -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}).
 
+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.
+
 \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.
 
 \end{description}
 
-- 
MST

[PATCH v8 6/9] mmio: document ADMIN_VQ as reserved

[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 <mst@redhat.com>
---
 content.tex | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/content.tex b/content.tex
index 03cbd3f..9c74fa9 100644
--- a/content.tex
+++ b/content.tex
@@ -2357,6 +2357,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}
+
+At this time, devices and drivers utilizing Virtio Over MMIO
+do not support the following features:
+\begin{itemize}
+
+\item VIRTIO_F_ADMIN_VQ
+
+\end{itemize}
+
+These features are reserved for future use.
+
 \section{Virtio Over Channel I/O}\label{sec:Virtio Transport Options / Virtio Over Channel I/O}
 
 S/390 based virtual machines support neither PCI nor MMIO, so a
-- 
MST

[PATCH v8 7/9] ccw: document ADMIN_VQ as reserved

[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 <mst@redhat.com>
---
 content.tex | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/content.tex b/content.tex
index 9c74fa9..7111929 100644
--- a/content.tex
+++ b/content.tex
@@ -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}
+
+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
+
+\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
-- 
MST

[PATCH v8 8/9] admin: command list discovery

[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 <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 114 insertions(+), 16 deletions(-)

diff --git a/admin.tex b/admin.tex
index bfa63a2..589e06a 100644
--- a/admin.tex
+++ b/admin.tex
@@ -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}
 
-For all commands, \field{opcode}, \field{group_id} and if
+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
@@ -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 \\
 \hline \hline
-0000h - 7FFFh   & Group administration commands    \\
+0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+0001h & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
+0002h - 7FFFh & - &  Group administration commands    \\
 \hline
 8000h - FFFFh   & Reserved    \\
 \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.
 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,
@@ -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 \\
 \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 \\
 \hline
 other   & -    & group administration command error  \\
 \hline
 \end{tabular}
 
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+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  \\
+\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  \\
+\hline
+04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
+\hline
+other   & -    & group administration command error  \\
+\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.
+
+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
+command opcodes.
+
+It is assumed that all members in a group support and are used
+with the same list of commands.
+
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
 
 An administration virtqueue of an owner device is used to submit
-- 
MST

[PATCH v8 9/9] admin: conformance clauses

[Michael S. Tsirkin]

Add conformance clauses for admin commands and admin virtqueues.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/admin.tex b/admin.tex
index 589e06a..290cbbc 100644
--- a/admin.tex
+++ b/admin.tex
@@ -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
+\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.
+
+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
+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
+for different group types.
+
+Before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
+device MUST assume
+that the list of legal commands used by driver consists of two commands
+VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
+
+Device MUST set the list of legal commands used by driver
+with the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
+
+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.
+
+List of supported commands MUST NOT change: after reporting a
+given command as supported through VIRTIO_ADMIN_CMD_LIST_QUERY
+device MUST NOT later report it as unsupported.
+
+\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
+by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
+\field{group_type}.
+
+Driver MUST issue VIRTIO_ADMIN_CMD_LIST_USE
+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
+if it is not familiar with how the command opcode
+is used, since there could exist dependencies between
+command opcodes.
+
+Driver MUST NOT request (by VIRTIO_ADMIN_CMD_LIST_USE)
+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
+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
@@ -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
+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
+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
+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).
+
+Device MUST NOT fail a command solely because the buffers
+provided are shorter or longer than described in this
+specification.
+
+Device MUST process commands on a given administration virtqueues
+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
+longer than described in this specification.
+
+Driver SHOULD supply device-readable buffers at least as
+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
+structure as described in the specification, instead,
+the driver MUST assume that the structure
+outside the part of the buffer used by the device
+is set to zero.
+
+If multiple administration virtqueues have been configured,
+driver MUST ensure ordering for commands
+placed on different administration virtqueues.
-- 
MST

RE: [PATCH v8 8/9] admin: command list discovery

[Uminski, 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[];


-----Original Message-----
From: Michael S. Tsirkin <mst@redhat.com> 
Sent: Monday, November 21, 2022 2:26 AM
To: virtio-comment@lists.oasis-open.org; virtio-dev@lists.oasis-open.org; jasowang@redhat.com; mst@redhat.com; cohuck@redhat.com; sgarzare@redhat.com; stefanha@redhat.com; Jani, Nrupal <nrupal.jani@intel.com>; Uminski, Piotr <Piotr.Uminski@intel.com>; Yuan, Hang <hang.yuan@intel.com>
Cc: virtio@lists.oasis-open.org; Zhu, Lingshan <lingshan.zhu@intel.com>; pasic@linux.ibm.com; Shahaf Shuler <shahafs@nvidia.com>; Parav Pandit <parav@nvidia.com>; Max Gurtovoy <mgurtovoy@nvidia.com>
Subject: [PATCH v8 8/9] admin: command list discovery

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 <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 114 insertions(+), 16 deletions(-)

diff --git a/admin.tex b/admin.tex
index bfa63a2..589e06a 100644
--- a/admin.tex
+++ b/admin.tex
@@ -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}
 
-For all commands, \field{opcode}, \field{group_id} and if
+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 @@ -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 \\
 \hline \hline
-0000h - 7FFFh   & Group administration commands    \\
+0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+0001h & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
+0002h - 7FFFh & - &  Group administration commands    \\
 \hline
 8000h - FFFFh   & Reserved    \\
 \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.
 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, @@ -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 \\
 \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 \\
 \hline
 other   & -    & group administration command error  \\
 \hline
 \end{tabular}
 
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier} 
+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  \\
+\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  \\
+\hline
+04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
+\hline
+other   & -    & group administration command error  \\
+\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.
+
+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 command opcodes.
+
+It is assumed that all members in a group support and are used with the 
+same list of commands.
+
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
 
 An administration virtqueue of an owner device is used to submit
--
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.

[virtio] Re: [PATCH v8 1/9] virtio: document forward compatibility guarantees

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> Feature negotiation forms the basis of forward compatibility
> guarantees of virtio but has never been properly documented.
> Do it now.
>
> Suggested-by: Halil Pasic <pasic@linux.ibm.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  content.tex | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 3051399..1e91b1d 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -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.
>  
> +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."

> In particular, this is
> +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.
>  
> +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?

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

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


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio] Re: [PATCH v8 7/9] ccw: document ADMIN_VQ as reserved

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mst@redhat.com>
> ---
>  content.tex | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 9c74fa9..7111929 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -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/

> +
> +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?)

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


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [PATCH v8 7/9] ccw: document ADMIN_VQ as reserved

[Michael S. Tsirkin]

On Mon, Nov 21, 2022 at 04:53:22PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mst@redhat.com>
> > ---
> >  content.tex | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 9c74fa9..7111929 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -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/
> 
> > +
> > +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.

OK Makes sense.

MMIO has both.

I'm not sure how I should refer to shared memory - I think
ATM it's just VIRTIO_PMEM_F_SHMEM_REGION that's using it, right?


> (What is the situation for MMIO?)
> 
> > +
> > +\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

Re: [PATCH v8 7/9] ccw: document ADMIN_VQ as reserved

[Michael S. Tsirkin]

On Mon, Nov 21, 2022 at 04:53:22PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mst@redhat.com>
> > ---
> >  content.tex | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 9c74fa9..7111929 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -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/
> 
> > +
> > +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?)
> 
> > +
> > +\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 <mst@redhat.com>

diff --git a/content.tex b/content.tex
index 30f8efb..776fc56 100644
--- a/content.tex
+++ b/content.tex
@@ -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
 
 \end{itemize}
 

[virtio-dev] Re: [PATCH v8 7/9] ccw: document ADMIN_VQ as reserved

[Cornelia Huck]

On Mon, Nov 21 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mst@redhat.com>
>
> diff --git a/content.tex b/content.tex
> index 30f8efb..776fc56 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -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.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v8 7/9] ccw: document ADMIN_VQ as reserved

[Michael S. Tsirkin]

On Tue, Nov 22, 2022 at 09:50:35AM +0100, Cornelia Huck wrote:
> On Mon, Nov 21 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mst@redhat.com>
> >
> > diff --git a/content.tex b/content.tex
> > index 30f8efb..776fc56 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -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.

In fact VIRTIO_F_NOTIF_CONFIG_DATA is also not supported for MMIO.

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

Yes - if it's me that's going to work on this then I'd rather make it a
patch on top though, I'd like to work on ADMIN_VQ first as that
has people waiting on it. Work on hardening spec wording is something
that will never end ;)

-- 
MST

[virtio] Re: [PATCH v8 2/9] admin: introduce device group and related concepts

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  content.tex |  2 ++
>  2 files changed, 52 insertions(+)
>  create mode 100644 admin.tex
>
> diff --git a/admin.tex b/admin.tex
> new file mode 100644
> index 0000000..4337db0
> --- /dev/null
> +++ b/admin.tex
> @@ -0,0 +1,50 @@
> +\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. Owner device itself is not

s/Owner/The owner/

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

> +\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, ..."

> +	and what kind of control does the owner have.

s/does the owner have/the owner has/

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

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

"The following group types are currently specified:" ?

Less editing once we add a second one :)

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

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


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio-dev] Re: [PATCH v8 3/9] admin: introduce group administration commands

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>

(...)

> +\field{opcode} specifies the command. The valid
> +values for \field{opcode} can be found in the following table:
> +
> +\begin{tabular}{|l|l|}
> +\hline
> +opcode & Command Description \\
> +\hline \hline
> +0000h - 7FFFh   & Group administration commands    \\

I think we usually use the 0x notation for hex values?

> +\hline
> +8000h - FFFFh   & Reserved    \\
> +\hline
> +\end{tabular}
> +
> +The \field{group_id} specifies the device group.
> +The \field{group_member_id} specifies the member device within the group.
> +See section \ref{sec:Introduction / Terminology / Device group}
> +for the definition of the group identifier and group member
> +identifier.
> +
> +The following table describes possible \field{status} values,

s/,/;/

> +to simplify common implementations, these are intentionally

s/these/they/

> +matching common Linux names and error numbers:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Status & Name & Description \\
> +\hline \hline
> +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\

0x00?

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

s/was set//

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

s/was set//

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

I assume that this means that 0-4 are reserved for these error codes and
may not be used for something else, while 'other' is basically where
each group type may define their specific error codes? So maybe make the
last line

0x05 - & - & group type specific group administration command error \\


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [PATCH v8 4/9] admin: introduce virtio admin virtqueues

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  content.tex |  6 ++++--
>  2 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 507e188..bfa63a2 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -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.
> +
> +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."

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

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

s/Driver/The driver/

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

> +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."

> Device silently ignores any data falling outside

s/Device/The device/

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

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

> +for versioning.
> diff --git a/content.tex b/content.tex
> index 9deacbf..d2fd1de 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -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}.
>  
> +  \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." ?

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


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

[virtio-dev] Re: [PATCH v8 5/9] pci: add admin vq registers to virtio over pci

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  content.tex | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)

(...)

> @@ -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}).
>  
> +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?

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio] Re: [PATCH v8 8/9] admin: command list discovery

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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 <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 114 insertions(+), 16 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index bfa63a2..589e06a 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -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}
>  
> -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?

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

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

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

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

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

>  \hline
>  8000h - FFFFh   & Reserved    \\

0x8000 - 0xFFFF & - & Reserved \\

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

>  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,
> @@ -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".

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

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

status_qualifier

> +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?)

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

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

s/was set//

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

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

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


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


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio] Re: [PATCH v8 9/9] admin: conformance clauses

[Cornelia Huck]

On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> Add conformance clauses for admin commands and admin virtqueues.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
>
> diff --git a/admin.tex b/admin.tex
> index 589e06a..290cbbc 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -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/

> +\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 ..." ?

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

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

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

s/device/, the device/

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

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

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

> +with the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.

s/with/to/

> +
> +Device MUST validate commands against the list used by

s/Device/The device/

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

s/driver/the driver/

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

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

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

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

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

> +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."

> +
> +Driver MUST NOT request (by VIRTIO_ADMIN_CMD_LIST_USE)

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

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

> +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
> @@ -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/

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

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

> +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."

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

s/Device/The device/

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

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

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

s/Driver/The driver/

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

> +structure as described in the specification, instead,

s/,/;/

> +the driver MUST assume that the structure
> +outside the part of the buffer used by the device
> +is set to zero.
> +
> +If multiple administration virtqueues have been configured,
> +driver MUST ensure ordering for commands

s/driver/the driver/

> +placed on different administration virtqueues.


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [PATCH v8 8/9] admin: command list discovery

[Michael S. Tsirkin]

On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex | 130 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 114 insertions(+), 16 deletions(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index bfa63a2..589e06a 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -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}
> >  
> > -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?

Yes, will move.

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


Yes, will move.

> >  \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 \\
> 
> > +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 \\
> 
> > +0002h - 7FFFh & - &  Group administration commands    \\
> 
> 0x0002 - 0x7FFF & - & Further group administration commands \\
> 
> >  \hline
> >  8000h - FFFFh   & Reserved    \\
> 
> 0x8000 - 0xFFFF & - & Reserved \\

Using 0x instead of h suffix? Sure.

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

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


Yes, will move.

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

and this too.

> > @@ -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".
> 
> >  \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?

right.

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

right

> > +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?)

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).

> > +\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//
> 
> > +\hline
> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
> 
> s/was set//
> 
> > +\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.


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

Sounds good, thanks!

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


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

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

Re: [PATCH v8 9/9] admin: conformance clauses

[Michael S. Tsirkin]

On Tue, Nov 22, 2022 at 05:06:04PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Add conformance clauses for admin commands and admin virtqueues.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex | 122 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 122 insertions(+)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 589e06a..290cbbc 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -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/
> 
> > +\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 ..." ?
> 
> > +
> > +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/
> 
> > +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/
> 
> > +for different group types.
> > +
> > +Before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
> > +device MUST assume
> 
> s/device/, the device/
> 
> > +that the list of legal commands used by driver consists of two commands
> 
> s/driver/the driver/
> s/two/the two/
> 
> > +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/
> 
> > +with the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
> 
> s/with/to/
> 
> > +
> > +Device MUST validate commands against the list used by
> 
> s/Device/The device/
> 
> > +driver and MUST fail any commands not in the list with
> 
> s/driver/the driver/
> 
> > +\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/
> 
> > +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?

I feel it's a bad idea to allow this to change even across resets, at
least until we have a usecase where it's neeed. Basically
same as with features. Will document.


> > +
> > +\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/
> 
> > +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/
> 
> > +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/
> 
> > +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."
> 
> > +
> > +Driver MUST NOT request (by VIRTIO_ADMIN_CMD_LIST_USE)
> 
> s/Driver/The driver/
> s/by/via/
> 
> > +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/
> 
> > +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
> > @@ -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/
> 
> > +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/
> 
> > +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/
> 
> > +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."
> 
> > +
> > +Device MUST NOT fail a command solely because the buffers
> 
> s/Device/The device/
> 
> > +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/
> 
> > +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/
> 
> > +longer than described in this specification.
> > +
> > +Driver SHOULD supply device-readable buffers at least as
> 
> s/Driver/The driver/
> 
> > +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/
> 
> > +structure as described in the specification, instead,
> 
> s/,/;/
> 
> > +the driver MUST assume that the structure
> > +outside the part of the buffer used by the device
> > +is set to zero.
> > +
> > +If multiple administration virtqueues have been configured,
> > +driver MUST ensure ordering for commands
> 
> s/driver/the driver/
> 
> > +placed on different administration virtqueues.


Rest of suggestions all look good to me, thanks!

-- 
MST

Re: [PATCH v8 5/9] pci: add admin vq registers to virtio over pci

[Michael S. Tsirkin]

On Tue, Nov 22, 2022 at 03:46:38PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  content.tex | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> 
> (...)
> 
> > @@ -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}).
> >  
> > +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?

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


-- 
MST

Re: [PATCH v8 4/9] admin: introduce virtio admin virtqueues

[Michael S. Tsirkin]

On Tue, Nov 22, 2022 at 02:14:23PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex   | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  content.tex |  6 ++++--
> >  2 files changed, 57 insertions(+), 2 deletions(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 507e188..bfa63a2 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -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.
> > +
> > +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.

> 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."




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

> > +
> > +Administration virtqueues are used as follows:
> > +\begin{itemize}
> > +\item Driver submits the command using the \field{struct virtio_admin_cmd}
> 
> s/Driver/The driver/
> 
> > +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/
> 
> > +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."
> 
> > Device silently ignores any data falling outside
> 
> s/Device/The device/
> 
> > +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/
> 
> > +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/
> 
> > +for versioning.
> > diff --git a/content.tex b/content.tex
> > index 9deacbf..d2fd1de 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -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}.
> >  
> > +  \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." ?
> 
> > +
> >  \end{description}
> >  
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

other looks good will adopt, thanks!

Re: [PATCH v8 2/9] admin: introduce device group and related concepts

[Michael S. Tsirkin]

On Tue, Nov 22, 2022 at 01:11:19PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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 <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  content.tex |  2 ++
> >  2 files changed, 52 insertions(+)
> >  create mode 100644 admin.tex
> >
> > diff --git a/admin.tex b/admin.tex
> > new file mode 100644
> > index 0000000..4337db0
> > --- /dev/null
> > +++ b/admin.tex
> > @@ -0,0 +1,50 @@
> > +\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. Owner device itself is not
> 
> s/Owner/The owner/
> 
> > +	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.

> > +\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, ..."
> 
> > +	and what kind of control does the owner have.
> 
> s/does the owner have/the owner has/
> 
> > +	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.

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


> > +\end{description}
> > +
> > +A single group type is currently specified:
> 
> "The following group types are currently specified:" ?
> 
> Less editing once we add a second one :)
> 
> > +\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.)
> 
> > +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 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}


Others look good to me, will use, thanks!

-- 
MST

Re: [PATCH v8 1/9] virtio: document forward compatibility guarantees

[Michael S. Tsirkin]

On Mon, Nov 21, 2022 at 04:24:26PM +0100, Cornelia Huck wrote:
> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Feature negotiation forms the basis of forward compatibility
> > guarantees of virtio but has never been properly documented.
> > Do it now.
> >
> > Suggested-by: Halil Pasic <pasic@linux.ibm.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  content.tex | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 3051399..1e91b1d 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -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.
> >  
> > +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."
> 
> > In particular, this is
> > +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.
> >  
> > +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?

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


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

Anything violating this breaks our fundamental forward compability
assumptions: we need to be sure we can add feature bits without
breaking drivers. E.g. QEMU keeps adding feature bits
by default, and expects things to work. Right?

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

[virtio-dev] Re: [PATCH v8 1/9] virtio: document forward compatibility guarantees

[Cornelia Huck]

On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Nov 21, 2022 at 04:24:26PM +0100, Cornelia Huck wrote:
>> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >  \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.
>> >  
>> > +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?

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

>
>> 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).

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-comment] Re: [PATCH v8 2/9] admin: introduce device group and related concepts

[Cornelia Huck]

On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 22, 2022 at 01:11:19PM +0100, Cornelia Huck wrote:
>> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > +\item[Member device]
>> > +        a device within a group. Owner device itself is not
>> 
>> s/Owner/The owner/
>> 
>> > +	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.

Ok, I think we can expand this later, if needed.

>
>> > +\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, ..."
>> 
>> > +	and what kind of control does the owner have.
>> 
>> s/does the owner have/the owner has/
>> 
>> > +	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.
>
> I feel I went too broad here. Let's just drop the "It is envisioned"
> for now.

Ok, let's drop the "At the moment" prefix as well :)


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

Re: [PATCH v8 1/9] virtio: document forward compatibility guarantees

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 10:21:52AM +0100, Cornelia Huck wrote:
> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Nov 21, 2022 at 04:24:26PM +0100, Cornelia Huck wrote:
> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >  \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.
> >> >  
> >> > +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?
> 
> Hm, right. Let's keep this as MUST.
> 
> >
> >> 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).
> 
> > 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}

ok, thanks!

[virtio-dev] Re: [PATCH v8 4/9] admin: introduce virtio admin virtqueues

[Cornelia Huck]

On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 22, 2022 at 02:14:23PM +0100, Cornelia Huck wrote:
>> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > +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.

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.

>
>> 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."
>
>
>
>
>> > +
>> > +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?

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


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v8 4/9] admin: introduce virtio admin virtqueues

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 10:30:25AM +0100, Cornelia Huck wrote:
> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 22, 2022 at 02:14:23PM +0100, Cornelia Huck wrote:
> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > +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.
> 
> 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.


> >
> >> 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."
> >
> >
> >
> >
> >> > +
> >> > +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?
> 
> Ok, if we don't need it now, we can add it later.

[virtio-comment] Re: [PATCH v8 5/9] pci: add admin vq registers to virtio over pci

[Cornelia Huck]

On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 22, 2022 at 03:46:38PM +0100, Cornelia Huck wrote:
>> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > 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 <mgurtovoy@nvidia.com>
>> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> > ---
>> >  content.tex | 34 ++++++++++++++++++++++++++++++++++
>> >  1 file changed, 34 insertions(+)
>> 
>> (...)
>> 
>> > @@ -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}).
>> >  
>> > +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?

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

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

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


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: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
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/

Re: [PATCH v8 5/9] pci: add admin vq registers to virtio over pci

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 10:36:25AM +0100, Cornelia Huck wrote:
> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 22, 2022 at 03:46:38PM +0100, Cornelia Huck wrote:
> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > 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 <mgurtovoy@nvidia.com>
> >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >> > ---
> >> >  content.tex | 34 ++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 34 insertions(+)
> >> 
> >> (...)
> >> 
> >> > @@ -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}).
> >> >  
> >> > +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?
> 
> It should work fine, I guess; probably no need to spell it out
> explicitly.
> 
> >
> >> > +
> >> >  \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.
> >> >  
> >> >  \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?
> 
> By this reasoning, we probably should also add a comment for
> NOTIFICATION_DATA and RING_RESET? (On top, of course.)

Yes I think we should.

-- 
MST

[virtio] Re: [PATCH v8 8/9] admin: command list discovery

[Cornelia Huck]

On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
>> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Using 0x instead of h suffix? Sure.
>
> We should probably document the syntax in introduction.tex
> though this seems low priority.

Nod.
>> > +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).

Ok.

>
>> > +\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//
>> 
>> > +\hline
>> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
>> 
>> s/was set//
>> 
>> > +\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?

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

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?

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


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

[virtio-dev] Re: [PATCH v8 9/9] admin: conformance clauses

[Cornelia Huck]

On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Nov 22, 2022 at 05:06:04PM +0100, Cornelia Huck wrote:
>> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > +List of supported commands MUST NOT change: after reporting a
>> 
>> s/List/The list/
>> 
>> > +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?
>
> I feel it's a bad idea to allow this to change even across resets, at
> least until we have a usecase where it's neeed. Basically
> same as with features. Will document.

Ok.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v8 4/9] admin: introduce virtio admin virtqueues

[Cornelia Huck]

On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Nov 23, 2022 at 10:30:25AM +0100, Cornelia Huck wrote:
>> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Tue, Nov 22, 2022 at 02:14:23PM +0100, Cornelia Huck wrote:
>> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> > +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.
>> 
>> 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.

Then let's stick with the reange.


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

Re: [PATCH v8 8/9] admin: command list discovery

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 10:51:31AM +0100, Cornelia Huck wrote:
> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > Using 0x instead of h suffix? Sure.
> >
> > We should probably document the syntax in introduction.tex
> > though this seems low priority.
> 
> Nod.
> >> > +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).
> 
> Ok.
> 
> >
> >> > +\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//
> >> 
> >> > +\hline
> >> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
> >> 
> >> s/was set//
> >> 
> >> > +\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?

Not yet.

> >> > +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
> >> > +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.
> 
> 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?

I think so, yes. Good point I'll document that.


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

[virtio] Re: [PATCH v8 8/9] admin: command list discovery

[Cornelia Huck]

On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Nov 23, 2022 at 10:51:31AM +0100, Cornelia Huck wrote:
>> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
>> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> > +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).
>> 
>> Ok.
>> 
>> >
>> >> > +\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//
>> >> 
>> >> > +\hline
>> >> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
>> >> 
>> >> s/was set//
>> >> 
>> >> > +\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?
>
> Not yet.

Then maybe let's make the last line

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

?


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [PATCH v8 8/9] admin: command list discovery

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 11:09:26AM +0100, Cornelia Huck wrote:
> On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Nov 23, 2022 at 10:51:31AM +0100, Cornelia Huck wrote:
> >> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
> >> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> > +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).
> >> 
> >> Ok.
> >> 
> >> >
> >> >> > +\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//
> >> >> 
> >> >> > +\hline
> >> >> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
> >> >> 
> >> >> s/was set//
> >> >> 
> >> >> > +\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?
> >
> > Not yet.
> 
> Then maybe let's make the last line
> 
> 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?

-- 
MST

[virtio] Re: [PATCH v8 8/9] admin: command list discovery

[Cornelia Huck]

On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Nov 23, 2022 at 11:09:26AM +0100, Cornelia Huck wrote:
>> On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> 
>> > On Wed, Nov 23, 2022 at 10:51:31AM +0100, Cornelia Huck wrote:
>> >> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> 
>> >> > On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
>> >> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> >> > +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).
>> >> 
>> >> Ok.
>> >> 
>> >> >
>> >> >> > +\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//
>> >> >> 
>> >> >> > +\hline
>> >> >> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
>> >> >> 
>> >> >> s/was set//
>> >> >> 
>> >> >> > +\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?
>> >
>> > Not yet.
>> 
>> Then maybe let's make the last line
>> 
>> 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.)


---------------------------------------------------------------------
To unsubscribe from this mail list, you must leave the OASIS TC that 
generates this mail.  Follow this link to all your TCs in OASIS at:
https://www.oasis-open.org/apps/org/workgroup/portal/my_workgroups.php 

Re: [PATCH v8 8/9] admin: command list discovery

[Michael S. Tsirkin]

On Wed, Nov 23, 2022 at 11:33:40AM +0100, Cornelia Huck wrote:
> On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Nov 23, 2022 at 11:09:26AM +0100, Cornelia Huck wrote:
> >> On Wed, Nov 23 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> 
> >> > On Wed, Nov 23, 2022 at 10:51:31AM +0100, Cornelia Huck wrote:
> >> >> On Tue, Nov 22 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> 
> >> >> > On Tue, Nov 22, 2022 at 04:25:23PM +0100, Cornelia Huck wrote:
> >> >> >> On Sun, Nov 20 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> >> > +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).
> >> >> 
> >> >> Ok.
> >> >> 
> >> >> >
> >> >> >> > +\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//
> >> >> >> 
> >> >> >> > +\hline
> >> >> >> > +04h   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} was set  \\
> >> >> >> 
> >> >> >> s/was set//
> >> >> >> 
> >> >> >> > +\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?
> >> >
> >> > Not yet.
> >> 
> >> Then maybe let's make the last line
> >> 
> >> 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.)

Right. OK I'm fine with this and documenting that it should be handled
same as 0x0.

-- 
MST

Re: [virtio-comment] RE: [PATCH v8 8/9] admin: command list discovery

[Michael S. Tsirkin]

On Mon, Nov 21, 2022 at 11:06:14AM +0000, Uminski, Piotr wrote:
> 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

RE: [virtio-comment] RE: [PATCH v8 8/9] admin: command list discovery

[Uminski, Piotr]

> From: Michael S. Tsirkin <mst@redhat.com>
> 
> On Mon, Nov 21, 2022 at 11:06:14AM +0000, Uminski, Piotr wrote:
> > 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.
> 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. 

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

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

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

Re: [virtio-comment] RE: [PATCH v8 8/9] admin: command list discovery

[Michael S. Tsirkin]

On Thu, Dec 15, 2022 at 09:52:07AM +0000, Uminski, Piotr wrote:
> > From: Michael S. Tsirkin <mst@redhat.com>
> > 
> > On Mon, Nov 21, 2022 at 11:06:14AM +0000, Uminski, Piotr wrote:
> > > 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.
> > 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. 

which doesn't seem big.


I do think I see an issue though in that the driver does not currently
know whether the command buffer was truncated.  It sounds like a useful
piece of information at least for debugging.  I am thinking of adding a
"max-inbuf" value for device to return to driver (together with the
status) but it looks like it will typically repeat the used buffer size
- and duplicating values leads to bugs.

Better ideas?




> > 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.
> 
> > 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.
> 
> > 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: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> 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/