[PATCH v9 00/10] Introduce device group and device management

[PATCH v9 00/10] Introduce device group and device management

[Michael S. Tsirkin]

Change log:

since v8:
	addressed comments by Cornelia - as we agreed on list
	
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).

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


Michael S. Tsirkin (10):
  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
  ccw: document more reserved features

 admin.tex   | 435 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex | 111 +++++++++++++-
 2 files changed, 544 insertions(+), 2 deletions(-)
 create mode 100644 admin.tex

-- 
MST

[PATCH v9 01/10] 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/content.tex b/content.tex
index 3051399..e3203be 100644
--- a/content.tex
+++ b/content.tex
@@ -114,21 +114,63 @@ \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 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 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}
+
 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 v9 02/10] 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   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 47 insertions(+)
 create mode 100644 admin.tex

diff --git a/admin.tex b/admin.tex
new file mode 100644
index 0000000..6ebdd05
--- /dev/null
+++ b/admin.tex
@@ -0,0 +1,45 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
+
+It is occasionally useful to have a device control a group of
+other devices. Terminology used in such cases:
+
+\begin{description}
+\item[Device group]
+        or just group, includes zero or more devices.
+\item[Owner device]
+        or owner, the device controlling the group.
+\item[Member device]
+        a device within a group. The owner device itself is not
+	a member of the group.
+\item[Member identifier]
+        each member has this identifier, unique within the group
+	and used to address it through the owner device.
+\item[Group type identifier]
+	specifies what kind of member devices there are in a
+	group, how is the member identifier is interpreted
+	and what kind of control the owner has.
+	A given owner can control a single group of a given type,
+	thus the type and the owner together identify the group.
+\end{description}
+
+The following group types, and their identifiers, are currently specified):
+\begin{description}
+\item[SR-IOV group type (0x1)]
+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as the owner and includes
+all its SR-IOV virtual functions (VFs) as members (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+The PF device itself is not a member of the group.
+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value 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 e3203be..3f3585d 100644
--- a/content.tex
+++ b/content.tex
@@ -491,6 +491,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 v9 03/10] 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/admin.tex b/admin.tex
index 6ebdd05..1dca136 100644
--- a/admin.tex
+++ b/admin.tex
@@ -42,4 +42,102 @@ \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;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+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 & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}
+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common Linux names and error numbers:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\
+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
+0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
+\hline
+0x05-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}
-- 
MST

[PATCH v9 04/10] 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   | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  7 +++++--
 2 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/admin.tex b/admin.tex
index 1dca136..eeccd2e 100644
--- a/admin.tex
+++ b/admin.tex
@@ -141,3 +141,57 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 0x05-0xFFFF   & -    & reserved for future use \\
 \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.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
+or more adminstration virtqueues. The number and locations of the
+administration virtqueues are exposed by the owner device in a transport
+specific manner.
+
+The driver 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 The 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. The device silently truncates the structures
+to either the length submitted by the driver, or the length
+described in this specification, whichever is shorter.
+The device silently ignores any data falling outside
+the shorter of the two lengths. Any missing fields are 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. The 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 the driver using the full structure without concern
+for versioning.
diff --git a/content.tex b/content.tex
index 3f3585d..04235da 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}
@@ -6990,6 +6990,9 @@ \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 the device exposes one or more
+  administration virtqueues.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
MST

[PATCH v9 05/10] 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 04235da..aa68926 100644
--- a/content.tex
+++ b/content.tex
@@ -946,6 +946,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}
 
@@ -1031,6 +1035,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}
@@ -1117,6 +1134,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
@@ -6992,6 +7017,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
 
   \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
   administration virtqueues.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI
+	  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 v9 06/10] 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 aa68926..023f9b9 100644
--- a/content.tex
+++ b/content.tex
@@ -2362,6 +2362,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 v9 07/10] 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.

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

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

diff --git a/content.tex b/content.tex
index 023f9b9..6c3758d 100644
--- a/content.tex
+++ b/content.tex
@@ -2976,6 +2976,18 @@ \subsubsection{Resetting Devices}\label{sec:Virtio Transport Options / Virtio ov
 MAY also choose to verify reset completion by reading \field{device status} via
 CCW_CMD_READ_STATUS and checking whether it is 0 afterwards.
 
+\subsection{Features reserved for future use}\label{sec:Virtio Transport Options / Virtio over channel I/O / Features reserved for future use}
+
+At this time, devices and drivers utilizing Virtio over channel I/O
+do not support the following features:
+\begin{itemize}
+
+\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 v9 08/10] 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.

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

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

diff --git a/admin.tex b/admin.tex
index eeccd2e..eec12a9 100644
--- a/admin.tex
+++ b/admin.tex
@@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \hline
 opcode & Name & Command Description \\
 \hline \hline
-0x0000 - 0x7FFF & - &  Group administration commands    \\
+0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
+0x0002 - 0x7FFF & - &  Group administration commands    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved    \\
 \hline
@@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \hline
 \end{tabular}
 
+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.
+
 \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 v9 09/10] 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)

diff --git a/admin.tex b/admin.tex
index eec12a9..e83a9f5 100644
--- a/admin.tex
+++ b/admin.tex
@@ -222,6 +222,107 @@ \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}
+
+The device MUST validate \field{opcode}, \field{group_type} and
+\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
+nd if any of these has an invalid or unsupported value, set
+\field{status_qualifier} accordingly.
+
+If a command completes successfully, the device MUST set
+\field{status} to VIRTIO_ADMIN_STATUS_OK.
+
+If a command fails, the device MUST set
+\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
+
+If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
+device state MUST NOT change, that is the command MUST NOT have
+any side effects on the device, in particular the device MUST not
+enter an error state as a result of this command.
+
+If a command fails, the device state generally SHOULD NOT change,
+as far as possible.
+
+The 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,
+the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
+for different group types.
+
+After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
+the device MUST assume
+that the list of legal commands used by the driver consists of
+the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
+
+After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
+the device MUST set the list of legal commands used by the driver
+to the one supplied in \field{command_specific_data}.
+
+The device MUST set the list of legal commands used by the driver
+to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
+
+The device MUST validate commands against the list used by
+the 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.
+
+The list of supported commands MUST NOT shrink (but MAY expand):
+after reporting a given command as supported through
+VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
+as unsupported.  Further, after a given set of commands has been
+used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
+device or system reset the device SHOULD complete successfully
+any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
+list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
+after a device or system reset, the device MUST not fail it
+solely because of the command list used.  Failure to do so would
+interfere with resuming from suspend and error recovery.
+
+
+\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+
+The driver MAY discover whether device supports a specific group type
+by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
+\field{group_type}.
+
+The 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).
+
+The driver SHOULD NOT set bits in device_admin_cmds
+if it is not familiar with how the command opcode
+is used, as dependencies between command opcodes might exist.
+
+The driver MUST NOT request (via 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.
+
+The 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.
+
+The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
+VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
+with respective bits cleared in \field{command_specific_data}.
+
+The driver MUST handle a command error with a reserved \field{status}
+value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
+(except possibly for different error reporting/diagnostic messages).
+
+The driver MUST handle a command error with a reserved
+\field{status_qualifier} value in the same way as
+\field{status_qualifier} set to
+VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
+different error reporting/diagnostic messages).
+
 \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
 
 An administration virtqueue of an owner device is used to submit
@@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
 new fields can be added to the tail of a structure,
 with the driver using the full structure without concern
 for versioning.
+
+\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+
+The 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}
+
+The 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}
+
+The device SHOULD initialize the device-writeable buffer
+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.
+
+The device MUST NOT fail a command solely because the buffers
+provided are shorter or longer than described in this
+specification.
+
+The device MUST process commands on a given administration 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}
+
+The driver MAY supply device-readable and device-writeable buffers
+longer than described in this specification.
+
+The 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).
+
+The driver MUST NOT assume that the 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,
+the driver MUST ensure ordering for commands
+placed on different administration virtqueues.
-- 
MST

[PATCH RFC v9 10/10] ccw: document more reserved features

[Michael S. Tsirkin]

vq reset and shared memory are unsupported, too.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---


This is not strictly part of this patchset, but just to
give you the idea of how we can expand this down the road.

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

diff --git a/content.tex b/content.tex
index 6c3758d..bfcf9fe 100644
--- a/content.tex
+++ b/content.tex
@@ -2983,6 +2983,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}
 
-- 
MST

Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 3051399..e3203be 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -114,21 +114,63 @@ \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 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.)

It looks to me if we want to clarify like this, feature negotiation is
not sufficient. Do we need to do something similar in other basic
facilities? Generally, we probably need to do this for facilities that
are similar to features (status, virtqueue size and others).

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

I'm not sure this is the best way or whether we've defined the
"unexpected features" before. It might be better to say that to accept
the features that the driver understands.

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

Do we need a similar part for the device?

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

This sentence is incomplete.

Thanks

> --
> MST
>

Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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   | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  content.tex |  2 ++
>  2 files changed, 47 insertions(+)
>  create mode 100644 admin.tex
>
> diff --git a/admin.tex b/admin.tex
> new file mode 100644
> index 0000000..6ebdd05
> --- /dev/null
> +++ b/admin.tex
> @@ -0,0 +1,45 @@
> +\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
> +
> +It is occasionally useful to have a device control a group of
> +other devices. Terminology used in such cases:
> +
> +\begin{description}
> +\item[Device group]
> +        or just group, includes zero or more devices.
> +\item[Owner device]
> +        or owner, the device controlling the group.
> +\item[Member device]
> +        a device within a group. The owner device itself is not
> +       a member of the group.
> +\item[Member identifier]
> +        each member has this identifier, unique within the group
> +       and used to address it through the owner device.
> +\item[Group type identifier]
> +       specifies what kind of member devices there are in a
> +       group, how is the member identifier is interpreted
> +       and what kind of control the owner has.
> +       A given owner can control a single group of a given type,
> +       thus the type and the owner together identify the group.
> +\end{description}
> +
> +The following group types, and their identifiers, are currently specified):
> +\begin{description}
> +\item[SR-IOV group type (0x1)]
> +This device group has a PCI Single Root I/O Virtualization
> +(SR-IOV) physical function (PF) device as the owner and includes
> +all its SR-IOV virtual functions (VFs) as members (see
> +\hyperref[intro:PCIe]{[PCIe]}).

So I wonder what's the advantage of using a global identifier over the
transport specific one. There's almost no way for CCW/MMIO to use
SR-IOV. Limiting it to PCI seems much easier and avoids layer
violation.

Thanks

> +
> +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 e3203be..3f3585d 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -491,6 +491,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
>

[virtio-dev] Re: [PATCH v9 03/10] admin: introduce group administration commands

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>
> diff --git a/admin.tex b/admin.tex
> index 6ebdd05..1dca136 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -42,4 +42,102 @@ \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.

This excludes any possible runtime configuration, is this intended?


> +
> +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;
> +        le16 status_qualifier;
> +        /* unused, reserved for future extensions */
> +        u8 reserved2[4];
> +        u8 command_specific_result[];
> +};
> +\end{lstlisting}
> +
> +For all commands, \field{opcode}, \field{group_type} and if
> +necessary \field{group_member_id} and \field{command_specific_data} are
> +set by the driver, and the owner device sets \field{status} and if
> +needed \field{status_qualifier} and
> +\field{command_specific_result}.
> +
> +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 & Name & Command Description \\
> +\hline \hline
> +0x0000 - 0x7FFF & - &  Group administration commands    \\
> +\hline
> +0x8000 - 0xFFFF & - & Reserved    \\
> +\hline
> +\end{tabular}
> +
> +The \field{group_type} specifies the group type identifier.

So this means there's no way for having a group specific adminq? E.g
one queue for SR-IOV and another for SIOV.

> +The \field{group_member_id} specifies the member identifier within the group.
> +See section \ref{sec:Introduction / Terminology / Device group}
> +for the definition of the group type identifier and group member
> +identifier.
> +
> +The following table describes possible \field{status} values;
> +to simplify common implementations, they are intentionally
> +matching common Linux names and error numbers:

I may miss something but I don't see how it matches Linux error numbers.

Thanks

> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Status (decimal) & Name & Description \\
> +\hline \hline
> +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> +\hline
> +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
> +\hline
> +other   & -    & group administration command error  \\
> +\hline
> +\end{tabular}
> +
> +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
> +0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> +\hline
> +0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> +\hline
> +0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> +\hline
> +0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
> +\hline
> +0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
> +\hline
> +0x05-0xFFFF   & -    & reserved for future use \\
> +\hline
> +\end{tabular}
> --
> MST
>


---------------------------------------------------------------------
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 v9 05/10] pci: add admin vq registers to virtio over pci

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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(+)
>
> diff --git a/content.tex b/content.tex
> index 04235da..aa68926 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -946,6 +946,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 */

If we are adding more queue related functionality, it would be placed
under the above two fields which seems not elegant. I wonder if a new
structure is needed here.

Thanks

>  };
>  \end{lstlisting}
>
> @@ -1031,6 +1035,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}
> @@ -1117,6 +1134,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
> @@ -6992,6 +7017,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
>    \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
>    administration virtqueues.
> +  At the moment this feature is only supported for devices using
> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> +         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
>

Re: [PATCH v9 06/10] mmio: document ADMIN_VQ as reserved

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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 aa68926..023f9b9 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2362,6 +2362,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

Do we need to add VIRTIO_F_SR_IOV here? And probably SR-IOV group type?

Thanks

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

Re: [PATCH v9 08/10] admin: command list discovery

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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.
>
> An alternative is per-type VQs. This is possible but will
> require more per-transport work. Discovery through the vq
> helps keep things contained.

This seems to complicate the migration (compatibility) of the adminq
itself. We need check both features and command list.

More below.

>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/admin.tex b/admin.tex
> index eeccd2e..eec12a9 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline
>  opcode & Name & Command Description \\
>  \hline \hline
> -0x0000 - 0x7FFF & - &  Group administration commands    \\
> +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> +0x0002 - 0x7FFF & - &  Group administration commands    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved    \\
>  \hline
> @@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline
>  \end{tabular}
>
> +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.

Is this simpler if we map group type instead of a single opcode? E.g
virtqueue transport will introduce 10+ commands, using a bit for a
specific opcode will complicate things.

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

Any reason we need to invent a negotiation process here? (This seems
only useful when some commands/opcodes are mutually exclusive)

Thanks

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

Re: [PATCH v9 09/10] admin: conformance clauses

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 158 insertions(+)
>
> diff --git a/admin.tex b/admin.tex
> index eec12a9..e83a9f5 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -222,6 +222,107 @@ \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}
> +
> +The device MUST validate \field{opcode}, \field{group_type} and
> +\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
> +nd if any of these has an invalid or unsupported value, set

typo

> +\field{status_qualifier} accordingly.
> +
> +If a command completes successfully, the device MUST set
> +\field{status} to VIRTIO_ADMIN_STATUS_OK.
> +
> +If a command fails, the device MUST set
> +\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
> +
> +If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
> +device state MUST NOT change, that is the command MUST NOT have
> +any side effects on the device, in particular the device MUST not
> +enter an error state as a result of this command.
> +
> +If a command fails, the device state generally SHOULD NOT change,
> +as far as possible.
> +
> +The 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,
> +the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
> +for different group types.
> +
> +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
> +the device MUST assume
> +that the list of legal commands used by the driver consists of
> +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> +
> +After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
> +the device MUST set the list of legal commands used by the driver
> +to the one supplied in \field{command_specific_data}.
> +
> +The device MUST set the list of legal commands used by the driver
> +to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
> +
> +The device MUST validate commands against the list used by
> +the 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.
> +
> +The list of supported commands MUST NOT shrink (but MAY expand):
> +after reporting a given command as supported through
> +VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
> +as unsupported.  Further, after a given set of commands has been
> +used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
> +device or system reset the device SHOULD complete successfully
> +any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
> +list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
> +after a device or system reset, the device MUST not fail it
> +solely because of the command list used.  Failure to do so would
> +interfere with resuming from suspend and error recovery.
> +
> +
> +\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
> +
> +The driver MAY discover whether device supports a specific group type
> +by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
> +\field{group_type}.
> +
> +The 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).
> +
> +The driver SHOULD NOT set bits in device_admin_cmds
> +if it is not familiar with how the command opcode
> +is used, as dependencies between command opcodes might exist.
> +
> +The driver MUST NOT request (via 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.
> +
> +The 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.
> +
> +The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
> +VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
> +with respective bits cleared in \field{command_specific_data}.
> +
> +The driver MUST handle a command error with a reserved \field{status}
> +value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
> +(except possibly for different error reporting/diagnostic messages).
> +
> +The driver MUST handle a command error with a reserved
> +\field{status_qualifier} value in the same way as
> +\field{status_qualifier} set to
> +VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
> +different error reporting/diagnostic messages).
> +
>  \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
>
>  An administration virtqueue of an owner device is used to submit
> @@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
>  new fields can be added to the tail of a structure,
>  with the driver using the full structure without concern
>  for versioning.
> +
> +\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
> +
> +The 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

