[PATCH RFC v7 0/8] Introduce device group and device management

[PATCH RFC v7 0/8] Introduce device group and device management

[Michael S. Tsirkin]

This is back to RFC status, in particular because I did not yet
have the time to work on any of the actual commands.

Change log:
	- 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:
	Conformance statements! This proposal has almost
	none and this won't do.

	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.


Max Gurtovoy (5):
  Introduce device group
  Introduce group administration commands
  Introduce virtio admin virtqueue
  Add admin_queue_index register to PCI common configuration structure
  admin command list discovery

Michael S. Tsirkin (3):
  MMIO: disallow using admin vq bit
  ccw: disallow ADMIN_VQ
  admin: document that structures can be shorter or longer

 admin.tex       | 221 ++++++++++++++++++++++++++++++++++++++++++++++++
 conformance.tex |   1 +
 content.tex     |  40 ++++++++-
 3 files changed, 260 insertions(+), 2 deletions(-)
 create mode 100644 admin.tex

-- 
MST

[PATCH RFC v7 1/8] Introduce device group

[Michael S. Tsirkin]

From: Max Gurtovoy <mgurtovoy@nvidia.com>

Each device group has a type. For now, define one initial type of device
groups: SR-IOV type.

SR-IOV type - A virtio PCI SR-IOV physical function (PF) and its
PCI SR-IOV virtual functions (VFs). This group may contain one or more
virtio devices.

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

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

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 51 insertions(+)
 create mode 100644 admin.tex

diff --git a/admin.tex b/admin.tex
new file mode 100644
index 0000000..6e9434a
--- /dev/null
+++ b/admin.tex
@@ -0,0 +1,49 @@
+\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[Parent device]
+        or parent, the device controlling the group.
+\item[Member device]
+        a device within a group. Parent device itself may, or may
+	not be a member of the group.
+\item[Member identifier]
+        each member has this indentifier, unique within the group
+	and used to address it through the parent device.
+\item[Group type]
+	specifies what kind of member devices there are in a
+	group, how is the member identifier intepreted
+	and what kind of control does the parent have.
+\item[Group identifier]
+	each group has this identifier, unique within the parent device.
+	this specifies the group type and possibly selects the
+	group if multiple groups of the same type can be controlled by the same
+	parent device.
+\end{description}
+
+A single group type is currently specified:
+\begin{description}
+\item[SR-IOV group type]
+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as a parent 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.
+
+At most one group of this type can exist per PF, and
+the group identifier for a group of this type is always 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 parent 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 e863709..6e26dff 100644
--- a/content.tex
+++ b/content.tex
@@ -449,6 +449,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 RFC v7 2/8] Introduce group administration commands

[Michael S. Tsirkin]

From: Max Gurtovoy <mgurtovoy@nvidia.com>

These commands are used for essential administrative and management
operations.

Following patches will introduce an interface for submitting these
commands to the device.

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

diff --git a/admin.tex b/admin.tex
index 6e9434a..4840dd4 100644
--- a/admin.tex
+++ b/admin.tex
@@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 \item[Parent device]
         or parent, the device controlling the group.
 \item[Member device]
-        a device within a group. Parent device itself may, or may
-	not be a member of the group.
+        a device within a group. Parent device itself is not
+	a member of the group. In the future it is envisoned that
+	new group types may be introduced where the parent
+	device is a member of the group.
 \item[Member identifier]
         each member has this indentifier, unique within the group
 	and used to address it through the parent device.
@@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 	and what kind of control does the parent have.
 \item[Group identifier]
 	each group has this identifier, unique within the parent device.
-	this specifies the group type and possibly selects the
-	group if multiple groups of the same type can be controlled by the same
-	parent device.
+	This specifies the group type. In the future it is
+	envisoned that new group types will be introduced where the group
+	identifier also selects the group if multiple groups of the same
+	type can be controlled by the same parent device.
 \end{description}
 
 A single group type is currently specified:
@@ -46,4 +49,90 @@ \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 / Group administration commands}
+
+Group administration commands can be issued through a parent
+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_id;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        u8 status;
+        u8 command_specific_error;
+        /* unused, reserved for future extensions */
+        u8 reserved2[6];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_id} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the parent device sets \field{status} and if
+needed \field{command_specific_error} and
+\field{command_specific_result}.
+
+As a rule, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Command Description \\
+\hline \hline
+0000h - 7FFFh   & Group administration commands    \\
+\hline
+8000h - FFFFh   & Reserved    \\
+\hline
+\end{tabular}
+
+The \field{group_id} specifies the device group.
+The \field{group_member_id} specifies the member device within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group identifier and group member
+identifier.
+
+The following table describes possible \field{status} values:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
+\hline
+02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
+\hline
+03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
+\hline
+04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
+\hline
+05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
+\hline
+\end{tabular}
+
+The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
+VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
+contents of \field{command_specific_error}
+holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
+\field{command_specific_error} value is undefined and should be ignored by the driver.
 
-- 
MST

[PATCH RFC v7 3/8] Introduce virtio admin virtqueue

[Michael S. Tsirkin]

From: Max Gurtovoy <mgurtovoy@nvidia.com>

In one of the many use cases a user wants to manipulate features and
configuration of the virtio devices regardless of the device type
(net/block/console). For that the admin command set introduced. The
admin virtqueue will be the first management 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 elegant way, this patch introduces a new
admin virtqueue interface.

Manipulate features via admin virtqueue is asynchronous, scalable, easy
to extend and doesn't require additional and expensive on-die resources
to be allocated for every new feature that will be added in the future.

Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 admin.tex   | 9 +++++++++
 content.tex | 6 ++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/admin.tex b/admin.tex
index 4840dd4..a0008c7 100644
--- a/admin.tex
+++ b/admin.tex
@@ -136,3 +136,12 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
 \field{command_specific_error} value is undefined and should be ignored by the driver.
 
+\section{Administration Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
+
+An administration virtqueue of a parent device is used to submit
+group administration commands.
+
+An administration virtqueue exists for a certain parent device if
+VIRTIO_F_ADMIN_VQ feature has been negotiated. The index of the
+administration virtqueue is exposed by the parent device in a
+transport specific manner.
diff --git a/content.tex b/content.tex
index 6e26dff..6ce1c07 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}
@@ -6946,6 +6946,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   that the driver can reset a queue individually.
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
+  \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
-- 
MST

[PATCH RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Michael S. Tsirkin]

From: Max Gurtovoy <mgurtovoy@nvidia.com>

This new register will be used for querying the index of the admin
virtqueue of a virtio device. To configure, reset or enable the admin
virtqueue, 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/content.tex b/content.tex
index 6ce1c07..297cb4a 100644
--- a/content.tex
+++ b/content.tex
@@ -904,6 +904,9 @@ \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 */
 };
 \end{lstlisting}
 
@@ -989,6 +992,10 @@ \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 administration virtqueue.
+        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}
@@ -1075,6 +1082,9 @@ \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, the driver MUST
+configure the administration virtqueue using the value of \field{admin_queue_index}.
+
 \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
@@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
   \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
+  At the moment this feature is only supported for devices using
+  \ref{sec:Virtio Transport Options / Virtio Over PCI
+	  Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
+	  as the transport.
 
 \end{description}
 
-- 
MST

[PATCH RFC v7 5/8] MMIO: disallow using admin vq bit

[Michael S. Tsirkin]

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 conformance.tex | 1 +
 content.tex     | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index c3c1d3e..161b5e0 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -383,6 +383,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 An MMIO device MUST conform to the following normative statements:
 
 \begin{itemize}
+\item \ref{devicenormative:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}
 \end{itemize}
 
diff --git a/content.tex b/content.tex
index 297cb4a..76b5a28 100644
--- a/content.tex
+++ b/content.tex
@@ -2079,6 +2079,11 @@ \subsection{MMIO-specific Initialization And Device Operation}\label{sec:Virtio
 
 \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}
 
+\devicenormative{\paragraph}{Device Initialization}{Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}
+
+Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
+DeviceFeatures.
+
 \drivernormative{\paragraph}{Device Initialization}{Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Device Initialization}
 
 The driver MUST start the device initialization by reading and
@@ -2093,6 +2098,9 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
 Further initialization MUST follow the procedure described in
 \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{sec:General Initialization And Device Operation / Device Initialization}.
 
+Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
+DriverFeatures even if offered by the device.
+
 \subsubsection{Virtqueue Configuration}\label{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Virtqueue Configuration}
 
 The driver will typically initialize the virtual queue in the following way:
-- 
MST

[PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

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

diff --git a/content.tex b/content.tex
index 76b5a28..53be680 100644
--- a/content.tex
+++ b/content.tex
@@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
 uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
 combination.
 
+\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
+
+Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
+DeviceFeatures.
+
+\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
+
+Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
+DriverFeatures even if offered by the device.
+
 \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
 
 The device's configuration space is located in host memory.
-- 
MST

[PATCH RFC v7 7/8] admin: document that structures can be shorter or longer

[Michael S. Tsirkin]

ensures forward and backward compatibility as long as
we tuck new structures at the end.

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

diff --git a/admin.tex b/admin.tex
index a0008c7..99b6c2a 100644
--- a/admin.tex
+++ b/admin.tex
@@ -136,6 +136,12 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
 \field{command_specific_error} value is undefined and should be ignored by the driver.
 
+It is legal for the driver to submit commands with device-writeable and
+device-readable structures both shorter and longer than what
+is described in this specification. Device silently truncates the
+structures to the shorter of the two (submitted by driver and
+described in this specification).
+
 \section{Administration Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
 
 An administration virtqueue of a parent device is used to submit
-- 
MST

[PATCH RFC v7 8/8] admin command list discovery

[Michael S. Tsirkin]

From: Max Gurtovoy <mgurtovoy@nvidia.com>

Add commands to find out which commands does each group support.
This will be useful once we have multiple group types.

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

diff --git a/admin.tex b/admin.tex
index 99b6c2a..5956c48 100644
--- a/admin.tex
+++ b/admin.tex
@@ -96,9 +96,11 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 
 \begin{tabular}{|l|l|}
 \hline
-opcode & Command Description \\
+opcode & Name & Command Description \\
 \hline \hline
-0000h - 7FFFh   & Group administration commands    \\
+0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+0001h & VIRTIO_ADMIN_CMD_LIST_ACCEPT & Provides to device list of commands used for this group type \\
+0002h - 7FFFh & - &  Group administration commands    \\
 \hline
 8000h - FFFFh   & Reserved    \\
 \hline
@@ -142,6 +144,72 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 structures to the shorter of the two (submitted by driver and
 described in this specification).
 
+Before sending any other commands for any member of a specific
+group to the device, the driver issues the commands
+VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT.
+
+
+Commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT
+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. For example, the array of size 2 including
+the values 0x3 and 0x1 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_ACCEPT) must
+always be set in \field{device_admin_cmds}.
+
+
+For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
+The \field{group_member_id} is unused. It must be set to zero by driver.
+This command has no command specific data.
+The device, upon success, returns a 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_ACCEPT, \field{opcode}
+is set to 0x1.
+The \field{group_member_id} is unused. It must be set to zero by driver.
+The 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.  before sending
+any commands for any member of a group.
+
+The driver then accepts some of the opcodes by sending to
+the device the command VIRTIO_ADMIN_CMD_LIST_ACCEPT 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 SHOULD NOT set bits in device_admin_cmds
+if they are not familiar with how the command opcode
+is used, since devices MAY 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 Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
 
 An administration virtqueue of a parent device is used to submit
-- 
MST

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Halil Pasic]

On Fri, 12 Aug 2022 13:19:20 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  content.tex | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index 76b5a28..53be680 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
>  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
>  combination.
>  
> +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> +
> +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> +DeviceFeatures.
> +
> +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> +
> +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> +DriverFeatures even if offered by the device.
> +

I'm not sure I understand the intention here. I believe what we try to
accomplish here is the following. The Channel I/O transport *currently*
does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
the Channel I/O transport. Or am I wrong?

If my assumptions are right, then the old incarnation of the spec could
contradict the new incarnation of the spec. Thus I would prefer something
like.

"""
Currently the following features are not supported by the Channel I/O
transport:
* VIRTIO_F_ADMIN_VQ
"""


If we want, we can also state what needs to be done in general when
features are unsupported by the transport. And yes, that normative
material in my opinion.

Regards,
Halil

>  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
>  
>  The device's configuration space is located in host memory.

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
> On Fri, 12 Aug 2022 13:19:20 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  content.tex | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/content.tex b/content.tex
> > index 76b5a28..53be680 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
> >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> >  combination.
> >  
> > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > +
> > +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > +DeviceFeatures.
> > +
> > +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > +
> > +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > +DriverFeatures even if offered by the device.
> > +
> 
> I'm not sure I understand the intention here. I believe what we try to
> accomplish here is the following. The Channel I/O transport *currently*
> does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> the Channel I/O transport. Or am I wrong?
>
> If my assumptions are right, then the old incarnation of the spec could
> contradict the new incarnation of the spec. Thus I would prefer something
> like.

Relaxing requirenents is always okay.

> 
> """
> Currently the following features are not supported by the Channel I/O
> transport:
> * VIRTIO_F_ADMIN_VQ
> """

what I want to prevent is driver saying "oh device will not set ADMIN_VQ
so it's ok to acknowledge it if offered, it is never offered even
though it does not suport it".
because then it becomes impossible to know when actually a new driver
appears with actual support.

So, Maybe just add text

Note: future versions of this specification will allow setting ADMIN_VQ
for driver and device. Device MUST NOT assume driver does not
acknowledge ADMIN_VQ if offered.

And similarly for drivers:

Note: future versions of this specification will allow setting ADMIN_VQ
for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.

> 
> If we want, we can also state what needs to be done in general when
> features are unsupported by the transport. And yes, that normative
> material in my opinion.
> 
> Regards,
> Halil


Are there other examples? I want to call out the list explicitly because
it is so easy to enable an extra feature by mistake.


> >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> >  
> >  The device's configuration space is located in host memory.

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

On Tue, Aug 16, 2022 at 11:48:51AM -0400, Michael S. Tsirkin wrote:
> On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
> > On Fri, 12 Aug 2022 13:19:20 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  content.tex | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 76b5a28..53be680 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
> > >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> > >  combination.
> > >  
> > > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > +DeviceFeatures.
> > > +
> > > +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > +DriverFeatures even if offered by the device.
> > > +
> > 
> > I'm not sure I understand the intention here. I believe what we try to
> > accomplish here is the following. The Channel I/O transport *currently*
> > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > the Channel I/O transport. Or am I wrong?
> >
> > If my assumptions are right, then the old incarnation of the spec could
> > contradict the new incarnation of the spec. Thus I would prefer something
> > like.
> 
> Relaxing requirenents is always okay.
> 
> > 
> > """
> > Currently the following features are not supported by the Channel I/O
> > transport:
> > * VIRTIO_F_ADMIN_VQ
> > """
> 
> what I want to prevent is driver saying "oh device will not set ADMIN_VQ
> so it's ok to acknowledge it if offered, it is never offered even
> though it does not suport it".
> because then it becomes impossible to know when actually a new driver
> appears with actual support.
> 
> So, Maybe just add text
> 
> Note: future versions of this specification will allow setting ADMIN_VQ
> for driver and device. Device MUST NOT assume driver does not
> acknowledge ADMIN_VQ if offered.
> 
> And similarly for drivers:
> 
> Note: future versions of this specification will allow setting ADMIN_VQ
> for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.
> 
> > 
> > If we want, we can also state what needs to be done in general when
> > features are unsupported by the transport. And yes, that normative
> > material in my opinion.
> > 
> > Regards,
> > Halil
> 
> 
> Are there other examples? I want to call out the list explicitly because
> it is so easy to enable an extra feature by mistake.
> 

And also, I don't *want* to make it easy to add features only to some
transports. Possible, ok, but not easy.

> > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > >  
> > >  The device's configuration space is located in host memory.

[virtio-comment] Re: [PATCH RFC v7 1/8] Introduce device group

[Max Gurtovoy]

On 8/12/2022 8:18 PM, Michael S. Tsirkin wrote:
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> Each device group has a type. For now, define one initial type of device
> groups: SR-IOV type.
>
> SR-IOV type - A virtio PCI SR-IOV physical function (PF) and its
> PCI SR-IOV virtual functions (VFs). This group may contain one or more
> virtio devices.
>
> Each device group has a unique identifier. This identifier is the group
> identifier.
>
> Each device within a device group has a unique identifier. This identifier
> is the group member identifier.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>   content.tex |  2 ++
>   2 files changed, 51 insertions(+)
>   create mode 100644 admin.tex
>
> diff --git a/admin.tex b/admin.tex
> new file mode 100644
> index 0000000..6e9434a
> --- /dev/null
> +++ b/admin.tex
> @@ -0,0 +1,49 @@
> +\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[Parent device]
> +        or parent, the device controlling the group.
> +\item[Member device]
> +        a device within a group. Parent device itself may, or may
> +	not be a member of the group.
> +\item[Member identifier]
> +        each member has this indentifier, unique within the group
> +	and used to address it through the parent device.
> +\item[Group type]
> +	specifies what kind of member devices there are in a
> +	group, how is the member identifier intepreted
> +	and what kind of control does the parent have.
> +\item[Group identifier]
> +	each group has this identifier, unique within the parent device.
> +	this specifies the group type and possibly selects the
> +	group if multiple groups of the same type can be controlled by the same
> +	parent device.
> +\end{description}
> +
> +A single group type is currently specified:
> +\begin{description}
> +\item[SR-IOV group type]
> +This device group has a PCI Single Root I/O Virtualization
> +(SR-IOV) physical function (PF) device as a parent 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.

This is different from my V6. Any reason to remove the PF from the group ?

If we send admin-cmd with group_member_id == 0x0 and group_id == 0x1 it 
will go to the PF, right ?

For the command list discovery for example..

> +
> +At most one group of this type can exist per PF, and
> +the group identifier for a group of this type is always 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 parent 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 e863709..6e26dff 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -449,6 +449,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

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 RFC v7 2/8] Introduce group administration commands

