[PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Max Gurtovoy]

Hi,
A virtio subsystem definition will help extending the virtio specefication for
various future features that require a notion of grouping devices together or
managing devices inside a group. It also might be used splitting or sharing a
single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
devices). A virtio subsystem include one or more virtio devices.

Also introduce the admin facility to allow manipulating features and configurations
in a generic manner. Using the admin command set, one can manipulate the device itself
and/or to manipulate, if possible, another device within the same virtio subsystem (the
following patch set).

The admin virtqueue is the first management interface to issue Admin commands from
the admin command set.

The admin virtqueue interface will be extended in the future with more and more
features that some of them already in discussions. Some of these features don't
fit to MMIO/config_space characteristics, therefore a queue is selected to address
admin commands.

Motivation for choosing admin queue:
1. It is anticipated that admin queue will be used for managing and configuring
   many different type of resources. For example,
   a. PCI PF configuring PCI VF attributes.
   b. virtio device creating/destroying/configuring subfunctions discussed in [1]
   c. composing device config space of VF or SF such as mac address, number of VQs, virtio features

   Mapping all of them as configuration registers to MMIO will require large MMIO space,
   if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
   requires on-chip resources to complete within MMIO access latencies. Such resources are very
   expensive.

2. Such limitation can be overcome by having smaller MMIO register set to build
   a command request response interface. However, such MMIO based command interface
   will be limited to serve single outstanding command execution. Such limitation can
   resulting in high device creation and composing time which can affect VM startup time.
   Often device can queue and service multiple commands in parallel, such command interface
   cannot use parallelism offered by the device.

3. When a command wants to DMA data from one or more physical addresses, for example in the future a
   live migration command may need to fetch device state consist of config space, tens of
   VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
   Packing one or more DMA addresses over new command interface will be burden some and continue
   to suffer single outstanding command execution latencies. Such limitation is not good for time
   sensitive live migration use cases.

4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
   descriptors. Similar mechanism exist today for device specific configuration - the control VQ.

[1] https://lists.oasis-open.org/archives/virtio-comment/202108/msg00025.html

This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
This series include the comments and fixes from V1-V3 of the initial patch set from above.
The following series introduce the management devices and MSI-X configuration of virtio devices.

Open issues:
1. CCW and MMIO specification for admin_queue_index register

Max Gurtovoy (5):
  virtio: Introduce virtio subsystem
  Introduce Admin Command Set
  Introduce DEVICE INFO Admin command
  Add virtio Admin virtqueue
  Add miscellaneous configuration structure for PCI

 admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
 conformance.tex  |   3 +
 content.tex      |  33 ++++++++-
 introduction.tex |  20 ++++++
 4 files changed, 231 insertions(+), 2 deletions(-)
 create mode 100644 admin.tex

-- 
2.21.0

[PATCH v1 1/5] virtio: Introduce virtio subsystem

[Max Gurtovoy]

A virtio subsystem may contain one or more virtio devices. All of the
devices that make up a virtio subsystem share the same virtio subsystem
unique identifier. This identifier is the virtio qualified name (VQN).
Each device within a virtio subsystem has a unique identifier. This
identifier is the virtio device id (vdev_id). The combination of these
identifiers forms a globally unique value identifies a virtio device.

Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 introduction.tex | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/introduction.tex b/introduction.tex
index 6d52717..8e6611e 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -240,5 +240,23 @@ \section{Constant Specifications}
 refer to values 1 and 2 of Fld respectively. Further, VIRTIO_FLD_XXX refers to
 either VIRTIO_FLD_A or VIRTIO_FLD_B.
 
+\section{Definitions}\label{sec:Introduction / Definitions}
+
+\subsection{virtio device}\label{sec:Introduction / Definitions / virtio device}
+
+An entity that implements virtio specification.
+
+\subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subsystem}
+
+A virtio subsystem includes one or more virtio devices.
+Each virtio subsystem has a unique virtio qualified name (VQN) that is permanent for the lifetime of the virtio subsystem.
+The VQN is a 128-bit UUID. It is RECOMMENDED to use UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
+Virtio devices within one virtio subsystem share the same VQN. Each virtio device has a unique virtio
+device id (vdev_id) within a virtio subsystem. A valid vdev_id is a 64-bit field in the range of
+0x0 - 0xFFFFFFFFFFFFFFF0. Vdev_id 0xFFFFFFFFFFFFFFFF is a broadcast value that is used to specify all the
+virtio devices in a virtio subsystem and isn't a valid vdev_id.
+
+The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
+
 \newpage
 
-- 
2.21.0

[virtio-comment] [PATCH v1 2/5] Introduce Admin Command Set

[Max Gurtovoy]

This command set is used for essential administrative and management
operations such as identify and resource management.

Admin commands should be submitted to a well defined management
interface.

Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
 content.tex      |   2 +
 introduction.tex |   2 +
 3 files changed, 133 insertions(+)
 create mode 100644 admin.tex

diff --git a/admin.tex b/admin.tex
new file mode 100644
index 0000000..1e30e01
--- /dev/null
+++ b/admin.tex
@@ -0,0 +1,129 @@
+\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}
+
+The Admin command set defines the commands that can be issued to a well defined management
+interface. All the Admin commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 command;
+        /*
+         * 0 - self
+         * 1 - 65535 are reserved
+         */
+        le16 dst_type;
+        /* reserved for common cmd fields */
+        u8 reserved[20];
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        u8 status;
+        u8 command_specific_error;
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+The following table describes the generic Admin status codes:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
+\hline
+03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
+\hline
+\end{tabular}
+
+The \field{command}, \field{dst_type} and \field{command_specific_data} are
+set by the driver, and the device sets the \field{status}, the
+\field{command_specific_error} and the \field{command_specific_result},
+if needed.
+
+Reserved common fields are ignored by the device and to be zeroed by the driver.
+
+The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
+
+The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
+
+The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
+
+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 content 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.
+
+The following table describes the Admin command set:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Opcode & Command & M/O \\
+\hline \hline
+0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
+\hline
+0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
+\hline
+0002h - 7FFFh   & Generic admin cmds    & -  \\
+\hline
+8000h - FFFFh   & Reserved    & - \\
+\hline
+\end{tabular}
+
+\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
+
+The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
+The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
+Other common fields are reserved for this command and zeroed.
+
+This command upon success, returns a data buffer that describes information about the target virtio device.
+This returned data buffer is of form:
+\begin{lstlisting}
+struct virtio_admin_device_identify_result {
+       /* For compatibility - indicates which of the below fields were returned
+        * (1 means that field was returned):
+        * Bit 0 - vdev_id
+        * Bit 1 - optional_caps_support
+        * Bits 2 - 63 - reserved for future fields
+        */
+       le64 attrs_mask;
+       le64 vdev_id;
+       /* This field indicates which of the below optional admin
+        * capabilities are supported by the device:
+        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
+        * Bits 1 - 63 - reserved for future capabilities.
+        */
+       le64 optional_caps_support;
+       u8 reserved[4072];
+};
+\end{lstlisting}
+
+\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
+
+The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
+The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
+Other common fields are reserved for this command and zeroed.
+
+This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
+This returned data buffer is of form:
+\begin{lstlisting}
+struct virtio_admin_subsystem_identify_result {
+       /* For compatibility - indicates which of the below fields were returned
+        * (1 means that field was returned):
+        * Bit 0 - vqn
+        * Bit 1 - num_supported_vdevs
+        * Bit 2 - max_vdev_id
+        * Bits 3 - 63 - reserved for future fields
+        */
+       le64 attrs_mask;
+       u8 vqn[16];
+       /* Number of supported virtio devices by the subsystem */
+       le64 num_supported_vdevs;
+       /* Maximum value of a valid vdev_id for the subsystem */
+       le64 max_vdev_id;
+       u8 reserved[4056];
+};
+\end{lstlisting}
diff --git a/content.tex b/content.tex
index c6f116c..2e1df84 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
diff --git a/introduction.tex b/introduction.tex
index 8e6611e..94dd7a2 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
 
 The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
 
+VQN and vdev_id are exposed via Admin Command Set.
+
 \newpage
 
-- 
2.21.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://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/

[PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Max Gurtovoy]

The DEVICE INFO command will return a basic information for a virtio
device.

Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 admin.tex | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/admin.tex b/admin.tex
index 1e30e01..9a8969b 100644
--- a/admin.tex
+++ b/admin.tex
@@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
 \hline
 0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
 \hline
-0002h - 7FFFh   & Generic admin cmds    & -  \\
+0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
+\hline
+0003h - 7FFFh   & Generic admin cmds    & -  \\
 \hline
 8000h - FFFFh   & Reserved    & - \\
 \hline
@@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
        /* This field indicates which of the below optional admin
         * capabilities are supported by the device:
         * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
-        * Bits 1 - 63 - reserved for future capabilities.
+        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
+        * Bits 2 - 63 - reserved for future capabilities.
         */
        le64 optional_caps_support;
        u8 reserved[4072];
@@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
        u8 reserved[4056];
 };
 \end{lstlisting}
+
+\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
+
+The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
+The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
+
+The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
+Upon success, the returned data buffer is of form:
+\begin{lstlisting}
+struct virtio_admin_device_info_result {
+        /* For compatibility - indicates which of the below fields were returned
+         * (1 means that field was returned):
+         * Bit 0 - vf_number
+         * Bits 1 - 63 - reserved for future fields
+         */
+        le64 attrs_mask;
+        /* The virtual function number */
+        le16 vf_number;
+        u8 reserved[1014];
+};
+\end{lstlisting}
+
+\begin{note}
+{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
+In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
+specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
+\end{note}
+
-- 
2.21.0

[PATCH v1 4/5] Add virtio Admin virtqueue

[Max Gurtovoy]

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). Some of this configuration is generic enough. i.e
Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
such features query and manipulation by its parent PCI PF. 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.

Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 admin.tex       | 17 +++++++++++++++++
 conformance.tex |  1 +
 content.tex     |  6 ++++--
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/admin.tex b/admin.tex
index 9a8969b..ed9f6bd 100644
--- a/admin.tex
+++ b/admin.tex
@@ -158,3 +158,20 @@ \subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Vi
 specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
 \end{note}
 
+\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
+
+An admin virtqueue is a management interface of a device that can be used to send administrative
+commands to manipulate various features of the device and/or to manipulate
+various features, if possible, of another device within the same virtio subsystem
+(see \ref{sec:Introduction / Definitions / virtio subsystem}).
+
+An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ is
+negotiated. The index of the admin virtqueue is exposed by the device in a
+transport specific manner.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin virtqueue to send all admin commands.
+
+\devicenormative{\subsection}{Admin Virtqueues}{Basic Facilities of a Virtio Device / Admin Virtqueues}
+A device that advertises VIRTIO_F_ADMIN_VQ capability MUST support all the mandatory admin commands.
+
+A device that advertises VIRTIO_F_ADMIN_VQ capability MAY support one or more optional admin commands.
diff --git a/conformance.tex b/conformance.tex
index 42f8537..129831c 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -341,6 +341,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Available Buffer Notification Suppression}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
 \item \ref{devicenormative:Reserved Feature Bits}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Admin Virtqueues}
 \end{itemize}
 
 \conformance{\subsection}{PCI Device Conformance}\label{sec:Conformance / Device Conformance / PCI Device Conformance}
diff --git a/content.tex b/content.tex
index 2e1df84..163cb34 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}
@@ -6849,6 +6849,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}
-- 
2.21.0

[PATCH v1 5/5] Add miscellaneous configuration structure for PCI

[Max Gurtovoy]

This new structure will be used for adding new miscellaneous registers
for a virtio device configuration layout.

For now, only admin_queue_index register is added. Admin virtqueue index
does not depend on the device type. Hence, add a PCI capability to read
the admin virtqueue index.

Reviewed-by: Parav Pandit <parav@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 conformance.tex |  2 ++
 content.tex     | 25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index 129831c..e31645e 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -102,6 +102,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
 \end{itemize}
 
 \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
@@ -363,6 +364,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
 \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
+\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
 \end{itemize}
 
 \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
diff --git a/content.tex b/content.tex
index 163cb34..bf46192 100644
--- a/content.tex
+++ b/content.tex
@@ -712,6 +712,7 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
 \item ISR Status
 \item Device-specific configuration (optional)
 \item PCI configuration access
+\item Miscellaneous configuration
 \end{itemize}
 
 Each structure can be mapped by a Base Address register (BAR) belonging to
@@ -771,6 +772,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
 #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
 /* Vendor-specific data */
 #define VIRTIO_PCI_CAP_VENDOR_CFG        9
+/* Miscellaneous configuration */
+#define VIRTIO_PCI_CAP_MISC_CFG          10
 \end{lstlisting}
 
         Any other value is reserved for future use.
@@ -1352,6 +1355,28 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
 specified by some other Virtio Structure PCI Capability
 of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
 
+\subsubsection{Miscellaneous configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
+
+The miscellaneous configuration structure is found at the bar and offset within the VIRTIO_PCI_CAP_MISC_CFG capability.
+Its layout is below.
+\begin{lstlisting}
+struct virtio_pci_misc_cfg {
+        le16 admin_queue_index;         /* read-only for driver */
+};
+\end{lstlisting}
+
+\begin{description}
+\item[\field{admin_queue_index}]
+        The device uses this to report the index of the admin virtqueue.
+        This field is valid only if VIRTIO_F_ADMIN_VQ is set.
+\end{description}
+
+\devicenormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
+The device MUST present a valid \field{admin_queue_index} when VIRTIO_F_ADMIN_VQ is set.
+
+\drivernormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
+The driver MUST use the value of \field{admin_queue_index} to configure the admin virtqueue. For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}.
+
 \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
 
 Transitional devices MUST present part of configuration
-- 
2.21.0

Re: [PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 05:56:03PM +0200, Max Gurtovoy wrote:
> Hi,
> A virtio subsystem definition will help extending the virtio specefication for
> various future features that require a notion of grouping devices together or
> managing devices inside a group. It also might be used splitting or sharing a
> single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
> devices). A virtio subsystem include one or more virtio devices.


A large patch, need a bit more time for review. Meanwhile,
how about adding migration related capabilities?
I would very much like that to make progress before
people start using high overhead solutions like
VQ shadowing.


> Also introduce the admin facility to allow manipulating features and configurations
> in a generic manner. Using the admin command set, one can manipulate the device itself
> and/or to manipulate, if possible, another device within the same virtio subsystem (the
> following patch set).
> 
> The admin virtqueue is the first management interface to issue Admin commands from
> the admin command set.
> 
> The admin virtqueue interface will be extended in the future with more and more
> features that some of them already in discussions. Some of these features don't
> fit to MMIO/config_space characteristics, therefore a queue is selected to address
> admin commands.
> 
> Motivation for choosing admin queue:
> 1. It is anticipated that admin queue will be used for managing and configuring
>    many different type of resources. For example,
>    a. PCI PF configuring PCI VF attributes.
>    b. virtio device creating/destroying/configuring subfunctions discussed in [1]
>    c. composing device config space of VF or SF such as mac address, number of VQs, virtio features
> 
>    Mapping all of them as configuration registers to MMIO will require large MMIO space,
>    if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
>    requires on-chip resources to complete within MMIO access latencies. Such resources are very
>    expensive.
> 
> 2. Such limitation can be overcome by having smaller MMIO register set to build
>    a command request response interface. However, such MMIO based command interface
>    will be limited to serve single outstanding command execution. Such limitation can
>    resulting in high device creation and composing time which can affect VM startup time.
>    Often device can queue and service multiple commands in parallel, such command interface
>    cannot use parallelism offered by the device.
> 
> 3. When a command wants to DMA data from one or more physical addresses, for example in the future a
>    live migration command may need to fetch device state consist of config space, tens of
>    VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
>    Packing one or more DMA addresses over new command interface will be burden some and continue
>    to suffer single outstanding command execution latencies. Such limitation is not good for time
>    sensitive live migration use cases.
> 
> 4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
>    descriptors. Similar mechanism exist today for device specific configuration - the control VQ.
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202108/msg00025.html
> 
> This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
> This series include the comments and fixes from V1-V3 of the initial patch set from above.
> The following series introduce the management devices and MSI-X configuration of virtio devices.
> 
> Open issues:
> 1. CCW and MMIO specification for admin_queue_index register
> 
> Max Gurtovoy (5):
>   virtio: Introduce virtio subsystem
>   Introduce Admin Command Set
>   Introduce DEVICE INFO Admin command
>   Add virtio Admin virtqueue
>   Add miscellaneous configuration structure for PCI
> 
>  admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
>  conformance.tex  |   3 +
>  content.tex      |  33 ++++++++-
>  introduction.tex |  20 ++++++
>  4 files changed, 231 insertions(+), 2 deletions(-)
>  create mode 100644 admin.tex
> 
> -- 
> 2.21.0

Re: [PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Max Gurtovoy]

On 3/9/2022 9:42 AM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 05:56:03PM +0200, Max Gurtovoy wrote:
>> Hi,
>> A virtio subsystem definition will help extending the virtio specefication for
>> various future features that require a notion of grouping devices together or
>> managing devices inside a group. It also might be used splitting or sharing a
>> single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
>> devices). A virtio subsystem include one or more virtio devices.
>
> A large patch, need a bit more time for review. Meanwhile,
> how about adding migration related capabilities?
> I would very much like that to make progress before
> people start using high overhead solutions like
> VQ shadowing.

Sure I can start working on rebasing old LM proposal to virtio subsystem 
framework.

But can you be precise for what you mean capabilities ? only caps 
without the commands and LM logic ?

Initial feedback will be great for this series since every rebase cost a 
lot... and it grows if we add more caps and logic.

>
>
>> Also introduce the admin facility to allow manipulating features and configurations
>> in a generic manner. Using the admin command set, one can manipulate the device itself
>> and/or to manipulate, if possible, another device within the same virtio subsystem (the
>> following patch set).
>>
>> The admin virtqueue is the first management interface to issue Admin commands from
>> the admin command set.
>>
>> The admin virtqueue interface will be extended in the future with more and more
>> features that some of them already in discussions. Some of these features don't
>> fit to MMIO/config_space characteristics, therefore a queue is selected to address
>> admin commands.
>>
>> Motivation for choosing admin queue:
>> 1. It is anticipated that admin queue will be used for managing and configuring
>>     many different type of resources. For example,
>>     a. PCI PF configuring PCI VF attributes.
>>     b. virtio device creating/destroying/configuring subfunctions discussed in [1]
>>     c. composing device config space of VF or SF such as mac address, number of VQs, virtio features
>>
>>     Mapping all of them as configuration registers to MMIO will require large MMIO space,
>>     if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
>>     requires on-chip resources to complete within MMIO access latencies. Such resources are very
>>     expensive.
>>
>> 2. Such limitation can be overcome by having smaller MMIO register set to build
>>     a command request response interface. However, such MMIO based command interface
>>     will be limited to serve single outstanding command execution. Such limitation can
>>     resulting in high device creation and composing time which can affect VM startup time.
>>     Often device can queue and service multiple commands in parallel, such command interface
>>     cannot use parallelism offered by the device.
>>
>> 3. When a command wants to DMA data from one or more physical addresses, for example in the future a
>>     live migration command may need to fetch device state consist of config space, tens of
>>     VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
>>     Packing one or more DMA addresses over new command interface will be burden some and continue
>>     to suffer single outstanding command execution latencies. Such limitation is not good for time
>>     sensitive live migration use cases.
>>
>> 4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
>>     descriptors. Similar mechanism exist today for device specific configuration - the control VQ.
>>
>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F202108%2Fmsg00025.html&data=04%7C01%7Cmgurtovoy%40nvidia.com%7C22dbf3fcbd584246d1e708da01a068ac%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637824085715740541%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NovezyzVKrql1pL5nHiG5%2BhZtlXLflfozLc4X5kVnYQ%3D&reserved=0
>>
>> This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
>> This series include the comments and fixes from V1-V3 of the initial patch set from above.
>> The following series introduce the management devices and MSI-X configuration of virtio devices.
>>
>> Open issues:
>> 1. CCW and MMIO specification for admin_queue_index register
>>
>> Max Gurtovoy (5):
>>    virtio: Introduce virtio subsystem
>>    Introduce Admin Command Set
>>    Introduce DEVICE INFO Admin command
>>    Add virtio Admin virtqueue
>>    Add miscellaneous configuration structure for PCI
>>
>>   admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
>>   conformance.tex  |   3 +
>>   content.tex      |  33 ++++++++-
>>   introduction.tex |  20 ++++++
>>   4 files changed, 231 insertions(+), 2 deletions(-)
>>   create mode 100644 admin.tex
>>
>> -- 
>> 2.21.0

Re: [PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Michael S. Tsirkin]

On Thu, Mar 10, 2022 at 12:38:38PM +0200, Max Gurtovoy wrote:
> 
> On 3/9/2022 9:42 AM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 05:56:03PM +0200, Max Gurtovoy wrote:
> > > Hi,
> > > A virtio subsystem definition will help extending the virtio specefication for
> > > various future features that require a notion of grouping devices together or
> > > managing devices inside a group. It also might be used splitting or sharing a
> > > single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
> > > devices). A virtio subsystem include one or more virtio devices.
> > 
> > A large patch, need a bit more time for review. Meanwhile,
> > how about adding migration related capabilities?
> > I would very much like that to make progress before
> > people start using high overhead solutions like
> > VQ shadowing.
> 
> Sure I can start working on rebasing old LM proposal to virtio subsystem
> framework.
> 
> But can you be precise for what you mean capabilities ? only caps without
> the commands and LM logic ?

There are at least four distinct bits, and they can be worked on mostly
separately:


1.  We need a bunch of stuff to migrate a device to a different host right?
- device specific state
- transport state
- vq ring state
and of course we need
- ability to stop/resume device
This is useful by itself e.g. for snapshoting.

Then to reduce downtime we also need to run device during memory
migration, which requires support for

2. page faults (postcopy) and optionally
3. dirty tracking (precopy) - though dirty tracking can be done
with faults too, so maybe just faults.
Faults are definitely useful for a bunch of stuff like memory migration.
Dirty tracking is more of a boutique feature, but I guess uses
beyond memory migration can still be found.

4.  Finally, feature compatibility is a problem: not any configuration of a
device can be migrated to any other device. A simplest example is a
device feature not present on destination. Can be solved by not exposing
the feature to the guest. Another example is layout of pci configuration
space. Spec allows a lot of flexibility here, however things like
# of VQs will affect the memory bar size.
I am not exactly sure what we want to do in this space, maybe for
starters enumerating what are the things that need to match on source
and destination?
We can start with a non-normative sections describing the issues
generally at least.



> Initial feedback will be great for this series since every rebase cost a
> lot... and it grows if we add more caps and logic.
> 
> > 
> > 
> > > Also introduce the admin facility to allow manipulating features and configurations
> > > in a generic manner. Using the admin command set, one can manipulate the device itself
> > > and/or to manipulate, if possible, another device within the same virtio subsystem (the
> > > following patch set).
> > > 
> > > The admin virtqueue is the first management interface to issue Admin commands from
> > > the admin command set.
> > > 
> > > The admin virtqueue interface will be extended in the future with more and more
> > > features that some of them already in discussions. Some of these features don't
> > > fit to MMIO/config_space characteristics, therefore a queue is selected to address
> > > admin commands.
> > > 
> > > Motivation for choosing admin queue:
> > > 1. It is anticipated that admin queue will be used for managing and configuring
> > >     many different type of resources. For example,
> > >     a. PCI PF configuring PCI VF attributes.
> > >     b. virtio device creating/destroying/configuring subfunctions discussed in [1]
> > >     c. composing device config space of VF or SF such as mac address, number of VQs, virtio features
> > > 
> > >     Mapping all of them as configuration registers to MMIO will require large MMIO space,
> > >     if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
> > >     requires on-chip resources to complete within MMIO access latencies. Such resources are very
> > >     expensive.
> > > 
> > > 2. Such limitation can be overcome by having smaller MMIO register set to build
> > >     a command request response interface. However, such MMIO based command interface
> > >     will be limited to serve single outstanding command execution. Such limitation can
> > >     resulting in high device creation and composing time which can affect VM startup time.
> > >     Often device can queue and service multiple commands in parallel, such command interface
> > >     cannot use parallelism offered by the device.
> > > 
> > > 3. When a command wants to DMA data from one or more physical addresses, for example in the future a
> > >     live migration command may need to fetch device state consist of config space, tens of
> > >     VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
> > >     Packing one or more DMA addresses over new command interface will be burden some and continue
> > >     to suffer single outstanding command execution latencies. Such limitation is not good for time
> > >     sensitive live migration use cases.
> > > 
> > > 4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
> > >     descriptors. Similar mechanism exist today for device specific configuration - the control VQ.
> > > 
> > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F202108%2Fmsg00025.html&data=04%7C01%7Cmgurtovoy%40nvidia.com%7C22dbf3fcbd584246d1e708da01a068ac%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637824085715740541%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=NovezyzVKrql1pL5nHiG5%2BhZtlXLflfozLc4X5kVnYQ%3D&reserved=0
> > > 
> > > This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
> > > This series include the comments and fixes from V1-V3 of the initial patch set from above.
> > > The following series introduce the management devices and MSI-X configuration of virtio devices.
> > > 
> > > Open issues:
> > > 1. CCW and MMIO specification for admin_queue_index register
> > > 
> > > Max Gurtovoy (5):
> > >    virtio: Introduce virtio subsystem
> > >    Introduce Admin Command Set
> > >    Introduce DEVICE INFO Admin command
> > >    Add virtio Admin virtqueue
> > >    Add miscellaneous configuration structure for PCI
> > > 
> > >   admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   conformance.tex  |   3 +
> > >   content.tex      |  33 ++++++++-
> > >   introduction.tex |  20 ++++++
> > >   4 files changed, 231 insertions(+), 2 deletions(-)
> > >   create mode 100644 admin.tex
> > > 
> > > -- 
> > > 2.21.0

Re: [PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Max Gurtovoy]

On 3/10/2022 2:49 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 10, 2022 at 12:38:38PM +0200, Max Gurtovoy wrote:
>> On 3/9/2022 9:42 AM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2022 at 05:56:03PM +0200, Max Gurtovoy wrote:
>>>> Hi,
>>>> A virtio subsystem definition will help extending the virtio specefication for
>>>> various future features that require a notion of grouping devices together or
>>>> managing devices inside a group. It also might be used splitting or sharing a
>>>> single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
>>>> devices). A virtio subsystem include one or more virtio devices.
>>> A large patch, need a bit more time for review. Meanwhile,
>>> how about adding migration related capabilities?
>>> I would very much like that to make progress before
>>> people start using high overhead solutions like
>>> VQ shadowing.
>> Sure I can start working on rebasing old LM proposal to virtio subsystem
>> framework.
>>
>> But can you be precise for what you mean capabilities ? only caps without
>> the commands and LM logic ?
> There are at least four distinct bits, and they can be worked on mostly
> separately:
>
>
> 1.  We need a bunch of stuff to migrate a device to a different host right?
> - device specific state
> - transport state
> - vq ring state
> and of course we need
> - ability to stop/resume device
> This is useful by itself e.g. for snapshoting.
>
> Then to reduce downtime we also need to run device during memory
> migration, which requires support for
>
> 2. page faults (postcopy) and optionally
> 3. dirty tracking (precopy) - though dirty tracking can be done
> with faults too, so maybe just faults.
> Faults are definitely useful for a bunch of stuff like memory migration.
> Dirty tracking is more of a boutique feature, but I guess uses
> beyond memory migration can still be found.
>
> 4.  Finally, feature compatibility is a problem: not any configuration of a
> device can be migrated to any other device. A simplest example is a
> device feature not present on destination. Can be solved by not exposing
> the feature to the guest. Another example is layout of pci configuration
> space. Spec allows a lot of flexibility here, however things like
> # of VQs will affect the memory bar size.
> I am not exactly sure what we want to do in this space, maybe for
> starters enumerating what are the things that need to match on source
> and destination?
> We can start with a non-normative sections describing the issues
> generally at least.

MST,

I really like us to push these 5 patches before we deep dive to LM stuff.

This was our plan we agreed together - push infrastructure with 
relatively small feature (we choose MSIX management) to the spec.

This infrastructure should fit for future features such as: VQ 
management, LM management and more.

I think it does. Now the TG need to review and agree.

If we'll start talking about LM during this series review we will end up 
again with nothing merged to the spec and waste more precious time.

So I'm taking the bits above into account for the internal LM work that 
I'm preparing for the future (after we'll merge the current series).

agreed ?

>
>
>
>> Initial feedback will be great for this series since every rebase cost a
>> lot... and it grows if we add more caps and logic.
>>
>>>
>>>> Also introduce the admin facility to allow manipulating features and configurations
>>>> in a generic manner. Using the admin command set, one can manipulate the device itself
>>>> and/or to manipulate, if possible, another device within the same virtio subsystem (the
>>>> following patch set).
>>>>
>>>> The admin virtqueue is the first management interface to issue Admin commands from
>>>> the admin command set.
>>>>
>>>> The admin virtqueue interface will be extended in the future with more and more
>>>> features that some of them already in discussions. Some of these features don't
>>>> fit to MMIO/config_space characteristics, therefore a queue is selected to address
>>>> admin commands.
>>>>
>>>> Motivation for choosing admin queue:
>>>> 1. It is anticipated that admin queue will be used for managing and configuring
>>>>      many different type of resources. For example,
>>>>      a. PCI PF configuring PCI VF attributes.
>>>>      b. virtio device creating/destroying/configuring subfunctions discussed in [1]
>>>>      c. composing device config space of VF or SF such as mac address, number of VQs, virtio features
>>>>
>>>>      Mapping all of them as configuration registers to MMIO will require large MMIO space,
>>>>      if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
>>>>      requires on-chip resources to complete within MMIO access latencies. Such resources are very
>>>>      expensive.
>>>>
>>>> 2. Such limitation can be overcome by having smaller MMIO register set to build
>>>>      a command request response interface. However, such MMIO based command interface
>>>>      will be limited to serve single outstanding command execution. Such limitation can
>>>>      resulting in high device creation and composing time which can affect VM startup time.
>>>>      Often device can queue and service multiple commands in parallel, such command interface
>>>>      cannot use parallelism offered by the device.
>>>>
>>>> 3. When a command wants to DMA data from one or more physical addresses, for example in the future a
>>>>      live migration command may need to fetch device state consist of config space, tens of
>>>>      VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
>>>>      Packing one or more DMA addresses over new command interface will be burden some and continue
>>>>      to suffer single outstanding command execution latencies. Such limitation is not good for time
>>>>      sensitive live migration use cases.
>>>>
>>>> 4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
>>>>      descriptors. Similar mechanism exist today for device specific configuration - the control VQ.
>>>>
>>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F202108%2Fmsg00025.html&data=04%7C01%7Cmgurtovoy%40nvidia.com%7C99739e80d00a4fe3394b08da02947d10%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637825134024839108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PSDymc6cSXtjry%2BIx2oqSZUbdAaffmUov%2BMApHPnAbY%3D&reserved=0
>>>>
>>>> This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
>>>> This series include the comments and fixes from V1-V3 of the initial patch set from above.
>>>> The following series introduce the management devices and MSI-X configuration of virtio devices.
>>>>
>>>> Open issues:
>>>> 1. CCW and MMIO specification for admin_queue_index register
>>>>
>>>> Max Gurtovoy (5):
>>>>     virtio: Introduce virtio subsystem
>>>>     Introduce Admin Command Set
>>>>     Introduce DEVICE INFO Admin command
>>>>     Add virtio Admin virtqueue
>>>>     Add miscellaneous configuration structure for PCI
>>>>
>>>>    admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    conformance.tex  |   3 +
>>>>    content.tex      |  33 ++++++++-
>>>>    introduction.tex |  20 ++++++
>>>>    4 files changed, 231 insertions(+), 2 deletions(-)
>>>>    create mode 100644 admin.tex
>>>>
>>>> -- 
>>>> 2.21.0

Re: [virtio-comment] Re: [PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Michael S. Tsirkin]

On Thu, Mar 10, 2022 at 03:08:54PM +0200, Max Gurtovoy wrote:
> 
> On 3/10/2022 2:49 PM, Michael S. Tsirkin wrote:
> > On Thu, Mar 10, 2022 at 12:38:38PM +0200, Max Gurtovoy wrote:
> > > On 3/9/2022 9:42 AM, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 05:56:03PM +0200, Max Gurtovoy wrote:
> > > > > Hi,
> > > > > A virtio subsystem definition will help extending the virtio specefication for
> > > > > various future features that require a notion of grouping devices together or
> > > > > managing devices inside a group. It also might be used splitting or sharing a
> > > > > single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
> > > > > devices). A virtio subsystem include one or more virtio devices.
> > > > A large patch, need a bit more time for review. Meanwhile,
> > > > how about adding migration related capabilities?
> > > > I would very much like that to make progress before
> > > > people start using high overhead solutions like
> > > > VQ shadowing.
> > > Sure I can start working on rebasing old LM proposal to virtio subsystem
> > > framework.
> > > 
> > > But can you be precise for what you mean capabilities ? only caps without
> > > the commands and LM logic ?
> > There are at least four distinct bits, and they can be worked on mostly
> > separately:
> > 
> > 
> > 1.  We need a bunch of stuff to migrate a device to a different host right?
> > - device specific state
> > - transport state
> > - vq ring state
> > and of course we need
> > - ability to stop/resume device
> > This is useful by itself e.g. for snapshoting.
> > 
> > Then to reduce downtime we also need to run device during memory
> > migration, which requires support for
> > 
> > 2. page faults (postcopy) and optionally
> > 3. dirty tracking (precopy) - though dirty tracking can be done
> > with faults too, so maybe just faults.
> > Faults are definitely useful for a bunch of stuff like memory migration.
> > Dirty tracking is more of a boutique feature, but I guess uses
> > beyond memory migration can still be found.
> > 
> > 4.  Finally, feature compatibility is a problem: not any configuration of a
> > device can be migrated to any other device. A simplest example is a
> > device feature not present on destination. Can be solved by not exposing
> > the feature to the guest. Another example is layout of pci configuration
> > space. Spec allows a lot of flexibility here, however things like
> > # of VQs will affect the memory bar size.
> > I am not exactly sure what we want to do in this space, maybe for
> > starters enumerating what are the things that need to match on source
> > and destination?
> > We can start with a non-normative sections describing the issues
> > generally at least.
> 
> MST,
> 
> I really like us to push these 5 patches before we deep dive to LM stuff.
> 
> This was our plan we agreed together - push infrastructure with relatively
> small feature (we choose MSIX management) to the spec.
>
> This infrastructure should fit for future features such as: VQ management,
> LM management and more.
> 
> I think it does. Now the TG need to review and agree.
> 
> If we'll start talking about LM during this series review we will end up
> again with nothing merged to the spec and waste more precious time.

I am not sure that last sentence is true. Or to be more precise,
yes it's possible that the fastest way to merge admin queue
proposal is to avoid making sure it solves live migration, but
admin queue is not an end by itself.