s/"is set"/"are set"

> +\item discarding data that would be written outside the
> +specified device-writeable buffers.
> +\end{enumerate}
> +
> +The 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}
> +
> +The device SHOULD initialize the device-writeable buffer
> +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.
> +
> +The device MUST NOT fail a command solely because the buffers
> +provided are shorter or longer than described in this
> +specification.

I may miss something but how can it work if the buffer is shorter?

> +
> +The device MUST process commands on a given administration virtqueue
> +in the order in which they are queued.

Is it better to use "complete" than "process" here? Or we can reuse
part of the in-order description.

(Or maybe a flush command)

> +
> +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}
> +
> +The driver MAY supply device-readable and device-writeable buffers
> +longer than described in this specification.
> +
> +The 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).
> +
> +The driver MUST NOT assume that the 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.

Won't this have security implications? E.g information leak.

Thanks

> +
> +If multiple administration virtqueues have been configured,
> +the driver MUST ensure ordering for commands
> +placed on different administration virtqueues.
> --
> MST
>

Re: [PATCH RFC v9 10/10] ccw: document more reserved features

[Jason Wang]

On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> vq reset and shared memory are unsupported, too.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I wonder if it's better to split this series into two since it doesn't
have direct connection with the admin stuffs then the compatibility
part can go first.

Thanks

> ---
>
>
> This is not strictly part of this patchset, but just to
> give you the idea of how we can expand this down the road.
>
>  content.tex | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 6c3758d..bfcf9fe 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2983,6 +2983,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}
>
> --
> MST
>

Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 3051399..e3203be 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -114,21 +114,63 @@ \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 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.)
> 
> It looks to me if we want to clarify like this, feature negotiation is
> not sufficient. Do we need to do something similar in other basic
> facilities? Generally, we probably need to do this for facilities that
> are similar to features (status, virtqueue size and others).

I'm not sure about "not sufficient". It's sufficient as long
as you just want to extend features. What triggered this
work is adding a transport specific feature.


> > 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.
> 
> I'm not sure this is the best way or whether we've defined the
> "unexpected features" before. It might be better to say that to accept
> the features that the driver understands.

So specifically, if feature is documented as pci only and then
down the road we enable it for ccw. Does driver "understand" it? Or not?
It is clearly unexpected.
I am not too worried about using terms we didn't define this is not
part of a conformance statement.
And people seem to get the meaning ;)

> > +
> > +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 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}
> 
> Do we need a similar part for the device?

Nope, we don't need devices to accept features they did not offer.

> > +
> >  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
> 
> This sentence is incomplete.

Because it's context produced by git diff? That cuts off after 3 lines.

> Thanks
> 
> > --
> > MST
> >

Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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   | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  content.tex |  2 ++
> >  2 files changed, 47 insertions(+)
> >  create mode 100644 admin.tex
> >
> > diff --git a/admin.tex b/admin.tex
> > new file mode 100644
> > index 0000000..6ebdd05
> > --- /dev/null
> > +++ b/admin.tex
> > @@ -0,0 +1,45 @@
> > +\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
> > +
> > +It is occasionally useful to have a device control a group of
> > +other devices. Terminology used in such cases:
> > +
> > +\begin{description}
> > +\item[Device group]
> > +        or just group, includes zero or more devices.
> > +\item[Owner device]
> > +        or owner, the device controlling the group.
> > +\item[Member device]
> > +        a device within a group. The owner device itself is not
> > +       a member of the group.
> > +\item[Member identifier]
> > +        each member has this identifier, unique within the group
> > +       and used to address it through the owner device.
> > +\item[Group type identifier]
> > +       specifies what kind of member devices there are in a
> > +       group, how is the member identifier is interpreted
> > +       and what kind of control the owner has.
> > +       A given owner can control a single group of a given type,
> > +       thus the type and the owner together identify the group.
> > +\end{description}
> > +
> > +The following group types, and their identifiers, are currently specified):
> > +\begin{description}
> > +\item[SR-IOV group type (0x1)]
> > +This device group has a PCI Single Root I/O Virtualization
> > +(SR-IOV) physical function (PF) device as the owner and includes
> > +all its SR-IOV virtual functions (VFs) as members (see
> > +\hyperref[intro:PCIe]{[PCIe]}).
> 
> So I wonder what's the advantage of using a global identifier over the
> transport specific one. There's almost no way for CCW/MMIO to use
> SR-IOV. Limiting it to PCI seems much easier and avoids layer
> violation.
> 
> Thanks

So we burn up an identifier, ccw and mmio won't be able to use it.
Big deal? Why? And I think we might find a use for this with MMIO
down the road with some kind of passthrough. Who knows.



> > +
> > +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 e3203be..3f3585d 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -491,6 +491,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
> >

Re: [PATCH v9 03/10] admin: introduce group administration commands

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 01:52:41PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 6ebdd05..1dca136 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -42,4 +42,102 @@ \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.
> 
> This excludes any possible runtime configuration, is this intended?

No idea - what kind of runtime configuration are you talking about?
This explicitly says "to configure a member device" and that's
certainly at runtime.


> 
> > +
> > +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;
> > +        le16 status_qualifier;
> > +        /* unused, reserved for future extensions */
> > +        u8 reserved2[4];
> > +        u8 command_specific_result[];
> > +};
> > +\end{lstlisting}
> > +
> > +For all commands, \field{opcode}, \field{group_type} and if
> > +necessary \field{group_member_id} and \field{command_specific_data} are
> > +set by the driver, and the owner device sets \field{status} and if
> > +needed \field{status_qualifier} and
> > +\field{command_specific_result}.
> > +
> > +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 & Name & Command Description \\
> > +\hline \hline
> > +0x0000 - 0x7FFF & - &  Group administration commands    \\
> > +\hline
> > +0x8000 - 0xFFFF & - & Reserved    \\
> > +\hline
> > +\end{tabular}
> > +
> > +The \field{group_type} specifies the group type identifier.
> 
> So this means there's no way for having a group specific adminq? E.g
> one queue for SR-IOV and another for SIOV.
> 
> > +The \field{group_member_id} specifies the member identifier within the group.
> > +See section \ref{sec:Introduction / Terminology / Device group}
> > +for the definition of the group type identifier and group member
> > +identifier.
> > +
> > +The following table describes possible \field{status} values;
> > +to simplify common implementations, they are intentionally
> > +matching common Linux names and error numbers:
> 
> I may miss something but I don't see how it matches Linux error numbers.


include/uapi/asm-generic/errno-base.h:#define   EINVAL          22      /* Invalid argument */


> Thanks
> 
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Status (decimal) & Name & Description \\
> > +\hline \hline
> > +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > +\hline
> > +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
> > +\hline
> > +other   & -    & group administration command error  \\
> > +\hline
> > +\end{tabular}
> > +
> > +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
> > +0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> > +\hline
> > +0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> > +\hline
> > +0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> > +\hline
> > +0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
> > +\hline
> > +0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
> > +\hline
> > +0x05-0xFFFF   & -    & reserved for future use \\
> > +\hline
> > +\end{tabular}
> > --
> > MST
> >

Re: [PATCH v9 05/10] pci: add admin vq registers to virtio over pci

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 02:00:54PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 04235da..aa68926 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -946,6 +946,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 */
> 
> If we are adding more queue related functionality, it would be placed
> under the above two fields which seems not elegant. I wonder if a new
> structure is needed here.
> 
> Thanks

I don't expect more, if that happens we can always refactor it.
Generally this is not C, no one compiles this directly.
So we do not need to add structures unless
they are referred to in spec text.

> >  };
> >  \end{lstlisting}
> >
> > @@ -1031,6 +1035,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}
> > @@ -1117,6 +1134,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
> > @@ -6992,6 +7017,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >
> >    \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
> >    administration virtqueues.
> > +  At the moment this feature is only supported for devices using
> > +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> > +         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
> >

Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Jason Wang]

On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
> > On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 3051399..e3203be 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -114,21 +114,63 @@ \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 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.)
> >
> > It looks to me if we want to clarify like this, feature negotiation is
> > not sufficient. Do we need to do something similar in other basic
> > facilities? Generally, we probably need to do this for facilities that
> > are similar to features (status, virtqueue size and others).
>
> I'm not sure about "not sufficient". It's sufficient as long
> as you just want to extend features. What triggered this
> work is adding a transport specific feature.

E.g:

For status: Devices do not offer any status bit it would not be able to handle.
For virtqueue size:  Devices do not offer virtqueue size it would not
be able to handle.

?

>
>
> > > 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.
> >
> > I'm not sure this is the best way or whether we've defined the
> > "unexpected features" before. It might be better to say that to accept
> > the features that the driver understands.
>
> So specifically, if feature is documented as pci only and then
> down the road we enable it for ccw. Does driver "understand" it? Or not?
> It is clearly unexpected.
> I am not too worried about using terms we didn't define this is not
> part of a conformance statement.
> And people seem to get the meaning ;)

Ok.

Thanks

Re: [virtio-dev] Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Jason Wang]

On Thu, Nov 24, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
> > On Thu, Nov 24, 2022 at 5:08 AM 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   | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >  content.tex |  2 ++
> > >  2 files changed, 47 insertions(+)
> > >  create mode 100644 admin.tex
> > >
> > > diff --git a/admin.tex b/admin.tex
> > > new file mode 100644
> > > index 0000000..6ebdd05
> > > --- /dev/null
> > > +++ b/admin.tex
> > > @@ -0,0 +1,45 @@
> > > +\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
> > > +
> > > +It is occasionally useful to have a device control a group of
> > > +other devices. Terminology used in such cases:
> > > +
> > > +\begin{description}
> > > +\item[Device group]
> > > +        or just group, includes zero or more devices.
> > > +\item[Owner device]
> > > +        or owner, the device controlling the group.
> > > +\item[Member device]
> > > +        a device within a group. The owner device itself is not
> > > +       a member of the group.
> > > +\item[Member identifier]
> > > +        each member has this identifier, unique within the group
> > > +       and used to address it through the owner device.
> > > +\item[Group type identifier]
> > > +       specifies what kind of member devices there are in a
> > > +       group, how is the member identifier is interpreted
> > > +       and what kind of control the owner has.
> > > +       A given owner can control a single group of a given type,
> > > +       thus the type and the owner together identify the group.
> > > +\end{description}
> > > +
> > > +The following group types, and their identifiers, are currently specified):
> > > +\begin{description}
> > > +\item[SR-IOV group type (0x1)]
> > > +This device group has a PCI Single Root I/O Virtualization
> > > +(SR-IOV) physical function (PF) device as the owner and includes
> > > +all its SR-IOV virtual functions (VFs) as members (see
> > > +\hyperref[intro:PCIe]{[PCIe]}).
> >
> > So I wonder what's the advantage of using a global identifier over the
> > transport specific one. There's almost no way for CCW/MMIO to use
> > SR-IOV. Limiting it to PCI seems much easier and avoids layer
> > violation.
> >
> > Thanks
>
> So we burn up an identifier, ccw and mmio won't be able to use it.
> Big deal? Why?

Because it is transport specific. The basic facility should be
transport independent.

> And I think we might find a use for this with MMIO
> down the road with some kind of passthrough. Who knows.

Probably, but can it be modeled exactly as what SR-IOV looks like? Or
anyhow, it's not too late to define this for MMIO at that time. For
example, we know MSI-X may work for MMIO but we define it only for PCI
now.

Thanks

>
>
>
> > > +
> > > +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 e3203be..3f3585d 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -491,6 +491,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
> > >
>
>
> ---------------------------------------------------------------------
> 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 v9 03/10] admin: introduce group administration commands

[Jason Wang]

On Thu, Nov 24, 2022 at 3:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 24, 2022 at 01:52:41PM +0800, Jason Wang wrote:
> > On Thu, Nov 24, 2022 at 5:08 AM 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 98 insertions(+)
> > >
> > > diff --git a/admin.tex b/admin.tex
> > > index 6ebdd05..1dca136 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -42,4 +42,102 @@ \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.
> >
> > This excludes any possible runtime configuration, is this intended?
>
> No idea - what kind of runtime configuration are you talking about?
> This explicitly says "to configure a member device" and that's
> certainly at runtime.

Not sure, but if we say something like we, it means we need a group of
commands to support runtime stuff in the future?

>
>
> >
> > > +
> > > +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;
> > > +        le16 status_qualifier;
> > > +        /* unused, reserved for future extensions */
> > > +        u8 reserved2[4];
> > > +        u8 command_specific_result[];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +For all commands, \field{opcode}, \field{group_type} and if
> > > +necessary \field{group_member_id} and \field{command_specific_data} are
> > > +set by the driver, and the owner device sets \field{status} and if
> > > +needed \field{status_qualifier} and
> > > +\field{command_specific_result}.
> > > +
> > > +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 & Name & Command Description \\
> > > +\hline \hline
> > > +0x0000 - 0x7FFF & - &  Group administration commands    \\
> > > +\hline
> > > +0x8000 - 0xFFFF & - & Reserved    \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +The \field{group_type} specifies the group type identifier.
> >
> > So this means there's no way for having a group specific adminq? E.g
> > one queue for SR-IOV and another for SIOV.
> >
> > > +The \field{group_member_id} specifies the member identifier within the group.
> > > +See section \ref{sec:Introduction / Terminology / Device group}
> > > +for the definition of the group type identifier and group member
> > > +identifier.
> > > +
> > > +The following table describes possible \field{status} values;
> > > +to simplify common implementations, they are intentionally
> > > +matching common Linux names and error numbers:
> >
> > I may miss something but I don't see how it matches Linux error numbers.
>
>
> include/uapi/asm-generic/errno-base.h:#define   EINVAL          22      /* Invalid argument */

Ok, only this one was matched but any advantages of menting the
matching? I believe not all the error numbers are meaningful for a
device interface (e.g EINTR).

Thanks

>
>
> > Thanks
> >
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Status (decimal) & Name & Description \\
> > > +\hline \hline
> > > +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > > +\hline
> > > +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
> > > +\hline
> > > +other   & -    & group administration command error  \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
> > > +0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> > > +\hline
> > > +0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> > > +\hline
> > > +0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> > > +\hline
> > > +0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
> > > +\hline
> > > +0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
> > > +\hline
> > > +0x05-0xFFFF   & -    & reserved for future use \\
> > > +\hline
> > > +\end{tabular}
> > > --
> > > MST
> > >
>

Re: [PATCH v9 06/10] mmio: document ADMIN_VQ as reserved

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 02:03:30PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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 aa68926..023f9b9 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -2362,6 +2362,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
> 
> Do we need to add VIRTIO_F_SR_IOV here? And probably SR-IOV group type?
> 
> Thanks

yea we can add more as patches on top.
as all of VIRTIO_F_ADMIN_VQ is not there I don't think we need to
call out  SR-IOV group type separately.

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

Re: [PATCH v9 05/10] pci: add admin vq registers to virtio over pci

[Jason Wang]

On Thu, Nov 24, 2022 at 3:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Nov 24, 2022 at 02:00:54PM +0800, Jason Wang wrote:
> > On Thu, Nov 24, 2022 at 5:08 AM 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(+)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 04235da..aa68926 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -946,6 +946,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 */
> >
> > If we are adding more queue related functionality, it would be placed
> > under the above two fields which seems not elegant. I wonder if a new
> > structure is needed here.
> >
> > Thanks
>
> I don't expect more, if that happens we can always refactor it.