[Max Gurtovoy]

On 8/12/2022 8:19 PM, Michael S. Tsirkin wrote:
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> These commands are used for essential administrative and management
> operations.
>
> Following patches will introduce an interface for submitting these
> commands to the device.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 6e9434a..4840dd4 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>   \item[Parent device]
>           or parent, the device controlling the group.
>   \item[Member device]
> -        a device within a group. Parent device itself may, or may
> -	not be a member of the group.
> +        a device within a group. Parent device itself is not
> +	a member of the group. In the future it is envisoned that
> +	new group types may be introduced where the parent
> +	device is a member of the group.

If we will agree on the content, the above changed should be squashed to 
patch 1/8.


>   \item[Member identifier]
>           each member has this indentifier, unique within the group
>   	and used to address it through the parent device.
> @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>   	and what kind of control does the parent have.
>   \item[Group identifier]
>   	each group has this identifier, unique within the parent device.
> -	this specifies the group type and possibly selects the
> -	group if multiple groups of the same type can be controlled by the same
> -	parent device.
> +	This specifies the group type. In the future it is
> +	envisoned that new group types will be introduced where the group
> +	identifier also selects the group if multiple groups of the same
> +	type can be controlled by the same parent device.
If we will agree on the content, the above changed should be squashed to 
patch 1/8.
>   \end{description}
>   
>   A single group type is currently specified:
> @@ -46,4 +49,90 @@ \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 / Group administration commands}
> +
> +Group administration commands can be issued through a parent
> +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..

It also used for query/accept optional admin command as was introduced 
in patch 8/8.

We need to mention somewhere that if group_member_id == 0 the designated 
device is self (and reserve this value).

> +
> +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_id;
> +        /* unused, reserved for future extensions */
> +        u8 reserved1[12];
> +        le64 group_member_id;
> +        u8 command_specific_data[];
> +
> +        /* Device-writable part */
> +        u8 status;
> +        u8 command_specific_error;
> +        /* unused, reserved for future extensions */
> +        u8 reserved2[6];
> +        u8 command_specific_result[];
> +};
> +\end{lstlisting}
> +
> +For all commands, \field{opcode}, \field{group_id} and if
> +necessary \field{group_member_id} and \field{command_specific_data} are
> +set by the driver, and the parent device sets \field{status} and if
> +needed \field{command_specific_error} and
> +\field{command_specific_result}.
> +
> +As a rule, any unused device-readable fields are set to zero by the driver
> +and ignored by the device.  Any unused device-writeable fields are set to zero
> +by the device and ignored by the driver.
> +
> +\field{opcode} specifies the command. The valid
> +values for \field{opcode} can be found in the following table:
> +
> +\begin{tabular}{|l|l|}
> +\hline
> +opcode & Command Description \\
> +\hline \hline
> +0000h - 7FFFh   & Group administration commands    \\
> +\hline
> +8000h - FFFFh   & Reserved    \\
> +\hline
> +\end{tabular}
> +
> +The \field{group_id} specifies the device group.
> +The \field{group_member_id} specifies the member device within the group.
> +See section \ref{sec:Introduction / Terminology / Device group}
> +for the definition of the group identifier and group member
> +identifier.
> +
> +The following table describes possible \field{status} values:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Status & Name & Description \\
> +\hline \hline
> +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> +\hline
> +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> +\hline
> +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> +\hline
> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> +\hline
> +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> +\hline
> +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> +\hline
> +\end{tabular}
> +
> +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> +contents of \field{command_specific_error}
> +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> +\field{command_specific_error} value is undefined and should be ignored by the driver.
>   

[virtio-comment] Re: [PATCH RFC v7 3/8] Introduce virtio admin virtqueue

[Max Gurtovoy]

On 8/12/2022 8:19 PM, Michael S. Tsirkin wrote:
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> In one of the many use cases a user wants to manipulate features and
> configuration of the virtio devices regardless of the device type
> (net/block/console). For that the admin command set introduced. The
> admin virtqueue will be the first management 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 elegant way, this patch introduces a new
> admin virtqueue interface.
>
> Manipulate features via admin virtqueue is asynchronous, scalable, easy
> to extend and doesn't require additional and expensive on-die resources
> to be allocated for every new feature that will be added in the future.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   admin.tex   | 9 +++++++++
>   content.tex | 6 ++++--
>   2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 4840dd4..a0008c7 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -136,3 +136,12 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
>   \field{command_specific_error} value is undefined and should be ignored by the driver.
>   
> +\section{Administration Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
> +
> +An administration virtqueue of a parent device is used to submit
> +group administration commands.
> +
> +An administration virtqueue exists for a certain parent device if
> +VIRTIO_F_ADMIN_VQ feature has been negotiated. The index of the
> +administration virtqueue is exposed by the parent device in a
> +transport specific manner.
> diff --git a/content.tex b/content.tex
> index 6e26dff..6ce1c07 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}
> @@ -6946,6 +6946,8 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>     that the driver can reset a queue individually.
>     See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
>   
> +  \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
> +
>   \end{description}
>   
>   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

Looks good.


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 RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Max Gurtovoy]

On 8/12/2022 8:19 PM, Michael S. Tsirkin wrote:
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> This new register will be used for querying the index of the admin
> virtqueue of a virtio device. To configure, reset or enable the admin
> virtqueue, 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 | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 6ce1c07..297cb4a 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -904,6 +904,9 @@ \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 */
>   };
>   \end{lstlisting}
>   
> @@ -989,6 +992,10 @@ \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 administration virtqueue.
> +        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}
> @@ -1075,6 +1082,9 @@ \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, the driver MUST
> +configure the administration virtqueue using the value of \field{admin_queue_index}.
> +
>   \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
> @@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>     See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
>   
>     \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
> +  At the moment this feature is only supported for devices using
> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> +	  Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
> +	  as the transport.
>   
>   \end{description}
>   

Looks good.

[virtio-comment] Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Max Gurtovoy]

On 8/16/2022 6:50 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 16, 2022 at 11:48:51AM -0400, Michael S. Tsirkin wrote:
>> On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
>>> On Fri, 12 Aug 2022 13:19:20 -0400
>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>   content.tex | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/content.tex b/content.tex
>>>> index 76b5a28..53be680 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
>>>>   uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
>>>>   combination.
>>>>   
>>>> +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
>>>> +
>>>> +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
>>>> +DeviceFeatures.
>>>> +
>>>> +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
>>>> +
>>>> +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
>>>> +DriverFeatures even if offered by the device.
>>>> +
>>> I'm not sure I understand the intention here. I believe what we try to
>>> accomplish here is the following. The Channel I/O transport *currently*
>>> does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
>>> to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
>>> the Channel I/O transport. Or am I wrong?
>>>
>>> If my assumptions are right, then the old incarnation of the spec could
>>> contradict the new incarnation of the spec. Thus I would prefer something
>>> like.
>> Relaxing requirenents is always okay.
>>
>>> """
>>> Currently the following features are not supported by the Channel I/O
>>> transport:
>>> * VIRTIO_F_ADMIN_VQ
>>> """
>> what I want to prevent is driver saying "oh device will not set ADMIN_VQ
>> so it's ok to acknowledge it if offered, it is never offered even
>> though it does not suport it".
>> because then it becomes impossible to know when actually a new driver
>> appears with actual support.
>>
>> So, Maybe just add text
>>
>> Note: future versions of this specification will allow setting ADMIN_VQ
>> for driver and device. Device MUST NOT assume driver does not
>> acknowledge ADMIN_VQ if offered.
>>
>> And similarly for drivers:
>>
>> Note: future versions of this specification will allow setting ADMIN_VQ
>> for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.
>>
>>> If we want, we can also state what needs to be done in general when
>>> features are unsupported by the transport. And yes, that normative
>>> material in my opinion.
>>>
>>> Regards,
>>> Halil
>>
>> Are there other examples? I want to call out the list explicitly because
>> it is so easy to enable an extra feature by mistake.
>>
> And also, I don't *want* to make it easy to add features only to some
> transports. Possible, ok, but not easy.

Don't we have some wording about if a device doesn't offer feature bit 
X, the driver MUST NOT accept feature bit X ?

And solve it once and for all...

>>>>   \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
>>>>   
>>>>   The device's configuration space is located in host memory.

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

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

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

[virtio-comment] Re: [PATCH RFC v7 7/8] admin: document that structures can be shorter or longer

[Max Gurtovoy]

On 8/12/2022 8:19 PM, Michael S. Tsirkin wrote:
> ensures forward and backward compatibility as long as
> we tuck new structures at the end.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   admin.tex | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/admin.tex b/admin.tex
> index a0008c7..99b6c2a 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -136,6 +136,12 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
>   \field{command_specific_error} value is undefined and should be ignored by the driver.
>   
> +It is legal for the driver to submit commands with device-writeable and
> +device-readable structures both shorter and longer than what
> +is described in this specification. Device silently truncates the
> +structures to the shorter of the two (submitted by driver and
> +described in this specification).
> +

Why not limit this to only allow submitting longer structure provided by 
the driver ?

If the structures are shorter, return some error code that we will 
define (VIRTIO_ADMIN_STATUS_READABLE_DATA_SHORT, 
VIRTIO_ADMIN_STATUS_WRITABLE_DATA_SHORT).

Shorter data buffers can provide partial description to the device.

I'm trying not to restrict ourselves with this definition.

Driver that will start getting weird errors will not know what to do and 
why something went wrong.

>   \section{Administration Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
>   
>   An administration virtqueue of a parent device is used to submit

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 RFC v7 8/8] admin command list discovery

[Max Gurtovoy]

On 8/12/2022 8:19 PM, Michael S. Tsirkin wrote:
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> Add commands to find out which commands does each group support.
> This will be useful once we have multiple group types.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   admin.tex | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 99b6c2a..5956c48 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -96,9 +96,11 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   
>   \begin{tabular}{|l|l|}
>   \hline
> -opcode & Command Description \\
> +opcode & Name & Command Description \\
>   \hline \hline
> -0000h - 7FFFh   & Group administration commands    \\
> +0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> +0001h & VIRTIO_ADMIN_CMD_LIST_ACCEPT & Provides to device list of commands used for this group type \\
> +0002h - 7FFFh & - &  Group administration commands    \\
>   \hline
>   8000h - FFFFh   & Reserved    \\
>   \hline
> @@ -142,6 +144,72 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>   structures to the shorter of the two (submitted by driver and
>   described in this specification).
>   
> +Before sending any other commands for any member of a specific
> +group to the device, the driver issues the commands
> +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT.
> +
> +
> +Commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT
> +both use the following structure describing the
> +command opcodes:

Maybe we can create mandatory cmds VIRTIO_ADMIN_CMD_READABLE_SIZE_QUERY, 
VIRTIO_ADMIN_CMD_READABLE_SIZE_ACCEPT and 
VIRTIO_ADMIN_CMD_WRITABLE_SIZE_QUERY, 
VIRTIO_ADMIN_CMD_WRITABLE_SIZE_ACCEPT ?

So both sides will agree on the sizes and no surprises will happen 
during the runtime ?

> +
> +\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. For example, the array of size 2 including
> +the values 0x3 and 0x1 indicates that only opcodes 0, 1 and 32
> +are supported.
0x3 in device_admin_cmds[0] and 0x1 in device_admin_cmds[1].
> +
> +Accordingly, bits 0 and 1 corresponding to opcode 0
> +(VIRTIO_ADMIN_CMD_LIST_QUERY) and 1 (VIRTIO_ADMIN_CMD_LIST_ACCEPT) must
> +always be set in \field{device_admin_cmds}.
> +
> +
> +For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
> +The \field{group_member_id} is unused. It must be set to zero by driver.
> +This command has no command specific data.
> +The device, upon success, returns a 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_ACCEPT, \field{opcode}
> +is set to 0x1.
> +The \field{group_member_id} is unused. It must be set to zero by driver.
> +The 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.  before sending
> +any commands for any member of a group.
> +
> +The driver then accepts some of the opcodes by sending to
> +the device the command VIRTIO_ADMIN_CMD_LIST_ACCEPT 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 SHOULD NOT set bits in device_admin_cmds
> +if they are not familiar with how the command opcode
> +is used, since devices MAY 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 Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
>   
>   An administration virtqueue of a parent device is used to submit

Re: [PATCH RFC v7 1/8] Introduce device group

[Jason Wang]

On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> Each device group has a type. For now, define one initial type of device
> groups: SR-IOV type.
>
> SR-IOV type - A virtio PCI SR-IOV physical function (PF) and its
> PCI SR-IOV virtual functions (VFs). This group may contain one or more
> virtio devices.
>
> Each device group has a unique identifier. This identifier is the group
> identifier.
>
> Each device within a device group has a unique identifier. This identifier
> is the group member identifier.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  content.tex |  2 ++
>  2 files changed, 51 insertions(+)
>  create mode 100644 admin.tex
>
> diff --git a/admin.tex b/admin.tex
> new file mode 100644
> index 0000000..6e9434a
> --- /dev/null
> +++ b/admin.tex
> @@ -0,0 +1,49 @@
> +\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[Parent device]
> +        or parent, the device controlling the group.
> +\item[Member device]
> +        a device within a group. Parent device itself may, or may
> +       not be a member of the group.
> +\item[Member identifier]
> +        each member has this indentifier, unique within the group

Typo, should be "identifier"

> +       and used to address it through the parent device.
> +\item[Group type]
> +       specifies what kind of member devices there are in a
> +       group, how is the member identifier intepreted

"interpreted"

> +       and what kind of control does the parent have.
> +\item[Group identifier]
> +       each group has this identifier, unique within the parent device.
> +       this specifies the group type and possibly selects the
> +       group if multiple groups of the same type can be controlled by the same
> +       parent device.
> +\end{description}
> +
> +A single group type is currently specified:
> +\begin{description}
> +\item[SR-IOV group type]
> +This device group has a PCI Single Root I/O Virtualization
> +(SR-IOV) physical function (PF) device as a parent 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.
> +
> +At most one group of this type can exist per PF, and
> +the group identifier for a group of this type is always 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 parent and member devices for this group type use the Virtio
> +PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
> +\end{description}

I wonder if it's better to reserve types as transport specific ones
and then we can move the above to the part of PCI transport.

Thanks

> +
> +
> diff --git a/content.tex b/content.tex
> index e863709..6e26dff 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -449,6 +449,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-comment] Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Jason Wang]

On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> These commands are used for essential administrative and management
> operations.
>
> Following patches will introduce an interface for submitting these
> commands to the device.
>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 94 insertions(+), 5 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 6e9434a..4840dd4 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>  \item[Parent device]
>          or parent, the device controlling the group.
>  \item[Member device]
> -        a device within a group. Parent device itself may, or may
> -       not be a member of the group.
> +        a device within a group. Parent device itself is not
> +       a member of the group. In the future it is envisoned that
> +       new group types may be introduced where the parent
> +       device is a member of the group.

This part should go with patch 1?

>  \item[Member identifier]
>          each member has this indentifier, unique within the group
>         and used to address it through the parent device.
> @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>         and what kind of control does the parent have.
>  \item[Group identifier]
>         each group has this identifier, unique within the parent device.
> -       this specifies the group type and possibly selects the
> -       group if multiple groups of the same type can be controlled by the same
> -       parent device.
> +       This specifies the group type. In the future it is
> +       envisoned that new group types will be introduced where the group
> +       identifier also selects the group if multiple groups of the same
> +       type can be controlled by the same parent device.
>  \end{description}
>
>  A single group type is currently specified:
> @@ -46,4 +49,90 @@ \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 / Group administration commands}
> +
> +Group administration commands can be issued through a parent
> +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_id;

I wonder about the implication of this field. Does it mean:

1) a single admin virtqueue that manage two group of VFs

or

2) a single admin virtqueue that manages both VFs and SFs (then
'group_type' should be better?)

If it's 1, the group id seems not sufficient. If it's 2, I wonder if
it scales (using a single admin virtqueue to manage both VF and SF).

Thanks

> +        /* unused, reserved for future extensions */
> +        u8 reserved1[12];
> +        le64 group_member_id;
> +        u8 command_specific_data[];
> +
> +        /* Device-writable part */
> +        u8 status;
> +        u8 command_specific_error;
> +        /* unused, reserved for future extensions */
> +        u8 reserved2[6];
> +        u8 command_specific_result[];
> +};
> +\end{lstlisting}
> +
> +For all commands, \field{opcode}, \field{group_id} and if
> +necessary \field{group_member_id} and \field{command_specific_data} are
> +set by the driver, and the parent device sets \field{status} and if
> +needed \field{command_specific_error} and
> +\field{command_specific_result}.
> +
> +As a rule, any unused device-readable fields are set to zero by the driver
> +and ignored by the device.  Any unused device-writeable fields are set to zero
> +by the device and ignored by the driver.
> +
> +\field{opcode} specifies the command. The valid
> +values for \field{opcode} can be found in the following table:
> +
> +\begin{tabular}{|l|l|}
> +\hline
> +opcode & Command Description \\
> +\hline \hline
> +0000h - 7FFFh   & Group administration commands    \\
> +\hline
> +8000h - FFFFh   & Reserved    \\
> +\hline
> +\end{tabular}
> +
> +The \field{group_id} specifies the device group.
> +The \field{group_member_id} specifies the member device within the group.
> +See section \ref{sec:Introduction / Terminology / Device group}
> +for the definition of the group identifier and group member
> +identifier.
> +
> +The following table describes possible \field{status} values:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Status & Name & Description \\
> +\hline \hline
> +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> +\hline
> +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> +\hline
> +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> +\hline
> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> +\hline
> +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> +\hline
> +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> +\hline
> +\end{tabular}
> +
> +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> +contents of \field{command_specific_error}
> +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> +\field{command_specific_error} value is undefined and should be ignored by the driver.
>
> --
> 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 RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Jason Wang]