> So I'm taking the bits above into account for the internal LM work that I'm
> preparing for the future (after we'll merge the current series).
> 
> agreed ?

My advice is always to do work in the open and publish drafts of
the work even if it's not ready, but be very clear and open about
what is and what is not ready, including a TODO list in the
commit log. You can tag it RFC in subject and make it PATCH 6/5
so it's clear to people that it's a POC and not a final patch.
In particular it will be helpful to show that admin queue is
actually a good fit for this purpose.


> > 
> > 
> > 
> > > Initial feedback will be great for this series since every rebase cost a
> > > lot... and it grows if we add more caps and logic.
> > > 
> > > > 
> > > > > Also introduce the admin facility to allow manipulating features and configurations
> > > > > in a generic manner. Using the admin command set, one can manipulate the device itself
> > > > > and/or to manipulate, if possible, another device within the same virtio subsystem (the
> > > > > following patch set).
> > > > > 
> > > > > The admin virtqueue is the first management interface to issue Admin commands from
> > > > > the admin command set.
> > > > > 
> > > > > The admin virtqueue interface will be extended in the future with more and more
> > > > > features that some of them already in discussions. Some of these features don't
> > > > > fit to MMIO/config_space characteristics, therefore a queue is selected to address
> > > > > admin commands.
> > > > > 
> > > > > Motivation for choosing admin queue:
> > > > > 1. It is anticipated that admin queue will be used for managing and configuring
> > > > >      many different type of resources. For example,
> > > > >      a. PCI PF configuring PCI VF attributes.
> > > > >      b. virtio device creating/destroying/configuring subfunctions discussed in [1]
> > > > >      c. composing device config space of VF or SF such as mac address, number of VQs, virtio features
> > > > > 
> > > > >      Mapping all of them as configuration registers to MMIO will require large MMIO space,
> > > > >      if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
> > > > >      requires on-chip resources to complete within MMIO access latencies. Such resources are very
> > > > >      expensive.
> > > > > 
> > > > > 2. Such limitation can be overcome by having smaller MMIO register set to build
> > > > >      a command request response interface. However, such MMIO based command interface
> > > > >      will be limited to serve single outstanding command execution. Such limitation can
> > > > >      resulting in high device creation and composing time which can affect VM startup time.
> > > > >      Often device can queue and service multiple commands in parallel, such command interface
> > > > >      cannot use parallelism offered by the device.
> > > > > 
> > > > > 3. When a command wants to DMA data from one or more physical addresses, for example in the future a
> > > > >      live migration command may need to fetch device state consist of config space, tens of
> > > > >      VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
> > > > >      Packing one or more DMA addresses over new command interface will be burden some and continue
> > > > >      to suffer single outstanding command execution latencies. Such limitation is not good for time
> > > > >      sensitive live migration use cases.
> > > > > 
> > > > > 4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
> > > > >      descriptors. Similar mechanism exist today for device specific configuration - the control VQ.
> > > > > 
> > > > > [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F202108%2Fmsg00025.html&data=04%7C01%7Cmgurtovoy%40nvidia.com%7C99739e80d00a4fe3394b08da02947d10%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637825134024839108%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=PSDymc6cSXtjry%2BIx2oqSZUbdAaffmUov%2BMApHPnAbY%3D&reserved=0
> > > > > 
> > > > > This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
> > > > > This series include the comments and fixes from V1-V3 of the initial patch set from above.
> > > > > The following series introduce the management devices and MSI-X configuration of virtio devices.
> > > > > 
> > > > > Open issues:
> > > > > 1. CCW and MMIO specification for admin_queue_index register
> > > > > 
> > > > > Max Gurtovoy (5):
> > > > >     virtio: Introduce virtio subsystem
> > > > >     Introduce Admin Command Set
> > > > >     Introduce DEVICE INFO Admin command
> > > > >     Add virtio Admin virtqueue
> > > > >     Add miscellaneous configuration structure for PCI
> > > > > 
> > > > >    admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    conformance.tex  |   3 +
> > > > >    content.tex      |  33 ++++++++-
> > > > >    introduction.tex |  20 ++++++
> > > > >    4 files changed, 231 insertions(+), 2 deletions(-)
> > > > >    create mode 100644 admin.tex
> > > > > 
> > > > > -- 
> > > > > 2.21.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://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [PATCH v1 0/5] Introduce virtio subsystem and Admin virtqueue

[Max Gurtovoy]

On 3/20/2022 11:41 PM, Michael S. Tsirkin wrote:
> On Thu, Mar 10, 2022 at 03:08:54PM +0200, Max Gurtovoy wrote:
>> On 3/10/2022 2:49 PM, Michael S. Tsirkin wrote:
>>> On Thu, Mar 10, 2022 at 12:38:38PM +0200, Max Gurtovoy wrote:
>>>> On 3/9/2022 9:42 AM, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 02, 2022 at 05:56:03PM +0200, Max Gurtovoy wrote:
>>>>>> Hi,
>>>>>> A virtio subsystem definition will help extending the virtio specefication for
>>>>>> various future features that require a notion of grouping devices together or
>>>>>> managing devices inside a group. It also might be used splitting or sharing a
>>>>>> single virtio backend between multiple devices (e.g. Multipath IO for virtio-blk
>>>>>> devices). A virtio subsystem include one or more virtio devices.
>>>>> A large patch, need a bit more time for review. Meanwhile,
>>>>> how about adding migration related capabilities?
>>>>> I would very much like that to make progress before
>>>>> people start using high overhead solutions like
>>>>> VQ shadowing.
>>>> Sure I can start working on rebasing old LM proposal to virtio subsystem
>>>> framework.
>>>>
>>>> But can you be precise for what you mean capabilities ? only caps without
>>>> the commands and LM logic ?
>>> There are at least four distinct bits, and they can be worked on mostly
>>> separately:
>>>
>>>
>>> 1.  We need a bunch of stuff to migrate a device to a different host right?
>>> - device specific state
>>> - transport state
>>> - vq ring state
>>> and of course we need
>>> - ability to stop/resume device
>>> This is useful by itself e.g. for snapshoting.
>>>
>>> Then to reduce downtime we also need to run device during memory
>>> migration, which requires support for
>>>
>>> 2. page faults (postcopy) and optionally
>>> 3. dirty tracking (precopy) - though dirty tracking can be done
>>> with faults too, so maybe just faults.
>>> Faults are definitely useful for a bunch of stuff like memory migration.
>>> Dirty tracking is more of a boutique feature, but I guess uses
>>> beyond memory migration can still be found.
>>>
>>> 4.  Finally, feature compatibility is a problem: not any configuration of a
>>> device can be migrated to any other device. A simplest example is a
>>> device feature not present on destination. Can be solved by not exposing
>>> the feature to the guest. Another example is layout of pci configuration
>>> space. Spec allows a lot of flexibility here, however things like
>>> # of VQs will affect the memory bar size.
>>> I am not exactly sure what we want to do in this space, maybe for
>>> starters enumerating what are the things that need to match on source
>>> and destination?
>>> We can start with a non-normative sections describing the issues
>>> generally at least.
>> MST,
>>
>> I really like us to push these 5 patches before we deep dive to LM stuff.
>>
>> This was our plan we agreed together - push infrastructure with relatively
>> small feature (we choose MSIX management) to the spec.
>>
>> This infrastructure should fit for future features such as: VQ management,
>> LM management and more.
>>
>> I think it does. Now the TG need to review and agree.
>>
>> If we'll start talking about LM during this series review we will end up
>> again with nothing merged to the spec and waste more precious time.
> I am not sure that last sentence is true. Or to be more precise,
> yes it's possible that the fastest way to merge admin queue
> proposal is to avoid making sure it solves live migration, but
> admin queue is not an end by itself.

Not by itself, of course. We use it for other features such as MSIX 
configuration (that I posted in the TG mailing list several months ago), 
remember ?

And it can be extended to other features by other members as well as 
soon as we'll merge it.

>
>> So I'm taking the bits above into account for the internal LM work that I'm
>> preparing for the future (after we'll merge the current series).
>>
>> agreed ?
> My advice is always to do work in the open and publish drafts of
> the work even if it's not ready, but be very clear and open about
> what is and what is not ready, including a TODO list in the
> commit log. You can tag it RFC in subject and make it PATCH 6/5
> so it's clear to people that it's a POC and not a final patch.
> In particular it will be helpful to show that admin queue is
> actually a good fit for this purpose.

We already agreed that admin queue is a good fit. You said it in your 
own words.

I would like us to continue our initial plan that you proposed and that 
we build a plan of records according to it.

Changing strategy in V5 is not something we should do.

Lets stick to the original plan please.

Any comments for this series ? Cornelia/Jason ? or can we merge it as-is ?

>
>
>>>
>>>
>>>> Initial feedback will be great for this series since every rebase cost a
>>>> lot... and it grows if we add more caps and logic.
>>>>
>>>>>> Also introduce the admin facility to allow manipulating features and configurations
>>>>>> in a generic manner. Using the admin command set, one can manipulate the device itself
>>>>>> and/or to manipulate, if possible, another device within the same virtio subsystem (the
>>>>>> following patch set).
>>>>>>
>>>>>> The admin virtqueue is the first management interface to issue Admin commands from
>>>>>> the admin command set.
>>>>>>
>>>>>> The admin virtqueue interface will be extended in the future with more and more
>>>>>> features that some of them already in discussions. Some of these features don't
>>>>>> fit to MMIO/config_space characteristics, therefore a queue is selected to address
>>>>>> admin commands.
>>>>>>
>>>>>> Motivation for choosing admin queue:
>>>>>> 1. It is anticipated that admin queue will be used for managing and configuring
>>>>>>       many different type of resources. For example,
>>>>>>       a. PCI PF configuring PCI VF attributes.
>>>>>>       b. virtio device creating/destroying/configuring subfunctions discussed in [1]
>>>>>>       c. composing device config space of VF or SF such as mac address, number of VQs, virtio features
>>>>>>
>>>>>>       Mapping all of them as configuration registers to MMIO will require large MMIO space,
>>>>>>       if done for each VF/SF. Such MMIO implementation in physical devices such as PCI PF and VF
>>>>>>       requires on-chip resources to complete within MMIO access latencies. Such resources are very
>>>>>>       expensive.
>>>>>>
>>>>>> 2. Such limitation can be overcome by having smaller MMIO register set to build
>>>>>>       a command request response interface. However, such MMIO based command interface
>>>>>>       will be limited to serve single outstanding command execution. Such limitation can
>>>>>>       resulting in high device creation and composing time which can affect VM startup time.
>>>>>>       Often device can queue and service multiple commands in parallel, such command interface
>>>>>>       cannot use parallelism offered by the device.
>>>>>>
>>>>>> 3. When a command wants to DMA data from one or more physical addresses, for example in the future a
>>>>>>       live migration command may need to fetch device state consist of config space, tens of
>>>>>>       VQs state, VLAN and MAC table, per VQ partial outstanding block IO list database and more.
>>>>>>       Packing one or more DMA addresses over new command interface will be burden some and continue
>>>>>>       to suffer single outstanding command execution latencies. Such limitation is not good for time
>>>>>>       sensitive live migration use cases.
>>>>>>
>>>>>> 4. A virtio queue overcomes all the above limitations. It also supports DMA and multiple outstanding
>>>>>>       descriptors. Similar mechanism exist today for device specific configuration - the control VQ.
>>>>>>
>>>>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F202108%2Fmsg00025.html&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=%2BPEha9meUnHSg1fiwm7Z7Pv6UgvCLeLka3A2THirvt8%3D&reserved=0
>>>>>>
>>>>>> This series was extended and splitted from the V3 of the "VIRTIO: Provision maximum MSI-X vectors for a VF".
>>>>>> This series include the comments and fixes from V1-V3 of the initial patch set from above.
>>>>>> The following series introduce the management devices and MSI-X configuration of virtio devices.
>>>>>>
>>>>>> Open issues:
>>>>>> 1. CCW and MMIO specification for admin_queue_index register
>>>>>>
>>>>>> Max Gurtovoy (5):
>>>>>>      virtio: Introduce virtio subsystem
>>>>>>      Introduce Admin Command Set
>>>>>>      Introduce DEVICE INFO Admin command
>>>>>>      Add virtio Admin virtqueue
>>>>>>      Add miscellaneous configuration structure for PCI
>>>>>>
>>>>>>     admin.tex        | 177 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     conformance.tex  |   3 +
>>>>>>     content.tex      |  33 ++++++++-
>>>>>>     introduction.tex |  20 ++++++
>>>>>>     4 files changed, 231 insertions(+), 2 deletions(-)
>>>>>>     create mode 100644 admin.tex
>>>>>>
>>>>>> -- 
>>>>>> 2.21.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&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=HaslbICc4GrV%2FesN2KO%2BsA12Gif%2Fbmi0%2BSzkLXPJvFU%3D&reserved=0
>> Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=R2lnpEmQ3nfDXQOYkRzkdzitviwOLEuhYDMQKkchaOc%3D&reserved=0
>> List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Uc8pPeEvC7m2Afui8b8%2Fxk0J5bUlFSsjZ3Jsr%2BQsBY0%3D&reserved=0
>> Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TCrF3rJ2I94Zgevi4DaTeI2mO%2FL69CarkYs11UgaNRg%3D&reserved=0
>> Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Jkph2NA2cZKAvPmj0zcgypu%2BtwI0yVH%2Bpo4G8ndmAKc%3D&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&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=HaslbICc4GrV%2FesN2KO%2BsA12Gif%2Fbmi0%2BSzkLXPJvFU%3D&reserved=0
> Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=R2lnpEmQ3nfDXQOYkRzkdzitviwOLEuhYDMQKkchaOc%3D&reserved=0
> List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Uc8pPeEvC7m2Afui8b8%2Fxk0J5bUlFSsjZ3Jsr%2BQsBY0%3D&reserved=0
> Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=TCrF3rJ2I94Zgevi4DaTeI2mO%2FL69CarkYs11UgaNRg%3D&reserved=0
> Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&data=04%7C01%7Cmgurtovoy%40nvidia.com%7Cf061ce52d05b4d41f26508da0aba6bfa%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637834093644643841%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Jkph2NA2cZKAvPmj0zcgypu%2BtwI0yVH%2Bpo4G8ndmAKc%3D&reserved=0
>

[virtio-dev] Re: [PATCH v1 1/5] virtio: Introduce virtio subsystem

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 05:56:04PM +0200, Max Gurtovoy wrote:
> A virtio subsystem may contain one or more virtio devices. All of the
> devices that make up a virtio subsystem share the same virtio subsystem
> unique identifier. This identifier is the virtio qualified name (VQN).
> Each device within a virtio subsystem has a unique identifier. This
> identifier is the virtio device id (vdev_id). The combination of these
> identifiers forms a globally unique value identifies a virtio device.
> 
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

OK linux and qemu releases finally settling down so I have the time
to review this, sorry about the delay.

> ---
>  introduction.tex | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/introduction.tex b/introduction.tex
> index 6d52717..8e6611e 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -240,5 +240,23 @@ \section{Constant Specifications}
>  refer to values 1 and 2 of Fld respectively. Further, VIRTIO_FLD_XXX refers to
>  either VIRTIO_FLD_A or VIRTIO_FLD_B.
>  
> +\section{Definitions}\label{sec:Introduction / Definitions}

I think this belongs in Terminology section.

> +
> +\subsection{virtio device}\label{sec:Introduction / Definitions / virtio device}
> +
> +An entity that implements virtio specification.

Applies to a driver equally ... if we are trying to explain it further,
then I would say something along the lines of:

virtio specifies an interface
for a two-way communication between two parties called
a device and a driver. driver is the party that initiates
the communication setup and initialization.
in common usage device is part of a hypervisor and driver part
of a guest running within a VM on this hypervisor,
but other uses are not precluded.


> +\subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subsystem}
> +
> +A virtio subsystem includes one or more virtio devices.
> +Each virtio subsystem has a unique virtio qualified name (VQN) that is permanent for the lifetime of the virtio subsystem.

the naming here is unfortunate. "subsystem" generally refers to
something like a group of drivers within linux, and to a type of
device in pci. VQN refers to a VQ number in the spec.

I would suggest simply "device group".

And please preface this with a bit of informal text explaining that
sometimes it is useful to refer to a group of devices
as a whole.


> +The VQN is a 128-bit UUID. It is RECOMMENDED to use UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.


Do we really need the UUID at all? It is unique within which context?
Can we split the UUID from this proposal?

> +Virtio devices within one virtio subsystem share the same VQN.

share in which sense? I don't see any way to retrieve the VQN from
the device itself.

> Each virtio device has a unique virtio
> +device id (vdev_id) within a virtio subsystem. A valid vdev_id is a 64-bit field in the range of
> +0x0 - 0xFFFFFFFFFFFFFFF0.

are other values reserved?

> Vdev_id 0xFFFFFFFFFFFFFFFF is a broadcast value that is used to specify all the
> +virtio devices in a virtio subsystem and isn't a valid vdev_id.

broadcast is a bit confusing. Just say "Vdev_id value 0xFFFFFFFFFFFFFFFF
refers to all devices in a group".

I think we should also add a special value meaning "this device itself".

> +
> +The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
> +
>  \newpage



> -- 
> 2.21.0


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

Re: [PATCH v1 2/5] Introduce Admin Command Set

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote:
> This command set is used for essential administrative and management
> operations such as identify and resource management.
> 
> Admin commands should be submitted to a well defined management
> interface.
> 
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
>  content.tex      |   2 +
>  introduction.tex |   2 +
>  3 files changed, 133 insertions(+)
>  create mode 100644 admin.tex
> 
> diff --git a/admin.tex b/admin.tex
> new file mode 100644
> index 0000000..1e30e01
> --- /dev/null
> +++ b/admin.tex
> @@ -0,0 +1,129 @@
> +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}

> +
> +The Admin command set defines the commands that can be issued to a well defined management
> +interface.

This is shorthand for what? Administration commands?
Let's eschew abbreviation pls.
Also, management is a bit oblique, and only the reader can judge how well it's defined :)


I would rephrase this along the lines of

" For security and ease of management reasons, devices can expose an additional interface
  to access the device besides the ones specified in the
  transports section. For example, a host management system
  might want to access the device while in use by driver,
  or to configure the device before it is initialized by
  the driver.
  This access is facilitated through a set of administration commands.

"


speaking of this, are we still going to use the "driver"
terminology for uses of this?


> All the Admin commands are of the following form:
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd {
> +        /* Device-readable part */
> +        le16 command;
> +        /*
> +         * 0 - self
> +         * 1 - 65535 are reserved
> +         */
> +        le16 dst_type;

I think we want to add the vdev id here in the generic command
and just have a special id that means "self".


> +        /* reserved for common cmd fields */
> +        u8 reserved[20];
> +        u8 command_specific_data[];
> +
> +        /* Device-writable part */
> +        u8 status;
> +        u8 command_specific_error;
> +        u8 command_specific_result[];
> +};
> +\end{lstlisting}
> +
> +The following table describes the generic Admin status codes:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
> +\hline
> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
> +\hline
> +\end{tabular}
> +
> +The \field{command}, \field{dst_type} and \field{command_specific_data} are
> +set by the driver, and the device sets the \field{status}, the
> +\field{command_specific_error} and the \field{command_specific_result},
> +if needed.
> +
> +Reserved common fields are ignored by the device and to be zeroed by the driver.
> +
> +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
> +
> +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
> +
> +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
> +
> +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 content 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.
> +
> +The following table describes the Admin command set:
> +
> +\begin{tabular}{|l|l|l|}
> +\hline
> +Opcode & Command & M/O \\
> +\hline \hline
> +0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
> +\hline
> +0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
> +\hline
> +0002h - 7FFFh   & Generic admin cmds    & -  \\
> +\hline
> +8000h - FFFFh   & Reserved    & - \\
> +\hline
> +\end{tabular}
> +
> +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
> +
> +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
> +Other common fields are reserved for this command and zeroed.
> +
> +This command upon success, returns a data buffer

let's avoid using buffer since we want to allow non-DMA uses
IIUC. just "result" is ok, right?

> that describes information about the target virtio device.

target being what? destination?

> +This returned data buffer is of form:
> +\begin{lstlisting}
> +struct virtio_admin_device_identify_result {
> +       /* For compatibility - indicates which of the below fields were returned

compatibility with what?

> +        * (1 means that field was returned):
> +        * Bit 0 - vdev_id
> +        * Bit 1 - optional_caps_support
> +        * Bits 2 - 63 - reserved for future fields
> +        */
> +       le64 attrs_mask;

this seems to be some kind of thing listing supported commands, except
it's one way as opposed to negotiated like feature bits.  From
experience this kind of one way capability might be problematic since
you never know what will be used.  How about just using feature bits
instead? How many of these do you expect to be there?

If not, please add a description of this.
E.g. along the lines of

	Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ??



> +       le64 vdev_id;

So this is a way to query the id of device itself? Since the only dst
type is self ...


> +       /* This field indicates which of the below optional admin
> +        * capabilities are supported by the device:
> +        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
> +        * Bits 1 - 63 - reserved for future capabilities.
> +        */
> +       le64 optional_caps_support;
> +       u8 reserved[4072];

What exactly is this reserved thing? Does this mean the structure is
always exactly 4k?

> +};
> +\end{lstlisting}
> +
> +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
> +
> +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
> +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
> +Other common fields are reserved for this command and zeroed.

zeroed by driver?

> +
> +This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
> +This returned data buffer is of form:
> +\begin{lstlisting}
> +struct virtio_admin_subsystem_identify_result {
> +       /* For compatibility - indicates which of the below fields were returned
> +        * (1 means that field was returned):
> +        * Bit 0 - vqn
> +        * Bit 1 - num_supported_vdevs
> +        * Bit 2 - max_vdev_id
> +        * Bits 3 - 63 - reserved for future fields
> +        */
> +       le64 attrs_mask;

add description here.

> +       u8 vqn[16];
> +       /* Number of supported virtio devices by the subsystem */
> +       le64 num_supported_vdevs;
> +       /* Maximum value of a valid vdev_id for the subsystem */
> +       le64 max_vdev_id;
> +       u8 reserved[4056];
> +};
> +\end{lstlisting}
> diff --git a/content.tex b/content.tex
> index c6f116c..2e1df84 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
> diff --git a/introduction.tex b/introduction.tex
> index 8e6611e..94dd7a2 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
>  
>  The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
>  
> +VQN and vdev_id are exposed via Admin Command Set.
> +
>  \newpage
>  
> -- 
> 2.21.0

Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
> The DEVICE INFO command will return a basic information for a virtio
> device.
> 
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  admin.tex | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/admin.tex b/admin.tex
> index 1e30e01..9a8969b 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
>  \hline
>  0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>  \hline
> -0002h - 7FFFh   & Generic admin cmds    & -  \\
> +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
> +\hline
> +0003h - 7FFFh   & Generic admin cmds    & -  \\
>  \hline
>  8000h - FFFFh   & Reserved    & - \\
>  \hline
> @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
>         /* This field indicates which of the below optional admin
>          * capabilities are supported by the device:
>          * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
> -        * Bits 1 - 63 - reserved for future capabilities.
> +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
> +        * Bits 2 - 63 - reserved for future capabilities.
>          */
>         le64 optional_caps_support;
>         u8 reserved[4072];
> @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
>         u8 reserved[4056];
>  };
>  \end{lstlisting}
> +
> +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
> +
> +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
> +
> +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
> +Upon success, the returned data buffer is of form:
> +\begin{lstlisting}
> +struct virtio_admin_device_info_result {
> +        /* For compatibility - indicates which of the below fields were returned
> +         * (1 means that field was returned):

So how is this supposed to be handled. You need to write this up.
Also having each command have a bitmap seems like an overkill
to me. But if that is the way you are going just put
this in the generic part.


> +         * Bit 0 - vf_number
> +         * Bits 1 - 63 - reserved for future fields

we generally say "future use" not "future fields".

> +         */
> +        le64 attrs_mask;
> +        /* The virtual function number */
> +        le16 vf_number;
> +        u8 reserved[1014];
> +};
> +\end{lstlisting}
> +
> +\begin{note}
> +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
> +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
> +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
> +\end{note}


I personally would just make vf # *be* the device ID, with no need for an
extra indirection. It doesn't hurt much though ...

But I do not seem to understand how it all works here. I think a generic
description explaining how all this is used is necessary, pls add a
writeup.

E.g.

So I start with a device and through a feature bit I find out it
supports admin commands. Then I query that and find the max vdev id.
Query the ID of each device. Then through that query VF number.
Something like this.

Looks in device operation section for some examples.


> +
> -- 
> 2.21.0

Re: [PATCH v1 4/5] Add virtio Admin virtqueue

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 05:56:07PM +0200, Max Gurtovoy wrote:
> 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). Some of this configuration is generic enough. i.e
> Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
> such features query and manipulation by its parent PCI PF. 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.
> 
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  admin.tex       | 17 +++++++++++++++++
>  conformance.tex |  1 +
>  content.tex     |  6 ++++--
>  3 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/admin.tex b/admin.tex
> index 9a8969b..ed9f6bd 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -158,3 +158,20 @@ \subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Vi
>  specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
>  \end{note}
>  
> +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
> +
> +An admin virtqueue is a management interface of a device that can be used to send administrative
> +commands to manipulate various features of the device and/or to manipulate
> +various features, if possible, of another device within the same virtio subsystem
> +(see \ref{sec:Introduction / Definitions / virtio subsystem}).
> +
> +An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ is
> +negotiated. The index of the admin virtqueue is exposed by the device in a
> +transport specific manner.
> +
> +If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin virtqueue to send all admin commands.
> +

Add links please, and explain how do commands map to VQ buffers.
E.g. I guess device readable an output buffer and device writeable an input
buffer...

> +\devicenormative{\subsection}{Admin Virtqueues}{Basic Facilities of a Virtio Device / Admin Virtqueues}
> +A device that advertises VIRTIO_F_ADMIN_VQ capability MUST support all the mandatory admin commands.
> +
> +A device that advertises VIRTIO_F_ADMIN_VQ capability MAY support one or more optional admin commands.
> diff --git a/conformance.tex b/conformance.tex
> index 42f8537..129831c 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -341,6 +341,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Available Buffer Notification Suppression}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
>  \item \ref{devicenormative:Reserved Feature Bits}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Admin Virtqueues}
>  \end{itemize}
>  
>  \conformance{\subsection}{PCI Device Conformance}\label{sec:Conformance / Device Conformance / PCI Device Conformance}
> diff --git a/content.tex b/content.tex
> index 2e1df84..163cb34 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}
> @@ -6849,6 +6849,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}
> -- 
> 2.21.0

Re: [PATCH v1 5/5] Add miscellaneous configuration structure for PCI

[Michael S. Tsirkin]

On Wed, Mar 02, 2022 at 05:56:08PM +0200, Max Gurtovoy wrote:
> This new structure will be used for adding new miscellaneous registers
> for a virtio device configuration layout.
> 
> For now, only admin_queue_index register is added. Admin virtqueue index
> does not depend on the device type. Hence, add a PCI capability to read
> the admin virtqueue index.
> 
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> ---
>  conformance.tex |  2 ++
>  content.tex     | 25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+)
> 
> diff --git a/conformance.tex b/conformance.tex
> index 129831c..e31645e 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -102,6 +102,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>  \end{itemize}
>  
>  \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> @@ -363,6 +364,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>  \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>  \end{itemize}
>  
>  \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> diff --git a/content.tex b/content.tex
> index 163cb34..bf46192 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -712,6 +712,7 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>  \item ISR Status
>  \item Device-specific configuration (optional)
>  \item PCI configuration access
> +\item Miscellaneous configuration
>  \end{itemize}
>  
>  Each structure can be mapped by a Base Address register (BAR) belonging to
> @@ -771,6 +772,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>  #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
>  /* Vendor-specific data */
>  #define VIRTIO_PCI_CAP_VENDOR_CFG        9
> +/* Miscellaneous configuration */
> +#define VIRTIO_PCI_CAP_MISC_CFG          10
>  \end{lstlisting}
>  
>          Any other value is reserved for future use.
> @@ -1352,6 +1355,28 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
>  specified by some other Virtio Structure PCI Capability
>  of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
>  
> +\subsubsection{Miscellaneous configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> +
> +The miscellaneous configuration structure is found at the bar and offset within the VIRTIO_PCI_CAP_MISC_CFG capability.
> +Its layout is below.
> +\begin{lstlisting}
> +struct virtio_pci_misc_cfg {
> +        le16 admin_queue_index;         /* read-only for driver */
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +\item[\field{admin_queue_index}]
> +        The device uses this to report the index of the admin virtqueue.
> +        This field is valid only if VIRTIO_F_ADMIN_VQ is set.
> +\end{description}
> +
> +\devicenormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> +The device MUST present a valid \field{admin_queue_index} when VIRTIO_F_ADMIN_VQ is set.
> +
> +\drivernormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> +The driver MUST use the value of \field{admin_queue_index} to configure the admin virtqueue. For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}.
> +
>  \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>  
>  Transitional devices MUST present part of configuration

This is useful, but I think we'll want this for all transports then.



> -- 
> 2.21.0

[virtio-comment] Re: [PATCH v1 1/5] virtio: Introduce virtio subsystem

[Max Gurtovoy]

On 4/4/2022 3:03 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 05:56:04PM +0200, Max Gurtovoy wrote:
>> A virtio subsystem may contain one or more virtio devices. All of the
>> devices that make up a virtio subsystem share the same virtio subsystem
>> unique identifier. This identifier is the virtio qualified name (VQN).
>> Each device within a virtio subsystem has a unique identifier. This
>> identifier is the virtio device id (vdev_id). The combination of these
>> identifiers forms a globally unique value identifies a virtio device.
>>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> OK linux and qemu releases finally settling down so I have the time
> to review this, sorry about the delay.
>
>> ---
>>   introduction.tex | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/introduction.tex b/introduction.tex
>> index 6d52717..8e6611e 100644
>> --- a/introduction.tex
>> +++ b/introduction.tex
>> @@ -240,5 +240,23 @@ \section{Constant Specifications}
>>   refer to values 1 and 2 of Fld respectively. Further, VIRTIO_FLD_XXX refers to
>>   either VIRTIO_FLD_A or VIRTIO_FLD_B.
>>   
>> +\section{Definitions}\label{sec:Introduction / Definitions}
> I think this belongs in Terminology section.

Ok, I can move all definitions to Terminology section.

>
>> +
>> +\subsection{virtio device}\label{sec:Introduction / Definitions / virtio device}
>> +
>> +An entity that implements virtio specification.
> Applies to a driver equally ... if we are trying to explain it further,
> then I would say something along the lines of:
>
> virtio specifies an interface
> for a two-way communication between two parties called
> a device and a driver. driver is the party that initiates
> the communication setup and initialization.
> in common usage device is part of a hypervisor and driver part
> of a guest running within a VM on this hypervisor,
> but other uses are not precluded.

I think this is true for the paravirt world. I wouldn't mention hyperv 
and guest in such generic chapters.

So lets do:

"virtio specifies an interface for a two-way communication between two 
parties called a device and a driver.

   driver is the party that initiates the communication setup and 
initialization. Device is the party that implements the virtio 
specification."


>
>
>> +\subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subsystem}
>> +
>> +A virtio subsystem includes one or more virtio devices.
>> +Each virtio subsystem has a unique virtio qualified name (VQN) that is permanent for the lifetime of the virtio subsystem.
> the naming here is unfortunate. "subsystem" generally refers to
> something like a group of drivers within linux, and to a type of