I may miss something but it should be a part of ABI. How could we
refactor it without breaking ABI?

Thanks

> Generally this is not C, no one compiles this directly.
> So we do not need to add structures unless
> they are referred to in spec text.
>
> > >  };
> > >  \end{lstlisting}
> > >
> > > @@ -1031,6 +1035,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}
> > > @@ -1117,6 +1134,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
> > > @@ -6992,6 +7017,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >
> > >    \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
> > >    administration virtqueues.
> > > +  At the moment this feature is only supported for devices using
> > > +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> > > +         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
> > >
>

Re: [PATCH v9 03/10] admin: introduce group administration commands

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 03:42:18PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 3:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Nov 24, 2022 at 01:52:41PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 5:08 AM 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 98 insertions(+)
> > > >
> > > > diff --git a/admin.tex b/admin.tex
> > > > index 6ebdd05..1dca136 100644
> > > > --- a/admin.tex
> > > > +++ b/admin.tex
> > > > @@ -42,4 +42,102 @@ \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.
> > >
> > > This excludes any possible runtime configuration, is this intended?
> >
> > No idea - what kind of runtime configuration are you talking about?
> > This explicitly says "to configure a member device" and that's
> > certainly at runtime.
> 
> Not sure, but if we say something like we, it means we need a group of
> commands to support runtime stuff in the future?

we'll need commands to provision members - is this what you
mean? for sure.

> >
> >
> > >
> > > > +
> > > > +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;
> > > > +        le16 status_qualifier;
> > > > +        /* unused, reserved for future extensions */
> > > > +        u8 reserved2[4];
> > > > +        u8 command_specific_result[];
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +For all commands, \field{opcode}, \field{group_type} and if
> > > > +necessary \field{group_member_id} and \field{command_specific_data} are
> > > > +set by the driver, and the owner device sets \field{status} and if
> > > > +needed \field{status_qualifier} and
> > > > +\field{command_specific_result}.
> > > > +
> > > > +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 & Name & Command Description \\
> > > > +\hline \hline
> > > > +0x0000 - 0x7FFF & - &  Group administration commands    \\
> > > > +\hline
> > > > +0x8000 - 0xFFFF & - & Reserved    \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +The \field{group_type} specifies the group type identifier.
> > >
> > > So this means there's no way for having a group specific adminq? E.g
> > > one queue for SR-IOV and another for SIOV.
> > >
> > > > +The \field{group_member_id} specifies the member identifier within the group.
> > > > +See section \ref{sec:Introduction / Terminology / Device group}
> > > > +for the definition of the group type identifier and group member
> > > > +identifier.
> > > > +
> > > > +The following table describes possible \field{status} values;
> > > > +to simplify common implementations, they are intentionally
> > > > +matching common Linux names and error numbers:
> > >
> > > I may miss something but I don't see how it matches Linux error numbers.
> >
> >
> > include/uapi/asm-generic/errno-base.h:#define   EINVAL          22      /* Invalid argument */
> 
> Ok, only this one was matched but any advantages of menting the
> matching? I believe not all the error numbers are meaningful for a
> device interface (e.g EINTR).
> 
> Thanks

Makes it easier for driver to decide on an error to
report to userspace. If we don't use all error codes
that's fine.




> >
> >
> > > Thanks
> > >
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Status (decimal) & Name & Description \\
> > > > +\hline \hline
> > > > +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > > > +\hline
> > > > +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
> > > > +\hline
> > > > +other   & -    & group administration command error  \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
> > > > +0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> > > > +\hline
> > > > +0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> > > > +\hline
> > > > +0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> > > > +\hline
> > > > +0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
> > > > +\hline
> > > > +0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
> > > > +\hline
> > > > +0x05-0xFFFF   & -    & reserved for future use \\
> > > > +\hline
> > > > +\end{tabular}
> > > > --
> > > > MST
> > > >
> >

Re: [PATCH v9 05/10] pci: add admin vq registers to virtio over pci

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 03:46:00PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 3:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Nov 24, 2022 at 02:00:54PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 5:08 AM 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(+)
> > > >
> > > > diff --git a/content.tex b/content.tex
> > > > index 04235da..aa68926 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -946,6 +946,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 */
> > >
> > > If we are adding more queue related functionality, it would be placed
> > > under the above two fields which seems not elegant. I wonder if a new
> > > structure is needed here.
> > >
> > > Thanks
> >
> > I don't expect more, if that happens we can always refactor it.
> 
> I may miss something but it should be a part of ABI. How could we
> refactor it without breaking ABI?
> 
> Thanks

         /* About the administration virtqueue. */
-        le16 admin_queue_index;         /* read-only for driver */
-        le16 admin_queue_num;         /* read-only for driver */
+        struct { /* read-only for driver */
+        	le16 index;
+        	le16 num;
+	 } admin_queue;

does not affect the ABI. But the order of things makes it less readable.

We would also have to change references to admin_queue_index with
admin_queue.index and explain the "." notation in introduction.


All doable but this patchset is large enough as it is.



> > Generally this is not C, no one compiles this directly.
> > So we do not need to add structures unless
> > they are referred to in spec text.
> >
> > > >  };
> > > >  \end{lstlisting}
> > > >
> > > > @@ -1031,6 +1035,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}
> > > > @@ -1117,6 +1134,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
> > > > @@ -6992,6 +7017,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > > >
> > > >    \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
> > > >    administration virtqueues.
> > > > +  At the moment this feature is only supported for devices using
> > > > +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> > > > +         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
> > > >
> >

Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 03:34:19PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/content.tex b/content.tex
> > > > index 3051399..e3203be 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -114,21 +114,63 @@ \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 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.)
> > >
> > > It looks to me if we want to clarify like this, feature negotiation is
> > > not sufficient. Do we need to do something similar in other basic
> > > facilities? Generally, we probably need to do this for facilities that
> > > are similar to features (status, virtqueue size and others).
> >
> > I'm not sure about "not sufficient". It's sufficient as long
> > as you just want to extend features. What triggered this
> > work is adding a transport specific feature.
> 
> E.g:
> 
> For status: Devices do not offer any status bit it would not be able to handle.
> For virtqueue size:  Devices do not offer virtqueue size it would not
> be able to handle.
> 
> ?

Jason I think what you miss here is this part:

"even though drivers are not
supposed to accept them in the first place even if offered, according to
this version of the specification"

does not apply to status and virtqueue size.


Let me clarify what all this means.
It seems safe for a device to offer a reserved feature bit
since drivers are not supposed to accept it.
This text says device must not rely on this.

How would this apply to status or vq size? I don't see.










> >
> >
> > > > 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.
> > >
> > > I'm not sure this is the best way or whether we've defined the
> > > "unexpected features" before. It might be better to say that to accept
> > > the features that the driver understands.
> >
> > So specifically, if feature is documented as pci only and then
> > down the road we enable it for ccw. Does driver "understand" it? Or not?
> > It is clearly unexpected.
> > I am not too worried about using terms we didn't define this is not
> > part of a conformance statement.
> > And people seem to get the meaning ;)
> 
> Ok.
> 
> Thanks

Re: [virtio-dev] Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 03:37:42PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 5:08 AM 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   | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > > >  content.tex |  2 ++
> > > >  2 files changed, 47 insertions(+)
> > > >  create mode 100644 admin.tex
> > > >
> > > > diff --git a/admin.tex b/admin.tex
> > > > new file mode 100644
> > > > index 0000000..6ebdd05
> > > > --- /dev/null
> > > > +++ b/admin.tex
> > > > @@ -0,0 +1,45 @@
> > > > +\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
> > > > +
> > > > +It is occasionally useful to have a device control a group of
> > > > +other devices. Terminology used in such cases:
> > > > +
> > > > +\begin{description}
> > > > +\item[Device group]
> > > > +        or just group, includes zero or more devices.
> > > > +\item[Owner device]
> > > > +        or owner, the device controlling the group.
> > > > +\item[Member device]
> > > > +        a device within a group. The owner device itself is not
> > > > +       a member of the group.
> > > > +\item[Member identifier]
> > > > +        each member has this identifier, unique within the group
> > > > +       and used to address it through the owner device.
> > > > +\item[Group type identifier]
> > > > +       specifies what kind of member devices there are in a
> > > > +       group, how is the member identifier is interpreted
> > > > +       and what kind of control the owner has.
> > > > +       A given owner can control a single group of a given type,
> > > > +       thus the type and the owner together identify the group.
> > > > +\end{description}
> > > > +
> > > > +The following group types, and their identifiers, are currently specified):
> > > > +\begin{description}
> > > > +\item[SR-IOV group type (0x1)]
> > > > +This device group has a PCI Single Root I/O Virtualization
> > > > +(SR-IOV) physical function (PF) device as the owner and includes
> > > > +all its SR-IOV virtual functions (VFs) as members (see
> > > > +\hyperref[intro:PCIe]{[PCIe]}).
> > >
> > > So I wonder what's the advantage of using a global identifier over the
> > > transport specific one. There's almost no way for CCW/MMIO to use
> > > SR-IOV. Limiting it to PCI seems much easier and avoids layer
> > > violation.
> > >
> > > Thanks
> >
> > So we burn up an identifier, ccw and mmio won't be able to use it.
> > Big deal? Why?
> 
> Because it is transport specific. The basic facility should be
> transport independent.

I tried this but the result is spread all over the spec
and does not result in a readable cohesive whole.
So we give up on the transport independent purity a bit and it
seems worth it.
Also explained this in the cover letter - have you seen that?



> > And I think we might find a use for this with MMIO
> > down the road with some kind of passthrough. Who knows.
> 
> Probably, but can it be modeled exactly as what SR-IOV looks like? Or
> anyhow, it's not too late to define this for MMIO at that time. For
> example, we know MSI-X may work for MMIO but we define it only for PCI
> now.
> 
> Thanks

Right. So if we reserve the id for all transports then it will
be easy to do.


> >
> >
> >
> > > > +
> > > > +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 e3203be..3f3585d 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -491,6 +491,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
> > > >
> >
> >
> > ---------------------------------------------------------------------
> > 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 v9 08/10] admin: command list discovery

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 02:40:15PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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.
> >
> > An alternative is per-type VQs. This is possible but will
> > require more per-transport work. Discovery through the vq
> > helps keep things contained.
> 
> This seems to complicate the migration (compatibility) of the adminq
> itself. We need check both features and command list.

Yes. There's no way around this though - commands need to
be per group and a pci device can host at least both
SIOV and SRIOV children.


> More below.
> 
> >
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index eeccd2e..eec12a9 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >  \hline
> >  opcode & Name & Command Description \\
> >  \hline \hline
> > -0x0000 - 0x7FFF & - &  Group administration commands    \\
> > +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> > +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> > +0x0002 - 0x7FFF & - &  Group administration commands    \\
> >  \hline
> >  0x8000 - 0xFFFF & - & Reserved    \\
> >  \hline
> > @@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >  \hline
> >  \end{tabular}
> >
> > +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.
> 
> Is this simpler if we map group type instead of a single opcode? E.g
> virtqueue transport will introduce 10+ commands, using a bit for a
> specific opcode will complicate things.

I think more flexibility is better, and bits are cheap.


> > +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.
> 
> Any reason we need to invent a negotiation process here? (This seems
> only useful when some commands/opcodes are mutually exclusive)
> 
> Thanks

yes, features don't help because they are per owner.
this is per group.

imagine vq transport. SET_FEATURES commands sets the features
for the child. we need something to negotiate with owner
but still within the vq.



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

Re: [PATCH RFC v9 10/10] ccw: document more reserved features

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 02:53:12PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > vq reset and shared memory are unsupported, too.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> I wonder if it's better to split this series into two since it doesn't
> have direct connection with the admin stuffs then the compatibility
> part can go first.
> 
> Thanks

cover letter explains that this is not part of the series,
here for informational purposes.

I don't really have time to work on cleaning up ccw but once
this lands others can take this up.


> > ---
> >
> >
> > This is not strictly part of this patchset, but just to
> > give you the idea of how we can expand this down the road.
> >
> >  content.tex | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 6c3758d..bfcf9fe 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -2983,6 +2983,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}
> >
> > --
> > MST
> >

Re: [PATCH v9 09/10] admin: conformance clauses

[Michael S. Tsirkin]

On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
> On Thu, Nov 24, 2022 at 5:08 AM 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 158 insertions(+)
> >
> > diff --git a/admin.tex b/admin.tex
> > index eec12a9..e83a9f5 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -222,6 +222,107 @@ \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}
> > +
> > +The device MUST validate \field{opcode}, \field{group_type} and
> > +\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
> > +nd if any of these has an invalid or unsupported value, set
> 
> typo

thanks!

> > +\field{status_qualifier} accordingly.
> > +
> > +If a command completes successfully, the device MUST set
> > +\field{status} to VIRTIO_ADMIN_STATUS_OK.
> > +
> > +If a command fails, the device MUST set
> > +\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
> > +
> > +If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
> > +device state MUST NOT change, that is the command MUST NOT have
> > +any side effects on the device, in particular the device MUST not
> > +enter an error state as a result of this command.
> > +
> > +If a command fails, the device state generally SHOULD NOT change,
> > +as far as possible.
> > +
> > +The 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,
> > +the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
> > +for different group types.
> > +
> > +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
> > +the device MUST assume
> > +that the list of legal commands used by the driver consists of
> > +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> > +
> > +After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
> > +the device MUST set the list of legal commands used by the driver
> > +to the one supplied in \field{command_specific_data}.
> > +
> > +The device MUST set the list of legal commands used by the driver
> > +to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
> > +
> > +The device MUST validate commands against the list used by
> > +the 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.
> > +
> > +The list of supported commands MUST NOT shrink (but MAY expand):
> > +after reporting a given command as supported through
> > +VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
> > +as unsupported.  Further, after a given set of commands has been
> > +used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
> > +device or system reset the device SHOULD complete successfully
> > +any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
> > +list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
> > +after a device or system reset, the device MUST not fail it
> > +solely because of the command list used.  Failure to do so would
> > +interfere with resuming from suspend and error recovery.
> > +
> > +
> > +\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
> > +
> > +The driver MAY discover whether device supports a specific group type
> > +by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
> > +\field{group_type}.
> > +
> > +The 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).
> > +
> > +The driver SHOULD NOT set bits in device_admin_cmds
> > +if it is not familiar with how the command opcode
> > +is used, as dependencies between command opcodes might exist.
> > +
> > +The driver MUST NOT request (via 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.
> > +
> > +The 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.
> > +
> > +The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
> > +VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
> > +with respective bits cleared in \field{command_specific_data}.
> > +
> > +The driver MUST handle a command error with a reserved \field{status}
> > +value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
> > +(except possibly for different error reporting/diagnostic messages).
> > +
> > +The driver MUST handle a command error with a reserved
> > +\field{status_qualifier} value in the same way as
> > +\field{status_qualifier} set to
> > +VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
> > +different error reporting/diagnostic messages).
> > +
> >  \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
> >
> >  An administration virtqueue of an owner device is used to submit
> > @@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
> >  new fields can be added to the tail of a structure,
> >  with the driver using the full structure without concern
> >  for versioning.
> > +
> > +\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
> > +
> > +The 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
> 
> s/"is set"/"are set"

thanks

> > +\item discarding data that would be written outside the
> > +specified device-writeable buffers.
> > +\end{enumerate}
> > +
> > +The 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}
> > +
> > +The device SHOULD initialize the device-writeable buffer
> > +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.
> > +
> > +The device MUST NOT fail a command solely because the buffers
> > +provided are shorter or longer than described in this
> > +specification.
> 
> I may miss something but how can it work if the buffer is shorter?

driver does not care what's there.

this is mostly for forward compatibility - we'll add more fields and
I don't want to explain separately that old drivers post
short buffers with less fields.


> > +
> > +The device MUST process commands on a given administration virtqueue
> > +in the order in which they are queued.
> 
> Is it better to use "complete" than "process" here?

I am not sure we need to require they complete in order, unless
IN_ORDER is set (unfortunately it is global for all of device).


> Or we can reuse
> part of the in-order description.
> 
> (Or maybe a flush command)


these can be used if offered.