On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> This new register will be used for querying the index of the admin
> virtqueue of a virtio device. To configure, reset or enable the admin
> virtqueue, 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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/content.tex b/content.tex
> index 6ce1c07..297cb4a 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -904,6 +904,9 @@ \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 */

Is there a chance that we may have multiple admin virtqueues?

Thanks


>  };
>  \end{lstlisting}
>
> @@ -989,6 +992,10 @@ \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 administration virtqueue.
> +        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}
> @@ -1075,6 +1082,9 @@ \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, the driver MUST
> +configure the administration virtqueue using the value of \field{admin_queue_index}.
> +
>  \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
> @@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
>
>    \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
> +  At the moment this feature is only supported for devices using
> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> +         Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
> +         as the transport.
>
>  \end{description}
>
> --
> MST
>

[virtio] Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> >
> > These commands are used for essential administrative and management
> > operations.
> >
> > Following patches will introduce an interface for submitting these
> > commands to the device.
> >
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 94 insertions(+), 5 deletions(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 6e9434a..4840dd4 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> >  \item[Parent device]
> >          or parent, the device controlling the group.
> >  \item[Member device]
> > -        a device within a group. Parent device itself may, or may
> > -       not be a member of the group.
> > +        a device within a group. Parent device itself is not
> > +       a member of the group. In the future it is envisoned that
> > +       new group types may be introduced where the parent
> > +       device is a member of the group.
> 
> This part should go with patch 1?
> 
> >  \item[Member identifier]
> >          each member has this indentifier, unique within the group
> >         and used to address it through the parent device.
> > @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> >         and what kind of control does the parent have.
> >  \item[Group identifier]
> >         each group has this identifier, unique within the parent device.
> > -       this specifies the group type and possibly selects the
> > -       group if multiple groups of the same type can be controlled by the same
> > -       parent device.
> > +       This specifies the group type. In the future it is
> > +       envisoned that new group types will be introduced where the group
> > +       identifier also selects the group if multiple groups of the same
> > +       type can be controlled by the same parent device.
> >  \end{description}
> >
> >  A single group type is currently specified:
> > @@ -46,4 +49,90 @@ \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 / Group administration commands}
> > +
> > +Group administration commands can be issued through a parent
> > +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_id;
> 
> I wonder about the implication of this field. Does it mean:
> 
> 1) a single admin virtqueue that manage two group of VFs
> 
> or
> 
> 2) a single admin virtqueue that manages both VFs and SFs (then
> 'group_type' should be better?)
> 
> If it's 1, the group id seems not sufficient. If it's 2, I wonder if
> it scales (using a single admin virtqueue to manage both VF and SF).
> 
> Thanks

2 at the moment.  Not sure I understand why wouldn't it scale. I don't
see a fundamental difference between 1 or 2 VQs. We are not going to be
adding 1000s of them,

> > +        /* unused, reserved for future extensions */
> > +        u8 reserved1[12];
> > +        le64 group_member_id;
> > +        u8 command_specific_data[];
> > +
> > +        /* Device-writable part */
> > +        u8 status;
> > +        u8 command_specific_error;
> > +        /* unused, reserved for future extensions */
> > +        u8 reserved2[6];
> > +        u8 command_specific_result[];
> > +};
> > +\end{lstlisting}
> > +
> > +For all commands, \field{opcode}, \field{group_id} and if
> > +necessary \field{group_member_id} and \field{command_specific_data} are
> > +set by the driver, and the parent device sets \field{status} and if
> > +needed \field{command_specific_error} and
> > +\field{command_specific_result}.
> > +
> > +As a rule, any unused device-readable fields are set to zero by the driver
> > +and ignored by the device.  Any unused device-writeable fields are set to zero
> > +by the device and ignored by the driver.
> > +
> > +\field{opcode} specifies the command. The valid
> > +values for \field{opcode} can be found in the following table:
> > +
> > +\begin{tabular}{|l|l|}
> > +\hline
> > +opcode & Command Description \\
> > +\hline \hline
> > +0000h - 7FFFh   & Group administration commands    \\
> > +\hline
> > +8000h - FFFFh   & Reserved    \\
> > +\hline
> > +\end{tabular}
> > +
> > +The \field{group_id} specifies the device group.
> > +The \field{group_member_id} specifies the member device within the group.
> > +See section \ref{sec:Introduction / Terminology / Device group}
> > +for the definition of the group identifier and group member
> > +identifier.
> > +
> > +The following table describes possible \field{status} values:
> > +
> > +\begin{tabular}{|l|l|l|}
> > +\hline
> > +Status & Name & Description \\
> > +\hline \hline
> > +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > +\hline
> > +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> > +\hline
> > +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> > +\hline
> > +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> > +\hline
> > +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> > +\hline
> > +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> > +\hline
> > +\end{tabular}
> > +
> > +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> > +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> > +contents of \field{command_specific_error}
> > +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> > +\field{command_specific_error} value is undefined and should be ignored by the driver.
> >
> > --
> > MST
> >


---------------------------------------------------------------------
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 RFC v7 8/8] admin command list discovery

[Jason Wang]

On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>
> Add commands to find out which commands does each group support.
> This will be useful once we have multiple group types.

I still wonder if it's better to just use different types of
virtqueues, it seems more simple than this.

Thanks

>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  admin.tex | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/admin.tex b/admin.tex
> index 99b6c2a..5956c48 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -96,9 +96,11 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>
>  \begin{tabular}{|l|l|}
>  \hline
> -opcode & Command Description \\
> +opcode & Name & Command Description \\
>  \hline \hline
> -0000h - 7FFFh   & Group administration commands    \\
> +0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> +0001h & VIRTIO_ADMIN_CMD_LIST_ACCEPT & Provides to device list of commands used for this group type \\
> +0002h - 7FFFh & - &  Group administration commands    \\
>  \hline
>  8000h - FFFFh   & Reserved    \\
>  \hline
> @@ -142,6 +144,72 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  structures to the shorter of the two (submitted by driver and
>  described in this specification).
>
> +Before sending any other commands for any member of a specific
> +group to the device, the driver issues the commands
> +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT.
> +
> +
> +Commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT
> +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. For example, the array of size 2 including
> +the values 0x3 and 0x1 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_ACCEPT) must
> +always be set in \field{device_admin_cmds}.
> +
> +
> +For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
> +The \field{group_member_id} is unused. It must be set to zero by driver.
> +This command has no command specific data.
> +The device, upon success, returns a 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_ACCEPT, \field{opcode}
> +is set to 0x1.
> +The \field{group_member_id} is unused. It must be set to zero by driver.
> +The 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.  before sending
> +any commands for any member of a group.
> +
> +The driver then accepts some of the opcodes by sending to
> +the device the command VIRTIO_ADMIN_CMD_LIST_ACCEPT 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 SHOULD NOT set bits in device_admin_cmds
> +if they are not familiar with how the command opcode
> +is used, since devices MAY 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 Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
>
>  An administration virtqueue of a parent device is used to submit
> --
> MST
>

Re: [PATCH RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 04:49:38PM +0800, Jason Wang wrote:
> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> >
> > This new register will be used for querying the index of the admin
> > virtqueue of a virtio device. To configure, reset or enable the admin
> > virtqueue, 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 | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/content.tex b/content.tex
> > index 6ce1c07..297cb4a 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -904,6 +904,9 @@ \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 */
> 
> Is there a chance that we may have multiple admin virtqueues?
> 
> Thanks

I don't see many reasons but if we ever need that we can always add
another register.


> 
> >  };
> >  \end{lstlisting}
> >
> > @@ -989,6 +992,10 @@ \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 administration virtqueue.
> > +        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}
> > @@ -1075,6 +1082,9 @@ \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, the driver MUST
> > +configure the administration virtqueue using the value of \field{admin_queue_index}.
> > +
> >  \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
> > @@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >    See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
> >
> >    \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
> > +  At the moment this feature is only supported for devices using
> > +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> > +         Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
> > +         as the transport.
> >
> >  \end{description}
> >
> > --
> > MST
> >

Re: [PATCH RFC v7 8/8] admin command list discovery

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 04:51:39PM +0800, Jason Wang wrote:
> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> >
> > Add commands to find out which commands does each group support.
> > This will be useful once we have multiple group types.
> 
> I still wonder if it's better to just use different types of
> virtqueues, it seems more simple than this.
> 
> Thanks

I am not sure these commands are needed. Maybe make do with feature bits
for now. But VQs won't solve anything that feature bits won't.

> >
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> >
> > diff --git a/admin.tex b/admin.tex
> > index 99b6c2a..5956c48 100644
> > --- a/admin.tex
> > +++ b/admin.tex
> > @@ -96,9 +96,11 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >
> >  \begin{tabular}{|l|l|}
> >  \hline
> > -opcode & Command Description \\
> > +opcode & Name & Command Description \\
> >  \hline \hline
> > -0000h - 7FFFh   & Group administration commands    \\
> > +0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> > +0001h & VIRTIO_ADMIN_CMD_LIST_ACCEPT & Provides to device list of commands used for this group type \\
> > +0002h - 7FFFh & - &  Group administration commands    \\
> >  \hline
> >  8000h - FFFFh   & Reserved    \\
> >  \hline
> > @@ -142,6 +144,72 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> >  structures to the shorter of the two (submitted by driver and
> >  described in this specification).
> >
> > +Before sending any other commands for any member of a specific
> > +group to the device, the driver issues the commands
> > +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT.
> > +
> > +
> > +Commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT
> > +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. For example, the array of size 2 including
> > +the values 0x3 and 0x1 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_ACCEPT) must
> > +always be set in \field{device_admin_cmds}.
> > +
> > +
> > +For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
> > +The \field{group_member_id} is unused. It must be set to zero by driver.
> > +This command has no command specific data.
> > +The device, upon success, returns a 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_ACCEPT, \field{opcode}
> > +is set to 0x1.
> > +The \field{group_member_id} is unused. It must be set to zero by driver.
> > +The 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.  before sending
> > +any commands for any member of a group.
> > +
> > +The driver then accepts some of the opcodes by sending to
> > +the device the command VIRTIO_ADMIN_CMD_LIST_ACCEPT 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 SHOULD NOT set bits in device_admin_cmds
> > +if they are not familiar with how the command opcode
> > +is used, since devices MAY 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 Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
> >
> >  An administration virtqueue of a parent device is used to submit
> > --
> > MST
> >

Re: [PATCH RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Max Gurtovoy]

On 8/18/2022 11:49 AM, Jason Wang wrote:
> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>
>> This new register will be used for querying the index of the admin
>> virtqueue of a virtio device. To configure, reset or enable the admin
>> virtqueue, 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 | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/content.tex b/content.tex
>> index 6ce1c07..297cb4a 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -904,6 +904,9 @@ \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 */
> Is there a chance that we may have multiple admin virtqueues?

No, same as we probably can't have multiple ctrl virtqueues..


>
> Thanks
>
>
>>   };
>>   \end{lstlisting}
>>
>> @@ -989,6 +992,10 @@ \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 administration virtqueue.
>> +        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}
>> @@ -1075,6 +1082,9 @@ \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, the driver MUST
>> +configure the administration virtqueue using the value of \field{admin_queue_index}.
>> +
>>   \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
>> @@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>     See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
>>
>>     \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
>> +  At the moment this feature is only supported for devices using
>> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
>> +         Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
>> +         as the transport.
>>
>>   \end{description}
>>
>> --
>> MST
>>

Re: [PATCH RFC v7 1/8] Introduce device group

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 04:37:33PM +0800, Jason Wang wrote:
> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> >
> > Each device group has a type. For now, define one initial type of device
> > groups: SR-IOV type.
> >
> > SR-IOV type - A virtio PCI SR-IOV physical function (PF) and its
> > PCI SR-IOV virtual functions (VFs). This group may contain one or more
> > virtio devices.
> >
> > Each device group has a unique identifier. This identifier is the group
> > identifier.
> >
> > Each device within a device group has a unique identifier. This identifier
> > is the group member identifier.
> >
> > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  content.tex |  2 ++
> >  2 files changed, 51 insertions(+)
> >  create mode 100644 admin.tex
> >
> > diff --git a/admin.tex b/admin.tex
> > new file mode 100644
> > index 0000000..6e9434a
> > --- /dev/null
> > +++ b/admin.tex
> > @@ -0,0 +1,49 @@
> > +\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[Parent device]
> > +        or parent, the device controlling the group.
> > +\item[Member device]
> > +        a device within a group. Parent device itself may, or may
> > +       not be a member of the group.
> > +\item[Member identifier]
> > +        each member has this indentifier, unique within the group
> 
> Typo, should be "identifier"
> 
> > +       and used to address it through the parent device.
> > +\item[Group type]
> > +       specifies what kind of member devices there are in a
> > +       group, how is the member identifier intepreted
> 
> "interpreted"
> 
> > +       and what kind of control does the parent have.
> > +\item[Group identifier]
> > +       each group has this identifier, unique within the parent device.
> > +       this specifies the group type and possibly selects the
> > +       group if multiple groups of the same type can be controlled by the same
> > +       parent device.
> > +\end{description}
> > +
> > +A single group type is currently specified:
> > +\begin{description}
> > +\item[SR-IOV group type]
> > +This device group has a PCI Single Root I/O Virtualization
> > +(SR-IOV) physical function (PF) device as a parent 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.
> > +
> > +At most one group of this type can exist per PF, and
> > +the group identifier for a group of this type is always 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 parent and member devices for this group type use the Virtio
> > +PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
> > +\end{description}
> 
> I wonder if it's better to reserve types as transport specific ones
> and then we can move the above to the part of PCI transport.
> 
> Thanks

The point is that PCI groups are not really part of PCI
transport. They are a layer above.

We can move them down the road easily enough but for now it seems better
for readers if all the management things are collected in one place.



> > +
> > +
> > diff --git a/content.tex b/content.tex
> > index e863709..6e26dff 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -449,6 +449,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: [virtio-dev] Re: [PATCH RFC v7 8/8] admin command list discovery

[Zhu, Lingshan]

On 8/18/2022 4:51 PM, Jason Wang wrote:
> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>
>> Add commands to find out which commands does each group support.
>> This will be useful once we have multiple group types.
> I still wonder if it's better to just use different types of
> virtqueues, it seems more simple than this.
Agree!
>
> Thanks
>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>   admin.tex | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 70 insertions(+), 2 deletions(-)
>>
>> diff --git a/admin.tex b/admin.tex
>> index 99b6c2a..5956c48 100644
>> --- a/admin.tex
>> +++ b/admin.tex
>> @@ -96,9 +96,11 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>>
>>   \begin{tabular}{|l|l|}
>>   \hline
>> -opcode & Command Description \\
>> +opcode & Name & Command Description \\
>>   \hline \hline
>> -0000h - 7FFFh   & Group administration commands    \\
>> +0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
>> +0001h & VIRTIO_ADMIN_CMD_LIST_ACCEPT & Provides to device list of commands used for this group type \\
>> +0002h - 7FFFh & - &  Group administration commands    \\
>>   \hline
>>   8000h - FFFFh   & Reserved    \\
>>   \hline
>> @@ -142,6 +144,72 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>>   structures to the shorter of the two (submitted by driver and
>>   described in this specification).
>>
>> +Before sending any other commands for any member of a specific
>> +group to the device, the driver issues the commands
>> +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT.
>> +
>> +
>> +Commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT
>> +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. For example, the array of size 2 including
>> +the values 0x3 and 0x1 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_ACCEPT) must
>> +always be set in \field{device_admin_cmds}.
>> +
>> +
>> +For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
>> +The \field{group_member_id} is unused. It must be set to zero by driver.
>> +This command has no command specific data.
>> +The device, upon success, returns a 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_ACCEPT, \field{opcode}
>> +is set to 0x1.
>> +The \field{group_member_id} is unused. It must be set to zero by driver.
>> +The 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.  before sending
>> +any commands for any member of a group.
>> +
>> +The driver then accepts some of the opcodes by sending to
>> +the device the command VIRTIO_ADMIN_CMD_LIST_ACCEPT 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 SHOULD NOT set bits in device_admin_cmds
>> +if they are not familiar with how the command opcode
>> +is used, since devices MAY 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 Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
>>
>>   An administration virtqueue of a parent device is used to submit
>> --
>> 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: [virtio-dev] Re: [PATCH RFC v7 8/8] admin command list discovery

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 04:56:43PM +0800, Zhu, Lingshan wrote:
> 
> 
> On 8/18/2022 4:51 PM, Jason Wang wrote:
> > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > 
> > > Add commands to find out which commands does each group support.
> > > This will be useful once we have multiple group types.
> > I still wonder if it's better to just use different types of
> > virtqueues, it seems more simple than this.
> Agree!

Not sure what you are agreeing to.

But I'm not sure about this command anyway, I think I will just drop it.