Not always.


> device in pci. VQN refers to a VQ number in the spec.
>
> I would suggest simply "device group".

So you prefer call it "virtio group" instead of "virtio subsystem" and 
rename VQN to VGQN (Virtio Group Qualified Name) ?

device group doesn't sounds right.

>
> And please preface this with a bit of informal text explaining that
> sometimes it is useful to refer to a group of devices
> as a whole.
>
>
>> +The VQN is a 128-bit UUID. It is RECOMMENDED to use UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>
> Do we really need the UUID at all? It is unique within which context?
> Can we split the UUID from this proposal?
Why ? This is the identifier of the group. This is the whole point of 
the proposal.
>> +Virtio devices within one virtio subsystem share the same VQN.
> share in which sense? I don't see any way to retrieve the VQN from
> the device itself.

Didn't you review the admin cmds in the next patch ?


>
>> Each virtio device has a unique virtio
>> +device id (vdev_id) within a virtio subsystem. A valid vdev_id is a 64-bit field in the range of
>> +0x0 - 0xFFFFFFFFFFFFFFF0.
> are other values reserved?

0xFFFFFFFFFFFFFFF1 - 0xFFFFFFFFFFFFFFFE are reserved.

>> Vdev_id 0xFFFFFFFFFFFFFFFF is a broadcast value that is used to specify all the
>> +virtio devices in a virtio subsystem and isn't a valid vdev_id.
> broadcast is a bit confusing. Just say "Vdev_id value 0xFFFFFFFFFFFFFFFF
> refers to all devices in a group".

Ok.

>
> I think we should also add a special value meaning "this device itself".

This is the id inside the group. The "device itself" reference is a 
property of commands.

No need for self value. Each device knows it's id.

>
>> +
>> +The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
>> +
>>   \newpage
>
>
>> -- 
>> 2.21.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://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 v1 2/5] Introduce Admin Command Set

[Max Gurtovoy]

On 4/4/2022 3:50 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote:
>> This command set is used for essential administrative and management
>> operations such as identify and resource management.
>>
>> Admin commands should be submitted to a well defined management
>> interface.
>>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
>>   content.tex      |   2 +
>>   introduction.tex |   2 +
>>   3 files changed, 133 insertions(+)
>>   create mode 100644 admin.tex
>>
>> diff --git a/admin.tex b/admin.tex
>> new file mode 100644
>> index 0000000..1e30e01
>> --- /dev/null
>> +++ b/admin.tex
>> @@ -0,0 +1,129 @@
>> +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}
>> +
>> +The Admin command set defines the commands that can be issued to a well defined management
>> +interface.
> This is shorthand for what? Administration commands?
> Let's eschew abbreviation pls.
> Also, management is a bit oblique, and only the reader can judge how well it's defined :)
>
>
> I would rephrase this along the lines of
>
> " For security and ease of management reasons, devices can expose an additional interface

Why should we add security here ?

Lets keep it simple.

>    to access the device besides the ones specified in the
>    transports section. For example, a host management system
>    might want to access the device while in use by driver,
>    or to configure the device before it is initialized by
>    the driver.
>    This access is facilitated through a set of administration commands.
>
> "
>
>
> speaking of this, are we still going to use the "driver"
> terminology for uses of this?

There is no other way to access a device from the host/guess other than 
driver.

A management system will have some interface provided by a driver to 
access a device.

>
>
>> All the Admin commands are of the following form:
>> +
>> +\begin{lstlisting}
>> +struct virtio_admin_cmd {
>> +        /* Device-readable part */
>> +        le16 command;
>> +        /*
>> +         * 0 - self
>> +         * 1 - 65535 are reserved
>> +         */
>> +        le16 dst_type;
> I think we want to add the vdev id here in the generic command
> and just have a special id that means "self".

vdev id was added in later patch. No need for special id means self. 
Each device has only 1 id and not 2.

This was proposed by Cornelia IIRC.

>
>
>> +        /* reserved for common cmd fields */
>> +        u8 reserved[20];
>> +        u8 command_specific_data[];
>> +
>> +        /* Device-writable part */
>> +        u8 status;
>> +        u8 command_specific_error;
>> +        u8 command_specific_result[];
>> +};
>> +\end{lstlisting}
>> +
>> +The following table describes the generic Admin status codes:
>> +
>> +\begin{tabular}{|l|l|l|}
>> +\hline
>> +Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
>> +\hline
>> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
>> +\hline
>> +\end{tabular}
>> +
>> +The \field{command}, \field{dst_type} and \field{command_specific_data} are
>> +set by the driver, and the device sets the \field{status}, the
>> +\field{command_specific_error} and the \field{command_specific_result},
>> +if needed.
>> +
>> +Reserved common fields are ignored by the device and to be zeroed by the driver.
>> +
>> +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
>> +
>> +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
>> +
>> +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
>> +
>> +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 content 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.
>> +
>> +The following table describes the Admin command set:
>> +
>> +\begin{tabular}{|l|l|l|}
>> +\hline
>> +Opcode & Command & M/O \\
>> +\hline \hline
>> +0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
>> +\hline
>> +0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>> +\hline
>> +0002h - 7FFFh   & Generic admin cmds    & -  \\
>> +\hline
>> +8000h - FFFFh   & Reserved    & - \\
>> +\hline
>> +\end{tabular}
>> +
>> +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
>> +
>> +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
>> +Other common fields are reserved for this command and zeroed.
>> +
>> +This command upon success, returns a data buffer
> let's avoid using buffer since we want to allow non-DMA uses
> IIUC. just "result" is ok, right?

How would device write data to host memory ?

driver prepare a buffer and device will write to this buffer.

>
>> that describes information about the target virtio device.
> target being what? destination?

yes.


>
>> +This returned data buffer is of form:
>> +\begin{lstlisting}
>> +struct virtio_admin_device_identify_result {
>> +       /* For compatibility - indicates which of the below fields were returned
> compatibility with what?
I will remove "compatibility". It just indicates the valid fields.
>
>> +        * (1 means that field was returned):
>> +        * Bit 0 - vdev_id
>> +        * Bit 1 - optional_caps_support
>> +        * Bits 2 - 63 - reserved for future fields
>> +        */
>> +       le64 attrs_mask;
> this seems to be some kind of thing listing supported commands, except
> it's one way as opposed to negotiated like feature bits.  From
> experience this kind of one way capability might be problematic since
> you never know what will be used.  How about just using feature bits
> instead? How many of these do you expect to be there?

It's not features.

I don't see a need for negotiation here.

>
> If not, please add a description of this.
> E.g. along the lines of
>
> 	Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ??

I described bit by bit and mentioned that this is a mask. For each bit 
the value of 1 is valid.


>
>
>> +       le64 vdev_id;
> So this is a way to query the id of device itself? Since the only dst
> type is self ...

For example yes. This is one of the things you can get from this command.

>
>
>> +       /* This field indicates which of the below optional admin
>> +        * capabilities are supported by the device:
>> +        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
>> +        * Bits 1 - 63 - reserved for future capabilities.
>> +        */
>> +       le64 optional_caps_support;
>> +       u8 reserved[4072];
> What exactly is this reserved thing? Does this mean the structure is
> always exactly 4k?

Yes the result buffer size is 4k.


>> +};
>> +\end{lstlisting}
>> +
>> +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
>> +
>> +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
>> +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
>> +Other common fields are reserved for this command and zeroed.
> zeroed by driver?

Yes.


>
>> +
>> +This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
>> +This returned data buffer is of form:
>> +\begin{lstlisting}
>> +struct virtio_admin_subsystem_identify_result {
>> +       /* For compatibility - indicates which of the below fields were returned
>> +        * (1 means that field was returned):
>> +        * Bit 0 - vqn
>> +        * Bit 1 - num_supported_vdevs
>> +        * Bit 2 - max_vdev_id
>> +        * Bits 3 - 63 - reserved for future fields
>> +        */
>> +       le64 attrs_mask;
> add description here.

Ok.

>> +       u8 vqn[16];
>> +       /* Number of supported virtio devices by the subsystem */
>> +       le64 num_supported_vdevs;
>> +       /* Maximum value of a valid vdev_id for the subsystem */
>> +       le64 max_vdev_id;
>> +       u8 reserved[4056];
>> +};
>> +\end{lstlisting}
>> diff --git a/content.tex b/content.tex
>> index c6f116c..2e1df84 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
>> diff --git a/introduction.tex b/introduction.tex
>> index 8e6611e..94dd7a2 100644
>> --- a/introduction.tex
>> +++ b/introduction.tex
>> @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
>>   
>>   The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
>>   
>> +VQN and vdev_id are exposed via Admin Command Set.
>> +
>>   \newpage
>>   
>> -- 
>> 2.21.0

Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Max Gurtovoy]

On 4/4/2022 3:57 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
>> The DEVICE INFO command will return a basic information for a virtio
>> device.
>>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   admin.tex | 35 +++++++++++++++++++++++++++++++++--
>>   1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/admin.tex b/admin.tex
>> index 1e30e01..9a8969b 100644
>> --- a/admin.tex
>> +++ b/admin.tex
>> @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
>>   \hline
>>   0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>>   \hline
>> -0002h - 7FFFh   & Generic admin cmds    & -  \\
>> +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
>> +\hline
>> +0003h - 7FFFh   & Generic admin cmds    & -  \\
>>   \hline
>>   8000h - FFFFh   & Reserved    & - \\
>>   \hline
>> @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
>>          /* This field indicates which of the below optional admin
>>           * capabilities are supported by the device:
>>           * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
>> -        * Bits 1 - 63 - reserved for future capabilities.
>> +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
>> +        * Bits 2 - 63 - reserved for future capabilities.
>>           */
>>          le64 optional_caps_support;
>>          u8 reserved[4072];
>> @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
>>          u8 reserved[4056];
>>   };
>>   \end{lstlisting}
>> +
>> +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
>> +
>> +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
>> +
>> +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
>> +Upon success, the returned data buffer is of form:
>> +\begin{lstlisting}
>> +struct virtio_admin_device_info_result {
>> +        /* For compatibility - indicates which of the below fields were returned
>> +         * (1 means that field was returned):
> So how is this supposed to be handled. You need to write this up.
> Also having each command have a bitmap seems like an overkill
> to me. But if that is the way you are going just put
> this in the generic part.
>
>
>> +         * Bit 0 - vf_number
>> +         * Bits 1 - 63 - reserved for future fields
> we generally say "future use" not "future fields".

ok.


>> +         */
>> +        le64 attrs_mask;
>> +        /* The virtual function number */
>> +        le16 vf_number;
>> +        u8 reserved[1014];
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{note}
>> +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
>> +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
>> +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
>> +\end{note}
>
> I personally would just make vf # *be* the device ID, with no need for an
> extra indirection. It doesn't hurt much though ...

This is implementation specific.

You need to have a way to identify the vf number.

>
> But I do not seem to understand how it all works here. I think a generic
> description explaining how all this is used is necessary, pls add a
> writeup.
>
> E.g.
>
> So I start with a device and through a feature bit I find out it
> supports admin commands. Then I query that and find the max vdev id.
> Query the ID of each device. Then through that query VF number.
> Something like this.
>
> Looks in device operation section for some examples.

Ok I'll try to add more explanation.


>
>
>> +
>> -- 
>> 2.21.0

Re: [PATCH v1 4/5] Add virtio Admin virtqueue

[Max Gurtovoy]

On 4/4/2022 4:02 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 05:56:07PM +0200, Max Gurtovoy wrote:
>> 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). Some of this configuration is generic enough. i.e
>> Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
>> such features query and manipulation by its parent PCI PF. 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.
>>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   admin.tex       | 17 +++++++++++++++++
>>   conformance.tex |  1 +
>>   content.tex     |  6 ++++--
>>   3 files changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/admin.tex b/admin.tex
>> index 9a8969b..ed9f6bd 100644
>> --- a/admin.tex
>> +++ b/admin.tex
>> @@ -158,3 +158,20 @@ \subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Vi
>>   specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
>>   \end{note}
>>   
>> +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
>> +
>> +An admin virtqueue is a management interface of a device that can be used to send administrative
>> +commands to manipulate various features of the device and/or to manipulate
>> +various features, if possible, of another device within the same virtio subsystem
>> +(see \ref{sec:Introduction / Definitions / virtio subsystem}).
>> +
>> +An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ is
>> +negotiated. The index of the admin virtqueue is exposed by the device in a
>> +transport specific manner.
>> +
>> +If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin virtqueue to send all admin commands.
>> +
> Add links please, and explain how do commands map to VQ buffers.

I added a link to the definitions section and I'll replace it with the 
new proposed terminology chapter.

What other links should be added ?

> E.g. I guess device readable an output buffer and device writeable an input
> buffer...

The buffers transfer from/to device/driver are using existing mechanism. 
Nothing new here.

It will just repeat existing chapters.

>
>> +\devicenormative{\subsection}{Admin Virtqueues}{Basic Facilities of a Virtio Device / Admin Virtqueues}
>> +A device that advertises VIRTIO_F_ADMIN_VQ capability MUST support all the mandatory admin commands.
>> +
>> +A device that advertises VIRTIO_F_ADMIN_VQ capability MAY support one or more optional admin commands.
>> diff --git a/conformance.tex b/conformance.tex
>> index 42f8537..129831c 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -341,6 +341,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Available Buffer Notification Suppression}
>>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
>>   \item \ref{devicenormative:Reserved Feature Bits}
>> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Admin Virtqueues}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{PCI Device Conformance}\label{sec:Conformance / Device Conformance / PCI Device Conformance}
>> diff --git a/content.tex b/content.tex
>> index 2e1df84..163cb34 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}
>> @@ -6849,6 +6849,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}
>> -- 
>> 2.21.0

Re: [PATCH v1 5/5] Add miscellaneous configuration structure for PCI

[Max Gurtovoy]

On 4/4/2022 4:04 PM, Michael S. Tsirkin wrote:
> On Wed, Mar 02, 2022 at 05:56:08PM +0200, Max Gurtovoy wrote:
>> This new structure will be used for adding new miscellaneous registers
>> for a virtio device configuration layout.
>>
>> For now, only admin_queue_index register is added. Admin virtqueue index
>> does not depend on the device type. Hence, add a PCI capability to read
>> the admin virtqueue index.
>>
>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>> ---
>>   conformance.tex |  2 ++
>>   content.tex     | 25 +++++++++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/conformance.tex b/conformance.tex
>> index 129831c..e31645e 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -102,6 +102,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
>> @@ -363,6 +364,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>>   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>   \end{itemize}
>>   
>>   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
>> diff --git a/content.tex b/content.tex
>> index 163cb34..bf46192 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -712,6 +712,7 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>>   \item ISR Status
>>   \item Device-specific configuration (optional)
>>   \item PCI configuration access
>> +\item Miscellaneous configuration
>>   \end{itemize}
>>   
>>   Each structure can be mapped by a Base Address register (BAR) belonging to
>> @@ -771,6 +772,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>>   #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
>>   /* Vendor-specific data */
>>   #define VIRTIO_PCI_CAP_VENDOR_CFG        9
>> +/* Miscellaneous configuration */
>> +#define VIRTIO_PCI_CAP_MISC_CFG          10
>>   \end{lstlisting}
>>   
>>           Any other value is reserved for future use.
>> @@ -1352,6 +1355,28 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
>>   specified by some other Virtio Structure PCI Capability
>>   of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
>>   
>> +\subsubsection{Miscellaneous configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>> +
>> +The miscellaneous configuration structure is found at the bar and offset within the VIRTIO_PCI_CAP_MISC_CFG capability.
>> +Its layout is below.
>> +\begin{lstlisting}
>> +struct virtio_pci_misc_cfg {
>> +        le16 admin_queue_index;         /* read-only for driver */
>> +};
>> +\end{lstlisting}
>> +
>> +\begin{description}
>> +\item[\field{admin_queue_index}]
>> +        The device uses this to report the index of the admin virtqueue.
>> +        This field is valid only if VIRTIO_F_ADMIN_VQ is set.
>> +\end{description}
>> +
>> +\devicenormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>> +The device MUST present a valid \field{admin_queue_index} when VIRTIO_F_ADMIN_VQ is set.
>> +
>> +\drivernormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>> +The driver MUST use the value of \field{admin_queue_index} to configure the admin virtqueue. For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}.
>> +
>>   \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>>   
>>   Transitional devices MUST present part of configuration
> This is useful, but I think we'll want this for all transports then.

I think that adding more transports can be incremental and should be a 
no-go for merging this patch set.

We discussed this in the previous version and nobody had a conclusion on 
the right approach for other transports.

>
>
>> -- 
>> 2.21.0

Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Michael S. Tsirkin]

On Mon, Apr 04, 2022 at 06:44:11PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 3:57 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
> > > The DEVICE INFO command will return a basic information for a virtio
> > > device.
> > > 
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > >   admin.tex | 35 +++++++++++++++++++++++++++++++++--
> > >   1 file changed, 33 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > index 1e30e01..9a8969b 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
> > >   \hline
> > >   0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
> > >   \hline
> > > -0002h - 7FFFh   & Generic admin cmds    & -  \\
> > > +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
> > > +\hline
> > > +0003h - 7FFFh   & Generic admin cmds    & -  \\
> > >   \hline
> > >   8000h - FFFFh   & Reserved    & - \\
> > >   \hline
> > > @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
> > >          /* This field indicates which of the below optional admin
> > >           * capabilities are supported by the device:
> > >           * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
> > > -        * Bits 1 - 63 - reserved for future capabilities.
> > > +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
> > > +        * Bits 2 - 63 - reserved for future capabilities.
> > >           */
> > >          le64 optional_caps_support;
> > >          u8 reserved[4072];
> > > @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
> > >          u8 reserved[4056];
> > >   };
> > >   \end{lstlisting}
> > > +
> > > +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
> > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
> > > +Upon success, the returned data buffer is of form:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_device_info_result {
> > > +        /* For compatibility - indicates which of the below fields were returned
> > > +         * (1 means that field was returned):
> > So how is this supposed to be handled. You need to write this up.
> > Also having each command have a bitmap seems like an overkill
> > to me. But if that is the way you are going just put
> > this in the generic part.
> > 
> > 
> > > +         * Bit 0 - vf_number
> > > +         * Bits 1 - 63 - reserved for future fields
> > we generally say "future use" not "future fields".
> 
> ok.
> 
> 
> > > +         */
> > > +        le64 attrs_mask;
> > > +        /* The virtual function number */
> > > +        le16 vf_number;
> > > +        u8 reserved[1014];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{note}
> > > +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
> > > +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
> > > +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
> > > +\end{note}
> > 
> > I personally would just make vf # *be* the device ID, with no need for an
> > extra indirection. It doesn't hurt much though ...
> 
> This is implementation specific.
> 
> You need to have a way to identify the vf number.

So my point is, I see no need to have a random set
of VFs or have an abstract ID as an indirection layer -
we can just say id == VF# and always manage all VF #s up to N.

This will cut out a bunch of commands.

And hey, if we see the need for an indirection down the road we
will be add it as a separate dst_type.

> > 
> > But I do not seem to understand how it all works here. I think a generic
> > description explaining how all this is used is necessary, pls add a
> > writeup.
> > 
> > E.g.
> > 
> > So I start with a device and through a feature bit I find out it
> > supports admin commands. Then I query that and find the max vdev id.
> > Query the ID of each device. Then through that query VF number.
> > Something like this.
> > 
> > Looks in device operation section for some examples.
> 
> Ok I'll try to add more explanation.
> 
> 
> > 
> > 
> > > +
> > > -- 
> > > 2.21.0

Re: [PATCH v1 4/5] Add virtio Admin virtqueue

[Michael S. Tsirkin]

On Mon, Apr 04, 2022 at 06:49:19PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 4:02 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 05:56:07PM +0200, Max Gurtovoy wrote:
> > > 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). Some of this configuration is generic enough. i.e
> > > Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
> > > such features query and manipulation by its parent PCI PF. 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.
> > > 
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > >   admin.tex       | 17 +++++++++++++++++
> > >   conformance.tex |  1 +
> > >   content.tex     |  6 ++++--
> > >   3 files changed, 22 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > index 9a8969b..ed9f6bd 100644
> > > --- a/admin.tex
> > > +++ b/admin.tex
> > > @@ -158,3 +158,20 @@ \subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Vi
> > >   specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
> > >   \end{note}
> > > +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
> > > +
> > > +An admin virtqueue is a management interface of a device that can be used to send administrative
> > > +commands to manipulate various features of the device and/or to manipulate
> > > +various features, if possible, of another device within the same virtio subsystem
> > > +(see \ref{sec:Introduction / Definitions / virtio subsystem}).
> > > +
> > > +An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ is
> > > +negotiated. The index of the admin virtqueue is exposed by the device in a
> > > +transport specific manner.
> > > +
> > > +If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin virtqueue to send all admin commands.
> > > +
> > Add links please, and explain how do commands map to VQ buffers.
> 
> I added a link to the definitions section and I'll replace it with the new
> proposed terminology chapter.