> > +
> > +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}
> > +
> > +The driver MAY supply device-readable and device-writeable buffers
> > +longer than described in this specification.
> > +
> > +The 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).
> > +
> > +The driver MUST NOT assume that the 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.
> 
> Won't this have security implications? E.g information leak.
> 
> Thanks

hmm this is not clear enough. it should ignore the contents
and behave as if it was set to zero. Will fix.


> > +
> > +If multiple administration virtqueues have been configured,
> > +the driver MUST ensure ordering for commands
> > +placed on different administration virtqueues.
> > --
> > MST
> >

[virtio-dev] Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Cornelia Huck]

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

> On Thu, Nov 24, 2022 at 03:34:19PM +0800, Jason Wang wrote:
>> On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >
>> > On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
>> > > On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
>> > > >  1 file changed, 42 insertions(+)
>> > > >
>> > > > diff --git a/content.tex b/content.tex
>> > > > index 3051399..e3203be 100644
>> > > > --- a/content.tex
>> > > > +++ b/content.tex
>> > > > @@ -114,21 +114,63 @@ \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 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.)
>> > >
>> > > It looks to me if we want to clarify like this, feature negotiation is
>> > > not sufficient. Do we need to do something similar in other basic
>> > > facilities? Generally, we probably need to do this for facilities that
>> > > are similar to features (status, virtqueue size and others).
>> >
>> > I'm not sure about "not sufficient". It's sufficient as long
>> > as you just want to extend features. What triggered this
>> > work is adding a transport specific feature.
>> 
>> E.g:
>> 
>> For status: Devices do not offer any status bit it would not be able to handle.
>> For virtqueue size:  Devices do not offer virtqueue size it would not
>> be able to handle.
>> 
>> ?
>
> Jason I think what you miss here is this part:
>
> "even though drivers are not
> supposed to accept them in the first place even if offered, according to
> this version of the specification"
>
> does not apply to status and virtqueue size.
>
>
> Let me clarify what all this means.
> It seems safe for a device to offer a reserved feature bit
> since drivers are not supposed to accept it.
> This text says device must not rely on this.
>
> How would this apply to status or vq size? I don't see.

Me neither... for the status, it's about either the driver noting its
progress, or the device indicating that a reset is needed. The only case
where setting something requires kind of an ack is FEATURES_OK, and
there we already spell out the conditions clearly. For the queue size,
we specify that the device states what it can support, and that the
driver may only reduce it, that seems clear enough to me.


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

Re: [virtio-dev] Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Cornelia Huck]

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

> On Thu, Nov 24, 2022 at 03:37:42PM +0800, Jason Wang wrote:
>> On Thu, Nov 24, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >
>> > On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
>> > > On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> > > > +The following group types, and their identifiers, are currently specified):
>> > > > +\begin{description}
>> > > > +\item[SR-IOV group type (0x1)]
>> > > > +This device group has a PCI Single Root I/O Virtualization
>> > > > +(SR-IOV) physical function (PF) device as the owner and includes
>> > > > +all its SR-IOV virtual functions (VFs) as members (see
>> > > > +\hyperref[intro:PCIe]{[PCIe]}).
>> > >
>> > > So I wonder what's the advantage of using a global identifier over the
>> > > transport specific one. There's almost no way for CCW/MMIO to use
>> > > SR-IOV. Limiting it to PCI seems much easier and avoids layer
>> > > violation.
>> > >
>> > > Thanks
>> >
>> > So we burn up an identifier, ccw and mmio won't be able to use it.
>> > Big deal? Why?
>> 
>> Because it is transport specific. The basic facility should be
>> transport independent.
>
> I tried this but the result is spread all over the spec
> and does not result in a readable cohesive whole.
> So we give up on the transport independent purity a bit and it
> seems worth it.
> Also explained this in the cover letter - have you seen that?
>
>
>
>> > And I think we might find a use for this with MMIO
>> > down the road with some kind of passthrough. Who knows.
>> 
>> Probably, but can it be modeled exactly as what SR-IOV looks like? Or
>> anyhow, it's not too late to define this for MMIO at that time. For
>> example, we know MSI-X may work for MMIO but we define it only for PCI
>> now.
>> 
>> Thanks
>
> Right. So if we reserve the id for all transports then it will
> be easy to do.

Also, if we go with transport-specific ids, we might end up with
different ids per transport when we add a future transport-independent
feature. The global id space really should be big enough to accommodate
even single-transport groups.


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

Re: [virtio-dev] Re: [PATCH v9 03/10] admin: introduce group administration commands

[Cornelia Huck]

On Thu, Nov 24 2022, Jason Wang <jasowang@redhat.com> wrote:

> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> +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;
>> +        le16 status_qualifier;
>> +        /* unused, reserved for future extensions */
>> +        u8 reserved2[4];
>> +        u8 command_specific_result[];
>> +};
>> +\end{lstlisting}
>> +
>> +For all commands, \field{opcode}, \field{group_type} and if
>> +necessary \field{group_member_id} and \field{command_specific_data} are
>> +set by the driver, and the owner device sets \field{status} and if
>> +needed \field{status_qualifier} and
>> +\field{command_specific_result}.
>> +
>> +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 & Name & Command Description \\
>> +\hline \hline
>> +0x0000 - 0x7FFF & - &  Group administration commands    \\
>> +\hline
>> +0x8000 - 0xFFFF & - & Reserved    \\
>> +\hline
>> +\end{tabular}
>> +
>> +The \field{group_type} specifies the group type identifier.
>
> So this means there's no way for having a group specific adminq? E.g
> one queue for SR-IOV and another for SIOV.

I don't see anything that prevents the device from exposing two admin
queues... do you mean that we should provide a mechanism for the device
to mandate that only commands for a certain group id can go on a certain
admin queue?

>
>> +The \field{group_member_id} specifies the member identifier within the group.
>> +See section \ref{sec:Introduction / Terminology / Device group}
>> +for the definition of the group type identifier and group member
>> +identifier.


---------------------------------------------------------------------
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 v9 09/10] admin: conformance clauses

[Cornelia Huck]

On Thu, Nov 24 2022, Jason Wang <jasowang@redhat.com> wrote:

> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> +The 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
>
> s/"is set"/"are set"

I think "is set" is actually correct.


---------------------------------------------------------------------
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 v9 01/10] virtio: document forward compatibility guarantees

[Jason Wang]

在 2022/11/24 20:05, Cornelia Huck 写道:
> On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Nov 24, 2022 at 03:34:19PM +0800, Jason Wang wrote:
>>> On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
>>>>> On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 42 insertions(+)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 3051399..e3203be 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -114,21 +114,63 @@ \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 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.)
>>>>> It looks to me if we want to clarify like this, feature negotiation is
>>>>> not sufficient. Do we need to do something similar in other basic
>>>>> facilities? Generally, we probably need to do this for facilities that
>>>>> are similar to features (status, virtqueue size and others).
>>>> I'm not sure about "not sufficient". It's sufficient as long
>>>> as you just want to extend features. What triggered this
>>>> work is adding a transport specific feature.
>>> E.g:
>>>
>>> For status: Devices do not offer any status bit it would not be able to handle.
>>> For virtqueue size:  Devices do not offer virtqueue size it would not
>>> be able to handle.
>>>
>>> ?
>> Jason I think what you miss here is this part:
>>
>> "even though drivers are not
>> supposed to accept them in the first place even if offered, according to
>> this version of the specification"
>>
>> does not apply to status and virtqueue size.
>>
>>
>> Let me clarify what all this means.
>> It seems safe for a device to offer a reserved feature bit


This depends really on the behaviour of the drivers.


>> since drivers are not supposed to accept it.


So this is the case of the ADMIN_VQ.


>> This text says device must not rely on this.
>>
>> How would this apply to status or vq size? I don't see.
> Me neither... for the status, it's about either the driver noting its
> progress, or the device indicating that a reset is needed. The only case
> where setting something requires kind of an ack is FEATURES_OK, and
> there we already spell out the conditions clearly.


I basically meant something like:

Assuming we have a feature like VIRTIO_RING_F_NEW and a new status bit 
was mapped to this feature, VIRTIO_CONFIG_S_NEW. And for some reason 
this feature is reserved for some transports. Should we mention device 
does not offer VIRTIO_CONFIG_S_NEW as well, or we assume it is implied 
that we don't offer VIRTIO_CONFIG_S_NEW in this case?


> For the queue size,
> we specify that the device states what it can support, and that the
> driver may only reduce it, that seems clear enough to me.


Similar to the above, assuming a feature VIRTIO_R_F_MAXSIZE_XXX, and it 
is reserved. Should we mention that the new max virtqueue size should 
not be advertised or it is implied in the feature advertisement?

Thanks

Re: [virtio-dev] Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Jason Wang]

在 2022/11/24 20:12, Cornelia Huck 写道:
> On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Nov 24, 2022 at 03:37:42PM +0800, Jason Wang wrote:
>>> On Thu, Nov 24, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
>>>>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> +The following group types, and their identifiers, are currently specified):
>>>>>> +\begin{description}
>>>>>> +\item[SR-IOV group type (0x1)]
>>>>>> +This device group has a PCI Single Root I/O Virtualization
>>>>>> +(SR-IOV) physical function (PF) device as the owner and includes
>>>>>> +all its SR-IOV virtual functions (VFs) as members (see
>>>>>> +\hyperref[intro:PCIe]{[PCIe]}).
>>>>> So I wonder what's the advantage of using a global identifier over the
>>>>> transport specific one. There's almost no way for CCW/MMIO to use
>>>>> SR-IOV. Limiting it to PCI seems much easier and avoids layer
>>>>> violation.
>>>>>
>>>>> Thanks
>>>> So we burn up an identifier, ccw and mmio won't be able to use it.
>>>> Big deal? Why?
>>> Because it is transport specific. The basic facility should be
>>> transport independent.
>> I tried this but the result is spread all over the spec
>> and does not result in a readable cohesive whole.


I may miss something, but it looks to me it's just a subsection in the 
PCI transport to describe the SR-IOV group identifier.


>> So we give up on the transport independent purity a bit and it
>> seems worth it.
>> Also explained this in the cover letter - have you seen that?


Sorry, I don't.


>>
>>
>>
>>>> And I think we might find a use for this with MMIO
>>>> down the road with some kind of passthrough. Who knows.
>>> Probably, but can it be modeled exactly as what SR-IOV looks like? Or
>>> anyhow, it's not too late to define this for MMIO at that time. For
>>> example, we know MSI-X may work for MMIO but we define it only for PCI
>>> now.
>>>
>>> Thanks
>> Right. So if we reserve the id for all transports then it will
>> be easy to do.
> Also, if we go with transport-specific ids, we might end up with
> different ids per transport when we add a future transport-independent
> feature.


I'm not sure this is a real problem:

1) all the basic facilities are now transport-independent but they are 
implemented via different transport specific registers/commands/offsets etc.
2) I'm not quite sure there would be a transport-independent group 
identifier other than the virtqueue transport, so I wonder if it makes 
sense to split the id space into global ones as well as the transport 
specific ones


>   The global id space really should be big enough to accommodate
> even single-transport groups.


Yes, so it's not about the space but about whether or it it's 
worth/correct/expensive to describe a transport specific identifier in 
the basic facility part.

Thanks


>
>
> ---------------------------------------------------------------------
> 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 v9 03/10] admin: introduce group administration commands

[Jason Wang]

在 2022/11/24 16:03, Michael S. Tsirkin 写道:
> On Thu, Nov 24, 2022 at 03:42:18PM +0800, Jason Wang wrote:
>> On Thu, Nov 24, 2022 at 3:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Thu, Nov 24, 2022 at 01:52:41PM +0800, Jason Wang wrote:
>>>> On Thu, Nov 24, 2022 at 5:08 AM 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 98 insertions(+)
>>>>>
>>>>> diff --git a/admin.tex b/admin.tex
>>>>> index 6ebdd05..1dca136 100644
>>>>> --- a/admin.tex
>>>>> +++ b/admin.tex
>>>>> @@ -42,4 +42,102 @@ \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.
>>>> This excludes any possible runtime configuration, is this intended?
>>> No idea - what kind of runtime configuration are you talking about?
>>> This explicitly says "to configure a member device" and that's
>>> certainly at runtime.
>> Not sure, but if we say something like we, it means we need a group of
>> commands to support runtime stuff in the future?
> we'll need commands to provision members - is this what you
> mean? for sure.


Yes.


>
>>>
>>>>> +
>>>>> +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;
>>>>> +        le16 status_qualifier;
>>>>> +        /* unused, reserved for future extensions */
>>>>> +        u8 reserved2[4];
>>>>> +        u8 command_specific_result[];
>>>>> +};
>>>>> +\end{lstlisting}
>>>>> +
>>>>> +For all commands, \field{opcode}, \field{group_type} and if
>>>>> +necessary \field{group_member_id} and \field{command_specific_data} are
>>>>> +set by the driver, and the owner device sets \field{status} and if
>>>>> +needed \field{status_qualifier} and
>>>>> +\field{command_specific_result}.
>>>>> +
>>>>> +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 & Name & Command Description \\
>>>>> +\hline \hline
>>>>> +0x0000 - 0x7FFF & - &  Group administration commands    \\
>>>>> +\hline
>>>>> +0x8000 - 0xFFFF & - & Reserved    \\
>>>>> +\hline
>>>>> +\end{tabular}
>>>>> +
>>>>> +The \field{group_type} specifies the group type identifier.
>>>> So this means there's no way for having a group specific adminq? E.g
>>>> one queue for SR-IOV and another for SIOV.
>>>>
>>>>> +The \field{group_member_id} specifies the member identifier within the group.
>>>>> +See section \ref{sec:Introduction / Terminology / Device group}
>>>>> +for the definition of the group type identifier and group member
>>>>> +identifier.
>>>>> +
>>>>> +The following table describes possible \field{status} values;
>>>>> +to simplify common implementations, they are intentionally
>>>>> +matching common Linux names and error numbers:
>>>> I may miss something but I don't see how it matches Linux error numbers.
>>>
>>> include/uapi/asm-generic/errno-base.h:#define   EINVAL          22      /* Invalid argument */
>> Ok, only this one was matched but any advantages of menting the
>> matching? I believe not all the error numbers are meaningful for a
>> device interface (e.g EINTR).
>>
>> Thanks
> Makes it easier for driver to decide on an error to
> report to userspace. If we don't use all error codes
> that's fine.


Ok.

Thanks


>
>
>
>
>>>
>>>> Thanks
>>>>
>>>>> +
>>>>> +\begin{tabular}{|l|l|l|}
>>>>> +\hline
>>>>> +Status (decimal) & Name & Description \\
>>>>> +\hline \hline
>>>>> +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
>>>>> +\hline
>>>>> +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
>>>>> +\hline
>>>>> +other   & -    & group administration command error  \\
>>>>> +\hline
>>>>> +\end{tabular}
>>>>> +
>>>>> +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
>>>>> +0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
>>>>> +\hline
>>>>> +0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
>>>>> +\hline
>>>>> +0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
>>>>> +\hline
>>>>> +0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
>>>>> +\hline
>>>>> +0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
>>>>> +\hline
>>>>> +0x05-0xFFFF   & -    & reserved for future use \\
>>>>> +\hline
>>>>> +\end{tabular}
>>>>> --
>>>>> MST
>>>>>


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 v9 05/10] pci: add admin vq registers to virtio over pci

[Jason Wang]

在 2022/11/24 16:09, Michael S. Tsirkin 写道:
> On Thu, Nov 24, 2022 at 03:46:00PM +0800, Jason Wang wrote:
>> On Thu, Nov 24, 2022 at 3:14 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> On Thu, Nov 24, 2022 at 02:00:54PM +0800, Jason Wang wrote:
>>>> On Thu, Nov 24, 2022 at 5:08 AM 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(+)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index 04235da..aa68926 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -946,6 +946,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 */
>>>> If we are adding more queue related functionality, it would be placed
>>>> under the above two fields which seems not elegant. I wonder if a new
>>>> structure is needed here.
>>>>
>>>> Thanks
>>> I don't expect more, if that happens we can always refactor it.
>> I may miss something but it should be a part of ABI. How could we
>> refactor it without breaking ABI?
>>
>> Thanks
>           /* About the administration virtqueue. */
> -        le16 admin_queue_index;         /* read-only for driver */
> -        le16 admin_queue_num;         /* read-only for driver */
> +        struct { /* read-only for driver */
> +        	le16 index;
> +        	le16 num;
> +	 } admin_queue;
>
> does not affect the ABI. But the order of things makes it less readable.