> > 
> > Thanks
> > 
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >   admin.tex | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >   1 file changed, 70 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > index 99b6c2a..5956c48 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -96,9 +96,11 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > > 
> > >   \begin{tabular}{|l|l|}
> > >   \hline
> > > -opcode & Command Description \\
> > > +opcode & Name & Command Description \\
> > >   \hline \hline
> > > -0000h - 7FFFh   & Group administration commands    \\
> > > +0000h & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
> > > +0001h & VIRTIO_ADMIN_CMD_LIST_ACCEPT & Provides to device list of commands used for this group type \\
> > > +0002h - 7FFFh & - &  Group administration commands    \\
> > >   \hline
> > >   8000h - FFFFh   & Reserved    \\
> > >   \hline
> > > @@ -142,6 +144,72 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
> > >   structures to the shorter of the two (submitted by driver and
> > >   described in this specification).
> > > 
> > > +Before sending any other commands for any member of a specific
> > > +group to the device, the driver issues the commands
> > > +VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT.
> > > +
> > > +
> > > +Commands VIRTIO_ADMIN_CMD_LIST_QUERY and VIRTIO_ADMIN_CMD_LIST_ACCEPT
> > > +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. For example, the array of size 2 including
> > > +the values 0x3 and 0x1 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_ACCEPT) must
> > > +always be set in \field{device_admin_cmds}.
> > > +
> > > +
> > > +For the command VIRTIO_ADMIN_CMD_LIST_QUERY, \field{opcode} is set to 0x0.
> > > +The \field{group_member_id} is unused. It must be set to zero by driver.
> > > +This command has no command specific data.
> > > +The device, upon success, returns a 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_ACCEPT, \field{opcode}
> > > +is set to 0x1.
> > > +The \field{group_member_id} is unused. It must be set to zero by driver.
> > > +The 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.  before sending
> > > +any commands for any member of a group.
> > > +
> > > +The driver then accepts some of the opcodes by sending to
> > > +the device the command VIRTIO_ADMIN_CMD_LIST_ACCEPT 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 SHOULD NOT set bits in device_admin_cmds
> > > +if they are not familiar with how the command opcode
> > > +is used, since devices MAY 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 Virtqueue}\label{sec:Basic Facilities of a Virtio Device / Group Administration Virtqueues}
> > > 
> > >   An administration virtqueue of a parent device is used to submit
> > > --
> > > 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: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Halil Pasic]

On Tue, 16 Aug 2022 11:48:43 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
> > On Fri, 12 Aug 2022 13:19:20 -0400
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  content.tex | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/content.tex b/content.tex
> > > index 76b5a28..53be680 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
> > >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> > >  combination.
> > >  
> > > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > +DeviceFeatures.
> > > +
> > > +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > +DriverFeatures even if offered by the device.
> > > +  
> > 
> > I'm not sure I understand the intention here. I believe what we try to
> > accomplish here is the following. The Channel I/O transport *currently*
> > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > the Channel I/O transport. Or am I wrong?
> >
> > If my assumptions are right, then the old incarnation of the spec could
> > contradict the new incarnation of the spec. Thus I would prefer something
> > like.  
> 
> Relaxing requirenents is always okay.

Are you telling me, that for instance a driver author may not rely on
even the MUST type device normative behavior stated by the spec, because
future incarnations of the spec could relax the requirements towards this
particular device, for example by removing that device normative
statement?

I always imagined, if the spec says the device or the driver MUST
"something", then I as the implementer of the other end (driver or
device, can rely on that "something"). If this assumption is wrong then
I'm have to re-examine my entire mental model of the spec.

> 
> > 
> > """
> > Currently the following features are not supported by the Channel I/O
> > transport:
> > * VIRTIO_F_ADMIN_VQ
> > """  
> 
> what I want to prevent is driver saying "oh device will not set ADMIN_VQ
> so it's ok to acknowledge it if offered, it is never offered even
> though it does not suport it".
> because then it becomes impossible to know when actually a new driver
> appears with actual support.

Fair point!

I would prefer a driver normative which goes like this:

"""
A driver SHOULD NOT accept features (i.e. have code that would do so if
the feature is offered) if the feature is not supported by the driver
(e.g. because unsupported by the transport), even if the specification
implies that the device can not offer these features in the first place
(e.g. because the feature is not yet supported by the transport.
"""

And a similar device normative as well, which just that it may not offer
such features.

"""
Note: The rationale behind the [reference to the normative] is that
while some features can not be implemented within the boundaries of the
current virtio specification, future incarnations of the specificaton may
make such implementations possible.  A most prominent example is optional
features dependent on optional virtio facilities whose transport specific
implementation is not yet specified for some transports. Should one end
gain the ability to support these features, the old implementation which
made the assumption that the other end will make sure these features are
not negotiated would end up negotiating something it can't actually
support.
"""



> 
> So, Maybe just add text
> 
> Note: future versions of this specification will allow setting ADMIN_VQ
> for driver and device. Device MUST NOT assume driver does not
> acknowledge ADMIN_VQ if offered.

I would not lean out of the window and promise something with regards to
future versions of this spec.

> 
> And similarly for drivers:
> 
> Note: future versions of this specification will allow setting ADMIN_VQ
> for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.
> 

I think we can then make a note which references the generic normative
for each feature affected where it suits us.

> > 
> > If we want, we can also state what needs to be done in general when
> > features are unsupported by the transport. And yes, that normative
> > material in my opinion.
> > 
> > Regards,
> > Halil  
> 
> 
> Are there other examples? I want to call out the list explicitly because
> it is so easy to enable an extra feature by mistake.
> 

I don't think CCW supports the shared memory yet... But I may be wrong.

> 
> > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > >  
> > >  The device's configuration space is located in host memory.  
> 

Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Jason Wang]

On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
> > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > >
> > > These commands are used for essential administrative and management
> > > operations.
> > >
> > > Following patches will introduce an interface for submitting these
> > > commands to the device.
> > >
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 94 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/admin.tex b/admin.tex
> > > index 6e9434a..4840dd4 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> > >  \item[Parent device]
> > >          or parent, the device controlling the group.
> > >  \item[Member device]
> > > -        a device within a group. Parent device itself may, or may
> > > -       not be a member of the group.
> > > +        a device within a group. Parent device itself is not
> > > +       a member of the group. In the future it is envisoned that
> > > +       new group types may be introduced where the parent
> > > +       device is a member of the group.
> >
> > This part should go with patch 1?
> >
> > >  \item[Member identifier]
> > >          each member has this indentifier, unique within the group
> > >         and used to address it through the parent device.
> > > @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> > >         and what kind of control does the parent have.
> > >  \item[Group identifier]
> > >         each group has this identifier, unique within the parent device.
> > > -       this specifies the group type and possibly selects the
> > > -       group if multiple groups of the same type can be controlled by the same
> > > -       parent device.
> > > +       This specifies the group type. In the future it is
> > > +       envisoned that new group types will be introduced where the group
> > > +       identifier also selects the group if multiple groups of the same
> > > +       type can be controlled by the same parent device.
> > >  \end{description}
> > >
> > >  A single group type is currently specified:
> > > @@ -46,4 +49,90 @@ \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 / Group administration commands}
> > > +
> > > +Group administration commands can be issued through a parent
> > > +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_id;
> >
> > I wonder about the implication of this field. Does it mean:
> >
> > 1) a single admin virtqueue that manage two group of VFs
> >
> > or
> >
> > 2) a single admin virtqueue that manages both VFs and SFs (then
> > 'group_type' should be better?)
> >
> > If it's 1, the group id seems not sufficient. If it's 2, I wonder if
> > it scales (using a single admin virtqueue to manage both VF and SF).
> >
> > Thanks
>
> 2 at the moment.  Not sure I understand why wouldn't it scale.

Scaling as the number of member devices increases (e.g 1K or 10K). Or
it's too early to care about those?

Do we gain better latency if the implementation limits the number of
member devices per admin virtqueue?

> I don't
> see a fundamental difference between 1 or 2 VQs. We are not going to be
> adding 1000s of them,

Yes, but not sure if it will cause delay for e.g live migration
downtime in the future.

Thanks

>
> > > +        /* unused, reserved for future extensions */
> > > +        u8 reserved1[12];
> > > +        le64 group_member_id;
> > > +        u8 command_specific_data[];
> > > +
> > > +        /* Device-writable part */
> > > +        u8 status;
> > > +        u8 command_specific_error;
> > > +        /* unused, reserved for future extensions */
> > > +        u8 reserved2[6];
> > > +        u8 command_specific_result[];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +For all commands, \field{opcode}, \field{group_id} and if
> > > +necessary \field{group_member_id} and \field{command_specific_data} are
> > > +set by the driver, and the parent device sets \field{status} and if
> > > +needed \field{command_specific_error} and
> > > +\field{command_specific_result}.
> > > +
> > > +As a rule, any unused device-readable fields are set to zero by the driver
> > > +and ignored by the device.  Any unused device-writeable fields are set to zero
> > > +by the device and ignored by the driver.
> > > +
> > > +\field{opcode} specifies the command. The valid
> > > +values for \field{opcode} can be found in the following table:
> > > +
> > > +\begin{tabular}{|l|l|}
> > > +\hline
> > > +opcode & Command Description \\
> > > +\hline \hline
> > > +0000h - 7FFFh   & Group administration commands    \\
> > > +\hline
> > > +8000h - FFFFh   & Reserved    \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +The \field{group_id} specifies the device group.
> > > +The \field{group_member_id} specifies the member device within the group.
> > > +See section \ref{sec:Introduction / Terminology / Device group}
> > > +for the definition of the group identifier and group member
> > > +identifier.
> > > +
> > > +The following table describes possible \field{status} values:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Status & Name & Description \\
> > > +\hline \hline
> > > +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > > +\hline
> > > +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> > > +\hline
> > > +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> > > +\hline
> > > +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> > > +\hline
> > > +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> > > +\hline
> > > +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> > > +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> > > +contents of \field{command_specific_error}
> > > +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> > > +\field{command_specific_error} value is undefined and should be ignored by the driver.
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Jason Wang]

On Thu, Aug 18, 2022 at 4:55 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
>
> On 8/18/2022 11:49 AM, Jason Wang wrote:
> > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> >>
> >> This new register will be used for querying the index of the admin
> >> virtqueue of a virtio device. To configure, reset or enable the admin
> >> virtqueue, 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 | 14 ++++++++++++++
> >>   1 file changed, 14 insertions(+)
> >>
> >> diff --git a/content.tex b/content.tex
> >> index 6ce1c07..297cb4a 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -904,6 +904,9 @@ \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 */
> > Is there a chance that we may have multiple admin virtqueues?
>
> No, same as we probably can't have multiple ctrl virtqueues..

For control virtqueue, it's technically possible but doesn't have too
much value.

But for admin virtqueue we can:

admin vq 1: group 1
admin vq 2: group 2

?

Thanks

>
>
> >
> > Thanks
> >
> >
> >>   };
> >>   \end{lstlisting}
> >>
> >> @@ -989,6 +992,10 @@ \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 administration virtqueue.
> >> +        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}
> >> @@ -1075,6 +1082,9 @@ \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, the driver MUST
> >> +configure the administration virtqueue using the value of \field{admin_queue_index}.
> >> +
> >>   \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
> >> @@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>     See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
> >>
> >>     \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
> >> +  At the moment this feature is only supported for devices using
> >> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> >> +         Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
> >> +         as the transport.
> >>
> >>   \end{description}
> >>
> >> --
> >> MST
> >>
>

Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Michael S. Tsirkin]

On Fri, Aug 19, 2022 at 08:26:50AM +0800, Jason Wang wrote:
> On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
> > > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > >
> > > > These commands are used for essential administrative and management
> > > > operations.
> > > >
> > > > Following patches will introduce an interface for submitting these
> > > > commands to the device.
> > > >
> > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > >  1 file changed, 94 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/admin.tex b/admin.tex
> > > > index 6e9434a..4840dd4 100644
> > > > --- a/admin.tex
> > > > +++ b/admin.tex
> > > > @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> > > >  \item[Parent device]
> > > >          or parent, the device controlling the group.
> > > >  \item[Member device]
> > > > -        a device within a group. Parent device itself may, or may
> > > > -       not be a member of the group.
> > > > +        a device within a group. Parent device itself is not
> > > > +       a member of the group. In the future it is envisoned that
> > > > +       new group types may be introduced where the parent
> > > > +       device is a member of the group.
> > >
> > > This part should go with patch 1?
> > >
> > > >  \item[Member identifier]
> > > >          each member has this indentifier, unique within the group
> > > >         and used to address it through the parent device.
> > > > @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> > > >         and what kind of control does the parent have.
> > > >  \item[Group identifier]
> > > >         each group has this identifier, unique within the parent device.
> > > > -       this specifies the group type and possibly selects the
> > > > -       group if multiple groups of the same type can be controlled by the same
> > > > -       parent device.
> > > > +       This specifies the group type. In the future it is
> > > > +       envisoned that new group types will be introduced where the group
> > > > +       identifier also selects the group if multiple groups of the same
> > > > +       type can be controlled by the same parent device.
> > > >  \end{description}
> > > >
> > > >  A single group type is currently specified:
> > > > @@ -46,4 +49,90 @@ \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 / Group administration commands}
> > > > +
> > > > +Group administration commands can be issued through a parent
> > > > +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_id;
> > >
> > > I wonder about the implication of this field. Does it mean:
> > >
> > > 1) a single admin virtqueue that manage two group of VFs
> > >
> > > or
> > >
> > > 2) a single admin virtqueue that manages both VFs and SFs (then
> > > 'group_type' should be better?)
> > >
> > > If it's 1, the group id seems not sufficient. If it's 2, I wonder if
> > > it scales (using a single admin virtqueue to manage both VF and SF).
> > >
> > > Thanks
> >
> > 2 at the moment.  Not sure I understand why wouldn't it scale.
> 
> Scaling as the number of member devices increases (e.g 1K or 10K). Or
> it's too early to care about those?

member devices have vqs, so up to 2^15 of these and
ring size happens to be up to 32K too. Seems to match!

> Do we gain better latency if the implementation limits the number of
> member devices per admin virtqueue?

Oh if we see that it's a problem we could have multiple admin vqs just
for performance, and allow guest to send commands on any of these.
Hmm I am not sure whether we should worry, but I think
it might become an actual issue when we have LM since that
might want to send faults from device to host.

So depends on how painful it's going to make things for us. Let me ask
you, do you have an idea about how we'll expose the multiple admin vqs
without too much pain?

Should we have a #admin vqs register? And then after regular vqs
immediately come the admin vqs? And a follow up question would be
how it will work later, if we later want another type of vq
(and I think LM might want one, for faults)?

> 
> > I don't
> > see a fundamental difference between 1 or 2 VQs. We are not going to be
> > adding 1000s of them,
> 
> Yes, but not sure if it will cause delay for e.g live migration
> downtime in the future.
> 
> Thanks

yea, maybe we'll add several of these guys.

> >
> > > > +        /* unused, reserved for future extensions */
> > > > +        u8 reserved1[12];
> > > > +        le64 group_member_id;
> > > > +        u8 command_specific_data[];
> > > > +
> > > > +        /* Device-writable part */
> > > > +        u8 status;
> > > > +        u8 command_specific_error;
> > > > +        /* unused, reserved for future extensions */
> > > > +        u8 reserved2[6];
> > > > +        u8 command_specific_result[];
> > > > +};
> > > > +\end{lstlisting}
> > > > +
> > > > +For all commands, \field{opcode}, \field{group_id} and if
> > > > +necessary \field{group_member_id} and \field{command_specific_data} are
> > > > +set by the driver, and the parent device sets \field{status} and if
> > > > +needed \field{command_specific_error} and
> > > > +\field{command_specific_result}.
> > > > +
> > > > +As a rule, any unused device-readable fields are set to zero by the driver
> > > > +and ignored by the device.  Any unused device-writeable fields are set to zero
> > > > +by the device and ignored by the driver.
> > > > +
> > > > +\field{opcode} specifies the command. The valid
> > > > +values for \field{opcode} can be found in the following table:
> > > > +
> > > > +\begin{tabular}{|l|l|}
> > > > +\hline
> > > > +opcode & Command Description \\
> > > > +\hline \hline
> > > > +0000h - 7FFFh   & Group administration commands    \\
> > > > +\hline
> > > > +8000h - FFFFh   & Reserved    \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +The \field{group_id} specifies the device group.
> > > > +The \field{group_member_id} specifies the member device within the group.
> > > > +See section \ref{sec:Introduction / Terminology / Device group}
> > > > +for the definition of the group identifier and group member
> > > > +identifier.
> > > > +
> > > > +The following table describes possible \field{status} values:
> > > > +
> > > > +\begin{tabular}{|l|l|l|}
> > > > +\hline
> > > > +Status & Name & Description \\
> > > > +\hline \hline
> > > > +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > > > +\hline
> > > > +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> > > > +\hline
> > > > +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> > > > +\hline
> > > > +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> > > > +\hline
> > > > +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> > > > +\hline
> > > > +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> > > > +\hline
> > > > +\end{tabular}
> > > > +
> > > > +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> > > > +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> > > > +contents of \field{command_specific_error}
> > > > +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> > > > +\field{command_specific_error} value is undefined and should be ignored by the driver.
> > > >
> > > > --
> > > > MST
> > > >
> >

Re: [PATCH RFC v7 4/8] Add admin_queue_index register to PCI common configuration structure

[Michael S. Tsirkin]