That explains what a subsystem is.

> What other links should be added ?

A link to where it's explained how do admin commands look.

> > E.g. I guess device readable an output buffer and device writeable an input
> > buffer...
> 
> The buffers transfer from/to device/driver are using existing mechanism.
> Nothing new here.
> 
> It will just repeat existing chapters.

There could be some repetition but it's necessary to make things
explicit.  Look at e.g. 
subsection{Device Operation}\label{sec:Device Types / Block Device / Device Operation}
as a lightweight example for how to explain it.




> > 
> > > +\devicenormative{\subsection}{Admin Virtqueues}{Basic Facilities of a Virtio Device / Admin Virtqueues}
> > > +A device that advertises VIRTIO_F_ADMIN_VQ capability MUST support all the mandatory admin commands.
> > > +
> > > +A device that advertises VIRTIO_F_ADMIN_VQ capability MAY support one or more optional admin commands.
> > > diff --git a/conformance.tex b/conformance.tex
> > > index 42f8537..129831c 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -341,6 +341,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Available Buffer Notification Suppression}
> > >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > >   \item \ref{devicenormative:Reserved Feature Bits}
> > > +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Admin Virtqueues}
> > >   \end{itemize}
> > >   \conformance{\subsection}{PCI Device Conformance}\label{sec:Conformance / Device Conformance / PCI Device Conformance}
> > > diff --git a/content.tex b/content.tex
> > > index 2e1df84..163cb34 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}
> > > @@ -6849,6 +6849,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}
> > > -- 
> > > 2.21.0

Re: [PATCH v1 5/5] Add miscellaneous configuration structure for PCI

[Michael S. Tsirkin]

On Mon, Apr 04, 2022 at 06:52:27PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 4:04 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 05:56:08PM +0200, Max Gurtovoy wrote:
> > > This new structure will be used for adding new miscellaneous registers
> > > for a virtio device configuration layout.
> > > 
> > > For now, only admin_queue_index register is added. Admin virtqueue index
> > > does not depend on the device type. Hence, add a PCI capability to read
> > > the admin virtqueue index.
> > > 
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > >   conformance.tex |  2 ++
> > >   content.tex     | 25 +++++++++++++++++++++++++
> > >   2 files changed, 27 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index 129831c..e31645e 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -102,6 +102,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> > > @@ -363,6 +364,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
> > >   \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > >   \end{itemize}
> > >   \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> > > diff --git a/content.tex b/content.tex
> > > index 163cb34..bf46192 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -712,6 +712,7 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
> > >   \item ISR Status
> > >   \item Device-specific configuration (optional)
> > >   \item PCI configuration access
> > > +\item Miscellaneous configuration
> > >   \end{itemize}
> > >   Each structure can be mapped by a Base Address register (BAR) belonging to
> > > @@ -771,6 +772,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
> > >   #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
> > >   /* Vendor-specific data */
> > >   #define VIRTIO_PCI_CAP_VENDOR_CFG        9
> > > +/* Miscellaneous configuration */
> > > +#define VIRTIO_PCI_CAP_MISC_CFG          10
> > >   \end{lstlisting}
> > >           Any other value is reserved for future use.
> > > @@ -1352,6 +1355,28 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
> > >   specified by some other Virtio Structure PCI Capability
> > >   of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
> > > +\subsubsection{Miscellaneous configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > +
> > > +The miscellaneous configuration structure is found at the bar and offset within the VIRTIO_PCI_CAP_MISC_CFG capability.
> > > +Its layout is below.
> > > +\begin{lstlisting}
> > > +struct virtio_pci_misc_cfg {
> > > +        le16 admin_queue_index;         /* read-only for driver */
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\begin{description}
> > > +\item[\field{admin_queue_index}]
> > > +        The device uses this to report the index of the admin virtqueue.
> > > +        This field is valid only if VIRTIO_F_ADMIN_VQ is set.
> > > +\end{description}
> > > +
> > > +\devicenormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > +The device MUST present a valid \field{admin_queue_index} when VIRTIO_F_ADMIN_VQ is set.

besides, is must have a misc config capability if it has
VIRTIO_F_ADMIN_VQ.


> > > +
> > > +\drivernormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > +The driver MUST use the value of \field{admin_queue_index} to configure the admin virtqueue. For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}.
> > > +
> > >   \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> > >   Transitional devices MUST present part of configuration
> > This is useful, but I think we'll want this for all transports then.
> 
> I think that adding more transports can be incremental and should be a no-go
> for merging this patch set.
> 
> We discussed this in the previous version and nobody had a conclusion on the
> right approach for other transports.

I think this referred to the msi-x thing not the the
misc config capability - this is the first version where
that appeared, right?  Cornelia what do you think?

In any case, in that case maybe besides saying it's transport-specific
also mention that it is currently only defined for the PCI transport.

> > 
> > 
> > > -- 
> > > 2.21.0

Re: [PATCH v1 2/5] Introduce Admin Command Set

[Michael S. Tsirkin]

On Mon, Apr 04, 2022 at 06:35:16PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 3:50 PM, Michael S. Tsirkin wrote:
> > On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote:
> > > This command set is used for essential administrative and management
> > > operations such as identify and resource management.
> > > 
> > > Admin commands should be submitted to a well defined management
> > > interface.
> > > 
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > ---
> > >   admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   content.tex      |   2 +
> > >   introduction.tex |   2 +
> > >   3 files changed, 133 insertions(+)
> > >   create mode 100644 admin.tex
> > > 
> > > diff --git a/admin.tex b/admin.tex
> > > new file mode 100644
> > > index 0000000..1e30e01
> > > --- /dev/null
> > > +++ b/admin.tex
> > > @@ -0,0 +1,129 @@
> > > +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}
> > > +
> > > +The Admin command set defines the commands that can be issued to a well defined management
> > > +interface.
> > This is shorthand for what? Administration commands?
> > Let's eschew abbreviation pls.
> > Also, management is a bit oblique, and only the reader can judge how well it's defined :)
> > 
> > 
> > I would rephrase this along the lines of
> > 
> > " For security and ease of management reasons, devices can expose an additional interface
> 
> Why should we add security here ?
> 
> Lets keep it simple.

you want some motivation, this has consistently been a problem with this
project, motivation is not well documented. I say add all that can be
remotely relevant in here.  The security refers to IOMMU using VF# for
protection, I think it's relevant.

> >    to access the device besides the ones specified in the
> >    transports section. For example, a host management system
> >    might want to access the device while in use by driver,
> >    or to configure the device before it is initialized by
> >    the driver.
> >    This access is facilitated through a set of administration commands.
> > 
> > "
> > 
> > 
> > speaking of this, are we still going to use the "driver"
> > terminology for uses of this?
> 
> There is no other way to access a device from the host/guess other than
> driver.
> 
> A management system will have some interface provided by a driver to access
> a device.


Heh. However with admin commands a driver of device X accesses device Y.
It is not *the driver* - it's a different driver.
Given we are already getting confused, it looks like we need to differentiate
between the two types of driver by using some other term here.
I suggest "administrator" but "manager" or "management driver" could be
other options. I wonder what do others think.

> > 
> > 
> > > All the Admin commands are of the following form:
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_admin_cmd {
> > > +        /* Device-readable part */
> > > +        le16 command;
> > > +        /*
> > > +         * 0 - self
> > > +         * 1 - 65535 are reserved
> > > +         */
> > > +        le16 dst_type;
> > I think we want to add the vdev id here in the generic command
> > and just have a special id that means "self".
> 
> vdev id was added in later patch. No need for special id means self. Each
> device has only 1 id and not 2.
> 
> This was proposed by Cornelia IIRC.

Yea, it's ok with that patch. The way it's split isn't ideal
unfortunately maybe move it into this patch in the next version.

> > 
> > 
> > > +        /* reserved for common cmd fields */
> > > +        u8 reserved[20];
> > > +        u8 command_specific_data[];
> > > +
> > > +        /* Device-writable part */
> > > +        u8 status;
> > > +        u8 command_specific_error;
> > > +        u8 command_specific_result[];
> > > +};
> > > +\end{lstlisting}
> > > +
> > > +The following table describes the generic Admin status codes:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
> > > +\hline
> > > +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +The \field{command}, \field{dst_type} and \field{command_specific_data} are
> > > +set by the driver, and the device sets the \field{status}, the
> > > +\field{command_specific_error} and the \field{command_specific_result},
> > > +if needed.
> > > +
> > > +Reserved common fields are ignored by the device and to be zeroed by the driver.
> > > +
> > > +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
> > > +
> > > +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
> > > +
> > > +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
> > > +
> > > +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 content 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.
> > > +
> > > +The following table describes the Admin command set:
> > > +
> > > +\begin{tabular}{|l|l|l|}
> > > +\hline
> > > +Opcode & Command & M/O \\
> > > +\hline \hline
> > > +0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
> > > +\hline
> > > +0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
> > > +\hline
> > > +0002h - 7FFFh   & Generic admin cmds    & -  \\
> > > +\hline
> > > +8000h - FFFFh   & Reserved    & - \\
> > > +\hline
> > > +\end{tabular}
> > > +
> > > +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
> > > +
> > > +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
> > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
> > > +Other common fields are reserved for this command and zeroed.
> > > +
> > > +This command upon success, returns a data buffer
> > let's avoid using buffer since we want to allow non-DMA uses
> > IIUC. just "result" is ok, right?
> 
> How would device write data to host memory ?
> 
> driver prepare a buffer and device will write to this buffer.

commands could use MMIO with no DMA.

> > 
> > > that describes information about the target virtio device.
> > target being what? destination?
> 
> yes.

then make all wording consistent please.

> 
> > 
> > > +This returned data buffer is of form:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_device_identify_result {
> > > +       /* For compatibility - indicates which of the below fields were returned
> > compatibility with what?
> I will remove "compatibility". It just indicates the valid fields.
> > 
> > > +        * (1 means that field was returned):
> > > +        * Bit 0 - vdev_id
> > > +        * Bit 1 - optional_caps_support
> > > +        * Bits 2 - 63 - reserved for future fields
> > > +        */
> > > +       le64 attrs_mask;
> > this seems to be some kind of thing listing supported commands, except
> > it's one way as opposed to negotiated like feature bits.  From
> > experience this kind of one way capability might be problematic since
> > you never know what will be used.  How about just using feature bits
> > instead? How many of these do you expect to be there?
> 
> It's not features.
> 
> I don't see a need for negotiation here.

I guess I do ;) Donnu what do others think.


> > 
> > If not, please add a description of this.
> > E.g. along the lines of
> > 
> > 	Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ??
> 
> I described bit by bit and mentioned that this is a mask. For each bit the
> value of 1 is valid.


Look at e.g. RSS as a better example of documenting this.

> 
> > 
> > 
> > > +       le64 vdev_id;
> > So this is a way to query the id of device itself? Since the only dst
> > type is self ...
> 
> For example yes. This is one of the things you can get from this command.

I don't see why this is useful.

> > 
> > 
> > > +       /* This field indicates which of the below optional admin
> > > +        * capabilities are supported by the device:
> > > +        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
> > > +        * Bits 1 - 63 - reserved for future capabilities.
> > > +        */
> > > +       le64 optional_caps_support;
> > > +       u8 reserved[4072];
> > What exactly is this reserved thing? Does this mean the structure is
> > always exactly 4k?
> 
> Yes the result buffer size is 4k.
> 

That's huge, will be a problem if it's MMIO and not DMA, and
mostly just wasted. why not make it as big as it needs to be?

> > > +};
> > > +\end{lstlisting}
> > > +
> > > +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
> > > +
> > > +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
> > > +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
> > > +Other common fields are reserved for this command and zeroed.
> > zeroed by driver?
> 
> Yes.

you want to go over places like this and everywhere say who does what.

> 
> > 
> > > +
> > > +This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
> > > +This returned data buffer is of form:
> > > +\begin{lstlisting}
> > > +struct virtio_admin_subsystem_identify_result {
> > > +       /* For compatibility - indicates which of the below fields were returned
> > > +        * (1 means that field was returned):
> > > +        * Bit 0 - vqn
> > > +        * Bit 1 - num_supported_vdevs
> > > +        * Bit 2 - max_vdev_id
> > > +        * Bits 3 - 63 - reserved for future fields
> > > +        */
> > > +       le64 attrs_mask;
> > add description here.
> 
> Ok.
> 
> > > +       u8 vqn[16];
> > > +       /* Number of supported virtio devices by the subsystem */
> > > +       le64 num_supported_vdevs;
> > > +       /* Maximum value of a valid vdev_id for the subsystem */
> > > +       le64 max_vdev_id;
> > > +       u8 reserved[4056];
> > > +};
> > > +\end{lstlisting}
> > > diff --git a/content.tex b/content.tex
> > > index c6f116c..2e1df84 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
> > > diff --git a/introduction.tex b/introduction.tex
> > > index 8e6611e..94dd7a2 100644
> > > --- a/introduction.tex
> > > +++ b/introduction.tex
> > > @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
> > >   The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
> > > +VQN and vdev_id are exposed via Admin Command Set.
> > > +
> > >   \newpage
> > > -- 
> > > 2.21.0

[virtio-comment] Re: [PATCH v1 2/5] Introduce Admin Command Set

[Max Gurtovoy]

On 4/4/2022 7:26 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2022 at 06:35:16PM +0300, Max Gurtovoy wrote:
>> On 4/4/2022 3:50 PM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote:
>>>> This command set is used for essential administrative and management
>>>> operations such as identify and resource management.
>>>>
>>>> Admin commands should be submitted to a well defined management
>>>> interface.
>>>>
>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>> ---
>>>>    admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    content.tex      |   2 +
>>>>    introduction.tex |   2 +
>>>>    3 files changed, 133 insertions(+)
>>>>    create mode 100644 admin.tex
>>>>
>>>> diff --git a/admin.tex b/admin.tex
>>>> new file mode 100644
>>>> index 0000000..1e30e01
>>>> --- /dev/null
>>>> +++ b/admin.tex
>>>> @@ -0,0 +1,129 @@
>>>> +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}
>>>> +
>>>> +The Admin command set defines the commands that can be issued to a well defined management
>>>> +interface.
>>> This is shorthand for what? Administration commands?
>>> Let's eschew abbreviation pls.
>>> Also, management is a bit oblique, and only the reader can judge how well it's defined :)
>>>
>>>
>>> I would rephrase this along the lines of
>>>
>>> " For security and ease of management reasons, devices can expose an additional interface
>> Why should we add security here ?
>>
>> Lets keep it simple.
> you want some motivation, this has consistently been a problem with this
> project, motivation is not well documented. I say add all that can be
> remotely relevant in here.  The security refers to IOMMU using VF# for
> protection, I think it's relevant.

The spec should be simple and generic. IOMMU/VFs/Security is not related 
to the introduction of the admin command set.

Otherwise, it will grow for each example/reason we'll think in the future.

Also the interface is not relevant and was previously asked to be 
separate from the admin command set patch.

The motivation is clear but we can improve.

We can do:

"The Admin command set defines the commands that can be issued using a dedicated management interface.
  For example, a host management system might want to access and configure a device before it is initialized by
  the device driver. This can be done using a management device and its management driver."

>
>>>     to access the device besides the ones specified in the
>>>     transports section. For example, a host management system
>>>     might want to access the device while in use by driver,
>>>     or to configure the device before it is initialized by
>>>     the driver.
>>>     This access is facilitated through a set of administration commands.
>>>
>>> "
>>>
>>>
>>> speaking of this, are we still going to use the "driver"
>>> terminology for uses of this?
>> There is no other way to access a device from the host/guess other than
>> driver.
>>
>> A management system will have some interface provided by a driver to access
>> a device.
>
> Heh. However with admin commands a driver of device X accesses device Y.
> It is not *the driver* - it's a different driver.

see above. I called it device driver.

> Given we are already getting confused, it looks like we need to differentiate
> between the two types of driver by using some other term here.
> I suggest "administrator" but "manager" or "management driver" could be
> other options. I wonder what do others think.
see above.
>
>>>
>>>> All the Admin commands are of the following form:
>>>> +
>>>> +\begin{lstlisting}
>>>> +struct virtio_admin_cmd {
>>>> +        /* Device-readable part */
>>>> +        le16 command;
>>>> +        /*
>>>> +         * 0 - self
>>>> +         * 1 - 65535 are reserved
>>>> +         */
>>>> +        le16 dst_type;
>>> I think we want to add the vdev id here in the generic command
>>> and just have a special id that means "self".
>> vdev id was added in later patch. No need for special id means self. Each
>> device has only 1 id and not 2.
>>
>> This was proposed by Cornelia IIRC.
> Yea, it's ok with that patch. The way it's split isn't ideal
> unfortunately maybe move it into this patch in the next version.

I'll check if this make sense during the preparations of the next version.


>
>>>
>>>> +        /* reserved for common cmd fields */
>>>> +        u8 reserved[20];
>>>> +        u8 command_specific_data[];
>>>> +
>>>> +        /* Device-writable part */
>>>> +        u8 status;
>>>> +        u8 command_specific_error;
>>>> +        u8 command_specific_result[];
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +The following table describes the generic Admin status codes:
>>>> +
>>>> +\begin{tabular}{|l|l|l|}
>>>> +\hline
>>>> +Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
>>>> +\hline
>>>> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
>>>> +\hline
>>>> +\end{tabular}
>>>> +
>>>> +The \field{command}, \field{dst_type} and \field{command_specific_data} are
>>>> +set by the driver, and the device sets the \field{status}, the
>>>> +\field{command_specific_error} and the \field{command_specific_result},
>>>> +if needed.
>>>> +
>>>> +Reserved common fields are ignored by the device and to be zeroed by the driver.
>>>> +
>>>> +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
>>>> +
>>>> +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
>>>> +
>>>> +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
>>>> +
>>>> +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 content 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.
>>>> +
>>>> +The following table describes the Admin command set:
>>>> +
>>>> +\begin{tabular}{|l|l|l|}
>>>> +\hline
>>>> +Opcode & Command & M/O \\
>>>> +\hline \hline
>>>> +0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
>>>> +\hline
>>>> +0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>>>> +\hline
>>>> +0002h - 7FFFh   & Generic admin cmds    & -  \\
>>>> +\hline
>>>> +8000h - FFFFh   & Reserved    & - \\
>>>> +\hline
>>>> +\end{tabular}
>>>> +
>>>> +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
>>>> +
>>>> +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
>>>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
>>>> +Other common fields are reserved for this command and zeroed.
>>>> +
>>>> +This command upon success, returns a data buffer
>>> let's avoid using buffer since we want to allow non-DMA uses
>>> IIUC. just "result" is ok, right?
>> How would device write data to host memory ?
>>
>> driver prepare a buffer and device will write to this buffer.
> commands could use MMIO with no DMA.

what do you mean ?

Are you referring to the suggestion for configuration registers we 
already rejected (please read the cover letter) or something else ?

>
>>>> that describes information about the target virtio device.
>>> target being what? destination?
>> yes.
> then make all wording consistent please.
sure.
>
>>>> +This returned data buffer is of form:
>>>> +\begin{lstlisting}
>>>> +struct virtio_admin_device_identify_result {
>>>> +       /* For compatibility - indicates which of the below fields were returned
>>> compatibility with what?
>> I will remove "compatibility". It just indicates the valid fields.
>>>> +        * (1 means that field was returned):
>>>> +        * Bit 0 - vdev_id
>>>> +        * Bit 1 - optional_caps_support
>>>> +        * Bits 2 - 63 - reserved for future fields
>>>> +        */
>>>> +       le64 attrs_mask;
>>> this seems to be some kind of thing listing supported commands, except
>>> it's one way as opposed to negotiated like feature bits.  From
>>> experience this kind of one way capability might be problematic since
>>> you never know what will be used.  How about just using feature bits
>>> instead? How many of these do you expect to be there?
>> It's not features.
>>
>> I don't see a need for negotiation here.
> I guess I do ;) Donnu what do others think.

The device shouldn't care if a driver "accepts" the fact it support a 
specific command.

The driver can just issue this command.

There is no optimization that should be done if some command is 
supported by the driver (from the device perspective).

>
>
>>> If not, please add a description of this.
>>> E.g. along the lines of
>>>
>>> 	Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ??
>> I described bit by bit and mentioned that this is a mask. For each bit the
>> value of 1 is valid.
>
> Look at e.g. RSS as a better example of documenting this.
>
>>>
>>>> +       le64 vdev_id;
>>> So this is a way to query the id of device itself? Since the only dst
>>> type is self ...
>> For example yes. This is one of the things you can get from this command.
> I don't see why this is useful.

It helps identifying a device inside a virtio subsystem/group.

>
>>>
>>>> +       /* This field indicates which of the below optional admin
>>>> +        * capabilities are supported by the device:
>>>> +        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
>>>> +        * Bits 1 - 63 - reserved for future capabilities.
>>>> +        */
>>>> +       le64 optional_caps_support;
>>>> +       u8 reserved[4072];
>>> What exactly is this reserved thing? Does this mean the structure is
>>> always exactly 4k?
>> Yes the result buffer size is 4k.
>>
> That's huge, will be a problem if it's MMIO and not DMA, and
> mostly just wasted. why not make it as big as it needs to be?

But in another place you've asked why it is so small.

You need it to be fixed size since the format is the contract between 
the driver and the device.

We don't want to start negotiating on each bit.

>
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
>>>> +
>>>> +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
>>>> +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
>>>> +Other common fields are reserved for this command and zeroed.
>>> zeroed by driver?
>> Yes.
> you want to go over places like this and everywhere say who does what.

if it's clear I can remove it.

>
>>>> +
>>>> +This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
>>>> +This returned data buffer is of form:
>>>> +\begin{lstlisting}
>>>> +struct virtio_admin_subsystem_identify_result {
>>>> +       /* For compatibility - indicates which of the below fields were returned
>>>> +        * (1 means that field was returned):
>>>> +        * Bit 0 - vqn
>>>> +        * Bit 1 - num_supported_vdevs
>>>> +        * Bit 2 - max_vdev_id
>>>> +        * Bits 3 - 63 - reserved for future fields
>>>> +        */
>>>> +       le64 attrs_mask;
>>> add description here.
>> Ok.
>>
>>>> +       u8 vqn[16];
>>>> +       /* Number of supported virtio devices by the subsystem */
>>>> +       le64 num_supported_vdevs;
>>>> +       /* Maximum value of a valid vdev_id for the subsystem */
>>>> +       le64 max_vdev_id;
>>>> +       u8 reserved[4056];
>>>> +};
>>>> +\end{lstlisting}
>>>> diff --git a/content.tex b/content.tex
>>>> index c6f116c..2e1df84 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
>>>> diff --git a/introduction.tex b/introduction.tex
>>>> index 8e6611e..94dd7a2 100644
>>>> --- a/introduction.tex
>>>> +++ b/introduction.tex
>>>> @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
>>>>    The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
>>>> +VQN and vdev_id are exposed via Admin Command Set.
>>>> +
>>>>    \newpage
>>>> -- 
>>>> 2.21.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://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 v1 4/5] Add virtio Admin virtqueue

[Max Gurtovoy]

On 4/4/2022 7:13 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2022 at 06:49:19PM +0300, Max Gurtovoy wrote:
>> On 4/4/2022 4:02 PM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2022 at 05:56:07PM +0200, Max Gurtovoy wrote:
>>>> 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). Some of this configuration is generic enough. i.e
>>>> Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
>>>> such features query and manipulation by its parent PCI PF. 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.
>>>>
>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>> ---
>>>>    admin.tex       | 17 +++++++++++++++++
>>>>    conformance.tex |  1 +
>>>>    content.tex     |  6 ++++--
>>>>    3 files changed, 22 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/admin.tex b/admin.tex
>>>> index 9a8969b..ed9f6bd 100644
>>>> --- a/admin.tex
>>>> +++ b/admin.tex
>>>> @@ -158,3 +158,20 @@ \subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Vi
>>>>    specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
>>>>    \end{note}
>>>> +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
>>>> +
>>>> +An admin virtqueue is a management interface of a device that can be used to send administrative
>>>> +commands to manipulate various features of the device and/or to manipulate
>>>> +various features, if possible, of another device within the same virtio subsystem
>>>> +(see \ref{sec:Introduction / Definitions / virtio subsystem}).
>>>> +
>>>> +An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ is
>>>> +negotiated. The index of the admin virtqueue is exposed by the device in a
>>>> +transport specific manner.
>>>> +
>>>> +If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin virtqueue to send all admin commands.
>>>> +
>>> Add links please, and explain how do commands map to VQ buffers.
>> I added a link to the definitions section and I'll replace it with the new
>> proposed terminology chapter.
> That explains what a subsystem is.