Yes, so I basically meant if we need a dedicated capability for admin 
virtqueue.

(Btw, I wonder if we need a sentence to document the implication for 
notification area, e.g if vendor choose to use a large index but still 
want to use page-per-vq notification area).


>
> We would also have to change references to admin_queue_index with
> admin_queue.index and explain the "." notation in introduction.
>
>
> All doable but this patchset is large enough as it is.


Can we split the PCI part on top?

Thanks


>
>
>
>>> Generally this is not C, no one compiles this directly.
>>> So we do not need to add structures unless
>>> they are referred to in spec text.
>>>
>>>>>   };
>>>>>   \end{lstlisting}
>>>>>
>>>>> @@ -1031,6 +1035,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}
>>>>> @@ -1117,6 +1134,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
>>>>> @@ -6992,6 +7017,15 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>>>>
>>>>>     \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
>>>>>     administration virtqueues.
>>>>> +  At the moment this feature is only supported for devices using
>>>>> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
>>>>> +         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
>>>>>

Re: [virtio-comment] Re: [PATCH v9 08/10] admin: command list discovery

[Jason Wang]

在 2022/11/24 16:30, Michael S. Tsirkin 写道:
> On Thu, Nov 24, 2022 at 02:40:15PM +0800, Jason Wang wrote:
>> On Thu, Nov 24, 2022 at 5:08 AM 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.
>>>
>>> An alternative is per-type VQs. This is possible but will
>>> require more per-transport work. Discovery through the vq
>>> helps keep things contained.
>> This seems to complicate the migration (compatibility) of the adminq
>> itself. We need check both features and command list.
> Yes. There's no way around this though - commands need to
> be per group and a pci device can host at least both
> SIOV and SRIOV children.
>
>
>> More below.
>>
>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>   admin.tex | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 81 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/admin.tex b/admin.tex
>>> index eeccd2e..eec12a9 100644
>>> --- a/admin.tex
>>> +++ b/admin.tex
>>> @@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>>>   \hline
>>>   opcode & Name & Command Description \\
>>>   \hline \hline
>>> -0x0000 - 0x7FFF & - &  Group administration commands    \\
>>> +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>>> +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
>>> +0x0002 - 0x7FFF & - &  Group administration commands    \\
>>>   \hline
>>>   0x8000 - 0xFFFF & - & Reserved    \\
>>>   \hline
>>> @@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>>>   \hline
>>>   \end{tabular}
>>>
>>> +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.
>> Is this simpler if we map group type instead of a single opcode? E.g
>> virtqueue transport will introduce 10+ commands, using a bit for a
>> specific opcode will complicate things.
> I think more flexibility is better, and bits are cheap.


So we may end up with something like:

If VIRTIO_F_TRANSPORT_VQ is supported, device MUST return the following 
opcodes via VIRTIO_ADMIN_CMD_LIST_QUERY

[opX, opY, ....]

Probably fine.



>
>
>>> +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.
>> Any reason we need to invent a negotiation process here? (This seems
>> only useful when some commands/opcodes are mutually exclusive)
>>
>> Thanks
> yes, features don't help because they are per owner.
> this is per group.
>
> imagine vq transport. SET_FEATURES commands sets the features
> for the child. we need something to negotiate with owner
> but still within the vq.


Should we document what happens if driver use a opcode that it doesn't 
negotiate? (It looks to me we don't document this for at leave 
virtio-net control virtqueue).

Thanks


>
>
>>> +
>>> +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
>>>
>
> 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 v9 09/10] admin: conformance clauses

[Jason Wang]

在 2022/11/24 20:28, Cornelia Huck 写道:
> On Thu, Nov 24 2022, Jason Wang <jasowang@redhat.com> wrote:
>
>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> +The 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
>> s/"is set"/"are set"
> I think "is set" is actually correct.


You are right.

Thanks


>

Re: [PATCH v9 09/10] admin: conformance clauses

[Jason Wang]

在 2022/11/24 16:36, Michael S. Tsirkin 写道:
> On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
>> On Thu, Nov 24, 2022 at 5:08 AM 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 158 insertions(+)
>>>
>>> diff --git a/admin.tex b/admin.tex
>>> index eec12a9..e83a9f5 100644
>>> --- a/admin.tex
>>> +++ b/admin.tex
>>> @@ -222,6 +222,107 @@ \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}
>>> +
>>> +The device MUST validate \field{opcode}, \field{group_type} and
>>> +\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
>>> +nd if any of these has an invalid or unsupported value, set
>> typo
> thanks!
>
>>> +\field{status_qualifier} accordingly.
>>> +
>>> +If a command completes successfully, the device MUST set
>>> +\field{status} to VIRTIO_ADMIN_STATUS_OK.
>>> +
>>> +If a command fails, the device MUST set
>>> +\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
>>> +
>>> +If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
>>> +device state MUST NOT change, that is the command MUST NOT have
>>> +any side effects on the device, in particular the device MUST not
>>> +enter an error state as a result of this command.
>>> +
>>> +If a command fails, the device state generally SHOULD NOT change,
>>> +as far as possible.
>>> +
>>> +The 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,
>>> +the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
>>> +for different group types.
>>> +
>>> +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
>>> +the device MUST assume
>>> +that the list of legal commands used by the driver consists of
>>> +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
>>> +
>>> +After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
>>> +the device MUST set the list of legal commands used by the driver
>>> +to the one supplied in \field{command_specific_data}.
>>> +
>>> +The device MUST set the list of legal commands used by the driver
>>> +to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
>>> +
>>> +The device MUST validate commands against the list used by
>>> +the 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.
>>> +
>>> +The list of supported commands MUST NOT shrink (but MAY expand):
>>> +after reporting a given command as supported through
>>> +VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
>>> +as unsupported.  Further, after a given set of commands has been
>>> +used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
>>> +device or system reset the device SHOULD complete successfully
>>> +any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
>>> +list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
>>> +after a device or system reset, the device MUST not fail it
>>> +solely because of the command list used.  Failure to do so would
>>> +interfere with resuming from suspend and error recovery.
>>> +
>>> +
>>> +\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
>>> +
>>> +The driver MAY discover whether device supports a specific group type
>>> +by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
>>> +\field{group_type}.
>>> +
>>> +The 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).
>>> +
>>> +The driver SHOULD NOT set bits in device_admin_cmds
>>> +if it is not familiar with how the command opcode
>>> +is used, as dependencies between command opcodes might exist.
>>> +
>>> +The driver MUST NOT request (via 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.
>>> +
>>> +The 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.
>>> +
>>> +The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
>>> +VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
>>> +with respective bits cleared in \field{command_specific_data}.
>>> +
>>> +The driver MUST handle a command error with a reserved \field{status}
>>> +value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
>>> +(except possibly for different error reporting/diagnostic messages).
>>> +
>>> +The driver MUST handle a command error with a reserved
>>> +\field{status_qualifier} value in the same way as
>>> +\field{status_qualifier} set to
>>> +VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
>>> +different error reporting/diagnostic messages).
>>> +
>>>   \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
>>>
>>>   An administration virtqueue of an owner device is used to submit
>>> @@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
>>>   new fields can be added to the tail of a structure,
>>>   with the driver using the full structure without concern
>>>   for versioning.
>>> +
>>> +\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
>>> +
>>> +The 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
>> s/"is set"/"are set"
> thanks
>
>>> +\item discarding data that would be written outside the
>>> +specified device-writeable buffers.
>>> +\end{enumerate}
>>> +
>>> +The 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}
>>> +
>>> +The device SHOULD initialize the device-writeable buffer
>>> +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.
>>> +
>>> +The device MUST NOT fail a command solely because the buffers
>>> +provided are shorter or longer than described in this
>>> +specification.
>> I may miss something but how can it work if the buffer is shorter?
> driver does not care what's there.
>
> this is mostly for forward compatibility - we'll add more fields and
> I don't want to explain separately that old drivers post
> short buffers with less fields.


For example:

The patch said:

struct virtio_admin_cmd_list {
        /* Indicates which of the below fields were returned
        le32 device_admin_cmds[];
};

Does it mean the query can still succeed even if there's no space for 
virtio_admin_cmd_list in the writable buffer?


>
>
>>> +
>>> +The device MUST process commands on a given administration virtqueue
>>> +in the order in which they are queued.
>> Is it better to use "complete" than "process" here?
> I am not sure we need to require they complete in order, unless
> IN_ORDER is set (unfortunately it is global for all of device).


So I'm not sure the "process" can help in this case. Consider we have 
two commands X and Y. And Y depends on X.

If we say device processes the commands in order, driver can't submit Y 
until it see X is completed. This seems no changes if we allow the 
device to process the command out of order.


>
>> Or we can reuse
>> part of the in-order description.
>>
>> (Or maybe a flush command)
>
> these can be used if offered.
>
>>> +
>>> +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}
>>> +
>>> +The driver MAY supply device-readable and device-writeable buffers
>>> +longer than described in this specification.
>>> +
>>> +The 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).
>>> +
>>> +The driver MUST NOT assume that the 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.
>> Won't this have security implications? E.g information leak.
>>
>> Thanks
> hmm this is not clear enough. it should ignore the contents
> and behave as if it was set to zero. Will fix.


Ok.

Thanks


>
>
>>> +
>>> +If multiple administration virtqueues have been configured,
>>> +the driver MUST ensure ordering for commands
>>> +placed on different administration virtqueues.
>>> --
>>> MST
>>>

Re: [virtio-dev] Re: [PATCH v9 03/10] admin: introduce group administration commands

[Jason Wang]

在 2022/11/24 20:21, Cornelia Huck 写道:
> On Thu, Nov 24 2022, Jason Wang <jasowang@redhat.com> wrote:
>
>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>> +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;
>>> +        le16 status_qualifier;
>>> +        /* unused, reserved for future extensions */
>>> +        u8 reserved2[4];
>>> +        u8 command_specific_result[];
>>> +};
>>> +\end{lstlisting}
>>> +
>>> +For all commands, \field{opcode}, \field{group_type} and if
>>> +necessary \field{group_member_id} and \field{command_specific_data} are
>>> +set by the driver, and the owner device sets \field{status} and if
>>> +needed \field{status_qualifier} and
>>> +\field{command_specific_result}.
>>> +
>>> +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 & Name & Command Description \\
>>> +\hline \hline
>>> +0x0000 - 0x7FFF & - &  Group administration commands    \\
>>> +\hline
>>> +0x8000 - 0xFFFF & - & Reserved    \\
>>> +\hline
>>> +\end{tabular}
>>> +
>>> +The \field{group_type} specifies the group type identifier.
>> So this means there's no way for having a group specific adminq? E.g
>> one queue for SR-IOV and another for SIOV.
> I don't see anything that prevents the device from exposing two admin
> queues... do you mean that we should provide a mechanism for the device
> to mandate that only commands for a certain group id can go on a certain
> admin queue?


Yes, and rethink about this, it seems to be possible by only returning 
the specific commands for a single type of group via query.

Thanks


>
>>> +The \field{group_member_id} specifies the member identifier within the group.
>>> +See section \ref{sec:Introduction / Terminology / Device group}
>>> +for the definition of the group type identifier and group member
>>> +identifier.

[virtio-dev] Re: [PATCH v9 01/10] virtio: document forward compatibility guarantees

[Cornelia Huck]

On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:

> 在 2022/11/24 20:05, Cornelia Huck 写道:
>> On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Thu, Nov 24, 2022 at 03:34:19PM +0800, Jason Wang wrote:
>>>> On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
>>>>>> On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>   1 file changed, 42 insertions(+)
>>>>>>>
>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>> index 3051399..e3203be 100644
>>>>>>> --- a/content.tex
>>>>>>> +++ b/content.tex
>>>>>>> @@ -114,21 +114,63 @@ \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 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.)
>>>>>> It looks to me if we want to clarify like this, feature negotiation is
>>>>>> not sufficient. Do we need to do something similar in other basic
>>>>>> facilities? Generally, we probably need to do this for facilities that
>>>>>> are similar to features (status, virtqueue size and others).
>>>>> I'm not sure about "not sufficient". It's sufficient as long
>>>>> as you just want to extend features. What triggered this
>>>>> work is adding a transport specific feature.
>>>> E.g:
>>>>
>>>> For status: Devices do not offer any status bit it would not be able to handle.
>>>> For virtqueue size:  Devices do not offer virtqueue size it would not
>>>> be able to handle.
>>>>
>>>> ?
>>> Jason I think what you miss here is this part:
>>>
>>> "even though drivers are not
>>> supposed to accept them in the first place even if offered, according to
>>> this version of the specification"
>>>
>>> does not apply to status and virtqueue size.
>>>
>>>
>>> Let me clarify what all this means.
>>> It seems safe for a device to offer a reserved feature bit
>
>
> This depends really on the behaviour of the drivers.
>
>
>>> since drivers are not supposed to accept it.
>
>
> So this is the case of the ADMIN_VQ.
>
>
>>> This text says device must not rely on this.
>>>
>>> How would this apply to status or vq size? I don't see.
>> Me neither... for the status, it's about either the driver noting its
>> progress, or the device indicating that a reset is needed. The only case
>> where setting something requires kind of an ack is FEATURES_OK, and
>> there we already spell out the conditions clearly.
>
>
> I basically meant something like:
>
> Assuming we have a feature like VIRTIO_RING_F_NEW and a new status bit 
> was mapped to this feature, VIRTIO_CONFIG_S_NEW. And for some reason 
> this feature is reserved for some transports. Should we mention device 
> does not offer VIRTIO_CONFIG_S_NEW as well, or we assume it is implied 
> that we don't offer VIRTIO_CONFIG_S_NEW in this case?

I'm not sure that adding a feature-specific status bit would make sense,
given that the status bits either need to work before feature
negotiation is complete, or are actually needed for feature negotiation.
Also, the status-bit space is way more limited than the feature-bit
space. Therefore, I think we can safely ignore the status bits.

>
>
>> For the queue size,
>> we specify that the device states what it can support, and that the
>> driver may only reduce it, that seems clear enough to me.
>
>
> Similar to the above, assuming a feature VIRTIO_R_F_MAXSIZE_XXX, and it 
> is reserved. Should we mention that the new max virtqueue size should 
> not be advertised or it is implied in the feature advertisement?

I'd say it's implied in the feature bit handling already.


---------------------------------------------------------------------
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: [virtio-dev] Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Cornelia Huck]

On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:

> 在 2022/11/24 20:12, Cornelia Huck 写道:
>> On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Thu, Nov 24, 2022 at 03:37:42PM +0800, Jason Wang wrote:
>>>> On Thu, Nov 24, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
>>>>>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> +The following group types, and their identifiers, are currently specified):
>>>>>>> +\begin{description}
>>>>>>> +\item[SR-IOV group type (0x1)]
>>>>>>> +This device group has a PCI Single Root I/O Virtualization
>>>>>>> +(SR-IOV) physical function (PF) device as the owner and includes
>>>>>>> +all its SR-IOV virtual functions (VFs) as members (see
>>>>>>> +\hyperref[intro:PCIe]{[PCIe]}).
>>>>>> So I wonder what's the advantage of using a global identifier over the
>>>>>> transport specific one. There's almost no way for CCW/MMIO to use
>>>>>> SR-IOV. Limiting it to PCI seems much easier and avoids layer
>>>>>> violation.
>>>>>>
>>>>>> Thanks
>>>>> So we burn up an identifier, ccw and mmio won't be able to use it.
>>>>> Big deal? Why?
>>>> Because it is transport specific. The basic facility should be
>>>> transport independent.
>>> I tried this but the result is spread all over the spec
>>> and does not result in a readable cohesive whole.
>
>
> I may miss something, but it looks to me it's just a subsection in the 
> PCI transport to describe the SR-IOV group identifier.
>
>
>>> So we give up on the transport independent purity a bit and it
>>> seems worth it.
>>> Also explained this in the cover letter - have you seen that?
>
>
> Sorry, I don't.
>
>
>>>
>>>
>>>
>>>>> And I think we might find a use for this with MMIO
>>>>> down the road with some kind of passthrough. Who knows.
>>>> Probably, but can it be modeled exactly as what SR-IOV looks like? Or
>>>> anyhow, it's not too late to define this for MMIO at that time. For
>>>> example, we know MSI-X may work for MMIO but we define it only for PCI
>>>> now.
>>>>
>>>> Thanks
>>> Right. So if we reserve the id for all transports then it will
>>> be easy to do.
>> Also, if we go with transport-specific ids, we might end up with
>> different ids per transport when we add a future transport-independent
>> feature.
>
>
> I'm not sure this is a real problem:
>
> 1) all the basic facilities are now transport-independent but they are 
> implemented via different transport specific registers/commands/offsets etc.