On Fri, Aug 19, 2022 at 08:28:41AM +0800, Jason Wang wrote:
> On Thu, Aug 18, 2022 at 4:55 PM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
> >
> >
> > On 8/18/2022 11:49 AM, Jason Wang wrote:
> > > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > >>
> > >> This new register will be used for querying the index of the admin
> > >> virtqueue of a virtio device. To configure, reset or enable the admin
> > >> virtqueue, 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 | 14 ++++++++++++++
> > >>   1 file changed, 14 insertions(+)
> > >>
> > >> diff --git a/content.tex b/content.tex
> > >> index 6ce1c07..297cb4a 100644
> > >> --- a/content.tex
> > >> +++ b/content.tex
> > >> @@ -904,6 +904,9 @@ \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 */
> > > Is there a chance that we may have multiple admin virtqueues?
> >
> > No, same as we probably can't have multiple ctrl virtqueues..
> 
> For control virtqueue, it's technically possible but doesn't have too
> much value.
> 
> But for admin virtqueue we can:
> 
> admin vq 1: group 1
> admin vq 2: group 2
> 
> ?
> 
> Thanks

I don't see how *that* will solve scalability issues, e.g. a group might have
10k devices. Just allowing submitting commands to any of multiple admin
vqs might make sense. I'm looking for a clean interface to expose that
though and don't see it yet.


> >
> >
> > >
> > > Thanks
> > >
> > >
> > >>   };
> > >>   \end{lstlisting}
> > >>
> > >> @@ -989,6 +992,10 @@ \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 administration virtqueue.
> > >> +        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}
> > >> @@ -1075,6 +1082,9 @@ \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, the driver MUST
> > >> +configure the administration virtqueue using the value of \field{admin_queue_index}.
> > >> +
> > >>   \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
> > >> @@ -6947,6 +6957,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> > >>     See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
> > >>
> > >>     \item[VIRTIO_F_ADMIN_VQ (41)] This feature indicates that an administration virtqueue is supported.
> > >> +  At the moment this feature is only supported for devices using
> > >> +  \ref{sec:Virtio Transport Options / Virtio Over PCI
> > >> +         Bus}~\nameref{sec:Virtio Transport Options / Virtio Over PCI Bus}
> > >> +         as the transport.
> > >>
> > >>   \end{description}
> > >>
> > >> --
> > >> MST
> > >>
> >

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 03:39:58PM +0200, Halil Pasic wrote:
> On Tue, 16 Aug 2022 11:48:43 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
> > > On Fri, 12 Aug 2022 13:19:20 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  content.tex | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/content.tex b/content.tex
> > > > index 76b5a28..53be680 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
> > > >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> > > >  combination.
> > > >  
> > > > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > > +
> > > > +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > +DeviceFeatures.
> > > > +
> > > > +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > > +
> > > > +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > +DriverFeatures even if offered by the device.
> > > > +  
> > > 
> > > I'm not sure I understand the intention here. I believe what we try to
> > > accomplish here is the following. The Channel I/O transport *currently*
> > > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > > the Channel I/O transport. Or am I wrong?
> > >
> > > If my assumptions are right, then the old incarnation of the spec could
> > > contradict the new incarnation of the spec. Thus I would prefer something
> > > like.  
> > 
> > Relaxing requirenents is always okay.
> 
> Are you telling me, that for instance a driver author may not rely on
> even the MUST type device normative behavior stated by the spec, because
> future incarnations of the spec could relax the requirements towards this
> particular device, for example by removing that device normative
> statement?

> I always imagined, if the spec says the device or the driver MUST
> "something", then I as the implementer of the other end (driver or
> device, can rely on that "something"). If this assumption is wrong then
> I'm have to re-examine my entire mental model of the spec.

Generally yes.  Not if we explicitly tell it not to.

Like here:
	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
	 +DriverFeatures even if offered by the device.

This makes sure that drivers do not make an assumption that
devices do not set the bit. But yes, maybe spell it out:

	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
	 +DriverFeatures even if offered by the device.
	 +Driver MUST NOT assume that device does not offer VIRTIO_F_ADMIN_VQ.
	 +In particular driver MUST NOT fail feature negotiation if
	 +device offers VIRTIO_F_ADMIN_VQ.

ok now?


> 
> > 
> > > 
> > > """
> > > Currently the following features are not supported by the Channel I/O
> > > transport:
> > > * VIRTIO_F_ADMIN_VQ
> > > """  
> > 
> > what I want to prevent is driver saying "oh device will not set ADMIN_VQ
> > so it's ok to acknowledge it if offered, it is never offered even
> > though it does not suport it".
> > because then it becomes impossible to know when actually a new driver
> > appears with actual support.
> 
> Fair point!
> 
> I would prefer a driver normative which goes like this:
> 
> """
> A driver SHOULD NOT accept features (i.e. have code that would do so if
> the feature is offered) if the feature is not supported by the driver
> (e.g. because unsupported by the transport), even if the specification
> implies that the device can not offer these features in the first place
> (e.g. because the feature is not yet supported by the transport.
> """
> 
> And a similar device normative as well, which just that it may not offer
> such features.
> 
> """
> Note: The rationale behind the [reference to the normative] is that
> while some features can not be implemented within the boundaries of the
> current virtio specification, future incarnations of the specificaton may
> make such implementations possible.  A most prominent example is optional
> features dependent on optional virtio facilities whose transport specific
> implementation is not yet specified for some transports. Should one end
> gain the ability to support these features, the old implementation which
> made the assumption that the other end will make sure these features are
> not negotiated would end up negotiating something it can't actually
> support.
> """
> 
> 
> 
> > 
> > So, Maybe just add text
> > 
> > Note: future versions of this specification will allow setting ADMIN_VQ
> > for driver and device. Device MUST NOT assume driver does not
> > acknowledge ADMIN_VQ if offered.
> 
> I would not lean out of the window and promise something with regards to
> future versions of this spec.
> 
> > 
> > And similarly for drivers:
> > 
> > Note: future versions of this specification will allow setting ADMIN_VQ
> > for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.
> > 
> 
> I think we can then make a note which references the generic normative
> for each feature affected where it suits us.
> 
> > > 
> > > If we want, we can also state what needs to be done in general when
> > > features are unsupported by the transport. And yes, that normative
> > > material in my opinion.
> > > 
> > > Regards,
> > > Halil  
> > 
> > 
> > Are there other examples? I want to call out the list explicitly because
> > it is so easy to enable an extra feature by mistake.
> > 
> 
> I don't think CCW supports the shared memory yet... But I may be wrong.
> 
> > 
> > > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > > >  
> > > >  The device's configuration space is located in host memory.  
> > 

[virtio-comment] Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Jason Wang]

On Fri, Aug 19, 2022 at 11:28 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Aug 19, 2022 at 08:26:50AM +0800, Jason Wang wrote:
> > On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
> > > > On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > From: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > >
> > > > > These commands are used for essential administrative and management
> > > > > operations.
> > > > >
> > > > > Following patches will introduce an interface for submitting these
> > > > > commands to the device.
> > > > >
> > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > >  admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  1 file changed, 94 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/admin.tex b/admin.tex
> > > > > index 6e9434a..4840dd4 100644
> > > > > --- a/admin.tex
> > > > > +++ b/admin.tex
> > > > > @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> > > > >  \item[Parent device]
> > > > >          or parent, the device controlling the group.
> > > > >  \item[Member device]
> > > > > -        a device within a group. Parent device itself may, or may
> > > > > -       not be a member of the group.
> > > > > +        a device within a group. Parent device itself is not
> > > > > +       a member of the group. In the future it is envisoned that
> > > > > +       new group types may be introduced where the parent
> > > > > +       device is a member of the group.
> > > >
> > > > This part should go with patch 1?
> > > >
> > > > >  \item[Member identifier]
> > > > >          each member has this indentifier, unique within the group
> > > > >         and used to address it through the parent device.
> > > > > @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> > > > >         and what kind of control does the parent have.
> > > > >  \item[Group identifier]
> > > > >         each group has this identifier, unique within the parent device.
> > > > > -       this specifies the group type and possibly selects the
> > > > > -       group if multiple groups of the same type can be controlled by the same
> > > > > -       parent device.
> > > > > +       This specifies the group type. In the future it is
> > > > > +       envisoned that new group types will be introduced where the group
> > > > > +       identifier also selects the group if multiple groups of the same
> > > > > +       type can be controlled by the same parent device.
> > > > >  \end{description}
> > > > >
> > > > >  A single group type is currently specified:
> > > > > @@ -46,4 +49,90 @@ \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 / Group administration commands}
> > > > > +
> > > > > +Group administration commands can be issued through a parent
> > > > > +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_id;
> > > >
> > > > I wonder about the implication of this field. Does it mean:
> > > >
> > > > 1) a single admin virtqueue that manage two group of VFs
> > > >
> > > > or
> > > >
> > > > 2) a single admin virtqueue that manages both VFs and SFs (then
> > > > 'group_type' should be better?)
> > > >
> > > > If it's 1, the group id seems not sufficient. If it's 2, I wonder if
> > > > it scales (using a single admin virtqueue to manage both VF and SF).
> > > >
> > > > Thanks
> > >
> > > 2 at the moment.  Not sure I understand why wouldn't it scale.
> >
> > Scaling as the number of member devices increases (e.g 1K or 10K). Or
> > it's too early to care about those?
>
> member devices have vqs, so up to 2^15 of these and
> ring size happens to be up to 32K too. Seems to match!
>
> > Do we gain better latency if the implementation limits the number of
> > member devices per admin virtqueue?
>
> Oh if we see that it's a problem we could have multiple admin vqs just
> for performance, and allow guest to send commands on any of these.
> Hmm I am not sure whether we should worry, but I think
> it might become an actual issue when we have LM since that
> might want to send faults from device to host.
>
> So depends on how painful it's going to make things for us. Let me ask
> you, do you have an idea about how we'll expose the multiple admin vqs
> without too much pain?

Haven't thought about this. Just want to see if it's an actual issue.

>
> Should we have a #admin vqs register? And then after regular vqs
> immediately come the admin vqs?

Probably, but then it looks better to have a dedicated capability.

>  And a follow up question would be
> how it will work later, if we later want another type of vq
> (and I think LM might want one, for faults)?

Another capability?

Thanks

>
> >
> > > I don't
> > > see a fundamental difference between 1 or 2 VQs. We are not going to be
> > > adding 1000s of them,
> >
> > Yes, but not sure if it will cause delay for e.g live migration
> > downtime in the future.
> >
> > Thanks
>
> yea, maybe we'll add several of these guys.
>
> > >
> > > > > +        /* unused, reserved for future extensions */
> > > > > +        u8 reserved1[12];
> > > > > +        le64 group_member_id;
> > > > > +        u8 command_specific_data[];
> > > > > +
> > > > > +        /* Device-writable part */
> > > > > +        u8 status;
> > > > > +        u8 command_specific_error;
> > > > > +        /* unused, reserved for future extensions */
> > > > > +        u8 reserved2[6];
> > > > > +        u8 command_specific_result[];
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +For all commands, \field{opcode}, \field{group_id} and if
> > > > > +necessary \field{group_member_id} and \field{command_specific_data} are
> > > > > +set by the driver, and the parent device sets \field{status} and if
> > > > > +needed \field{command_specific_error} and
> > > > > +\field{command_specific_result}.
> > > > > +
> > > > > +As a rule, any unused device-readable fields are set to zero by the driver
> > > > > +and ignored by the device.  Any unused device-writeable fields are set to zero
> > > > > +by the device and ignored by the driver.
> > > > > +
> > > > > +\field{opcode} specifies the command. The valid
> > > > > +values for \field{opcode} can be found in the following table:
> > > > > +
> > > > > +\begin{tabular}{|l|l|}
> > > > > +\hline
> > > > > +opcode & Command Description \\
> > > > > +\hline \hline
> > > > > +0000h - 7FFFh   & Group administration commands    \\
> > > > > +\hline
> > > > > +8000h - FFFFh   & Reserved    \\
> > > > > +\hline
> > > > > +\end{tabular}
> > > > > +
> > > > > +The \field{group_id} specifies the device group.
> > > > > +The \field{group_member_id} specifies the member device within the group.
> > > > > +See section \ref{sec:Introduction / Terminology / Device group}
> > > > > +for the definition of the group identifier and group member
> > > > > +identifier.
> > > > > +
> > > > > +The following table describes possible \field{status} values:
> > > > > +
> > > > > +\begin{tabular}{|l|l|l|}
> > > > > +\hline
> > > > > +Status & Name & Description \\
> > > > > +\hline \hline
> > > > > +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> > > > > +\hline
> > > > > +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> > > > > +\hline
> > > > > +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> > > > > +\hline
> > > > > +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> > > > > +\hline
> > > > > +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> > > > > +\hline
> > > > > +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> > > > > +\hline
> > > > > +\end{tabular}
> > > > > +
> > > > > +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> > > > > +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> > > > > +contents of \field{command_specific_error}
> > > > > +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> > > > > +\field{command_specific_error} value is undefined and should be ignored by the driver.
> > > > >
> > > > > --
> > > > > 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 RFC v7 2/8] Introduce group administration commands

[Max Gurtovoy]

On 8/19/2022 7:37 AM, Jason Wang wrote:
> On Fri, Aug 19, 2022 at 11:28 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Fri, Aug 19, 2022 at 08:26:50AM +0800, Jason Wang wrote:
>>> On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
>>>>> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>>
>>>>>> These commands are used for essential administrative and management
>>>>>> operations.
>>>>>>
>>>>>> Following patches will introduce an interface for submitting these
>>>>>> commands to the device.
>>>>>>
>>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>> ---
>>>>>>   admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>   1 file changed, 94 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/admin.tex b/admin.tex
>>>>>> index 6e9434a..4840dd4 100644
>>>>>> --- a/admin.tex
>>>>>> +++ b/admin.tex
>>>>>> @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>>>>>>   \item[Parent device]
>>>>>>           or parent, the device controlling the group.
>>>>>>   \item[Member device]
>>>>>> -        a device within a group. Parent device itself may, or may
>>>>>> -       not be a member of the group.
>>>>>> +        a device within a group. Parent device itself is not
>>>>>> +       a member of the group. In the future it is envisoned that
>>>>>> +       new group types may be introduced where the parent
>>>>>> +       device is a member of the group.
>>>>> This part should go with patch 1?
>>>>>
>>>>>>   \item[Member identifier]
>>>>>>           each member has this indentifier, unique within the group
>>>>>>          and used to address it through the parent device.
>>>>>> @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>>>>>>          and what kind of control does the parent have.
>>>>>>   \item[Group identifier]
>>>>>>          each group has this identifier, unique within the parent device.
>>>>>> -       this specifies the group type and possibly selects the
>>>>>> -       group if multiple groups of the same type can be controlled by the same
>>>>>> -       parent device.
>>>>>> +       This specifies the group type. In the future it is
>>>>>> +       envisoned that new group types will be introduced where the group
>>>>>> +       identifier also selects the group if multiple groups of the same
>>>>>> +       type can be controlled by the same parent device.
>>>>>>   \end{description}
>>>>>>
>>>>>>   A single group type is currently specified:
>>>>>> @@ -46,4 +49,90 @@ \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 / Group administration commands}
>>>>>> +
>>>>>> +Group administration commands can be issued through a parent
>>>>>> +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_id;
>>>>> I wonder about the implication of this field. Does it mean:
>>>>>
>>>>> 1) a single admin virtqueue that manage two group of VFs
>>>>>
>>>>> or
>>>>>
>>>>> 2) a single admin virtqueue that manages both VFs and SFs (then
>>>>> 'group_type' should be better?)
>>>>>
>>>>> If it's 1, the group id seems not sufficient. If it's 2, I wonder if
>>>>> it scales (using a single admin virtqueue to manage both VF and SF).
>>>>>
>>>>> Thanks
>>>> 2 at the moment.  Not sure I understand why wouldn't it scale.
>>> Scaling as the number of member devices increases (e.g 1K or 10K). Or
>>> it's too early to care about those?
>> member devices have vqs, so up to 2^15 of these and
>> ring size happens to be up to 32K too. Seems to match!
>>
>>> Do we gain better latency if the implementation limits the number of
>>> member devices per admin virtqueue?
>> Oh if we see that it's a problem we could have multiple admin vqs just
>> for performance, and allow guest to send commands on any of these.
>> Hmm I am not sure whether we should worry, but I think
>> it might become an actual issue when we have LM since that
>> might want to send faults from device to host.
>>
>> So depends on how painful it's going to make things for us. Let me ask
>> you, do you have an idea about how we'll expose the multiple admin vqs
>> without too much pain?
> Haven't thought about this. Just want to see if it's an actual issue.

In LM you usually migrate 1 VM at a time. You don't migrate 1000 VMs at 
once.

AQ can have a depth of 32-128 and I don't see any issue with having 10k 
devices.

It's a queue for admin/mgmt/control path and having multiple of those 
will just complicate the solution.