I'm confused. Are we staying with "subsystem" or moving to "group" ? I 
prefer to stay.


>
>> What other links should be added ?
> A link to where it's explained how do admin commands look.

ok.

>
>>> E.g. I guess device readable an output buffer and device writeable an input
>>> buffer...
>> The buffers transfer from/to device/driver are using existing mechanism.
>> Nothing new here.
>>
>> It will just repeat existing chapters.
> There could be some repetition but it's necessary to make things
> explicit.  Look at e.g.
> subsection{Device Operation}\label{sec:Device Types / Block Device / Device Operation}
> as a lightweight example for how to explain it.

Ok, I'll check it.

The proposal is not small now, and I prefer not repeat on existing 
mechanisms in the spec.

Maybe I'll add a sentence or two.

>
>
>
>
>>>> +\devicenormative{\subsection}{Admin Virtqueues}{Basic Facilities of a Virtio Device / Admin Virtqueues}
>>>> +A device that advertises VIRTIO_F_ADMIN_VQ capability MUST support all the mandatory admin commands.
>>>> +
>>>> +A device that advertises VIRTIO_F_ADMIN_VQ capability MAY support one or more optional admin commands.
>>>> diff --git a/conformance.tex b/conformance.tex
>>>> index 42f8537..129831c 100644
>>>> --- a/conformance.tex
>>>> +++ b/conformance.tex
>>>> @@ -341,6 +341,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Available Buffer Notification Suppression}
>>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
>>>>    \item \ref{devicenormative:Reserved Feature Bits}
>>>> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Admin Virtqueues}
>>>>    \end{itemize}
>>>>    \conformance{\subsection}{PCI Device Conformance}\label{sec:Conformance / Device Conformance / PCI Device Conformance}
>>>> diff --git a/content.tex b/content.tex
>>>> index 2e1df84..163cb34 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}
>>>> @@ -6849,6 +6849,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}
>>>> -- 
>>>> 2.21.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://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [PATCH v1 5/5] Add miscellaneous configuration structure for PCI

[Max Gurtovoy]

On 4/4/2022 7:16 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2022 at 06:52:27PM +0300, Max Gurtovoy wrote:
>> On 4/4/2022 4:04 PM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2022 at 05:56:08PM +0200, Max Gurtovoy wrote:
>>>> This new structure will be used for adding new miscellaneous registers
>>>> for a virtio device configuration layout.
>>>>
>>>> For now, only admin_queue_index register is added. Admin virtqueue index
>>>> does not depend on the device type. Hence, add a PCI capability to read
>>>> the admin virtqueue index.
>>>>
>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>> ---
>>>>    conformance.tex |  2 ++
>>>>    content.tex     | 25 +++++++++++++++++++++++++
>>>>    2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/conformance.tex b/conformance.tex
>>>> index 129831c..e31645e 100644
>>>> --- a/conformance.tex
>>>> +++ b/conformance.tex
>>>> @@ -102,6 +102,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>>>    \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
>>>>    \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>>>    \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>>>> +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>>>    \end{itemize}
>>>>    \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
>>>> @@ -363,6 +364,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>>>    \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
>>>>    \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
>>>>    \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
>>>> +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>>>    \end{itemize}
>>>>    \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
>>>> diff --git a/content.tex b/content.tex
>>>> index 163cb34..bf46192 100644
>>>> --- a/content.tex
>>>> +++ b/content.tex
>>>> @@ -712,6 +712,7 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>>>>    \item ISR Status
>>>>    \item Device-specific configuration (optional)
>>>>    \item PCI configuration access
>>>> +\item Miscellaneous configuration
>>>>    \end{itemize}
>>>>    Each structure can be mapped by a Base Address register (BAR) belonging to
>>>> @@ -771,6 +772,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
>>>>    #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
>>>>    /* Vendor-specific data */
>>>>    #define VIRTIO_PCI_CAP_VENDOR_CFG        9
>>>> +/* Miscellaneous configuration */
>>>> +#define VIRTIO_PCI_CAP_MISC_CFG          10
>>>>    \end{lstlisting}
>>>>            Any other value is reserved for future use.
>>>> @@ -1352,6 +1355,28 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
>>>>    specified by some other Virtio Structure PCI Capability
>>>>    of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
>>>> +\subsubsection{Miscellaneous configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>>> +
>>>> +The miscellaneous configuration structure is found at the bar and offset within the VIRTIO_PCI_CAP_MISC_CFG capability.
>>>> +Its layout is below.
>>>> +\begin{lstlisting}
>>>> +struct virtio_pci_misc_cfg {
>>>> +        le16 admin_queue_index;         /* read-only for driver */
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +\begin{description}
>>>> +\item[\field{admin_queue_index}]
>>>> +        The device uses this to report the index of the admin virtqueue.
>>>> +        This field is valid only if VIRTIO_F_ADMIN_VQ is set.
>>>> +\end{description}
>>>> +
>>>> +\devicenormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>>> +The device MUST present a valid \field{admin_queue_index} when VIRTIO_F_ADMIN_VQ is set.
> besides, is must have a misc config capability if it has
> VIRTIO_F_ADMIN_VQ.

admin_queue_index is part of the misc config so it implies from that.

>
>
>>>> +
>>>> +\drivernormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
>>>> +The driver MUST use the value of \field{admin_queue_index} to configure the admin virtqueue. For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}.
>>>> +
>>>>    \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
>>>>    Transitional devices MUST present part of configuration
>>> This is useful, but I think we'll want this for all transports then.
>> I think that adding more transports can be incremental and should be a no-go
>> for merging this patch set.
>>
>> We discussed this in the previous version and nobody had a conclusion on the
>> right approach for other transports.

oh I had a typo. I meant shouldn't be a no-go.


> I think this referred to the msi-x thing not the the
> misc config capability - this is the first version where
> that appeared, right?  Cornelia what do you think?
>
> In any case, in that case maybe besides saying it's transport-specific
> also mention that it is currently only defined for the PCI transport.

I'll try to add it, but it might not be removed from the spec when 
somebody will add another transport admin support and will forget this 
comment.


>
>>>
>>>> -- 
>>>> 2.21.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=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=dWuRqSzUOaLhvJiplv2GH3GgTSBj%2BPLlYY4pRuUGI6Y%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=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=oKUUoOXqKpd4oaJc4AtB7FwU1wmnVv%2B2%2FLzUv%2FWwGho%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=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=cytTVlpAzn06%2BidXyenkhEx8ZhAFZZWyVPSlhVvrRHU%3D&amp;reserved=0
> Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=a2ORe6u0we6fL%2BnG8Wqkq3BRJY8XTj0jsQQ0YKqnTCA%3D&amp;reserved=0
> Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=5%2FjVkGXexgNZ%2BZq59B73tzJ6adJVqdoLvRuoCMDeoh8%3D&amp;reserved=0
>

[virtio-comment] Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Max Gurtovoy]

On 4/4/2022 7:09 PM, Michael S. Tsirkin wrote:
> On Mon, Apr 04, 2022 at 06:44:11PM +0300, Max Gurtovoy wrote:
>> On 4/4/2022 3:57 PM, Michael S. Tsirkin wrote:
>>> On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
>>>> The DEVICE INFO command will return a basic information for a virtio
>>>> device.
>>>>
>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>> ---
>>>>    admin.tex | 35 +++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 33 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/admin.tex b/admin.tex
>>>> index 1e30e01..9a8969b 100644
>>>> --- a/admin.tex
>>>> +++ b/admin.tex
>>>> @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
>>>>    \hline
>>>>    0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>>>>    \hline
>>>> -0002h - 7FFFh   & Generic admin cmds    & -  \\
>>>> +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
>>>> +\hline
>>>> +0003h - 7FFFh   & Generic admin cmds    & -  \\
>>>>    \hline
>>>>    8000h - FFFFh   & Reserved    & - \\
>>>>    \hline
>>>> @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
>>>>           /* This field indicates which of the below optional admin
>>>>            * capabilities are supported by the device:
>>>>            * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
>>>> -        * Bits 1 - 63 - reserved for future capabilities.
>>>> +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
>>>> +        * Bits 2 - 63 - reserved for future capabilities.
>>>>            */
>>>>           le64 optional_caps_support;
>>>>           u8 reserved[4072];
>>>> @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
>>>>           u8 reserved[4056];
>>>>    };
>>>>    \end{lstlisting}
>>>> +
>>>> +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
>>>> +
>>>> +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
>>>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
>>>> +
>>>> +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
>>>> +Upon success, the returned data buffer is of form:
>>>> +\begin{lstlisting}
>>>> +struct virtio_admin_device_info_result {
>>>> +        /* For compatibility - indicates which of the below fields were returned
>>>> +         * (1 means that field was returned):
>>> So how is this supposed to be handled. You need to write this up.
>>> Also having each command have a bitmap seems like an overkill
>>> to me. But if that is the way you are going just put
>>> this in the generic part.
>>>
>>>
>>>> +         * Bit 0 - vf_number
>>>> +         * Bits 1 - 63 - reserved for future fields
>>> we generally say "future use" not "future fields".
>> ok.
>>
>>
>>>> +         */
>>>> +        le64 attrs_mask;
>>>> +        /* The virtual function number */
>>>> +        le16 vf_number;
>>>> +        u8 reserved[1014];
>>>> +};
>>>> +\end{lstlisting}
>>>> +
>>>> +\begin{note}
>>>> +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
>>>> +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
>>>> +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
>>>> +\end{note}
>>> I personally would just make vf # *be* the device ID, with no need for an
>>> extra indirection. It doesn't hurt much though ...
>> This is implementation specific.
>>
>> You need to have a way to identify the vf number.
> So my point is, I see no need to have a random set
> of VFs or have an abstract ID as an indirection layer -
> we can just say id == VF# and always manage all VF #s up to N.

But the TG asked to add a vdev_id for future extensions and it helped me 
to define a subsystem.

This id can be == VF# in PCI but it is not a VF# for non VFs.

We agreed on that in previous discussions.

>
> This will cut out a bunch of commands.
>
> And hey, if we see the need for an indirection down the road we
> will be add it as a separate dst_type.

But we tried it without dst_type in the initial versions..

I worked hard to design it and now we go back ? and what will be in the 
next version ? add it again ?

I think the dst_type, vdev_id, subsystem framework is good and not too 
complicated.

>
>>> But I do not seem to understand how it all works here. I think a generic
>>> description explaining how all this is used is necessary, pls add a
>>> writeup.
>>>
>>> E.g.
>>>
>>> So I start with a device and through a feature bit I find out it
>>> supports admin commands. Then I query that and find the max vdev id.
>>> Query the ID of each device. Then through that query VF number.
>>> Something like this.
>>>
>>> Looks in device operation section for some examples.
>> Ok I'll try to add more explanation.
>>
>>
>>>
>>>> +
>>>> -- 
>>>> 2.21.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://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [PATCH v1 5/5] Add miscellaneous configuration structure for PCI

[Michael S. Tsirkin]