Yes, but in this case we're using the same basic structure, just with a
different id. I think that has a high potential for errors.

> 2) I'm not quite sure there would be a transport-independent group 
> identifier other than the virtqueue transport, so I wonder if it makes 
> sense to split the id space into global ones as well as the transport 
> specific ones

I think global vs. transport-specific is overkill here. I don't think we
expect a flood of new group types.

>
>
>>   The global id space really should be big enough to accommodate
>> even single-transport groups.
>
>
> Yes, so it's not about the space but about whether or it it's 
> worth/correct/expensive to describe a transport specific identifier in 
> the basic facility part.

IMHO, splitting this or using transport-specific namespaces is making
this complicated for not much in return. What do others think?


---------------------------------------------------------------------
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 v9 09/10] admin: conformance clauses

[Cornelia Huck]

On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:

> 在 2022/11/24 16:36, Michael S. Tsirkin 写道:
>> On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
>>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> +The device MUST NOT fail a command solely because the buffers
>>>> +provided are shorter or longer than described in this
>>>> +specification.
>>> I may miss something but how can it work if the buffer is shorter?
>> driver does not care what's there.
>>
>> this is mostly for forward compatibility - we'll add more fields and
>> I don't want to explain separately that old drivers post
>> short buffers with less fields.
>
>
> For example:
>
> The patch said:
>
> struct virtio_admin_cmd_list {
>         /* Indicates which of the below fields were returned
>         le32 device_admin_cmds[];
> };
>
> Does it mean the query can still succeed even if there's no space for 
> virtio_admin_cmd_list in the writable buffer?

Can we pick an absolute minimum length? All drivers need to be able to
at least accommodate this one. Or is it ok for the driver to send a
completely useless query that doesn't get any data back?


---------------------------------------------------------------------
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: [virtio-comment] Re: [PATCH v9 08/10] admin: command list discovery

[Michael S. Tsirkin]

On Fri, Nov 25, 2022 at 11:38:30AM +0800, Jason Wang wrote:
> 
> 在 2022/11/24 16:30, Michael S. Tsirkin 写道:
> > On Thu, Nov 24, 2022 at 02:40:15PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 5:08 AM 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.
> > > > 
> > > > An alternative is per-type VQs. This is possible but will
> > > > require more per-transport work. Discovery through the vq
> > > > helps keep things contained.
> > > This seems to complicate the migration (compatibility) of the adminq
> > > itself. We need check both features and command list.
> > Yes. There's no way around this though - commands need to
> > be per group and a pci device can host at least both
> > SIOV and SRIOV children.
> > 
> > 
> > > More below.
> > > 
> > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >   admin.tex | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 81 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/admin.tex b/admin.tex
> > > > index eeccd2e..eec12a9 100644
> > > > --- a/admin.tex
> > > > +++ b/admin.tex
> > > > @@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > > >   \hline
> > > >   opcode & Name & Command Description \\
> > > >   \hline \hline
> > > > -0x0000 - 0x7FFF & - &  Group administration commands    \\
> > > > +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> > > > +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> > > > +0x0002 - 0x7FFF & - &  Group administration commands    \\
> > > >   \hline
> > > >   0x8000 - 0xFFFF & - & Reserved    \\
> > > >   \hline
> > > > @@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > > >   \hline
> > > >   \end{tabular}
> > > > 
> > > > +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.
> > > Is this simpler if we map group type instead of a single opcode? E.g
> > > virtqueue transport will introduce 10+ commands, using a bit for a
> > > specific opcode will complicate things.
> > I think more flexibility is better, and bits are cheap.
> 
> 
> So we may end up with something like:
> 
> If VIRTIO_F_TRANSPORT_VQ is supported, device MUST return the following
> opcodes via VIRTIO_ADMIN_CMD_LIST_QUERY
> 
> [opX, opY, ....]
> 
> Probably fine.
> 

I think I would tie the commands to a group type.

> 
> > 
> > 
> > > > +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.
> > > Any reason we need to invent a negotiation process here? (This seems
> > > only useful when some commands/opcodes are mutually exclusive)
> > > 
> > > Thanks
> > yes, features don't help because they are per owner.
> > this is per group.
> > 
> > imagine vq transport. SET_FEATURES commands sets the features
> > for the child. we need something to negotiate with owner
> > but still within the vq.
> 
> 
> Should we document what happens if driver use a opcode that it doesn't
> negotiate? (It looks to me we don't document this for at leave virtio-net
> control virtqueue).
> 
> Thanks
> 

documented in the conformance section.


> > 
> > 
> > > > +
> > > > +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
> > > > 
> > 
> > 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 v9 09/10] admin: conformance clauses

[Michael S. Tsirkin]

On Fri, Nov 25, 2022 at 11:50:52AM +0800, Jason Wang wrote:
> 
> 在 2022/11/24 16:36, Michael S. Tsirkin 写道:
> > On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
> > > On Thu, Nov 24, 2022 at 5:08 AM 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 158 insertions(+)
> > > > 
> > > > diff --git a/admin.tex b/admin.tex
> > > > index eec12a9..e83a9f5 100644
> > > > --- a/admin.tex
> > > > +++ b/admin.tex
> > > > @@ -222,6 +222,107 @@ \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}
> > > > +
> > > > +The device MUST validate \field{opcode}, \field{group_type} and
> > > > +\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
> > > > +nd if any of these has an invalid or unsupported value, set
> > > typo
> > thanks!
> > 
> > > > +\field{status_qualifier} accordingly.
> > > > +
> > > > +If a command completes successfully, the device MUST set
> > > > +\field{status} to VIRTIO_ADMIN_STATUS_OK.
> > > > +
> > > > +If a command fails, the device MUST set
> > > > +\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
> > > > +
> > > > +If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
> > > > +device state MUST NOT change, that is the command MUST NOT have
> > > > +any side effects on the device, in particular the device MUST not
> > > > +enter an error state as a result of this command.
> > > > +
> > > > +If a command fails, the device state generally SHOULD NOT change,
> > > > +as far as possible.
> > > > +
> > > > +The 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,
> > > > +the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
> > > > +for different group types.
> > > > +
> > > > +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
> > > > +the device MUST assume
> > > > +that the list of legal commands used by the driver consists of
> > > > +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> > > > +
> > > > +After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
> > > > +the device MUST set the list of legal commands used by the driver
> > > > +to the one supplied in \field{command_specific_data}.
> > > > +
> > > > +The device MUST set the list of legal commands used by the driver
> > > > +to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
> > > > +
> > > > +The device MUST validate commands against the list used by
> > > > +the 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.
> > > > +
> > > > +The list of supported commands MUST NOT shrink (but MAY expand):
> > > > +after reporting a given command as supported through
> > > > +VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
> > > > +as unsupported.  Further, after a given set of commands has been
> > > > +used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
> > > > +device or system reset the device SHOULD complete successfully
> > > > +any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
> > > > +list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
> > > > +after a device or system reset, the device MUST not fail it
> > > > +solely because of the command list used.  Failure to do so would
> > > > +interfere with resuming from suspend and error recovery.
> > > > +
> > > > +
> > > > +\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
> > > > +
> > > > +The driver MAY discover whether device supports a specific group type
> > > > +by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
> > > > +\field{group_type}.
> > > > +
> > > > +The 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).
> > > > +
> > > > +The driver SHOULD NOT set bits in device_admin_cmds
> > > > +if it is not familiar with how the command opcode
> > > > +is used, as dependencies between command opcodes might exist.
> > > > +
> > > > +The driver MUST NOT request (via 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.
> > > > +
> > > > +The 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.
> > > > +
> > > > +The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
> > > > +VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
> > > > +with respective bits cleared in \field{command_specific_data}.
> > > > +
> > > > +The driver MUST handle a command error with a reserved \field{status}
> > > > +value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
> > > > +(except possibly for different error reporting/diagnostic messages).
> > > > +
> > > > +The driver MUST handle a command error with a reserved
> > > > +\field{status_qualifier} value in the same way as
> > > > +\field{status_qualifier} set to
> > > > +VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
> > > > +different error reporting/diagnostic messages).
> > > > +
> > > >   \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
> > > > 
> > > >   An administration virtqueue of an owner device is used to submit
> > > > @@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
> > > >   new fields can be added to the tail of a structure,
> > > >   with the driver using the full structure without concern
> > > >   for versioning.
> > > > +
> > > > +\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
> > > > +
> > > > +The 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
> > > s/"is set"/"are set"
> > thanks
> > 
> > > > +\item discarding data that would be written outside the
> > > > +specified device-writeable buffers.
> > > > +\end{enumerate}
> > > > +
> > > > +The 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}
> > > > +
> > > > +The device SHOULD initialize the device-writeable buffer
> > > > +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.
> > > > +
> > > > +The device MUST NOT fail a command solely because the buffers
> > > > +provided are shorter or longer than described in this
> > > > +specification.
> > > I may miss something but how can it work if the buffer is shorter?
> > driver does not care what's there.
> > 
> > this is mostly for forward compatibility - we'll add more fields and
> > I don't want to explain separately that old drivers post
> > short buffers with less fields.
> 
> 
> For example:
> 
> The patch said:
> 
> struct virtio_admin_cmd_list {
>        /* Indicates which of the below fields were returned
>        le32 device_admin_cmds[];
> };
> 
> Does it mean the query can still succeed even if there's no space for
> virtio_admin_cmd_list in the writable buffer?
> 

yes you just skip it. E.g. it's a way to check that query is supported.
And this (truncated buffer) is exactly what will happen when we have
e.g. 1k opcodes.

> > 
> > 
> > > > +
> > > > +The device MUST process commands on a given administration virtqueue
> > > > +in the order in which they are queued.
> > > Is it better to use "complete" than "process" here?
> > I am not sure we need to require they complete in order, unless
> > IN_ORDER is set (unfortunately it is global for all of device).
> 
> 
> So I'm not sure the "process" can help in this case. Consider we have two
> commands X and Y. And Y depends on X.
> 
> If we say device processes the commands in order, driver can't submit Y
> until it see X is completed.
> This seems no changes if we allow the device to
> process the command out of order.
> 
Depends on command.
Consider transport vq, some read following a write. read must not bypass
it but you do not need to wait for write.

> 
> > 
> > > Or we can reuse
> > > part of the in-order description.
> > > 
> > > (Or maybe a flush command)
> > 
> > these can be used if offered.
> > 
> > > > +
> > > > +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}
> > > > +
> > > > +The driver MAY supply device-readable and device-writeable buffers
> > > > +longer than described in this specification.
> > > > +
> > > > +The 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).
> > > > +
> > > > +The driver MUST NOT assume that the 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.
> > > Won't this have security implications? E.g information leak.
> > > 
> > > Thanks
> > hmm this is not clear enough. it should ignore the contents
> > and behave as if it was set to zero. Will fix.
> 
> 
> Ok.
> 
> Thanks
> 
> 
> > 
> > 
> > > > +
> > > > +If multiple administration virtqueues have been configured,
> > > > +the driver MUST ensure ordering for commands
> > > > +placed on different administration virtqueues.
> > > > --
> > > > MST
> > > > 

Re: [PATCH v9 09/10] admin: conformance clauses

[Michael S. Tsirkin]

On Fri, Nov 25, 2022 at 12:42:23PM +0100, Cornelia Huck wrote:
> On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:
> 
> > 在 2022/11/24 16:36, Michael S. Tsirkin 写道:
> >> On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
> >>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> +The device MUST NOT fail a command solely because the buffers
> >>>> +provided are shorter or longer than described in this
> >>>> +specification.
> >>> I may miss something but how can it work if the buffer is shorter?
> >> driver does not care what's there.
> >>
> >> this is mostly for forward compatibility - we'll add more fields and
> >> I don't want to explain separately that old drivers post
> >> short buffers with less fields.
> >
> >
> > For example:
> >
> > The patch said:
> >
> > struct virtio_admin_cmd_list {
> >         /* Indicates which of the below fields were returned
> >         le32 device_admin_cmds[];
> > };
> >
> > Does it mean the query can still succeed even if there's no space for 
> > virtio_admin_cmd_list in the writable buffer?
> 
> Can we pick an absolute minimum length? All drivers need to be able to
> at least accommodate this one. Or is it ok for the driver to send a
> completely useless query that doesn't get any data back?

I think it might be a way to check that a group exists.

-- 
MST

Re: [virtio-dev] Re: [PATCH v9 02/10] admin: introduce device group and related concepts

[Michael S. Tsirkin]

On Fri, Nov 25, 2022 at 11:23:09AM +0800, Jason Wang wrote:
> 
> 在 2022/11/24 20:12, Cornelia Huck 写道:
> > On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Thu, Nov 24, 2022 at 03:37:42PM +0800, Jason Wang wrote:
> > > > On Thu, Nov 24, 2022 at 3:08 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Thu, Nov 24, 2022 at 01:41:41PM +0800, Jason Wang wrote:
> > > > > > On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > > +The following group types, and their identifiers, are currently specified):
> > > > > > > +\begin{description}
> > > > > > > +\item[SR-IOV group type (0x1)]
> > > > > > > +This device group has a PCI Single Root I/O Virtualization
> > > > > > > +(SR-IOV) physical function (PF) device as the owner and includes
> > > > > > > +all its SR-IOV virtual functions (VFs) as members (see
> > > > > > > +\hyperref[intro:PCIe]{[PCIe]}).
> > > > > > So I wonder what's the advantage of using a global identifier over the
> > > > > > transport specific one. There's almost no way for CCW/MMIO to use
> > > > > > SR-IOV. Limiting it to PCI seems much easier and avoids layer
> > > > > > violation.
> > > > > > 
> > > > > > Thanks
> > > > > So we burn up an identifier, ccw and mmio won't be able to use it.
> > > > > Big deal? Why?
> > > > Because it is transport specific. The basic facility should be
> > > > transport independent.
> > > I tried this but the result is spread all over the spec
> > > and does not result in a readable cohesive whole.
> 
> 
> I may miss something, but it looks to me it's just a subsection in the PCI
> transport to describe the SR-IOV group identifier.
> 


right so everything is in admin.tex except for the identifier
which is in pci. I find this split annoying - not enough material
here where splitting makes things cleaner.
But we can move it down the road.


> > > So we give up on the transport independent purity a bit and it
> > > seems worth it.
> > > Also explained this in the cover letter - have you seen that?
> 
> 
> Sorry, I don't.
> 
> 
> > > 
> > > 
> > > 
> > > > > And I think we might find a use for this with MMIO
> > > > > down the road with some kind of passthrough. Who knows.
> > > > Probably, but can it be modeled exactly as what SR-IOV looks like? Or
> > > > anyhow, it's not too late to define this for MMIO at that time. For
> > > > example, we know MSI-X may work for MMIO but we define it only for PCI
> > > > now.
> > > > 
> > > > Thanks
> > > Right. So if we reserve the id for all transports then it will
> > > be easy to do.
> > Also, if we go with transport-specific ids, we might end up with
> > different ids per transport when we add a future transport-independent
> > feature.
> 
> 
> I'm not sure this is a real problem:
> 
> 1) all the basic facilities are now transport-independent but they are
> implemented via different transport specific registers/commands/offsets etc.
> 2) I'm not quite sure there would be a transport-independent group
> identifier other than the virtqueue transport, so I wonder if it makes sense
> to split the id space into global ones as well as the transport specific
> ones
> 
> 
> >   The global id space really should be big enough to accommodate
> > even single-transport groups.
> 
> 
> Yes, so it's not about the space but about whether or it it's
> worth/correct/expensive to describe a transport specific identifier in the
> basic facility part.
> 
> Thanks
> 
> 
> > 
> > 
> > ---------------------------------------------------------------------
> > 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 v9 09/10] admin: conformance clauses

[Cornelia Huck]

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

> On Fri, Nov 25, 2022 at 12:42:23PM +0100, Cornelia Huck wrote:
>> On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:
>> 
>> > 在 2022/11/24 16:36, Michael S. Tsirkin 写道:
>> >> On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
>> >>> On Thu, Nov 24, 2022 at 5:08 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >>>> +The device MUST NOT fail a command solely because the buffers
>> >>>> +provided are shorter or longer than described in this
>> >>>> +specification.
>> >>> I may miss something but how can it work if the buffer is shorter?
>> >> driver does not care what's there.
>> >>
>> >> this is mostly for forward compatibility - we'll add more fields and
>> >> I don't want to explain separately that old drivers post
>> >> short buffers with less fields.
>> >
>> >
>> > For example:
>> >
>> > The patch said:
>> >
>> > struct virtio_admin_cmd_list {
>> >         /* Indicates which of the below fields were returned
>> >         le32 device_admin_cmds[];
>> > };
>> >
>> > Does it mean the query can still succeed even if there's no space for 
>> > virtio_admin_cmd_list in the writable buffer?
>> 
>> Can we pick an absolute minimum length? All drivers need to be able to
>> at least accommodate this one. Or is it ok for the driver to send a
>> completely useless query that doesn't get any data back?
>
> I think it might be a way to check that a group exists.

Ok, so not completely useless.


---------------------------------------------------------------------
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 v9 09/10] admin: conformance clauses

[Jason Wang]

在 2022/11/25 19:47, Michael S. Tsirkin 写道:
> On Fri, Nov 25, 2022 at 11:50:52AM +0800, Jason Wang wrote:
>> 在 2022/11/24 16:36, Michael S. Tsirkin 写道:
>>> On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
>>>> On Thu, Nov 24, 2022 at 5:08 AM 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 158 insertions(+)
>>>>>
>>>>> diff --git a/admin.tex b/admin.tex
>>>>> index eec12a9..e83a9f5 100644
>>>>> --- a/admin.tex
>>>>> +++ b/admin.tex
>>>>> @@ -222,6 +222,107 @@ \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}
>>>>> +
>>>>> +The device MUST validate \field{opcode}, \field{group_type} and
>>>>> +\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
>>>>> +nd if any of these has an invalid or unsupported value, set
>>>> typo
>>> thanks!
>>>
>>>>> +\field{status_qualifier} accordingly.
>>>>> +
>>>>> +If a command completes successfully, the device MUST set
>>>>> +\field{status} to VIRTIO_ADMIN_STATUS_OK.
>>>>> +
>>>>> +If a command fails, the device MUST set
>>>>> +\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
>>>>> +
>>>>> +If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
>>>>> +device state MUST NOT change, that is the command MUST NOT have
>>>>> +any side effects on the device, in particular the device MUST not
>>>>> +enter an error state as a result of this command.
>>>>> +
>>>>> +If a command fails, the device state generally SHOULD NOT change,
>>>>> +as far as possible.
>>>>> +
>>>>> +The 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,
>>>>> +the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
>>>>> +for different group types.
>>>>> +
>>>>> +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
>>>>> +the device MUST assume
>>>>> +that the list of legal commands used by the driver consists of
>>>>> +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
>>>>> +
>>>>> +After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
>>>>> +the device MUST set the list of legal commands used by the driver
>>>>> +to the one supplied in \field{command_specific_data}.
>>>>> +
>>>>> +The device MUST set the list of legal commands used by the driver
>>>>> +to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
>>>>> +
>>>>> +The device MUST validate commands against the list used by
>>>>> +the 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.
>>>>> +
>>>>> +The list of supported commands MUST NOT shrink (but MAY expand):
>>>>> +after reporting a given command as supported through
>>>>> +VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
>>>>> +as unsupported.  Further, after a given set of commands has been
>>>>> +used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
>>>>> +device or system reset the device SHOULD complete successfully
>>>>> +any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
>>>>> +list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
>>>>> +after a device or system reset, the device MUST not fail it
>>>>> +solely because of the command list used.  Failure to do so would
>>>>> +interfere with resuming from suspend and error recovery.
>>>>> +
>>>>> +
>>>>> +\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
>>>>> +
>>>>> +The driver MAY discover whether device supports a specific group type
>>>>> +by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
>>>>> +\field{group_type}.
>>>>> +
>>>>> +The 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).
>>>>> +
>>>>> +The driver SHOULD NOT set bits in device_admin_cmds
>>>>> +if it is not familiar with how the command opcode
>>>>> +is used, as dependencies between command opcodes might exist.
>>>>> +
>>>>> +The driver MUST NOT request (via 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.
>>>>> +
>>>>> +The 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.
>>>>> +
>>>>> +The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
>>>>> +VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
>>>>> +with respective bits cleared in \field{command_specific_data}.
>>>>> +
>>>>> +The driver MUST handle a command error with a reserved \field{status}
>>>>> +value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
>>>>> +(except possibly for different error reporting/diagnostic messages).
>>>>> +
>>>>> +The driver MUST handle a command error with a reserved
>>>>> +\field{status_qualifier} value in the same way as
>>>>> +\field{status_qualifier} set to
>>>>> +VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
>>>>> +different error reporting/diagnostic messages).
>>>>> +
>>>>>    \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
>>>>>
>>>>>    An administration virtqueue of an owner device is used to submit
>>>>> @@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
>>>>>    new fields can be added to the tail of a structure,
>>>>>    with the driver using the full structure without concern
>>>>>    for versioning.
>>>>> +
>>>>> +\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
>>>>> +
>>>>> +The 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
>>>> s/"is set"/"are set"
>>> thanks
>>>
>>>>> +\item discarding data that would be written outside the
>>>>> +specified device-writeable buffers.
>>>>> +\end{enumerate}
>>>>> +
>>>>> +The 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}
>>>>> +
>>>>> +The device SHOULD initialize the device-writeable buffer
>>>>> +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.
>>>>> +
>>>>> +The device MUST NOT fail a command solely because the buffers
>>>>> +provided are shorter or longer than described in this
>>>>> +specification.
>>>> I may miss something but how can it work if the buffer is shorter?
>>> driver does not care what's there.
>>>
>>> this is mostly for forward compatibility - we'll add more fields and
>>> I don't want to explain separately that old drivers post
>>> short buffers with less fields.
>>
>> For example:
>>
>> The patch said:
>>
>> struct virtio_admin_cmd_list {
>>         /* Indicates which of the below fields were returned
>>         le32 device_admin_cmds[];
>> };
>>
>> Does it mean the query can still succeed even if there's no space for
>> virtio_admin_cmd_list in the writable buffer?
>>
> yes you just skip it. E.g. it's a way to check that query is supported.


Interesting, I thought it should be assumed by the driver that at least 
the QUERY is supported:

+After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
+the device MUST assume
+that the list of legal commands used by the driver consists of
+the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.


> And this (truncated buffer) is exactly what will happen when we have
> e.g. 1k opcodes.


Yes, this is what I understood.


>
>>>
>>>>> +
>>>>> +The device MUST process commands on a given administration virtqueue
>>>>> +in the order in which they are queued.
>>>> Is it better to use "complete" than "process" here?
>>> I am not sure we need to require they complete in order, unless
>>> IN_ORDER is set (unfortunately it is global for all of device).
>>
>> So I'm not sure the "process" can help in this case. Consider we have two
>> commands X and Y. And Y depends on X.
>>
>> If we say device processes the commands in order, driver can't submit Y
>> until it see X is completed.
>> This seems no changes if we allow the device to
>> process the command out of order.
>>
> Depends on command.
> Consider transport vq, some read following a write. read must not bypass
> it but you do not need to wait for write.


Right, but it looks to me that the "bypass" here means "complete before" 
here.

Thanks


>
>>>> Or we can reuse
>>>> part of the in-order description.
>>>>
>>>> (Or maybe a flush command)
>>> these can be used if offered.
>>>
>>>>> +
>>>>> +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}
>>>>> +
>>>>> +The driver MAY supply device-readable and device-writeable buffers
>>>>> +longer than described in this specification.
>>>>> +
>>>>> +The 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).
>>>>> +
>>>>> +The driver MUST NOT assume that the 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.
>>>> Won't this have security implications? E.g information leak.
>>>>
>>>> Thanks
>>> hmm this is not clear enough. it should ignore the contents
>>> and behave as if it was set to zero. Will fix.
>>
>> Ok.
>>
>> Thanks
>>
>>
>>>
>>>>> +
>>>>> +If multiple administration virtqueues have been configured,
>>>>> +the driver MUST ensure ordering for commands
>>>>> +placed on different administration virtqueues.
>>>>> --
>>>>> MST
>>>>>

Re: [virtio-comment] Re: [PATCH v9 08/10] admin: command list discovery

[Jason Wang]

在 2022/11/25 19:43, Michael S. Tsirkin 写道:
> On Fri, Nov 25, 2022 at 11:38:30AM +0800, Jason Wang wrote:
>> 在 2022/11/24 16:30, Michael S. Tsirkin 写道:
>>> On Thu, Nov 24, 2022 at 02:40:15PM +0800, Jason Wang wrote:
>>>> On Thu, Nov 24, 2022 at 5:08 AM 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.
>>>>>
>>>>> An alternative is per-type VQs. This is possible but will
>>>>> require more per-transport work. Discovery through the vq
>>>>> helps keep things contained.
>>>> This seems to complicate the migration (compatibility) of the adminq
>>>> itself. We need check both features and command list.
>>> Yes. There's no way around this though - commands need to
>>> be per group and a pci device can host at least both
>>> SIOV and SRIOV children.
>>>
>>>
>>>> More below.
>>>>
>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>    admin.tex | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>>    1 file changed, 81 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/admin.tex b/admin.tex
>>>>> index eeccd2e..eec12a9 100644
>>>>> --- a/admin.tex
>>>>> +++ b/admin.tex
>>>>> @@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>>>>>    \hline
>>>>>    opcode & Name & Command Description \\
>>>>>    \hline \hline
>>>>> -0x0000 - 0x7FFF & - &  Group administration commands    \\
>>>>> +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>>>>> +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
>>>>> +0x0002 - 0x7FFF & - &  Group administration commands    \\
>>>>>    \hline
>>>>>    0x8000 - 0xFFFF & - & Reserved    \\
>>>>>    \hline
>>>>> @@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>>>>>    \hline
>>>>>    \end{tabular}
>>>>>
>>>>> +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.
>>>> Is this simpler if we map group type instead of a single opcode? E.g
>>>> virtqueue transport will introduce 10+ commands, using a bit for a
>>>> specific opcode will complicate things.
>>> I think more flexibility is better, and bits are cheap.
>>
>> So we may end up with something like:
>>
>> If VIRTIO_F_TRANSPORT_VQ is supported, device MUST return the following
>> opcodes via VIRTIO_ADMIN_CMD_LIST_QUERY
>>
>> [opX, opY, ....]
>>
>> Probably fine.
>>
> I think I would tie the commands to a group type.


Right, that's what I understood, but it looks to me it can't be achieved 
via LIST_QUERY now (which is based on opcodes).

Thanks


>
>>>
>>>>> +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.
>>>> Any reason we need to invent a negotiation process here? (This seems
>>>> only useful when some commands/opcodes are mutually exclusive)
>>>>
>>>> Thanks
>>> yes, features don't help because they are per owner.
>>> this is per group.
>>>
>>> imagine vq transport. SET_FEATURES commands sets the features
>>> for the child. we need something to negotiate with owner
>>> but still within the vq.
>>
>> Should we document what happens if driver use a opcode that it doesn't
>> negotiate? (It looks to me we don't document this for at leave virtio-net
>> control virtqueue).
>>
>> Thanks
>>
> documented in the conformance section.
>
>
>>>
>>>>> +
>>>>> +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
>>>>>
>>> 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 v9 01/10] virtio: document forward compatibility guarantees

[Jason Wang]

在 2022/11/25 18:37, Cornelia Huck 写道:
> On Fri, Nov 25 2022, Jason Wang <jasowang@redhat.com> wrote:
>
>> 在 2022/11/24 20:05, Cornelia Huck 写道:
>>> On Thu, Nov 24 2022, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> On Thu, Nov 24, 2022 at 03:34:19PM +0800, Jason Wang wrote:
>>>>> On Thu, Nov 24, 2022 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Thu, Nov 24, 2022 at 12:33:52PM +0800, Jason Wang wrote:
>>>>>>> On Thu, Nov 24, 2022 at 5:08 AM 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 | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>    1 file changed, 42 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>>> index 3051399..e3203be 100644
>>>>>>>> --- a/content.tex
>>>>>>>> +++ b/content.tex
>>>>>>>> @@ -114,21 +114,63 @@ \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 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.)
>>>>>>> It looks to me if we want to clarify like this, feature negotiation is
>>>>>>> not sufficient. Do we need to do something similar in other basic
>>>>>>> facilities? Generally, we probably need to do this for facilities that
>>>>>>> are similar to features (status, virtqueue size and others).
>>>>>> I'm not sure about "not sufficient". It's sufficient as long
>>>>>> as you just want to extend features. What triggered this
>>>>>> work is adding a transport specific feature.
>>>>> E.g:
>>>>>
>>>>> For status: Devices do not offer any status bit it would not be able to handle.
>>>>> For virtqueue size:  Devices do not offer virtqueue size it would not
>>>>> be able to handle.
>>>>>
>>>>> ?
>>>> Jason I think what you miss here is this part:
>>>>
>>>> "even though drivers are not
>>>> supposed to accept them in the first place even if offered, according to
>>>> this version of the specification"
>>>>
>>>> does not apply to status and virtqueue size.
>>>>
>>>>
>>>> Let me clarify what all this means.
>>>> It seems safe for a device to offer a reserved feature bit
>>
>> This depends really on the behaviour of the drivers.
>>
>>
>>>> since drivers are not supposed to accept it.
>>
>> So this is the case of the ADMIN_VQ.
>>
>>
>>>> This text says device must not rely on this.
>>>>
>>>> How would this apply to status or vq size? I don't see.
>>> Me neither... for the status, it's about either the driver noting its
>>> progress, or the device indicating that a reset is needed. The only case
>>> where setting something requires kind of an ack is FEATURES_OK, and
>>> there we already spell out the conditions clearly.
>>
>> I basically meant something like:
>>
>> Assuming we have a feature like VIRTIO_RING_F_NEW and a new status bit
>> was mapped to this feature, VIRTIO_CONFIG_S_NEW. And for some reason
>> this feature is reserved for some transports. Should we mention device
>> does not offer VIRTIO_CONFIG_S_NEW as well, or we assume it is implied
>> that we don't offer VIRTIO_CONFIG_S_NEW in this case?
> I'm not sure that adding a feature-specific status bit would make sense,
> given that the status bits either need to work before feature
> negotiation is complete, or are actually needed for feature negotiation.


It can work only after the feature negotiation. E.g the device stop 
(only makes sense after DRIVER_OK is set).


> Also, the status-bit space is way more limited than the feature-bit
> space. Therefore, I think we can safely ignore the status bits.
>
>>
>>> For the queue size,
>>> we specify that the device states what it can support, and that the
>>> driver may only reduce it, that seems clear enough to me.
>>
>> Similar to the above, assuming a feature VIRTIO_R_F_MAXSIZE_XXX, and it
>> is reserved. Should we mention that the new max virtqueue size should
>> not be advertised or it is implied in the feature advertisement?
> I'd say it's implied in the feature bit handling already.


Ok.

Thanks

>

Re: [PATCH v9 09/10] admin: conformance clauses

[Michael S. Tsirkin]

On Mon, Nov 28, 2022 at 12:32:31PM +0800, Jason Wang wrote:
> 
> 在 2022/11/25 19:47, Michael S. Tsirkin 写道:
> > On Fri, Nov 25, 2022 at 11:50:52AM +0800, Jason Wang wrote:
> > > 在 2022/11/24 16:36, Michael S. Tsirkin 写道:
> > > > On Thu, Nov 24, 2022 at 02:51:21PM +0800, Jason Wang wrote:
> > > > > On Thu, Nov 24, 2022 at 5:08 AM 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 | 158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >    1 file changed, 158 insertions(+)
> > > > > > 
> > > > > > diff --git a/admin.tex b/admin.tex
> > > > > > index eec12a9..e83a9f5 100644
> > > > > > --- a/admin.tex
> > > > > > +++ b/admin.tex
> > > > > > @@ -222,6 +222,107 @@ \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}
> > > > > > +
> > > > > > +The device MUST validate \field{opcode}, \field{group_type} and
> > > > > > +\field{group_member_id}, set \field{status} to VIRTIO_ADMIN_STATUS_EINVAL and
> > > > > > +nd if any of these has an invalid or unsupported value, set
> > > > > typo
> > > > thanks!
> > > > 
> > > > > > +\field{status_qualifier} accordingly.
> > > > > > +
> > > > > > +If a command completes successfully, the device MUST set
> > > > > > +\field{status} to VIRTIO_ADMIN_STATUS_OK.
> > > > > > +
> > > > > > +If a command fails, the device MUST set
> > > > > > +\field{status} to a value different from VIRTIO_ADMIN_STATUS_OK.
> > > > > > +
> > > > > > +If \field{status} is set to VIRTIO_ADMIN_STATUS_EINVAL, the
> > > > > > +device state MUST NOT change, that is the command MUST NOT have
> > > > > > +any side effects on the device, in particular the device MUST not
> > > > > > +enter an error state as a result of this command.
> > > > > > +
> > > > > > +If a command fails, the device state generally SHOULD NOT change,
> > > > > > +as far as possible.
> > > > > > +
> > > > > > +The 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,
> > > > > > +the device MAY return different results for VIRTIO_ADMIN_CMD_LIST_QUERY
> > > > > > +for different group types.
> > > > > > +
> > > > > > +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
> > > > > > +the device MUST assume
> > > > > > +that the list of legal commands used by the driver consists of
> > > > > > +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.
> > > > > > +
> > > > > > +After completing VIRTIO_ADMIN_CMD_LIST_USE successfully,
> > > > > > +the device MUST set the list of legal commands used by the driver
> > > > > > +to the one supplied in \field{command_specific_data}.
> > > > > > +
> > > > > > +The device MUST set the list of legal commands used by the driver
> > > > > > +to the one supplied in VIRTIO_ADMIN_CMD_LIST_USE.
> > > > > > +
> > > > > > +The device MUST validate commands against the list used by
> > > > > > +the 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.
> > > > > > +
> > > > > > +The list of supported commands MUST NOT shrink (but MAY expand):
> > > > > > +after reporting a given command as supported through
> > > > > > +VIRTIO_ADMIN_CMD_LIST_QUERY the device MUST NOT later report it
> > > > > > +as unsupported.  Further, after a given set of commands has been
> > > > > > +used (via a successful VIRTIO_ADMIN_CMD_LIST_USE), then after a
> > > > > > +device or system reset the device SHOULD complete successfully
> > > > > > +any following calls to VIRTIO_ADMIN_CMD_LIST_USE with the same
> > > > > > +list of commands; if this command VIRTIO_ADMIN_CMD_LIST_USE fails
> > > > > > +after a device or system reset, the device MUST not fail it
> > > > > > +solely because of the command list used.  Failure to do so would
> > > > > > +interfere with resuming from suspend and error recovery.
> > > > > > +
> > > > > > +
> > > > > > +\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
> > > > > > +
> > > > > > +The driver MAY discover whether device supports a specific group type
> > > > > > +by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
> > > > > > +\field{group_type}.
> > > > > > +
> > > > > > +The 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).
> > > > > > +
> > > > > > +The driver SHOULD NOT set bits in device_admin_cmds
> > > > > > +if it is not familiar with how the command opcode
> > > > > > +is used, as dependencies between command opcodes might exist.
> > > > > > +
> > > > > > +The driver MUST NOT request (via 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.
> > > > > > +
> > > > > > +The 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.
> > > > > > +
> > > > > > +The driver MAY block use of VIRTIO_ADMIN_CMD_LIST_QUERY and
> > > > > > +VIRTIO_ADMIN_CMD_LIST_USE by issuing VIRTIO_ADMIN_CMD_LIST_USE
> > > > > > +with respective bits cleared in \field{command_specific_data}.
> > > > > > +
> > > > > > +The driver MUST handle a command error with a reserved \field{status}
> > > > > > +value in the same way as \field{status} set to VIRTIO_ADMIN_STATUS_EINVAL
> > > > > > +(except possibly for different error reporting/diagnostic messages).
> > > > > > +
> > > > > > +The driver MUST handle a command error with a reserved
> > > > > > +\field{status_qualifier} value in the same way as
> > > > > > +\field{status_qualifier} set to
> > > > > > +VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND (except possibly for
> > > > > > +different error reporting/diagnostic messages).
> > > > > > +
> > > > > >    \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
> > > > > > 
> > > > > >    An administration virtqueue of an owner device is used to submit
> > > > > > @@ -275,3 +376,60 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
> > > > > >    new fields can be added to the tail of a structure,
> > > > > >    with the driver using the full structure without concern
> > > > > >    for versioning.
> > > > > > +
> > > > > > +\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
> > > > > > +
> > > > > > +The 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
> > > > > s/"is set"/"are set"
> > > > thanks
> > > > 
> > > > > > +\item discarding data that would be written outside the
> > > > > > +specified device-writeable buffers.
> > > > > > +\end{enumerate}
> > > > > > +
> > > > > > +The 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}
> > > > > > +
> > > > > > +The device SHOULD initialize the device-writeable buffer
> > > > > > +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.
> > > > > > +
> > > > > > +The device MUST NOT fail a command solely because the buffers
> > > > > > +provided are shorter or longer than described in this
> > > > > > +specification.
> > > > > I may miss something but how can it work if the buffer is shorter?
> > > > driver does not care what's there.
> > > > 
> > > > this is mostly for forward compatibility - we'll add more fields and
> > > > I don't want to explain separately that old drivers post
> > > > short buffers with less fields.
> > > 
> > > For example:
> > > 
> > > The patch said:
> > > 
> > > struct virtio_admin_cmd_list {
> > >         /* Indicates which of the below fields were returned
> > >         le32 device_admin_cmds[];
> > > };
> > > 
> > > Does it mean the query can still succeed even if there's no space for
> > > virtio_admin_cmd_list in the writable buffer?
> > > 
> > yes you just skip it. E.g. it's a way to check that query is supported.
> 
> 
> Interesting, I thought it should be assumed by the driver that at least the
> QUERY is supported:
> 
> +After reset, and before receiving VIRTIO_ADMIN_CMD_LIST_USE for a given group type
> +the device MUST assume
> +that the list of legal commands used by the driver consists of
> +the two commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_USE.

Oh I see what the confusion is.  It's supported if the group exists for
the given owner.  E.g. a VF won't support a SRIOV type, it's designed
for a PF.

> 
> > And this (truncated buffer) is exactly what will happen when we have
> > e.g. 1k opcodes.
> 
> 
> Yes, this is what I understood.
> 
> 
> > 
> > > > 
> > > > > > +
> > > > > > +The device MUST process commands on a given administration virtqueue
> > > > > > +in the order in which they are queued.
> > > > > Is it better to use "complete" than "process" here?
> > > > I am not sure we need to require they complete in order, unless
> > > > IN_ORDER is set (unfortunately it is global for all of device).
> > > 
> > > So I'm not sure the "process" can help in this case. Consider we have two
> > > commands X and Y. And Y depends on X.
> > > 
> > > If we say device processes the commands in order, driver can't submit Y
> > > until it see X is completed.
> > > This seems no changes if we allow the device to
> > > process the command out of order.
> > > 
> > Depends on command.
> > Consider transport vq, some read following a write. read must not bypass
> > it but you do not need to wait for write.
> 
> 
> Right, but it looks to me that the "bypass" here means "complete before"
> here.
> 
> Thanks
> 
> 
> > 
> > > > > Or we can reuse
> > > > > part of the in-order description.
> > > > > 
> > > > > (Or maybe a flush command)
> > > > these can be used if offered.
> > > > 
> > > > > > +
> > > > > > +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}
> > > > > > +
> > > > > > +The driver MAY supply device-readable and device-writeable buffers
> > > > > > +longer than described in this specification.
> > > > > > +
> > > > > > +The 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).
> > > > > > +
> > > > > > +The driver MUST NOT assume that the 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.
> > > > > Won't this have security implications? E.g information leak.
> > > > > 
> > > > > Thanks
> > > > hmm this is not clear enough. it should ignore the contents
> > > > and behave as if it was set to zero. Will fix.
> > > 
> > > Ok.
> > > 
> > > Thanks
> > > 
> > > 
> > > > 
> > > > > > +
> > > > > > +If multiple administration virtqueues have been configured,
> > > > > > +the driver MUST ensure ordering for commands
> > > > > > +placed on different administration virtqueues.
> > > > > > --
> > > > > > MST
> > > > > > 

Re: [virtio-comment] Re: [PATCH v9 08/10] admin: command list discovery

[Michael S. Tsirkin]

On Mon, Nov 28, 2022 at 12:34:43PM +0800, Jason Wang wrote:
> 
> 在 2022/11/25 19:43, Michael S. Tsirkin 写道:
> > On Fri, Nov 25, 2022 at 11:38:30AM +0800, Jason Wang wrote:
> > > 在 2022/11/24 16:30, Michael S. Tsirkin 写道:
> > > > On Thu, Nov 24, 2022 at 02:40:15PM +0800, Jason Wang wrote:
> > > > > On Thu, Nov 24, 2022 at 5:08 AM 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.
> > > > > > 
> > > > > > An alternative is per-type VQs. This is possible but will
> > > > > > require more per-transport work. Discovery through the vq
> > > > > > helps keep things contained.
> > > > > This seems to complicate the migration (compatibility) of the adminq
> > > > > itself. We need check both features and command list.
> > > > Yes. There's no way around this though - commands need to
> > > > be per group and a pci device can host at least both
> > > > SIOV and SRIOV children.
> > > > 
> > > > 
> > > > > More below.
> > > > > 
> > > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > > ---
> > > > > >    admin.tex | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > >    1 file changed, 81 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/admin.tex b/admin.tex
> > > > > > index eeccd2e..eec12a9 100644
> > > > > > --- a/admin.tex
> > > > > > +++ b/admin.tex
> > > > > > @@ -91,7 +91,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > > > > >    \hline
> > > > > >    opcode & Name & Command Description \\
> > > > > >    \hline \hline
> > > > > > -0x0000 - 0x7FFF & - &  Group administration commands    \\
> > > > > > +0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> > > > > > +0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
> > > > > > +0x0002 - 0x7FFF & - &  Group administration commands    \\
> > > > > >    \hline
> > > > > >    0x8000 - 0xFFFF & - & Reserved    \\
> > > > > >    \hline
> > > > > > @@ -142,6 +144,84 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > > > > >    \hline
> > > > > >    \end{tabular}
> > > > > > 
> > > > > > +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.
> > > > > Is this simpler if we map group type instead of a single opcode? E.g
> > > > > virtqueue transport will introduce 10+ commands, using a bit for a
> > > > > specific opcode will complicate things.
> > > > I think more flexibility is better, and bits are cheap.
> > > 
> > > So we may end up with something like:
> > > 
> > > If VIRTIO_F_TRANSPORT_VQ is supported, device MUST return the following
> > > opcodes via VIRTIO_ADMIN_CMD_LIST_QUERY
> > > 
> > > [opX, opY, ....]
> > > 
> > > Probably fine.
> > > 
> > I think I would tie the commands to a group type.
> 
> 
> Right, that's what I understood, but it looks to me it can't be achieved via
> LIST_QUERY now (which is based on opcodes).
> 
> Thanks
> 

My idea was that if a given group type is not supported by the owner
then VIRTIO_ADMIN_CMD_LIST_QUERY fails with invalid group type
status qualifier.


> > 
> > > > 
> > > > > > +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.
> > > > > Any reason we need to invent a negotiation process here? (This seems
> > > > > only useful when some commands/opcodes are mutually exclusive)
> > > > > 
> > > > > Thanks
> > > > yes, features don't help because they are per owner.
> > > > this is per group.
> > > > 
> > > > imagine vq transport. SET_FEATURES commands sets the features
> > > > for the child. we need something to negotiate with owner
> > > > but still within the vq.
> > > 
> > > Should we document what happens if driver use a opcode that it doesn't
> > > negotiate? (It looks to me we don't document this for at leave virtio-net
> > > control virtqueue).
> > > 
> > > Thanks
> > > 
> > documented in the conformance section.
> > 
> > 
> > > > 
> > > > > > +
> > > > > > +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
> > > > > > 
> > > > 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: [virtio-comment] [PATCH v9 04/10] admin: introduce virtio admin virtqueues

[Zhu Lingshan]

On 11/24/2022 5:08 AM, Michael S. Tsirkin 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   | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   content.tex |  7 +++++--
>   2 files changed, 59 insertions(+), 2 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 1dca136..eeccd2e 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -141,3 +141,57 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   0x05-0xFFFF   & -    & reserved for future use \\
>   \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.
> +
> +If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
> +or more adminstration virtqueues. The number and locations of the
> +administration virtqueues are exposed by the owner device in a transport
> +specific manner.
> +
> +The driver 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 The 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. The device silently truncates the structures
Maybe I have missed something, I did not
find the length defined in this series

And there is two buffers, does it implies two chained descriptors?
If so, we can calculate the data length by the descriptor length,
but just as what described in this series, we may extend the contents,
so it looks like we still need an explicit long enough data length.

Thanks
> +to either the length submitted by the driver, or the length
> +described in this specification, whichever is shorter.
> +The device silently ignores any data falling outside
> +the shorter of the two lengths. Any missing fields are 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. The 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 the driver using the full structure without concern
> +for versioning.
> diff --git a/content.tex b/content.tex
> index 3f3585d..04235da 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}
> @@ -6990,6 +6990,9 @@ \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 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/

Re: [virtio-comment] [PATCH v9 03/10] admin: introduce group administration commands

[Zhu Lingshan]

On 11/24/2022 5:08 AM, Michael S. Tsirkin 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 | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 98 insertions(+)
>
> diff --git a/admin.tex b/admin.tex
> index 6ebdd05..1dca136 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -42,4 +42,102 @@ \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.
If we want "transport vq" to rebase on "admin vq", then there can
be something more than administrative commands, like "config msix"
from the guest driver and kicking a vq.

Thanks
Zhu Lingshan
> +
> +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;
> +        le16 status_qualifier;
> +        /* unused, reserved for future extensions */
> +        u8 reserved2[4];
> +        u8 command_specific_result[];
> +};
> +\end{lstlisting}
> +
> +For all commands, \field{opcode}, \field{group_type} and if
> +necessary \field{group_member_id} and \field{command_specific_data} are
> +set by the driver, and the owner device sets \field{status} and if
> +needed \field{status_qualifier} and
> +\field{command_specific_result}.
> +
> +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 & Name & Command Description \\
> +\hline \hline
> +0x0000 - 0x7FFF & - &  Group administration commands    \\
> +\hline
> +0x8000 - 0xFFFF & - & Reserved    \\
> +\hline
> +\end{tabular}
> +
> +The \field{group_type} specifies the group type identifier.
> +The \field{group_member_id} specifies the member identifier within the group.
> +See section \ref{sec:Introduction / Terminology / Device group}
> +for the definition of the group type identifier and group member
> +identifier.
> +
> +The following table describes possible \field{status} values;
> +to simplify common implementations, they are intentionally
> +matching common Linux names and error numbers:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Status (decimal) & Name & Description \\
> +\hline \hline
> +00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> +\hline
> +22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
> +\hline
> +other   & -    & group administration command error  \\
> +\hline
> +\end{tabular}
> +
> +When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_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
> +0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
> +\hline
> +0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
> +\hline
> +0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
> +\hline
> +0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
> +\hline
> +0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEM    & unsupported or invalid \field{group_member_id} \\
> +\hline
> +0x05-0xFFFF   & -    & reserved for future use \\
> +\hline
> +\end{tabular}


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/