>
>> Should we have a #admin vqs register? And then after regular vqs
>> immediately come the admin vqs?
> Probably, but then it looks better to have a dedicated capability.
>
>>   And a follow up question would be
>> how it will work later, if we later want another type of vq
>> (and I think LM might want one, for faults)?
> Another capability?
>
> Thanks
>
>>>> I don't
>>>> see a fundamental difference between 1 or 2 VQs. We are not going to be
>>>> adding 1000s of them,
>>> Yes, but not sure if it will cause delay for e.g live migration
>>> downtime in the future.
>>>
>>> Thanks
>> yea, maybe we'll add several of these guys.
>>
>>>>>> +        /* unused, reserved for future extensions */
>>>>>> +        u8 reserved1[12];
>>>>>> +        le64 group_member_id;
>>>>>> +        u8 command_specific_data[];
>>>>>> +
>>>>>> +        /* Device-writable part */
>>>>>> +        u8 status;
>>>>>> +        u8 command_specific_error;
>>>>>> +        /* unused, reserved for future extensions */
>>>>>> +        u8 reserved2[6];
>>>>>> +        u8 command_specific_result[];
>>>>>> +};
>>>>>> +\end{lstlisting}
>>>>>> +
>>>>>> +For all commands, \field{opcode}, \field{group_id} and if
>>>>>> +necessary \field{group_member_id} and \field{command_specific_data} are
>>>>>> +set by the driver, and the parent device sets \field{status} and if
>>>>>> +needed \field{command_specific_error} and
>>>>>> +\field{command_specific_result}.
>>>>>> +
>>>>>> +As a rule, any unused device-readable fields are set to zero by the driver
>>>>>> +and ignored by the device.  Any unused device-writeable fields are set to zero
>>>>>> +by the device and ignored by the driver.
>>>>>> +
>>>>>> +\field{opcode} specifies the command. The valid
>>>>>> +values for \field{opcode} can be found in the following table:
>>>>>> +
>>>>>> +\begin{tabular}{|l|l|}
>>>>>> +\hline
>>>>>> +opcode & Command Description \\
>>>>>> +\hline \hline
>>>>>> +0000h - 7FFFh   & Group administration commands    \\
>>>>>> +\hline
>>>>>> +8000h - FFFFh   & Reserved    \\
>>>>>> +\hline
>>>>>> +\end{tabular}
>>>>>> +
>>>>>> +The \field{group_id} specifies the device group.
>>>>>> +The \field{group_member_id} specifies the member device within the group.
>>>>>> +See section \ref{sec:Introduction / Terminology / Device group}
>>>>>> +for the definition of the group identifier and group member
>>>>>> +identifier.
>>>>>> +
>>>>>> +The following table describes possible \field{status} values:
>>>>>> +
>>>>>> +\begin{tabular}{|l|l|l|}
>>>>>> +\hline
>>>>>> +Status & Name & Description \\
>>>>>> +\hline \hline
>>>>>> +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
>>>>>> +\hline
>>>>>> +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
>>>>>> +\hline
>>>>>> +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
>>>>>> +\hline
>>>>>> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
>>>>>> +\hline
>>>>>> +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
>>>>>> +\hline
>>>>>> +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
>>>>>> +\hline
>>>>>> +\end{tabular}
>>>>>> +
>>>>>> +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
>>>>>> +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
>>>>>> +contents of \field{command_specific_error}
>>>>>> +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
>>>>>> +\field{command_specific_error} value is undefined and should be ignored by the driver.
>>>>>>
>>>>>> --
>>>>>> MST
>>>>>>

Re: [virtio-comment] Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Jason Wang]

On Sat, Aug 20, 2022 at 7:41 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>
>
> On 8/19/2022 7:37 AM, Jason Wang wrote:
> > On Fri, Aug 19, 2022 at 11:28 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Fri, Aug 19, 2022 at 08:26:50AM +0800, Jason Wang wrote:
> >>> On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
> >>>>> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
> >>>>>>
> >>>>>> These commands are used for essential administrative and management
> >>>>>> operations.
> >>>>>>
> >>>>>> Following patches will introduce an interface for submitting these
> >>>>>> commands to the device.
> >>>>>>
> >>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >>>>>> ---
> >>>>>>   admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>>>>>   1 file changed, 94 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/admin.tex b/admin.tex
> >>>>>> index 6e9434a..4840dd4 100644
> >>>>>> --- a/admin.tex
> >>>>>> +++ b/admin.tex
> >>>>>> @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> >>>>>>   \item[Parent device]
> >>>>>>           or parent, the device controlling the group.
> >>>>>>   \item[Member device]
> >>>>>> -        a device within a group. Parent device itself may, or may
> >>>>>> -       not be a member of the group.
> >>>>>> +        a device within a group. Parent device itself is not
> >>>>>> +       a member of the group. In the future it is envisoned that
> >>>>>> +       new group types may be introduced where the parent
> >>>>>> +       device is a member of the group.
> >>>>> This part should go with patch 1?
> >>>>>
> >>>>>>   \item[Member identifier]
> >>>>>>           each member has this indentifier, unique within the group
> >>>>>>          and used to address it through the parent device.
> >>>>>> @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
> >>>>>>          and what kind of control does the parent have.
> >>>>>>   \item[Group identifier]
> >>>>>>          each group has this identifier, unique within the parent device.
> >>>>>> -       this specifies the group type and possibly selects the
> >>>>>> -       group if multiple groups of the same type can be controlled by the same
> >>>>>> -       parent device.
> >>>>>> +       This specifies the group type. In the future it is
> >>>>>> +       envisoned that new group types will be introduced where the group
> >>>>>> +       identifier also selects the group if multiple groups of the same
> >>>>>> +       type can be controlled by the same parent device.
> >>>>>>   \end{description}
> >>>>>>
> >>>>>>   A single group type is currently specified:
> >>>>>> @@ -46,4 +49,90 @@ \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 / Group administration commands}
> >>>>>> +
> >>>>>> +Group administration commands can be issued through a parent
> >>>>>> +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_id;
> >>>>> I wonder about the implication of this field. Does it mean:
> >>>>>
> >>>>> 1) a single admin virtqueue that manage two group of VFs
> >>>>>
> >>>>> or
> >>>>>
> >>>>> 2) a single admin virtqueue that manages both VFs and SFs (then
> >>>>> 'group_type' should be better?)
> >>>>>
> >>>>> If it's 1, the group id seems not sufficient. If it's 2, I wonder if
> >>>>> it scales (using a single admin virtqueue to manage both VF and SF).
> >>>>>
> >>>>> Thanks
> >>>> 2 at the moment.  Not sure I understand why wouldn't it scale.
> >>> Scaling as the number of member devices increases (e.g 1K or 10K). Or
> >>> it's too early to care about those?
> >> member devices have vqs, so up to 2^15 of these and
> >> ring size happens to be up to 32K too. Seems to match!
> >>
> >>> Do we gain better latency if the implementation limits the number of
> >>> member devices per admin virtqueue?
> >> Oh if we see that it's a problem we could have multiple admin vqs just
> >> for performance, and allow guest to send commands on any of these.
> >> Hmm I am not sure whether we should worry, but I think
> >> it might become an actual issue when we have LM since that
> >> might want to send faults from device to host.
> >>
> >> So depends on how painful it's going to make things for us. Let me ask
> >> you, do you have an idea about how we'll expose the multiple admin vqs
> >> without too much pain?
> > Haven't thought about this. Just want to see if it's an actual issue.
>
> In LM you usually migrate 1 VM at a time.

This only works if we can find a way to mandate it in the spec (which
I'm not sure it's possible).

> You don't migrate 1000 VMs at
> once.

Probably, but we may have other transactions in the meantime. They
need to compete with a single queue? Or maybe migration should go with
a dedicated virtqueue.

>
> AQ can have a depth of 32-128 and I don't see any issue with having 10k
> devices.

It depends on the API, e.g what does the dirty page tracking looks
like? Michael proposes a device page fault with partial order, if it's
the API can we end up with more than one device #PF?

Thanks

>
> It's a queue for admin/mgmt/control path and having multiple of those
> will just complicate the solution.
>
> >
> >> Should we have a #admin vqs register? And then after regular vqs
> >> immediately come the admin vqs?
> > Probably, but then it looks better to have a dedicated capability.
> >
> >>   And a follow up question would be
> >> how it will work later, if we later want another type of vq
> >> (and I think LM might want one, for faults)?
> > Another capability?
> >
> > Thanks
> >
> >>>> I don't
> >>>> see a fundamental difference between 1 or 2 VQs. We are not going to be
> >>>> adding 1000s of them,
> >>> Yes, but not sure if it will cause delay for e.g live migration
> >>> downtime in the future.
> >>>
> >>> Thanks
> >> yea, maybe we'll add several of these guys.
> >>
> >>>>>> +        /* unused, reserved for future extensions */
> >>>>>> +        u8 reserved1[12];
> >>>>>> +        le64 group_member_id;
> >>>>>> +        u8 command_specific_data[];
> >>>>>> +
> >>>>>> +        /* Device-writable part */
> >>>>>> +        u8 status;
> >>>>>> +        u8 command_specific_error;
> >>>>>> +        /* unused, reserved for future extensions */
> >>>>>> +        u8 reserved2[6];
> >>>>>> +        u8 command_specific_result[];
> >>>>>> +};
> >>>>>> +\end{lstlisting}
> >>>>>> +
> >>>>>> +For all commands, \field{opcode}, \field{group_id} and if
> >>>>>> +necessary \field{group_member_id} and \field{command_specific_data} are
> >>>>>> +set by the driver, and the parent device sets \field{status} and if
> >>>>>> +needed \field{command_specific_error} and
> >>>>>> +\field{command_specific_result}.
> >>>>>> +
> >>>>>> +As a rule, any unused device-readable fields are set to zero by the driver
> >>>>>> +and ignored by the device.  Any unused device-writeable fields are set to zero
> >>>>>> +by the device and ignored by the driver.
> >>>>>> +
> >>>>>> +\field{opcode} specifies the command. The valid
> >>>>>> +values for \field{opcode} can be found in the following table:
> >>>>>> +
> >>>>>> +\begin{tabular}{|l|l|}
> >>>>>> +\hline
> >>>>>> +opcode & Command Description \\
> >>>>>> +\hline \hline
> >>>>>> +0000h - 7FFFh   & Group administration commands    \\
> >>>>>> +\hline
> >>>>>> +8000h - FFFFh   & Reserved    \\
> >>>>>> +\hline
> >>>>>> +\end{tabular}
> >>>>>> +
> >>>>>> +The \field{group_id} specifies the device group.
> >>>>>> +The \field{group_member_id} specifies the member device within the group.
> >>>>>> +See section \ref{sec:Introduction / Terminology / Device group}
> >>>>>> +for the definition of the group identifier and group member
> >>>>>> +identifier.
> >>>>>> +
> >>>>>> +The following table describes possible \field{status} values:
> >>>>>> +
> >>>>>> +\begin{tabular}{|l|l|l|}
> >>>>>> +\hline
> >>>>>> +Status & Name & Description \\
> >>>>>> +\hline \hline
> >>>>>> +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
> >>>>>> +\hline
> >>>>>> +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
> >>>>>> +\hline
> >>>>>> +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
> >>>>>> +\hline
> >>>>>> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
> >>>>>> +\hline
> >>>>>> +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
> >>>>>> +\hline
> >>>>>> +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
> >>>>>> +\hline
> >>>>>> +\end{tabular}
> >>>>>> +
> >>>>>> +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
> >>>>>> +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
> >>>>>> +contents of \field{command_specific_error}
> >>>>>> +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
> >>>>>> +\field{command_specific_error} value is undefined and should be ignored by the driver.
> >>>>>>
> >>>>>> --
> >>>>>> 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-dev] Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Halil Pasic]

On Thu, 18 Aug 2022 23:57:39 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > > > I'm not sure I understand the intention here. I believe what we try to
> > > > accomplish here is the following. The Channel I/O transport *currently*
> > > > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > > > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > > > the Channel I/O transport. Or am I wrong?
> > > >
> > > > If my assumptions are right, then the old incarnation of the spec could
> > > > contradict the new incarnation of the spec. Thus I would prefer something
> > > > like.    
> > > 
> > > Relaxing requirenents is always okay.  
> > 
> > Are you telling me, that for instance a driver author may not rely on
> > even the MUST type device normative behavior stated by the spec, because
> > future incarnations of the spec could relax the requirements towards this
> > particular device, for example by removing that device normative
> > statement?  
> 
> > I always imagined, if the spec says the device or the driver MUST
> > "something", then I as the implementer of the other end (driver or
> > device, can rely on that "something"). If this assumption is wrong then
> > I'm have to re-examine my entire mental model of the spec.  
> 
> Generally yes.  Not if we explicitly tell it not to.
> 
> Like here:
> 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> 	 +DriverFeatures even if offered by the device.
> 
> This makes sure that drivers do not make an assumption that
> devices do not set the bit. But yes, maybe spell it out:
> 
> 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> 	 +DriverFeatures even if offered by the device.
> 	 +Driver MUST NOT assume that device does not offer VIRTIO_F_ADMIN_VQ.
> 	 +In particular driver MUST NOT fail feature negotiation if
> 	 +device offers VIRTIO_F_ADMIN_VQ.
> 
> ok now?

Sorry, it still does not work for me. But I may be wrong. My problem
is that what we mean is the following:

If the driver (where driver includes both the transport part and the
transport agnostic part) does not support VIRTIO_F_ADMIN_VQ then it must
not set VIRTIO_F_ADMIN_VQ. And any reasoning along the lines "hey the
device was not supposed to offer that bit in the first place" is
misguided.

The crucial part here is that the MUST NOT accept VIRTIO_F_ADMIN_VQ
partee is only applicable if the driver does not support
VIRTIO_F_ADMIN_VQ. That is, if we happen to extend the Channel I/O transport, and we
decide to implement VIRTIO_F_ADMIN_VQ for the over Channel I/O devices,
that MUST NOT accept does not get in the way.

My problem with your proposal is, that the MUST NOT is not guarded by a
proper precondition (it is a prohibition that does not allow for any
exceptions).

I would very much like Conny to chime in on this.

Regards,
Halil

Re: [virtio-comment] Re: [PATCH RFC v7 2/8] Introduce group administration commands

[Max Gurtovoy]

On 8/23/2022 6:32 AM, Jason Wang wrote:
> On Sat, Aug 20, 2022 at 7:41 AM Max Gurtovoy <mgurtovoy@nvidia.com> wrote:
>>
>> On 8/19/2022 7:37 AM, Jason Wang wrote:
>>> On Fri, Aug 19, 2022 at 11:28 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Fri, Aug 19, 2022 at 08:26:50AM +0800, Jason Wang wrote:
>>>>> On Thu, Aug 18, 2022 at 4:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Thu, Aug 18, 2022 at 04:46:45PM +0800, Jason Wang wrote:
>>>>>>> On Sat, Aug 13, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>> From: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>>>>
>>>>>>>> These commands are used for essential administrative and management
>>>>>>>> operations.
>>>>>>>>
>>>>>>>> Following patches will introduce an interface for submitting these
>>>>>>>> commands to the device.
>>>>>>>>
>>>>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>>>>> ---
>>>>>>>>    admin.tex | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>>>>>>    1 file changed, 94 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/admin.tex b/admin.tex
>>>>>>>> index 6e9434a..4840dd4 100644
>>>>>>>> --- a/admin.tex
>>>>>>>> +++ b/admin.tex
>>>>>>>> @@ -9,8 +9,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>>>>>>>>    \item[Parent device]
>>>>>>>>            or parent, the device controlling the group.
>>>>>>>>    \item[Member device]
>>>>>>>> -        a device within a group. Parent device itself may, or may
>>>>>>>> -       not be a member of the group.
>>>>>>>> +        a device within a group. Parent device itself is not
>>>>>>>> +       a member of the group. In the future it is envisoned that
>>>>>>>> +       new group types may be introduced where the parent
>>>>>>>> +       device is a member of the group.
>>>>>>> This part should go with patch 1?
>>>>>>>
>>>>>>>>    \item[Member identifier]
>>>>>>>>            each member has this indentifier, unique within the group
>>>>>>>>           and used to address it through the parent device.
>>>>>>>> @@ -20,9 +22,10 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
>>>>>>>>           and what kind of control does the parent have.
>>>>>>>>    \item[Group identifier]
>>>>>>>>           each group has this identifier, unique within the parent device.
>>>>>>>> -       this specifies the group type and possibly selects the
>>>>>>>> -       group if multiple groups of the same type can be controlled by the same
>>>>>>>> -       parent device.
>>>>>>>> +       This specifies the group type. In the future it is
>>>>>>>> +       envisoned that new group types will be introduced where the group
>>>>>>>> +       identifier also selects the group if multiple groups of the same
>>>>>>>> +       type can be controlled by the same parent device.
>>>>>>>>    \end{description}
>>>>>>>>
>>>>>>>>    A single group type is currently specified:
>>>>>>>> @@ -46,4 +49,90 @@ \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 / Group administration commands}
>>>>>>>> +
>>>>>>>> +Group administration commands can be issued through a parent
>>>>>>>> +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_id;
>>>>>>> I wonder about the implication of this field. Does it mean:
>>>>>>>
>>>>>>> 1) a single admin virtqueue that manage two group of VFs
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>> 2) a single admin virtqueue that manages both VFs and SFs (then
>>>>>>> 'group_type' should be better?)
>>>>>>>
>>>>>>> If it's 1, the group id seems not sufficient. If it's 2, I wonder if
>>>>>>> it scales (using a single admin virtqueue to manage both VF and SF).
>>>>>>>
>>>>>>> Thanks
>>>>>> 2 at the moment.  Not sure I understand why wouldn't it scale.
>>>>> Scaling as the number of member devices increases (e.g 1K or 10K). Or
>>>>> it's too early to care about those?
>>>> member devices have vqs, so up to 2^15 of these and
>>>> ring size happens to be up to 32K too. Seems to match!
>>>>
>>>>> Do we gain better latency if the implementation limits the number of
>>>>> member devices per admin virtqueue?
>>>> Oh if we see that it's a problem we could have multiple admin vqs just
>>>> for performance, and allow guest to send commands on any of these.
>>>> Hmm I am not sure whether we should worry, but I think
>>>> it might become an actual issue when we have LM since that
>>>> might want to send faults from device to host.
>>>>
>>>> So depends on how painful it's going to make things for us. Let me ask
>>>> you, do you have an idea about how we'll expose the multiple admin vqs
>>>> without too much pain?
>>> Haven't thought about this. Just want to see if it's an actual issue.
>> In LM you usually migrate 1 VM at a time.
> This only works if we can find a way to mandate it in the spec (which
> I'm not sure it's possible).