On Tue, Apr 05, 2022 at 02:20:04PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 7:16 PM, Michael S. Tsirkin wrote:
> > On Mon, Apr 04, 2022 at 06:52:27PM +0300, Max Gurtovoy wrote:
> > > On 4/4/2022 4:04 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 05:56:08PM +0200, Max Gurtovoy wrote:
> > > > > This new structure will be used for adding new miscellaneous registers
> > > > > for a virtio device configuration layout.
> > > > > 
> > > > > For now, only admin_queue_index register is added. Admin virtqueue index
> > > > > does not depend on the device type. Hence, add a PCI capability to read
> > > > > the admin virtqueue index.
> > > > > 
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > ---
> > > > >    conformance.tex |  2 ++
> > > > >    content.tex     | 25 +++++++++++++++++++++++++
> > > > >    2 files changed, 27 insertions(+)
> > > > > 
> > > > > diff --git a/conformance.tex b/conformance.tex
> > > > > index 129831c..e31645e 100644
> > > > > --- a/conformance.tex
> > > > > +++ b/conformance.tex
> > > > > @@ -102,6 +102,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > > > >    \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / PCI configuration access capability}
> > > > >    \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > > > >    \item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > > > +\item \ref{drivernormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > > >    \end{itemize}
> > > > >    \conformance{\subsection}{MMIO Driver Conformance}\label{sec:Conformance / Driver Conformance / MMIO Driver Conformance}
> > > > > @@ -363,6 +364,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > > > >    \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / MSI-X Vector Configuration}
> > > > >    \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Used Buffer Notifications}
> > > > >    \item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Notification of Device Configuration Changes}
> > > > > +\item \ref{devicenormative:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > > >    \end{itemize}
> > > > >    \conformance{\subsection}{MMIO Device Conformance}\label{sec:Conformance / Device Conformance / MMIO Device Conformance}
> > > > > diff --git a/content.tex b/content.tex
> > > > > index 163cb34..bf46192 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -712,6 +712,7 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
> > > > >    \item ISR Status
> > > > >    \item Device-specific configuration (optional)
> > > > >    \item PCI configuration access
> > > > > +\item Miscellaneous configuration
> > > > >    \end{itemize}
> > > > >    Each structure can be mapped by a Base Address register (BAR) belonging to
> > > > > @@ -771,6 +772,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
> > > > >    #define VIRTIO_PCI_CAP_SHARED_MEMORY_CFG 8
> > > > >    /* Vendor-specific data */
> > > > >    #define VIRTIO_PCI_CAP_VENDOR_CFG        9
> > > > > +/* Miscellaneous configuration */
> > > > > +#define VIRTIO_PCI_CAP_MISC_CFG          10
> > > > >    \end{lstlisting}
> > > > >            Any other value is reserved for future use.
> > > > > @@ -1352,6 +1355,28 @@ \subsubsection{PCI configuration access capability}\label{sec:Virtio Transport O
> > > > >    specified by some other Virtio Structure PCI Capability
> > > > >    of type other than \field{VIRTIO_PCI_CAP_PCI_CFG}.
> > > > > +\subsubsection{Miscellaneous configuration structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > > > +
> > > > > +The miscellaneous configuration structure is found at the bar and offset within the VIRTIO_PCI_CAP_MISC_CFG capability.
> > > > > +Its layout is below.
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_pci_misc_cfg {
> > > > > +        le16 admin_queue_index;         /* read-only for driver */
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +\begin{description}
> > > > > +\item[\field{admin_queue_index}]
> > > > > +        The device uses this to report the index of the admin virtqueue.
> > > > > +        This field is valid only if VIRTIO_F_ADMIN_VQ is set.
> > > > > +\end{description}
> > > > > +
> > > > > +\devicenormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > > > +The device MUST present a valid \field{admin_queue_index} when VIRTIO_F_ADMIN_VQ is set.
> > besides, is must have a misc config capability if it has
> > VIRTIO_F_ADMIN_VQ.
> 
> admin_queue_index is part of the misc config so it implies from that.

Explicit is better than implicit.

Besides, it is probably a good idea to specify what should driver do if
VIRTIO_F_ADMIN_VQ is set in host features but capability is not there. I
guess it must make sure it is cleared in guest features before setting
FEATURES_OK.




> > 
> > 
> > > > > +
> > > > > +\drivernormative{\paragraph}{Miscellaneous configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Miscellaneous configuration structure layout}
> > > > > +The driver MUST use the value of \field{admin_queue_index} to configure the admin virtqueue. For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}.
> > > > > +
> > > > >    \subsubsection{Legacy Interfaces: A Note on PCI Device Layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> > > > >    Transitional devices MUST present part of configuration
> > > > This is useful, but I think we'll want this for all transports then.
> > > I think that adding more transports can be incremental and should be a no-go
> > > for merging this patch set.
> > > 
> > > We discussed this in the previous version and nobody had a conclusion on the
> > > right approach for other transports.
> 
> oh I had a typo. I meant shouldn't be a no-go.
> 
> 
> > I think this referred to the msi-x thing not the the
> > misc config capability - this is the first version where
> > that appeared, right?  Cornelia what do you think?
> > 
> > In any case, in that case maybe besides saying it's transport-specific
> > also mention that it is currently only defined for the PCI transport.
> 
> I'll try to add it, but it might not be removed from the spec when somebody
> will add another transport admin support and will forget this comment.

An alternative is to add admin support chapters and just specify
these transports do not have admin support and must not set
VIRTIO_F_ADMIN_VQ, if driver sees VIRTIO_F_ADMIN_VQ
is must not set it.


> 
> > 
> > > > 
> > > > > -- 
> > > > > 2.21.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=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=dWuRqSzUOaLhvJiplv2GH3GgTSBj%2BPLlYY4pRuUGI6Y%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=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=oKUUoOXqKpd4oaJc4AtB7FwU1wmnVv%2B2%2FLzUv%2FWwGho%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=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=cytTVlpAzn06%2BidXyenkhEx8ZhAFZZWyVPSlhVvrRHU%3D&amp;reserved=0
> > Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=a2ORe6u0we6fL%2BnG8Wqkq3BRJY8XTj0jsQQ0YKqnTCA%3D&amp;reserved=0
> > Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7Ca1d60587d07741c5050d08da16568edc%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637846858292442161%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=5%2FjVkGXexgNZ%2BZq59B73tzJ6adJVqdoLvRuoCMDeoh8%3D&amp;reserved=0
> > 

Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Michael S. Tsirkin]

On Tue, Apr 05, 2022 at 02:27:35PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 7:09 PM, Michael S. Tsirkin wrote:
> > On Mon, Apr 04, 2022 at 06:44:11PM +0300, Max Gurtovoy wrote:
> > > On 4/4/2022 3:57 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
> > > > > The DEVICE INFO command will return a basic information for a virtio
> > > > > device.
> > > > > 
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > ---
> > > > >    admin.tex | 35 +++++++++++++++++++++++++++++++++--
> > > > >    1 file changed, 33 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/admin.tex b/admin.tex
> > > > > index 1e30e01..9a8969b 100644
> > > > > --- a/admin.tex
> > > > > +++ b/admin.tex
> > > > > @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
> > > > >    \hline
> > > > >    0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
> > > > >    \hline
> > > > > -0002h - 7FFFh   & Generic admin cmds    & -  \\
> > > > > +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
> > > > > +\hline
> > > > > +0003h - 7FFFh   & Generic admin cmds    & -  \\
> > > > >    \hline
> > > > >    8000h - FFFFh   & Reserved    & - \\
> > > > >    \hline
> > > > > @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
> > > > >           /* This field indicates which of the below optional admin
> > > > >            * capabilities are supported by the device:
> > > > >            * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
> > > > > -        * Bits 1 - 63 - reserved for future capabilities.
> > > > > +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
> > > > > +        * Bits 2 - 63 - reserved for future capabilities.
> > > > >            */
> > > > >           le64 optional_caps_support;
> > > > >           u8 reserved[4072];
> > > > > @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
> > > > >           u8 reserved[4056];
> > > > >    };
> > > > >    \end{lstlisting}
> > > > > +
> > > > > +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
> > > > > +
> > > > > +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
> > > > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
> > > > > +
> > > > > +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
> > > > > +Upon success, the returned data buffer is of form:
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_admin_device_info_result {
> > > > > +        /* For compatibility - indicates which of the below fields were returned
> > > > > +         * (1 means that field was returned):
> > > > So how is this supposed to be handled. You need to write this up.
> > > > Also having each command have a bitmap seems like an overkill
> > > > to me. But if that is the way you are going just put
> > > > this in the generic part.
> > > > 
> > > > 
> > > > > +         * Bit 0 - vf_number
> > > > > +         * Bits 1 - 63 - reserved for future fields
> > > > we generally say "future use" not "future fields".
> > > ok.
> > > 
> > > 
> > > > > +         */
> > > > > +        le64 attrs_mask;
> > > > > +        /* The virtual function number */
> > > > > +        le16 vf_number;
> > > > > +        u8 reserved[1014];
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +\begin{note}
> > > > > +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
> > > > > +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
> > > > > +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
> > > > > +\end{note}
> > > > I personally would just make vf # *be* the device ID, with no need for an
> > > > extra indirection. It doesn't hurt much though ...
> > > This is implementation specific.
> > > 
> > > You need to have a way to identify the vf number.
> > So my point is, I see no need to have a random set
> > of VFs or have an abstract ID as an indirection layer -
> > we can just say id == VF# and always manage all VF #s up to N.
> 
> But the TG asked to add a vdev_id for future extensions and it helped me to
> define a subsystem.
> 
> This id can be == VF# in PCI but it is not a VF# for non VFs.
> 
> We agreed on that in previous discussions.

yes that part is good.


> > 
> > This will cut out a bunch of commands.
> > 
> > And hey, if we see the need for an indirection down the road we
> > will be add it as a separate dst_type.
> 
> But we tried it without dst_type in the initial versions..
> 
> I worked hard to design it and now we go back ?

It's a fact of life, one has to iterate and some things have to be discarded.


> and what will be in the next
> version ? add it again ?

The abstraction of the id is a good thing. We do not need extra
commands for this though, just make it == VF# for SRIOB.


> I think the dst_type, vdev_id, subsystem framework is good and not too
> complicated.
>
> > 
> > > > But I do not seem to understand how it all works here. I think a generic
> > > > description explaining how all this is used is necessary, pls add a
> > > > writeup.
> > > > 
> > > > E.g.
> > > > 
> > > > So I start with a device and through a feature bit I find out it
> > > > supports admin commands. Then I query that and find the max vdev id.
> > > > Query the ID of each device. Then through that query VF number.
> > > > Something like this.
> > > > 
> > > > Looks in device operation section for some examples.
> > > Ok I'll try to add more explanation.
> > > 
> > > 
> > > > 
> > > > > +
> > > > > -- 
> > > > > 2.21.0

[virtio-dev] Re: [PATCH v1 2/5] Introduce Admin Command Set

[Michael S. Tsirkin]

On Tue, Apr 05, 2022 at 01:58:11PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 7:26 PM, Michael S. Tsirkin wrote:
> > On Mon, Apr 04, 2022 at 06:35:16PM +0300, Max Gurtovoy wrote:
> > > On 4/4/2022 3:50 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote:
> > > > > This command set is used for essential administrative and management
> > > > > operations such as identify and resource management.
> > > > > 
> > > > > Admin commands should be submitted to a well defined management
> > > > > interface.
> > > > > 
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > ---
> > > > >    admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >    content.tex      |   2 +
> > > > >    introduction.tex |   2 +
> > > > >    3 files changed, 133 insertions(+)
> > > > >    create mode 100644 admin.tex
> > > > > 
> > > > > diff --git a/admin.tex b/admin.tex
> > > > > new file mode 100644
> > > > > index 0000000..1e30e01
> > > > > --- /dev/null
> > > > > +++ b/admin.tex
> > > > > @@ -0,0 +1,129 @@
> > > > > +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}
> > > > > +
> > > > > +The Admin command set defines the commands that can be issued to a well defined management
> > > > > +interface.
> > > > This is shorthand for what? Administration commands?
> > > > Let's eschew abbreviation pls.
> > > > Also, management is a bit oblique, and only the reader can judge how well it's defined :)
> > > > 
> > > > 
> > > > I would rephrase this along the lines of
> > > > 
> > > > " For security and ease of management reasons, devices can expose an additional interface
> > > Why should we add security here ?
> > > 
> > > Lets keep it simple.
> > you want some motivation, this has consistently been a problem with this
> > project, motivation is not well documented. I say add all that can be
> > remotely relevant in here.  The security refers to IOMMU using VF# for
> > protection, I think it's relevant.
> 
> The spec should be simple and generic. IOMMU/VFs/Security is not related to
> the introduction of the admin command set.
> 
> Otherwise, it will grow for each example/reason we'll think in the future.
> 
> Also the interface is not relevant and was previously asked to be separate
> from the admin command set patch.
> 
> The motivation is clear but we can improve.
> 
> We can do:
> 
> "The Admin command set defines the commands that can be issued using a dedicated management interface.
>  For example, a host management system might want to access and configure a device before it is initialized by
>  the device driver. This can be done using a management device and its management driver."


Can we please cut out the word management? The above paragraph says
management 4 times. this does not make things clear, reader just
loses track.


> > 
> > > >     to access the device besides the ones specified in the
> > > >     transports section. For example, a host management system
> > > >     might want to access the device while in use by driver,
> > > >     or to configure the device before it is initialized by
> > > >     the driver.
> > > >     This access is facilitated through a set of administration commands.
> > > > 
> > > > "
> > > > 
> > > > 
> > > > speaking of this, are we still going to use the "driver"
> > > > terminology for uses of this?
> > > There is no other way to access a device from the host/guess other than
> > > driver.
> > > 
> > > A management system will have some interface provided by a driver to access
> > > a device.
> > 
> > Heh. However with admin commands a driver of device X accesses device Y.
> > It is not *the driver* - it's a different driver.
> 
> see above. I called it device driver.

not good, that is already in use.


> > Given we are already getting confused, it looks like we need to differentiate
> > between the two types of driver by using some other term here.
> > I suggest "administrator" but "manager" or "management driver" could be
> > other options. I wonder what do others think.
> see above.
> > 
> > > > 
> > > > > All the Admin commands are of the following form:
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_admin_cmd {
> > > > > +        /* Device-readable part */
> > > > > +        le16 command;
> > > > > +        /*
> > > > > +         * 0 - self
> > > > > +         * 1 - 65535 are reserved
> > > > > +         */
> > > > > +        le16 dst_type;
> > > > I think we want to add the vdev id here in the generic command
> > > > and just have a special id that means "self".
> > > vdev id was added in later patch. No need for special id means self. Each
> > > device has only 1 id and not 2.
> > > 
> > > This was proposed by Cornelia IIRC.
> > Yea, it's ok with that patch. The way it's split isn't ideal
> > unfortunately maybe move it into this patch in the next version.
> 
> I'll check if this make sense during the preparations of the next version.
> 
> 
> > 
> > > > 
> > > > > +        /* reserved for common cmd fields */
> > > > > +        u8 reserved[20];
> > > > > +        u8 command_specific_data[];
> > > > > +
> > > > > +        /* Device-writable part */
> > > > > +        u8 status;
> > > > > +        u8 command_specific_error;
> > > > > +        u8 command_specific_result[];
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +The following table describes the generic Admin status codes:
> > > > > +
> > > > > +\begin{tabular}{|l|l|l|}
> > > > > +\hline
> > > > > +Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
> > > > > +\hline
> > > > > +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
> > > > > +\hline
> > > > > +\end{tabular}
> > > > > +
> > > > > +The \field{command}, \field{dst_type} and \field{command_specific_data} are
> > > > > +set by the driver, and the device sets the \field{status}, the
> > > > > +\field{command_specific_error} and the \field{command_specific_result},
> > > > > +if needed.
> > > > > +
> > > > > +Reserved common fields are ignored by the device and to be zeroed by the driver.
> > > > > +
> > > > > +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
> > > > > +
> > > > > +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
> > > > > +
> > > > > +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
> > > > > +
> > > > > +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 content 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.
> > > > > +
> > > > > +The following table describes the Admin command set:
> > > > > +
> > > > > +\begin{tabular}{|l|l|l|}
> > > > > +\hline
> > > > > +Opcode & Command & M/O \\
> > > > > +\hline \hline
> > > > > +0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
> > > > > +\hline
> > > > > +0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
> > > > > +\hline
> > > > > +0002h - 7FFFh   & Generic admin cmds    & -  \\
> > > > > +\hline
> > > > > +8000h - FFFFh   & Reserved    & - \\
> > > > > +\hline
> > > > > +\end{tabular}
> > > > > +
> > > > > +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
> > > > > +
> > > > > +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
> > > > > +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
> > > > > +Other common fields are reserved for this command and zeroed.
> > > > > +
> > > > > +This command upon success, returns a data buffer
> > > > let's avoid using buffer since we want to allow non-DMA uses
> > > > IIUC. just "result" is ok, right?
> > > How would device write data to host memory ?
> > > 
> > > driver prepare a buffer and device will write to this buffer.
> > commands could use MMIO with no DMA.
> 
> what do you mean ?

I am referring to some way to pass commands that does not
involve the VQ. Jason asked for keeping that option open.

> 
> Are you referring to the suggestion for configuration registers we already
> rejected (please read the cover letter) or something else ?

Find another way to address Jason's comments then.

> > 
> > > > > that describes information about the target virtio device.
> > > > target being what? destination?
> > > yes.
> > then make all wording consistent please.
> sure.
> > 
> > > > > +This returned data buffer is of form:
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_admin_device_identify_result {
> > > > > +       /* For compatibility - indicates which of the below fields were returned
> > > > compatibility with what?
> > > I will remove "compatibility". It just indicates the valid fields.
> > > > > +        * (1 means that field was returned):
> > > > > +        * Bit 0 - vdev_id
> > > > > +        * Bit 1 - optional_caps_support
> > > > > +        * Bits 2 - 63 - reserved for future fields
> > > > > +        */
> > > > > +       le64 attrs_mask;
> > > > this seems to be some kind of thing listing supported commands, except
> > > > it's one way as opposed to negotiated like feature bits.  From
> > > > experience this kind of one way capability might be problematic since
> > > > you never know what will be used.  How about just using feature bits
> > > > instead? How many of these do you expect to be there?
> > > It's not features.
> > > 
> > > I don't see a need for negotiation here.
> > I guess I do ;) Donnu what do others think.
> 
> The device shouldn't care if a driver "accepts" the fact it support a
> specific command.
> 
> The driver can just issue this command.
> There is no optimization that should be done if some command is supported by
> the driver (from the device perspective).

This spec has been out there for some time.

Time and again it turned out to be useful very early to know the full
list of commands driver might use down the road.





> 
> > 
> > 
> > > > If not, please add a description of this.
> > > > E.g. along the lines of
> > > > 
> > > > 	Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ??
> > > I described bit by bit and mentioned that this is a mask. For each bit the
> > > value of 1 is valid.
> > 
> > Look at e.g. RSS as a better example of documenting this.
> > 
> > > > 
> > > > > +       le64 vdev_id;
> > > > So this is a way to query the id of device itself? Since the only dst
> > > > type is self ...
> > > For example yes. This is one of the things you can get from this command.
> > I don't see why this is useful.
> 
> It helps identifying a device inside a virtio subsystem/group.

You found the device somehow, you know the ID through that
bus specific method.


> > 
> > > > 
> > > > > +       /* This field indicates which of the below optional admin
> > > > > +        * capabilities are supported by the device:
> > > > > +        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
> > > > > +        * Bits 1 - 63 - reserved for future capabilities.
> > > > > +        */
> > > > > +       le64 optional_caps_support;
> > > > > +       u8 reserved[4072];
> > > > What exactly is this reserved thing? Does this mean the structure is
> > > > always exactly 4k?
> > > Yes the result buffer size is 4k.
> > > 
> > That's huge, will be a problem if it's MMIO and not DMA, and
> > mostly just wasted. why not make it as big as it needs to be?
> 
> But in another place you've asked why it is so small.
> 
> You need it to be fixed size since the format is the contract between the
> driver and the device.
> 
> We don't want to start negotiating on each bit.

Negotiating each field is exactly what virtio did for years now.
Attempts to burn up memory to have a fixed layout look like
going back to 0.9 times, this backfired at that time
and allocated memory was never enough.


> > 
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
> > > > > +
> > > > > +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
> > > > > +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
> > > > > +Other common fields are reserved for this command and zeroed.
> > > > zeroed by driver?
> > > Yes.
> > you want to go over places like this and everywhere say who does what.
> 
> if it's clear I can remove it.

the reverse, you need to add it, you just say "zeroed" etc,
should be "zeroed by device" etc etc.


> > 
> > > > > +
> > > > > +This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
> > > > > +This returned data buffer is of form:
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_admin_subsystem_identify_result {
> > > > > +       /* For compatibility - indicates which of the below fields were returned
> > > > > +        * (1 means that field was returned):
> > > > > +        * Bit 0 - vqn
> > > > > +        * Bit 1 - num_supported_vdevs
> > > > > +        * Bit 2 - max_vdev_id
> > > > > +        * Bits 3 - 63 - reserved for future fields
> > > > > +        */
> > > > > +       le64 attrs_mask;
> > > > add description here.
> > > Ok.
> > > 
> > > > > +       u8 vqn[16];
> > > > > +       /* Number of supported virtio devices by the subsystem */
> > > > > +       le64 num_supported_vdevs;
> > > > > +       /* Maximum value of a valid vdev_id for the subsystem */
> > > > > +       le64 max_vdev_id;
> > > > > +       u8 reserved[4056];
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > diff --git a/content.tex b/content.tex
> > > > > index c6f116c..2e1df84 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
> > > > > diff --git a/introduction.tex b/introduction.tex
> > > > > index 8e6611e..94dd7a2 100644
> > > > > --- a/introduction.tex
> > > > > +++ b/introduction.tex
> > > > > @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
> > > > >    The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
> > > > > +VQN and vdev_id are exposed via Admin Command Set.
> > > > > +
> > > > >    \newpage
> > > > > -- 
> > > > > 2.21.0


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

[virtio-dev] Re: [PATCH v1 4/5] Add virtio Admin virtqueue

[Michael S. Tsirkin]

On Tue, Apr 05, 2022 at 02:13:30PM +0300, Max Gurtovoy wrote:
> 
> On 4/4/2022 7:13 PM, Michael S. Tsirkin wrote:
> > On Mon, Apr 04, 2022 at 06:49:19PM +0300, Max Gurtovoy wrote:
> > > On 4/4/2022 4:02 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Mar 02, 2022 at 05:56:07PM +0200, Max Gurtovoy wrote:
> > > > > 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). Some of this configuration is generic enough. i.e
> > > > > Number of MSI-X vectors of a virtio PCI VF device. There is a need to do
> > > > > such features query and manipulation by its parent PCI PF. 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.
> > > > > 
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
> > > > > ---
> > > > >    admin.tex       | 17 +++++++++++++++++
> > > > >    conformance.tex |  1 +
> > > > >    content.tex     |  6 ++++--
> > > > >    3 files changed, 22 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/admin.tex b/admin.tex
> > > > > index 9a8969b..ed9f6bd 100644
> > > > > --- a/admin.tex
> > > > > +++ b/admin.tex
> > > > > @@ -158,3 +158,20 @@ \subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Vi
> > > > >    specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
> > > > >    \end{note}
> > > > > +\section{Admin Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Admin Virtqueues}
> > > > > +
> > > > > +An admin virtqueue is a management interface of a device that can be used to send administrative
> > > > > +commands to manipulate various features of the device and/or to manipulate
> > > > > +various features, if possible, of another device within the same virtio subsystem
> > > > > +(see \ref{sec:Introduction / Definitions / virtio subsystem}).
> > > > > +
> > > > > +An admin virtqueue exists for a certain device if VIRTIO_F_ADMIN_VQ is
> > > > > +negotiated. The index of the admin virtqueue is exposed by the device in a
> > > > > +transport specific manner.
> > > > > +
> > > > > +If VIRTIO_F_ADMIN_VQ has been negotiated, the driver will use the admin virtqueue to send all admin commands.
> > > > > +
> > > > Add links please, and explain how do commands map to VQ buffers.
> > > I added a link to the definitions section and I'll replace it with the new
> > > proposed terminology chapter.
> > That explains what a subsystem is.
> 
> I'm confused. Are we staying with "subsystem" or moving to "group" ? I
> prefer to stay.


I prefer renaming. you asked which links to add. I answered that you
have a link to definition of subsystem or whatever it is named,
and that is not enough imo.

> 
> > 
> > > What other links should be added ?
> > A link to where it's explained how do admin commands look.
> 
> ok.
> 
> > 
> > > > E.g. I guess device readable an output buffer and device writeable an input
> > > > buffer...
> > > The buffers transfer from/to device/driver are using existing mechanism.
> > > Nothing new here.
> > > 
> > > It will just repeat existing chapters.
> > There could be some repetition but it's necessary to make things
> > explicit.  Look at e.g.
> > subsection{Device Operation}\label{sec:Device Types / Block Device / Device Operation}
> > as a lightweight example for how to explain it.
> 
> Ok, I'll check it.
> 
> The proposal is not small now, and I prefer not repeat on existing
> mechanisms in the spec.
> 
> Maybe I'll add a sentence or two.

Yea, like that. Not a full explanation.


> > 
> > 
> > 
> > 
> > > > > +\devicenormative{\subsection}{Admin Virtqueues}{Basic Facilities of a Virtio Device / Admin Virtqueues}
> > > > > +A device that advertises VIRTIO_F_ADMIN_VQ capability MUST support all the mandatory admin commands.
> > > > > +
> > > > > +A device that advertises VIRTIO_F_ADMIN_VQ capability MAY support one or more optional admin commands.
> > > > > diff --git a/conformance.tex b/conformance.tex
> > > > > index 42f8537..129831c 100644
> > > > > --- a/conformance.tex
> > > > > +++ b/conformance.tex
> > > > > @@ -341,6 +341,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> > > > >    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / Available Buffer Notification Suppression}
> > > > >    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory Regions}
> > > > >    \item \ref{devicenormative:Reserved Feature Bits}
> > > > > +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Admin Virtqueues}
> > > > >    \end{itemize}
> > > > >    \conformance{\subsection}{PCI Device Conformance}\label{sec:Conformance / Device Conformance / PCI Device Conformance}
> > > > > diff --git a/content.tex b/content.tex
> > > > > index 2e1df84..163cb34 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}
> > > > > @@ -6849,6 +6849,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}
> > > > > -- 
> > > > > 2.21.0


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

[virtio-comment] Re: [PATCH v1 2/5] Introduce Admin Command Set

[Max Gurtovoy]