what only works ?

I don't understand.

>> You don't migrate 1000 VMs at
>> once.
> Probably, but we may have other transactions in the meantime. They
> need to compete with a single queue? Or maybe migration should go with
> a dedicated virtqueue.

MQ is needed for performance. By definition, admin queue is for 
administration and control.

Migration should use the AQ and not dedicated queue. I prefer not 
creating a queue per feature.

It doesn't scale and complicated.

>
>> AQ can have a depth of 32-128 and I don't see any issue with having 10k
>> devices.
> It depends on the API, e.g what does the dirty page tracking looks
> like? Michael proposes a device page fault with partial order, if it's
> the API can we end up with more than one device #PF?

What dirty pages has to do with AQ and device group patches at this stage ?

Didn't we agree to focus on this without raising unrelated topics to 
this particular submission ?

>
> Thanks
>
>> It's a queue for admin/mgmt/control path and having multiple of those
>> will just complicate the solution.
>>
>>>> Should we have a #admin vqs register? And then after regular vqs
>>>> immediately come the admin vqs?
>>> Probably, but then it looks better to have a dedicated capability.
>>>
>>>>    And a follow up question would be
>>>> how it will work later, if we later want another type of vq
>>>> (and I think LM might want one, for faults)?
>>> Another capability?
>>>
>>> Thanks
>>>
>>>>>> I don't
>>>>>> see a fundamental difference between 1 or 2 VQs. We are not going to be
>>>>>> adding 1000s of them,
>>>>> Yes, but not sure if it will cause delay for e.g live migration
>>>>> downtime in the future.
>>>>>
>>>>> Thanks
>>>> yea, maybe we'll add several of these guys.
>>>>
>>>>>>>> +        /* unused, reserved for future extensions */
>>>>>>>> +        u8 reserved1[12];
>>>>>>>> +        le64 group_member_id;
>>>>>>>> +        u8 command_specific_data[];
>>>>>>>> +
>>>>>>>> +        /* Device-writable part */
>>>>>>>> +        u8 status;
>>>>>>>> +        u8 command_specific_error;
>>>>>>>> +        /* unused, reserved for future extensions */
>>>>>>>> +        u8 reserved2[6];
>>>>>>>> +        u8 command_specific_result[];
>>>>>>>> +};
>>>>>>>> +\end{lstlisting}
>>>>>>>> +
>>>>>>>> +For all commands, \field{opcode}, \field{group_id} and if
>>>>>>>> +necessary \field{group_member_id} and \field{command_specific_data} are
>>>>>>>> +set by the driver, and the parent device sets \field{status} and if
>>>>>>>> +needed \field{command_specific_error} and
>>>>>>>> +\field{command_specific_result}.
>>>>>>>> +
>>>>>>>> +As a rule, any unused device-readable fields are set to zero by the driver
>>>>>>>> +and ignored by the device.  Any unused device-writeable fields are set to zero
>>>>>>>> +by the device and ignored by the driver.
>>>>>>>> +
>>>>>>>> +\field{opcode} specifies the command. The valid
>>>>>>>> +values for \field{opcode} can be found in the following table:
>>>>>>>> +
>>>>>>>> +\begin{tabular}{|l|l|}
>>>>>>>> +\hline
>>>>>>>> +opcode & Command Description \\
>>>>>>>> +\hline \hline
>>>>>>>> +0000h - 7FFFh   & Group administration commands    \\
>>>>>>>> +\hline
>>>>>>>> +8000h - FFFFh   & Reserved    \\
>>>>>>>> +\hline
>>>>>>>> +\end{tabular}
>>>>>>>> +
>>>>>>>> +The \field{group_id} specifies the device group.
>>>>>>>> +The \field{group_member_id} specifies the member device within the group.
>>>>>>>> +See section \ref{sec:Introduction / Terminology / Device group}
>>>>>>>> +for the definition of the group identifier and group member
>>>>>>>> +identifier.
>>>>>>>> +
>>>>>>>> +The following table describes possible \field{status} values:
>>>>>>>> +
>>>>>>>> +\begin{tabular}{|l|l|l|}
>>>>>>>> +\hline
>>>>>>>> +Status & Name & Description \\
>>>>>>>> +\hline \hline
>>>>>>>> +00h   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
>>>>>>>> +\hline
>>>>>>>> +01h   & VIRTIO_ADMIN_STATUS_CS_ERR    & command specific error  \\
>>>>>>>> +\hline
>>>>>>>> +02h   & VIRTIO_ADMIN_STATUS_OPCODE_UNSUPPORTED    & unsupported or invalid \field{opcode}  \\
>>>>>>>> +\hline
>>>>>>>> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field within command specific data  \\
>>>>>>>> +\hline
>>>>>>>> +04h   & VIRTIO_ADMIN_STATUS_INVALID_GROUP    & invalid group identifier was set  \\
>>>>>>>> +\hline
>>>>>>>> +05h   & VIRTIO_ADMIN_STATUS_INVALID_MEM    & invalid group member identifier was set  \\
>>>>>>>> +\hline
>>>>>>>> +\end{tabular}
>>>>>>>> +
>>>>>>>> +The \field{command_specific_error} should be inspected by the driver only if \field{status} is set to
>>>>>>>> +VIRTIO_ADMIN_STATUS_CS_ERR by the device. In this case, the
>>>>>>>> +contents of \field{command_specific_error}
>>>>>>>> +holds the command specific error. If \field{status} is not set to VIRTIO_ADMIN_STATUS_CS_ERR, the
>>>>>>>> +\field{command_specific_error} value is undefined and should be ignored by the driver.
>>>>>>>>
>>>>>>>> --
>>>>>>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610571694%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=FuZ%2B2lkRF4I6j6LwpHNf1FeRlu9g%2BBK%2Bef%2B%2BhRD0Ifs%3D&amp;reserved=0
>> Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610571694%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=9fDN%2FW7gNzeCFOBXqLO6OU6yA69BlF7TTbWfO%2BJzSYc%3D&amp;reserved=0
>> List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610571694%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Py0e2vOjUotnPVPiquEw1Hf13ZU%2F4jYJsG8ZKRfZ%2FD4%3D&amp;reserved=0
>> Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610571694%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=LhSfZh2bfd7pDyftQjHiBf2rkyWxVxorRZUQa6%2B8i4I%3D&amp;reserved=0
>> Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610571694%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nEMRhuQxvwBtEhe1RMIDxcOl9mGFL%2FO5anFlbpb12yU%3D&amp;reserved=0
>>
>
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610727260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=e46yx7FUWbhrrmgwHX8IxApsD%2BHbQTxBiakAFl0Af24%3D&amp;reserved=0
> Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610727260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=qHNRlKme%2FJeOzPU%2BAghHymr72YaPqRxhLpdzZZTNSW8%3D&amp;reserved=0
> List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610727260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=T9FQLS8lBQ%2FE1t9RWYXKpmzry3yjgFJJwgvwU4F338M%3D&amp;reserved=0
> Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610727260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=bIRyJuVxJGYjuKLiCSSREGtvfgOPOhjO2DlITRcY96g%3D&amp;reserved=0
> Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=05%7C01%7Cmgurtovoy%40nvidia.com%7C02d3a6db03a244f4d21308da84b817fb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637968223610727260%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Y0zMwXPz%2BCUr3%2BkMuPJTG4dw1euP1680sXSihZHN07U%3D&amp;reserved=0
>

Re: [virtio-dev] Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

On Wed, Aug 24, 2022 at 01:45:19AM +0200, Halil Pasic wrote:
> On Thu, 18 Aug 2022 23:57:39 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > > I'm not sure I understand the intention here. I believe what we try to
> > > > > accomplish here is the following. The Channel I/O transport *currently*
> > > > > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > > > > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > > > > the Channel I/O transport. Or am I wrong?
> > > > >
> > > > > If my assumptions are right, then the old incarnation of the spec could
> > > > > contradict the new incarnation of the spec. Thus I would prefer something
> > > > > like.    
> > > > 
> > > > Relaxing requirenents is always okay.  
> > > 
> > > Are you telling me, that for instance a driver author may not rely on
> > > even the MUST type device normative behavior stated by the spec, because
> > > future incarnations of the spec could relax the requirements towards this
> > > particular device, for example by removing that device normative
> > > statement?  
> > 
> > > I always imagined, if the spec says the device or the driver MUST
> > > "something", then I as the implementer of the other end (driver or
> > > device, can rely on that "something"). If this assumption is wrong then
> > > I'm have to re-examine my entire mental model of the spec.  
> > 
> > Generally yes.  Not if we explicitly tell it not to.
> > 
> > Like here:
> > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > 	 +DriverFeatures even if offered by the device.
> > 
> > This makes sure that drivers do not make an assumption that
> > devices do not set the bit. But yes, maybe spell it out:
> > 
> > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > 	 +DriverFeatures even if offered by the device.
> > 	 +Driver MUST NOT assume that device does not offer VIRTIO_F_ADMIN_VQ.
> > 	 +In particular driver MUST NOT fail feature negotiation if
> > 	 +device offers VIRTIO_F_ADMIN_VQ.
> > 
> > ok now?
> 
> Sorry, it still does not work for me. But I may be wrong. My problem
> is that what we mean is the following:
> 
> If the driver (where driver includes both the transport part and the
> transport agnostic part) does not support VIRTIO_F_ADMIN_VQ then it must
> not set VIRTIO_F_ADMIN_VQ. And any reasoning along the lines "hey the
> device was not supposed to offer that bit in the first place" is
> misguided.

Yes, this is exactly what I'm trying to prevent here.

> The crucial part here is that the MUST NOT accept VIRTIO_F_ADMIN_VQ
> partee is only applicable if the driver does not support
> VIRTIO_F_ADMIN_VQ. That is, if we happen to extend the Channel I/O transport, and we
> decide to implement VIRTIO_F_ADMIN_VQ for the over Channel I/O devices,
> that MUST NOT accept does not get in the way.

Then we'll describe how it works in the spec and then drop this.

> My problem with your proposal is, that the MUST NOT is not guarded by a
> proper precondition (it is a prohibition that does not allow for any
> exceptions).
> 
> I would very much like Conny to chime in on this.
> 
> Regards,
> Halil

But we do this all the time. We disallow some behaviour then
following spec versions start allowing it.

Basically removing a requirement is ok as long as the other side
does not rely on it.

For example, we had this for a while:


	The driver MUST ignore any vendor-specific capability structure which has
	a reserved \field{cfg_type} value.


but the meaning of a "reserved cfg_type" changed over time, allowing
driver to access new cfg_type values.



-- 
MST

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

On Thu, Aug 18, 2022 at 03:39:58PM +0200, Halil Pasic wrote:
> On Tue, 16 Aug 2022 11:48:43 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Aug 16, 2022 at 04:48:11PM +0200, Halil Pasic wrote:
> > > On Fri, 12 Aug 2022 13:19:20 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > >  content.tex | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/content.tex b/content.tex
> > > > index 76b5a28..53be680 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -2668,6 +2668,16 @@ \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Vi
> > > >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> > > >  combination.
> > > >  
> > > > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > > +
> > > > +Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > +DeviceFeatures.
> > > > +
> > > > +\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > > +
> > > > +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > +DriverFeatures even if offered by the device.
> > > > +  
> > > 
> > > I'm not sure I understand the intention here. I believe what we try to
> > > accomplish here is the following. The Channel I/O transport *currently*
> > > does not support the VIRTIO_F_ADMIN_VQ feature. It is not like we want
> > > to state that the feature VIRTIO_F_ADMIN_VQ won't ever be supported by
> > > the Channel I/O transport. Or am I wrong?
> > >
> > > If my assumptions are right, then the old incarnation of the spec could
> > > contradict the new incarnation of the spec. Thus I would prefer something
> > > like.  
> > 
> > Relaxing requirenents is always okay.
> 
> Are you telling me, that for instance a driver author may not rely on
> even the MUST type device normative behavior stated by the spec, because
> future incarnations of the spec could relax the requirements towards this
> particular device, for example by removing that device normative
> statement?
> 
> I always imagined, if the spec says the device or the driver MUST
> "something", then I as the implementer of the other end (driver or
> device, can rely on that "something"). If this assumption is wrong then
> I'm have to re-examine my entire mental model of the spec.
> 
> > 
> > > 
> > > """
> > > Currently the following features are not supported by the Channel I/O
> > > transport:
> > > * VIRTIO_F_ADMIN_VQ
> > > """  
> > 
> > what I want to prevent is driver saying "oh device will not set ADMIN_VQ
> > so it's ok to acknowledge it if offered, it is never offered even
> > though it does not suport it".
> > because then it becomes impossible to know when actually a new driver
> > appears with actual support.
> 
> Fair point!
> 
> I would prefer a driver normative which goes like this:
> 
> """
> A driver SHOULD NOT accept features (i.e. have code that would do so if
> the feature is offered) if the feature is not supported by the driver
> (e.g. because unsupported by the transport), even if the specification
> implies that the device can not offer these features in the first place
> (e.g. because the feature is not yet supported by the transport.
> """

ok. why not MUST NOT?

> And a similar device normative as well, which just that it may not offer
> such features.
> 
> """
> Note: The rationale behind the [reference to the normative] is that
> while some features can not be implemented within the boundaries of the
> current virtio specification, future incarnations of the specificaton may
> make such implementations possible.  A most prominent example is optional
> features dependent on optional virtio facilities whose transport specific
> implementation is not yet specified for some transports. Should one end
> gain the ability to support these features, the old implementation which
> made the assumption that the other end will make sure these features are
> not negotiated would end up negotiating something it can't actually
> support.
> """
> 
> 
> 
> > 
> > So, Maybe just add text
> > 
> > Note: future versions of this specification will allow setting ADMIN_VQ
> > for driver and device. Device MUST NOT assume driver does not
> > acknowledge ADMIN_VQ if offered.
> 
> I would not lean out of the window and promise something with regards to
> future versions of this spec.

s/will/might/

> > 
> > And similarly for drivers:
> > 
> > Note: future versions of this specification will allow setting ADMIN_VQ
> > for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.
> > 
> 
> I think we can then make a note which references the generic normative
> for each feature affected where it suits us.
> 
> > > 
> > > If we want, we can also state what needs to be done in general when
> > > features are unsupported by the transport. And yes, that normative
> > > material in my opinion.
> > > 
> > > Regards,
> > > Halil  
> > 
> > 
> > Are there other examples? I want to call out the list explicitly because
> > it is so easy to enable an extra feature by mistake.
> > 
> 
> I don't think CCW supports the shared memory yet... But I may be wrong.
> 
> > 
> > > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > > >  
> > > >  The device's configuration space is located in host memory.  
> > 

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Halil Pasic]

On Mon, 29 Aug 2022 14:28:05 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Aug 18, 2022 at 03:39:58PM +0200, Halil Pasic wrote:
[..]
> > Fair point!
> > 
> > I would prefer a driver normative which goes like this:
> > 
> > """
> > A driver SHOULD NOT accept features (i.e. have code that would do so if
> > the feature is offered) if the feature is not supported by the driver
> > (e.g. because unsupported by the transport), even if the specification
> > implies that the device can not offer these features in the first place
> > (e.g. because the feature is not yet supported by the transport.
> > """  
> 
> ok. why not MUST NOT?

I'm fine with MUST NOT. Since this is a general statement (i.e. not
scoped to ADMIN_VQ) I felt like SHOULD NOT is a bit safer because
provided somebody is doing this wrong for some feature already, it
wouldn't render that implementation outright non-compliant. But I
believe this is just a theoretical possibility. I'm fine with MUST NOT.

> 
> > And a similar device normative as well, which just that it may not offer
> > such features.
> > 
> > """
> > Note: The rationale behind the [reference to the normative] is that
> > while some features can not be implemented within the boundaries of the
> > current virtio specification, future incarnations of the specificaton may
> > make such implementations possible.  A most prominent example is optional
> > features dependent on optional virtio facilities whose transport specific
> > implementation is not yet specified for some transports. Should one end
> > gain the ability to support these features, the old implementation which
> > made the assumption that the other end will make sure these features are
> > not negotiated would end up negotiating something it can't actually
> > support.
> > """
> > 
> > 
> >   
> > > 
> > > So, Maybe just add text
> > > 
> > > Note: future versions of this specification will allow setting ADMIN_VQ
> > > for driver and device. Device MUST NOT assume driver does not
> > > acknowledge ADMIN_VQ if offered.  
> > 
> > I would not lean out of the window and promise something with regards to
> > future versions of this spec.  
> 
> s/will/might/

With this change it works like a charm!

> 
> > > 
> > > And similarly for drivers:
> > > 
> > > Note: future versions of this specification will allow setting ADMIN_VQ
> > > for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.
> > >   
> > 
> > I think we can then make a note which references the generic normative
> > for each feature affected where it suits us.
> >   
> > > > 
> > > > If we want, we can also state what needs to be done in general when
> > > > features are unsupported by the transport. And yes, that normative
> > > > material in my opinion.
> > > > 
> > > > Regards,
> > > > Halil    
> > > 
> > > 
> > > Are there other examples? I want to call out the list explicitly because
> > > it is so easy to enable an extra feature by mistake.
> > >   
> > 
> > I don't think CCW supports the shared memory yet... But I may be wrong.
> >   
> > >   
> > > > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > > > >  
> > > > >  The device's configuration space is located in host memory.    
> > >   
> 

Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Cornelia Huck]

[finally got around to looking at this thread]

On Tue, Aug 30 2022, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 29 Aug 2022 14:28:05 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> On Thu, Aug 18, 2022 at 03:39:58PM +0200, Halil Pasic wrote:
> [..]
>> > Fair point!
>> > 
>> > I would prefer a driver normative which goes like this:
>> > 
>> > """
>> > A driver SHOULD NOT accept features (i.e. have code that would do so if
>> > the feature is offered) if the feature is not supported by the driver
>> > (e.g. because unsupported by the transport), even if the specification
>> > implies that the device can not offer these features in the first place
>> > (e.g. because the feature is not yet supported by the transport.
>> > """  
>> 
>> ok. why not MUST NOT?
>
> I'm fine with MUST NOT. Since this is a general statement (i.e. not
> scoped to ADMIN_VQ) I felt like SHOULD NOT is a bit safer because
> provided somebody is doing this wrong for some feature already, it
> wouldn't render that implementation outright non-compliant. But I
> believe this is just a theoretical possibility. I'm fine with MUST NOT.

I'd assume that any driver that does this (accept a feature even if not
supported by the transport) today is already broken, so yes, MUST NOT is
the way to go here IMHO.

>
>> 
>> > And a similar device normative as well, which just that it may not offer
>> > such features.
>> > 
>> > """
>> > Note: The rationale behind the [reference to the normative] is that
>> > while some features can not be implemented within the boundaries of the
>> > current virtio specification, future incarnations of the specificaton may
>> > make such implementations possible.  A most prominent example is optional
>> > features dependent on optional virtio facilities whose transport specific
>> > implementation is not yet specified for some transports. Should one end
>> > gain the ability to support these features, the old implementation which
>> > made the assumption that the other end will make sure these features are
>> > not negotiated would end up negotiating something it can't actually
>> > support.
>> > """
>> > 
>> > 
>> >   
>> > > 
>> > > So, Maybe just add text
>> > > 
>> > > Note: future versions of this specification will allow setting ADMIN_VQ
>> > > for driver and device. Device MUST NOT assume driver does not
>> > > acknowledge ADMIN_VQ if offered.  
>> > 
>> > I would not lean out of the window and promise something with regards to
>> > future versions of this spec.  
>> 
>> s/will/might/
>
> With this change it works like a charm!

Works for me as well. (Although I'd use definite articles with device
and driver.)

>
>> 
>> > > 
>> > > And similarly for drivers:
>> > > 
>> > > Note: future versions of this specification will allow setting ADMIN_VQ
>> > > for driver and device. Drivers MUST NOT assume ADMIN_VQ if not offered.

Same here.

>> > >   
>> > 
>> > I think we can then make a note which references the generic normative
>> > for each feature affected where it suits us.
>> >   
>> > > > 
>> > > > If we want, we can also state what needs to be done in general when
>> > > > features are unsupported by the transport. And yes, that normative
>> > > > material in my opinion.
>> > > > 
>> > > > Regards,
>> > > > Halil    
>> > > 
>> > > 
>> > > Are there other examples? I want to call out the list explicitly because
>> > > it is so easy to enable an extra feature by mistake.
>> > >   
>> > 
>> > I don't think CCW supports the shared memory yet... But I may be wrong.

You are right about this one. I think we never figured out an
architecture that would work with a mix of virtio-ccw and virtio-pci
devices...

I think ring reset and notification data should also be on that list of
non-supported features. Things like SR_IOV obviously don't make sense
for ccw, so they will never be implemented.


---------------------------------------------------------------------
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] Re: [virtio-dev] Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Halil Pasic]

On Sun, 28 Aug 2022 05:35:53 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > > Like here:
> > > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > 	 +DriverFeatures even if offered by the device.
> > > 
> > > This makes sure that drivers do not make an assumption that
> > > devices do not set the bit. But yes, maybe spell it out:
> > > 
> > > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > 	 +DriverFeatures even if offered by the device.
> > > 	 +Driver MUST NOT assume that device does not offer VIRTIO_F_ADMIN_VQ.
> > > 	 +In particular driver MUST NOT fail feature negotiation if
> > > 	 +device offers VIRTIO_F_ADMIN_VQ.
> > > 
> > > ok now?  
> > 
> > Sorry, it still does not work for me. But I may be wrong. My problem
> > is that what we mean is the following:
> > 
> > If the driver (where driver includes both the transport part and the
> > transport agnostic part) does not support VIRTIO_F_ADMIN_VQ then it must
> > not set VIRTIO_F_ADMIN_VQ. And any reasoning along the lines "hey the
> > device was not supposed to offer that bit in the first place" is
> > misguided.  
> 
> Yes, this is exactly what I'm trying to prevent here.
> 
> > The crucial part here is that the MUST NOT accept VIRTIO_F_ADMIN_VQ
> > partee is only applicable if the driver does not support
> > VIRTIO_F_ADMIN_VQ. That is, if we happen to extend the Channel I/O transport, and we
> > decide to implement VIRTIO_F_ADMIN_VQ for the over Channel I/O devices,
> > that MUST NOT accept does not get in the way.  
> 
> Then we'll describe how it works in the spec and then drop this.
> 
> > My problem with your proposal is, that the MUST NOT is not guarded by a
> > proper precondition (it is a prohibition that does not allow for any
> > exceptions).
> > 
> > I would very much like Conny to chime in on this.
> > 
> > Regards,
> > Halil  
> 
> But we do this all the time. We disallow some behaviour then
> following spec versions start allowing it.
> 
> Basically removing a requirement is ok as long as the other side
> does not rely on it.
> 

Sorry I don't want to delay this any further. It is unlikely to
do any harm so I'm fine with moving forward with this.

TLDR: I agree: it would be safe to remove both of the statements
you introduce with this patch once the transport gains support
for the feature.

But I would still prefer being generic about the problem and maintaining
a list of not (yet) supported features rather than adding and removing
normative statement pairs. Just like we do for features, and also in your
cfg_type example.

The rest are my thoughts on the implications. 

You seem to say that all the normative statements may be classified
into two categories:
C1) the other side does not rely on this requirement
C2) the other side does rely on this requirement

The requirements form C1 can be removed form future versions of the
spec, and devices may start not honoring these without breaking
compatibility because the other side did not rely on these being held
up in the first place.

The requirements from C2 can not be removed from future versions of the
spec, because a device compliant to the previous version of the spec
that relies on this requirement being fulfilled would break. And
interchangeability is about compatibility and interoperability -- so
that is no good.

I understand that reasoning. But what I don't understand is the
following: how is the reader of the specification supposed to decide
if a certain requirement is a C1 requirement or a C2 requirement.
Misclassifying a requirement could obviously pose a problem.

Some examples where it may not be entirely obvious if it is C1 or C2:

2.1.2:
The device MUST NOT consume buffers or send any used buffer
notifications to the driver before DRIVER_OK. 

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

2.4.1
Drivers MUST NOT limit structure size and device configuration space
size.

After some more thinking... Regarding whether the other side (B) is
allowed to rely on a certain normative requirement or not really boils
down to the question is there a normative statement that explicitly prohibits
the other side (B) to rely on the property of the other side (A) that is
implied by the first normative statement. I think feature bits are a
good example.

I think, it would conceptually simplify things a lot if the avoided
normative statements that we hope to drop in the future. 

This is a rather academic discussion and probably not a worthwhile one.



> For example, we had this for a while:
> 
> 
> 	The driver MUST ignore any vendor-specific capability structure which has
> 	a reserved \field{cfg_type} value.
> 
> 
> but the meaning of a "reserved cfg_type" changed over time, allowing
> driver to access new cfg_type values.

Right! I completely agree, our goal is that version N and version N+M
should be interoperable.  And that does not mean that every
device/driver that is compliant to version N+M must also be compliant
to the version N of the spec. I agree that having when adding new
stuff the entity implementing the new stuff must be non-compliant
to the statements of the previous specification that say the unknown
stuff must be fenced.

Yet I prefer phrasings like this one where the phrasing of statement is
still valid for all future versions of the spec (just the parameters
change: in this case what is reserved and what is not).

Regards,
Halil

Re: [virtio] Re: [virtio-dev] Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Michael S. Tsirkin]

On Wed, Aug 31, 2022 at 04:33:49PM +0200, Halil Pasic wrote:
> On Sun, 28 Aug 2022 05:35:53 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > > > Like here:
> > > > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > 	 +DriverFeatures even if offered by the device.
> > > > 
> > > > This makes sure that drivers do not make an assumption that
> > > > devices do not set the bit. But yes, maybe spell it out:
> > > > 
> > > > 	 +Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
> > > > 	 +DriverFeatures even if offered by the device.
> > > > 	 +Driver MUST NOT assume that device does not offer VIRTIO_F_ADMIN_VQ.
> > > > 	 +In particular driver MUST NOT fail feature negotiation if
> > > > 	 +device offers VIRTIO_F_ADMIN_VQ.
> > > > 
> > > > ok now?  
> > > 
> > > Sorry, it still does not work for me. But I may be wrong. My problem
> > > is that what we mean is the following:
> > > 
> > > If the driver (where driver includes both the transport part and the
> > > transport agnostic part) does not support VIRTIO_F_ADMIN_VQ then it must
> > > not set VIRTIO_F_ADMIN_VQ. And any reasoning along the lines "hey the
> > > device was not supposed to offer that bit in the first place" is
> > > misguided.  
> > 
> > Yes, this is exactly what I'm trying to prevent here.
> > 
> > > The crucial part here is that the MUST NOT accept VIRTIO_F_ADMIN_VQ
> > > partee is only applicable if the driver does not support
> > > VIRTIO_F_ADMIN_VQ. That is, if we happen to extend the Channel I/O transport, and we
> > > decide to implement VIRTIO_F_ADMIN_VQ for the over Channel I/O devices,
> > > that MUST NOT accept does not get in the way.  
> > 
> > Then we'll describe how it works in the spec and then drop this.
> > 
> > > My problem with your proposal is, that the MUST NOT is not guarded by a
> > > proper precondition (it is a prohibition that does not allow for any
> > > exceptions).
> > > 
> > > I would very much like Conny to chime in on this.
> > > 
> > > Regards,
> > > Halil  
> > 
> > But we do this all the time. We disallow some behaviour then
> > following spec versions start allowing it.
> > 
> > Basically removing a requirement is ok as long as the other side
> > does not rely on it.
> > 
> 
> Sorry I don't want to delay this any further. It is unlikely to
> do any harm so I'm fine with moving forward with this.
> 
> TLDR: I agree: it would be safe to remove both of the statements
> you introduce with this patch once the transport gains support
> for the feature.
> 
> But I would still prefer being generic about the problem and maintaining
> a list of not (yet) supported features rather than adding and removing
> normative statement pairs. Just like we do for features, and also in your
> cfg_type example.
> 
> The rest are my thoughts on the implications. 
> 
> You seem to say that all the normative statements may be classified
> into two categories:
> C1) the other side does not rely on this requirement
> C2) the other side does rely on this requirement
> 
> The requirements form C1 can be removed form future versions of the
> spec, and devices may start not honoring these without breaking
> compatibility because the other side did not rely on these being held
> up in the first place.
> 
> The requirements from C2 can not be removed from future versions of the
> spec, because a device compliant to the previous version of the spec
> that relies on this requirement being fulfilled would break. And
> interchangeability is about compatibility and interoperability -- so
> that is no good.
> 
> I understand that reasoning. But what I don't understand is the
> following: how is the reader of the specification supposed to decide
> if a certain requirement is a C1 requirement or a C2 requirement.
> Misclassifying a requirement could obviously pose a problem.

The reader is supposed to satisfy all requirements.
Whether it is ok to rely on requirements is a problem that
unfortunately we don't always clarify.


> Some examples where it may not be entirely obvious if it is C1 or C2:
> 
> 2.1.2:
> The device MUST NOT consume buffers or send any used buffer
> notifications to the driver before DRIVER_OK. 
> 
> 2.2.1
> 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.
> 
> 2.4.1
> Drivers MUST NOT limit structure size and device configuration space
> size.
> 
> After some more thinking... Regarding whether the other side (B) is
> allowed to rely on a certain normative requirement or not really boils
> down to the question is there a normative statement that explicitly prohibits
> the other side (B) to rely on the property of the other side (A) that is
> implied by the first normative statement. I think feature bits are a
> good example.
> 
> I think, it would conceptually simplify things a lot if the avoided
> normative statements that we hope to drop in the future. 
> 
> This is a rather academic discussion and probably not a worthwhile one.

I absolutely understand where you are coming from... I'm fine just using
SHOULD NOT here for now.
Do you feel that's the way to go or have you changed your mind?
Couldn't figure it out.



But generally, I feel C1 is the default unless there is text that
explicitly says C2.


> 
> 
> > For example, we had this for a while:
> > 
> > 
> > 	The driver MUST ignore any vendor-specific capability structure which has
> > 	a reserved \field{cfg_type} value.
> > 
> > 
> > but the meaning of a "reserved cfg_type" changed over time, allowing
> > driver to access new cfg_type values.
> 
> Right! I completely agree, our goal is that version N and version N+M
> should be interoperable.  And that does not mean that every
> device/driver that is compliant to version N+M must also be compliant
> to the version N of the spec. I agree that having when adding new
> stuff the entity implementing the new stuff must be non-compliant
> to the statements of the previous specification that say the unknown
> stuff must be fenced.
> 
> Yet I prefer phrasings like this one where the phrasing of statement is
> still valid for all future versions of the spec (just the parameters
> change: in this case what is reserved and what is not).
> 
> Regards,
> Halil


I guess it's a matter of taste.
What worries me is that it's so easy to miss these implications.


-- 
MST

Re: [virtio] Re: [virtio-dev] Re: [virtio] [PATCH RFC v7 6/8] ccw: disallow ADMIN_VQ

[Halil Pasic]

On Wed, 31 Aug 2022 10:50:21 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> > You seem to say that all the normative statements may be classified
> > into two categories:
> > C1) the other side does not rely on this requirement
> > C2) the other side does rely on this requirement
> > 
> > The requirements form C1 can be removed form future versions of the
> > spec, and devices may start not honoring these without breaking
> > compatibility because the other side did not rely on these being held
> > up in the first place.
> > 
> > The requirements from C2 can not be removed from future versions of the
> > spec, because a device compliant to the previous version of the spec
> > that relies on this requirement being fulfilled would break. And
> > interchangeability is about compatibility and interoperability -- so
> > that is no good.
> > 
> > I understand that reasoning. But what I don't understand is the
> > following: how is the reader of the specification supposed to decide
> > if a certain requirement is a C1 requirement or a C2 requirement.
> > Misclassifying a requirement could obviously pose a problem.  
> 
> The reader is supposed to satisfy all requirements.

I fullheartheadly agree with that. Ideally just both sides (the
device and the driver) satisfying all the normative requirements would
guarantee a favorable outcome for the end user: interoperability and at
least basic functionality.

I'm not sure we are there yet with virtio. 

> Whether it is ok to rely on requirements is a problem that
> unfortunately we don't always clarify.

I believe, we want the people to rely on the spec. AFAIU the
normative statements are the parts of the spec that are the
deserve the most to be relied upon.

[..]


> > 
> > I think, it would conceptually simplify things a lot if the avoided
> > normative statements that we hope to drop in the future. 
> > 
> > This is a rather academic discussion and probably not a worthwhile one.  
> 
> I absolutely understand where you are coming from... I'm fine just using
> SHOULD NOT here for now.

If we stick with the statement pair like proposed in this patch, in my
opinion only MUST NOT works. Let me cite those:
"""
+\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
+
+Device MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
+DeviceFeatures.
+
+\drivernormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
+
+Driver MUST NOT set bit VIRTIO_F_ADMIN_VQ (bit 41) in
+DriverFeatures even if offered by the device.
"""

By the way I just noticed that in the second normative above the heading
says "Device Features" while the normative itself makes a statement about
"DriverFeatures".

IMHO we can remove both together, because any implementation that could
be adversely affected by the removal of both is non-conforming because
it violates one of these MUST NOT statements.

Also for the approach:  generic statement about handling feature
negotiations for features are non-viable in certain constellations (e.g.
some transport or the platform, or whatever lacks support or renders the
feature impossible to implement) + a list of features not supported by
certain transport, I came to prefer MUST NOT.

> Do you feel that's the way to go or have you changed your mind?
> Couldn't figure it out.
> 

I believe we are fine either way, but generic statement + list of cases
where this is known to apply is nicer to work with and evolve.
 
> 
> But generally, I feel C1 is the default unless there is text that
> explicitly says C2.
> 

Here we disagree. I believe most of our normative statements are such
that the can be relied upon by everyone including the other side.

[..]
> > Yet I prefer phrasings like this one where the phrasing of statement is
> > still valid for all future versions of the spec (just the parameters
> > change: in this case what is reserved and what is not).
> > 
> > Regards,
> > Halil  
> 
> 
> I guess it's a matter of taste.
> What worries me is that it's so easy to miss these implications.

I agree, it is a matter of taste. I believe what we code-wise want
is a transport specific mask both on the end of the device and on the
end of the driver which ensures that bits that corresponding features can
not be supported by entity that is doing its part of the negotiation are
not set. I believe having a list of such features in the specification
for each transport at an easy to locate place i s easier to map to code
than a bunch of normative statement pairs. And also manipulating this
list (adding to it, or removing form it) looks more straightforward to
me.

But I'm fine with either solution.

Regards,
Halil