On 4/5/2022 3:28 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 05, 2022 at 01:58:11PM +0300, Max Gurtovoy wrote:
>> On 4/4/2022 7:26 PM, Michael S. Tsirkin wrote:
>>> On Mon, Apr 04, 2022 at 06:35:16PM +0300, Max Gurtovoy wrote:
>>>> On 4/4/2022 3:50 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 02, 2022 at 05:56:05PM +0200, Max Gurtovoy wrote:
>>>>>> This command set is used for essential administrative and management
>>>>>> operations such as identify and resource management.
>>>>>>
>>>>>> Admin commands should be submitted to a well defined management
>>>>>> interface.
>>>>>>
>>>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>> ---
>>>>>>     admin.tex        | 129 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>     content.tex      |   2 +
>>>>>>     introduction.tex |   2 +
>>>>>>     3 files changed, 133 insertions(+)
>>>>>>     create mode 100644 admin.tex
>>>>>>
>>>>>> diff --git a/admin.tex b/admin.tex
>>>>>> new file mode 100644
>>>>>> index 0000000..1e30e01
>>>>>> --- /dev/null
>>>>>> +++ b/admin.tex
>>>>>> @@ -0,0 +1,129 @@
>>>>>> +\section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admin command set}
>>>>>> +
>>>>>> +The Admin command set defines the commands that can be issued to a well defined management
>>>>>> +interface.
>>>>> This is shorthand for what? Administration commands?
>>>>> Let's eschew abbreviation pls.
>>>>> Also, management is a bit oblique, and only the reader can judge how well it's defined :)
>>>>>
>>>>>
>>>>> I would rephrase this along the lines of
>>>>>
>>>>> " For security and ease of management reasons, devices can expose an additional interface
>>>> Why should we add security here ?
>>>>
>>>> Lets keep it simple.
>>> you want some motivation, this has consistently been a problem with this
>>> project, motivation is not well documented. I say add all that can be
>>> remotely relevant in here.  The security refers to IOMMU using VF# for
>>> protection, I think it's relevant.
>> The spec should be simple and generic. IOMMU/VFs/Security is not related to
>> the introduction of the admin command set.
>>
>> Otherwise, it will grow for each example/reason we'll think in the future.
>>
>> Also the interface is not relevant and was previously asked to be separate
>> from the admin command set patch.
>>
>> The motivation is clear but we can improve.
>>
>> We can do:
>>
>> "The Admin command set defines the commands that can be issued using a dedicated management interface.
>>   For example, a host management system might want to access and configure a device before it is initialized by
>>   the device driver. This can be done using a management device and its management driver."
>
> Can we please cut out the word management? The above paragraph says
> management 4 times. this does not make things clear, reader just
> loses track.
>
>
>>>>>      to access the device besides the ones specified in the
>>>>>      transports section. For example, a host management system
>>>>>      might want to access the device while in use by driver,
>>>>>      or to configure the device before it is initialized by
>>>>>      the driver.
>>>>>      This access is facilitated through a set of administration commands.
>>>>>
>>>>> "
>>>>>
>>>>>
>>>>> speaking of this, are we still going to use the "driver"
>>>>> terminology for uses of this?
>>>> There is no other way to access a device from the host/guess other than
>>>> driver.
>>>>
>>>> A management system will have some interface provided by a driver to access
>>>> a device.
>>> Heh. However with admin commands a driver of device X accesses device Y.
>>> It is not *the driver* - it's a different driver.
>> see above. I called it device driver.
> not good, that is already in use.
>
>
>>> Given we are already getting confused, it looks like we need to differentiate
>>> between the two types of driver by using some other term here.
>>> I suggest "administrator" but "manager" or "management driver" could be
>>> other options. I wonder what do others think.
>> see above.
>>>>>> All the Admin commands are of the following form:
>>>>>> +
>>>>>> +\begin{lstlisting}
>>>>>> +struct virtio_admin_cmd {
>>>>>> +        /* Device-readable part */
>>>>>> +        le16 command;
>>>>>> +        /*
>>>>>> +         * 0 - self
>>>>>> +         * 1 - 65535 are reserved
>>>>>> +         */
>>>>>> +        le16 dst_type;
>>>>> I think we want to add the vdev id here in the generic command
>>>>> and just have a special id that means "self".
>>>> vdev id was added in later patch. No need for special id means self. Each
>>>> device has only 1 id and not 2.
>>>>
>>>> This was proposed by Cornelia IIRC.
>>> Yea, it's ok with that patch. The way it's split isn't ideal
>>> unfortunately maybe move it into this patch in the next version.
>> I'll check if this make sense during the preparations of the next version.
>>
>>
>>>>>> +        /* reserved for common cmd fields */
>>>>>> +        u8 reserved[20];
>>>>>> +        u8 command_specific_data[];
>>>>>> +
>>>>>> +        /* Device-writable part */
>>>>>> +        u8 status;
>>>>>> +        u8 command_specific_error;
>>>>>> +        u8 command_specific_result[];
>>>>>> +};
>>>>>> +\end{lstlisting}
>>>>>> +
>>>>>> +The following table describes the generic Admin status codes:
>>>>>> +
>>>>>> +\begin{tabular}{|l|l|l|}
>>>>>> +\hline
>>>>>> +Opcode & Status & 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_COMMAND_UNSUPPORTED    & unsupported or invalid opcode  \\
>>>>>> +\hline
>>>>>> +03h   & VIRTIO_ADMIN_STATUS_INVALID_FIELD    & invalid field was set  \\
>>>>>> +\hline
>>>>>> +\end{tabular}
>>>>>> +
>>>>>> +The \field{command}, \field{dst_type} and \field{command_specific_data} are
>>>>>> +set by the driver, and the device sets the \field{status}, the
>>>>>> +\field{command_specific_error} and the \field{command_specific_result},
>>>>>> +if needed.
>>>>>> +
>>>>>> +Reserved common fields are ignored by the device and to be zeroed by the driver.
>>>>>> +
>>>>>> +The mandatory fields to be set by the driver, for all admin commands, are \field{command} and \field{dst_type}.
>>>>>> +
>>>>>> +The \field{command} defines the opcode for the command. The value for each command can be found in each command section.
>>>>>> +
>>>>>> +The \field{dst_type} defines the target virtio device for the command. This value should be set to 0 (self).
>>>>>> +
>>>>>> +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 content 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.
>>>>>> +
>>>>>> +The following table describes the Admin command set:
>>>>>> +
>>>>>> +\begin{tabular}{|l|l|l|}
>>>>>> +\hline
>>>>>> +Opcode & Command & M/O \\
>>>>>> +\hline \hline
>>>>>> +0000h   & VIRTIO_ADMIN_DEVICE_IDENTIFY    & M  \\
>>>>>> +\hline
>>>>>> +0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>>>>>> +\hline
>>>>>> +0002h - 7FFFh   & Generic admin cmds    & -  \\
>>>>>> +\hline
>>>>>> +8000h - FFFFh   & Reserved    & - \\
>>>>>> +\hline
>>>>>> +\end{tabular}
>>>>>> +
>>>>>> +\subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE IDENTIFY command}
>>>>>> +
>>>>>> +The VIRTIO_ADMIN_DEVICE_IDENTIFY command has no command specific data set by the driver.
>>>>>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_IDENTIFY.
>>>>>> +Other common fields are reserved for this command and zeroed.
>>>>>> +
>>>>>> +This command upon success, returns a data buffer
>>>>> let's avoid using buffer since we want to allow non-DMA uses
>>>>> IIUC. just "result" is ok, right?
>>>> How would device write data to host memory ?
>>>>
>>>> driver prepare a buffer and device will write to this buffer.
>>> commands could use MMIO with no DMA.
>> what do you mean ?
> I am referring to some way to pass commands that does not
> involve the VQ. Jason asked for keeping that option open.

This series is about admin command set and admin queue.

After we'll merge it or during the review, one can suggest a way to do that.

It doesn't block us from merging and from reviewing.

>
>> Are you referring to the suggestion for configuration registers we already
>> rejected (please read the cover letter) or something else ?
> Find another way to address Jason's comments then.

I'm not inventing another management interface besides the adminq. It's 
good enough for the needs of this patch set.

The community is welcomed to suggest more management interfaces down the 
road.

>
>>>>>> that describes information about the target virtio device.
>>>>> target being what? destination?
>>>> yes.
>>> then make all wording consistent please.
>> sure.
>>>>>> +This returned data buffer is of form:
>>>>>> +\begin{lstlisting}
>>>>>> +struct virtio_admin_device_identify_result {
>>>>>> +       /* For compatibility - indicates which of the below fields were returned
>>>>> compatibility with what?
>>>> I will remove "compatibility". It just indicates the valid fields.
>>>>>> +        * (1 means that field was returned):
>>>>>> +        * Bit 0 - vdev_id
>>>>>> +        * Bit 1 - optional_caps_support
>>>>>> +        * Bits 2 - 63 - reserved for future fields
>>>>>> +        */
>>>>>> +       le64 attrs_mask;
>>>>> this seems to be some kind of thing listing supported commands, except
>>>>> it's one way as opposed to negotiated like feature bits.  From
>>>>> experience this kind of one way capability might be problematic since
>>>>> you never know what will be used.  How about just using feature bits
>>>>> instead? How many of these do you expect to be there?
>>>> It's not features.
>>>>
>>>> I don't see a need for negotiation here.
>>> I guess I do ;) Donnu what do others think.
>> The device shouldn't care if a driver "accepts" the fact it support a
>> specific command.
>>
>> The driver can just issue this command.
>> There is no optimization that should be done if some command is supported by
>> the driver (from the device perspective).
> This spec has been out there for some time.

give me one good reason besides history.

>
> Time and again it turned out to be useful very early to know the full
> list of commands driver might use down the road.

It won't save even 1 line of code on the device side.

If you insist on that, please propose something.

>
>
>
>
>>>
>>>>> If not, please add a description of this.
>>>>> E.g. along the lines of
>>>>>
>>>>> 	Field \field{attrs_mask} contains a bitmask of valid ?? as defined in ??
>>>> I described bit by bit and mentioned that this is a mask. For each bit the
>>>> value of 1 is valid.
>>> Look at e.g. RSS as a better example of documenting this.
>>>
>>>>>> +       le64 vdev_id;
>>>>> So this is a way to query the id of device itself? Since the only dst
>>>>> type is self ...
>>>> For example yes. This is one of the things you can get from this command.
>>> I don't see why this is useful.
>> It helps identifying a device inside a virtio subsystem/group.
> You found the device somehow, you know the ID through that
> bus specific method.

somehow ? can you be more specific ?

are you thinking about automatic orchestration ?

What is the big deal to accept the proposal of vdev_id that was proposed 
by your team and you didn't nack on it in the past.

lets progress.

>
>
>>>>>> +       /* This field indicates which of the below optional admin
>>>>>> +        * capabilities are supported by the device:
>>>>>> +        * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
>>>>>> +        * Bits 1 - 63 - reserved for future capabilities.
>>>>>> +        */
>>>>>> +       le64 optional_caps_support;
>>>>>> +       u8 reserved[4072];
>>>>> What exactly is this reserved thing? Does this mean the structure is
>>>>> always exactly 4k?
>>>> Yes the result buffer size is 4k.
>>>>
>>> That's huge, will be a problem if it's MMIO and not DMA, and
>>> mostly just wasted. why not make it as big as it needs to be?
>> But in another place you've asked why it is so small.
>>
>> You need it to be fixed size since the format is the contract between the
>> driver and the device.
>>
>> We don't want to start negotiating on each bit.
> Negotiating each field is exactly what virtio did for years now.

again, same reason.

> Attempts to burn up memory to have a fixed layout look like
> going back to 0.9 times, this backfired at that time
> and allocated memory was never enough.

burn up memory ? where is this coming from ?

>
>>>>>> +};
>>>>>> +\end{lstlisting}
>>>>>> +
>>>>>> +\subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN SUBSYSTEM IDENTIFY command}
>>>>>> +
>>>>>> +The VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY command has no command specific data set by the driver.
>>>>>> +The \field{command} is set to VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY.
>>>>>> +Other common fields are reserved for this command and zeroed.
>>>>> zeroed by driver?
>>>> Yes.
>>> you want to go over places like this and everywhere say who does what.
>> if it's clear I can remove it.
> the reverse, you need to add it, you just say "zeroed" etc,
> should be "zeroed by device" etc etc.
>
can anything be implicit ?

even the simplest things such as setting reserved/unused bits that are 
sent by a driver obviously ?

>>>>>> +
>>>>>> +This command upon success, returns a data buffer that describes information about the target virtio device subsystem.
>>>>>> +This returned data buffer is of form:
>>>>>> +\begin{lstlisting}
>>>>>> +struct virtio_admin_subsystem_identify_result {
>>>>>> +       /* For compatibility - indicates which of the below fields were returned
>>>>>> +        * (1 means that field was returned):
>>>>>> +        * Bit 0 - vqn
>>>>>> +        * Bit 1 - num_supported_vdevs
>>>>>> +        * Bit 2 - max_vdev_id
>>>>>> +        * Bits 3 - 63 - reserved for future fields
>>>>>> +        */
>>>>>> +       le64 attrs_mask;
>>>>> add description here.
>>>> Ok.
>>>>
>>>>>> +       u8 vqn[16];
>>>>>> +       /* Number of supported virtio devices by the subsystem */
>>>>>> +       le64 num_supported_vdevs;
>>>>>> +       /* Maximum value of a valid vdev_id for the subsystem */
>>>>>> +       le64 max_vdev_id;
>>>>>> +       u8 reserved[4056];
>>>>>> +};
>>>>>> +\end{lstlisting}
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index c6f116c..2e1df84 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
>>>>>> diff --git a/introduction.tex b/introduction.tex
>>>>>> index 8e6611e..94dd7a2 100644
>>>>>> --- a/introduction.tex
>>>>>> +++ b/introduction.tex
>>>>>> @@ -258,5 +258,7 @@ \subsection{virtio subsystem}\label{sec:Introduction / Definitions / virtio subs
>>>>>>     The vdev_id value when combined with the VQN forms a globally unique value that identifies the virtio device.
>>>>>> +VQN and vdev_id are exposed via Admin Command Set.
>>>>>> +
>>>>>>     \newpage
>>>>>> -- 
>>>>>> 2.21.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://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

Re: [virtio-comment] Re: [PATCH v1 3/5] Introduce DEVICE INFO Admin command

[Max Gurtovoy]

On 4/5/2022 3:20 PM, Michael S. Tsirkin wrote:
> On Tue, Apr 05, 2022 at 02:27:35PM +0300, Max Gurtovoy wrote:
>> On 4/4/2022 7:09 PM, Michael S. Tsirkin wrote:
>>> On Mon, Apr 04, 2022 at 06:44:11PM +0300, Max Gurtovoy wrote:
>>>> On 4/4/2022 3:57 PM, Michael S. Tsirkin wrote:
>>>>> On Wed, Mar 02, 2022 at 05:56:06PM +0200, Max Gurtovoy wrote:
>>>>>> The DEVICE INFO command will return a basic information for a virtio
>>>>>> device.
>>>>>>
>>>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>>> ---
>>>>>>     admin.tex | 35 +++++++++++++++++++++++++++++++++--
>>>>>>     1 file changed, 33 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/admin.tex b/admin.tex
>>>>>> index 1e30e01..9a8969b 100644
>>>>>> --- a/admin.tex
>>>>>> +++ b/admin.tex
>>>>>> @@ -67,7 +67,9 @@ \section{Admin command set}\label{sec:Basic Facilities of a Virtio Device / Admi
>>>>>>     \hline
>>>>>>     0001h   & VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY    & O  \\
>>>>>>     \hline
>>>>>> -0002h - 7FFFh   & Generic admin cmds    & -  \\
>>>>>> +0002h   & VIRTIO_ADMIN_DEVICE_INFO    & O  \\
>>>>>> +\hline
>>>>>> +0003h - 7FFFh   & Generic admin cmds    & -  \\
>>>>>>     \hline
>>>>>>     8000h - FFFFh   & Reserved    & - \\
>>>>>>     \hline
>>>>>> @@ -94,7 +96,8 @@ \subsection{VIRTIO ADMIN DEVICE IDENTIFY command}\label{sec:Basic Facilities of
>>>>>>            /* This field indicates which of the below optional admin
>>>>>>             * capabilities are supported by the device:
>>>>>>             * Bit 0 - if set, VIRTIO_ADMIN_SUBSYSTEM_IDENTIFY supported
>>>>>> -        * Bits 1 - 63 - reserved for future capabilities.
>>>>>> +        * Bit 1 - if set, VIRTIO_ADMIN_DEVICE_INFO supported
>>>>>> +        * Bits 2 - 63 - reserved for future capabilities.
>>>>>>             */
>>>>>>            le64 optional_caps_support;
>>>>>>            u8 reserved[4072];
>>>>>> @@ -127,3 +130,31 @@ \subsection{VIRTIO ADMIN SUBSYSTEM IDENTIFY command}\label{sec:Basic Facilities
>>>>>>            u8 reserved[4056];
>>>>>>     };
>>>>>>     \end{lstlisting}
>>>>>> +
>>>>>> +\subsection{VIRTIO ADMIN DEVICE INFO command}\label{sec:Basic Facilities of a Virtio Device / Admin command set / VIRTIO ADMIN DEVICE INFO command}
>>>>>> +
>>>>>> +The VIRTIO_ADMIN_DEVICE_INFO command has no command specific data set by the driver.
>>>>>> +The \field{command} is set to VIRTIO_ADMIN_DEVICE_INFO.
>>>>>> +
>>>>>> +The VIRTIO_ADMIN_DEVICE_INFO upon success, returns a data buffer that describes a basic information for the target virtio device.
>>>>>> +Upon success, the returned data buffer is of form:
>>>>>> +\begin{lstlisting}
>>>>>> +struct virtio_admin_device_info_result {
>>>>>> +        /* For compatibility - indicates which of the below fields were returned
>>>>>> +         * (1 means that field was returned):
>>>>> So how is this supposed to be handled. You need to write this up.
>>>>> Also having each command have a bitmap seems like an overkill
>>>>> to me. But if that is the way you are going just put
>>>>> this in the generic part.
>>>>>
>>>>>
>>>>>> +         * Bit 0 - vf_number
>>>>>> +         * Bits 1 - 63 - reserved for future fields
>>>>> we generally say "future use" not "future fields".
>>>> ok.
>>>>
>>>>
>>>>>> +         */
>>>>>> +        le64 attrs_mask;
>>>>>> +        /* The virtual function number */
>>>>>> +        le16 vf_number;
>>>>>> +        u8 reserved[1014];
>>>>>> +};
>>>>>> +\end{lstlisting}
>>>>>> +
>>>>>> +\begin{note}
>>>>>> +{Bit 0 in \field{attrs_mask} will be set only if the target virtio device represents a PCI Virtual function.
>>>>>> +In this case, the \field{vf_number} value can't be greater than TotalVFs value as defined in the PCI
>>>>>> +specification and can't be equal to zero. If bit 0 is not set, the driver will ignore \field{vf_number}.}
>>>>>> +\end{note}
>>>>> I personally would just make vf # *be* the device ID, with no need for an
>>>>> extra indirection. It doesn't hurt much though ...
>>>> This is implementation specific.
>>>>
>>>> You need to have a way to identify the vf number.
>>> So my point is, I see no need to have a random set
>>> of VFs or have an abstract ID as an indirection layer -
>>> we can just say id == VF# and always manage all VF #s up to N.
>> But the TG asked to add a vdev_id for future extensions and it helped me to
>> define a subsystem.
>>
>> This id can be == VF# in PCI but it is not a VF# for non VFs.
>>
>> We agreed on that in previous discussions.
> yes that part is good.

So I'm keeping the vdev_id.

Lets progress.

Can we agree on one thing after such a long road please ?

>
>
>>> This will cut out a bunch of commands.
>>>
>>> And hey, if we see the need for an indirection down the road we
>>> will be add it as a separate dst_type.
>> But we tried it without dst_type in the initial versions..
>>
>> I worked hard to design it and now we go back ?
> It's a fact of life, one has to iterate and some things have to be discarded.

No.

The fact is the cycle here is over a year now:

1. one propose A.

2. other person review and ask to change it to B.

3. after half a year of nerve-racking discussions, one works hard to 
change it to B.

4. other person ask to come back and do A.

5. one changes to A.

6. other person goes back to bullet #2.


I want to progress. And now that you have time after QEMU and Linux 
submission it's the critical period.

I don't want to waste it on endless loops.

vdev_id was proposed by RH team and I create the entire virtio subsystem 
mechanism for the grouping that your team suggested.

Now, if there is nothing wrong with that (and I don't mean over 
engineering) - I would like to see a progress and not a cycle please.

You said that all is ok and just some wording is wrong. But it's not the 
case.

>
>> and what will be in the next
>> version ? add it again ?
> The abstraction of the id is a good thing. We do not need extra
> commands for this though, just make it == VF# for SRIOB.
>
The TG asked for a generic solution.

Lets progress.

>> I think the dst_type, vdev_id, subsystem framework is good and not too
>> complicated.
>>
>>>>> But I do not seem to understand how it all works here. I think a generic
>>>>> description explaining how all this is used is necessary, pls add a
>>>>> writeup.
>>>>>
>>>>> E.g.
>>>>>
>>>>> So I start with a device and through a feature bit I find out it
>>>>> supports admin commands. Then I query that and find the max vdev id.
>>>>> Query the ID of each device. Then through that query VF number.
>>>>> Something like this.
>>>>>
>>>>> Looks in device operation section for some examples.
>>>> Ok I'll try to add more explanation.
>>>>
>>>>
>>>>>> +
>>>>>> -- 
>>>>>> 2.21.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=04%7C01%7Cmgurtovoy%40nvidia.com%7C7f8ad483f1464791bedc08da16fec44f%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637847580718319493%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=ImAUqdj6r1FsL%2BGQsBgdCInDZusppiNli2Xzfr0Z%2B%2Fw%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=04%7C01%7Cmgurtovoy%40nvidia.com%7C7f8ad483f1464791bedc08da16fec44f%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637847580718319493%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=eX9qzJmZT3Y3w20YF4hwByGUIFd2FYRAljVbEbHgOfo%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=04%7C01%7Cmgurtovoy%40nvidia.com%7C7f8ad483f1464791bedc08da16fec44f%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637847580718319493%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=7C306Y4kOvzz95zhZ4V56PA0mo04d2RE1fwIgVkpSmk%3D&amp;reserved=0
> Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7C7f8ad483f1464791bedc08da16fec44f%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637847580718319493%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=KvNpokRcHZ3V9AayciKiA%2F5odbNVzc3qLgMry98hxHE%3D&amp;reserved=0
> Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&amp;data=04%7C01%7Cmgurtovoy%40nvidia.com%7C7f8ad483f1464791bedc08da16fec44f%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637847580718319493%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=DIFwDUqXkCuLRnegaJF0LGVnz8jmN6QmPp9xrP0Vgh4%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://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/