Introduce device group and device management

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented.
Do it now.

Suggested-by Halil Pasic <[email protected]>

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

---

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

content.tex +0 -0

index 0e474dd..0c2d917 100644
@@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a

Virtio Device / Feature B In particular, new fields in the device configuration space are indicated by offering a new feature bit.

content.tex +19 -0

+To keep the feature negotiation mechanism extensible, it is important
+that devices \em{do not} offer any feature bits that they would not be
+able to handle if the driver accepted them (even though drivers are not
+supposed to accept them in the first place even if offered, according
+to this version of the specification.) Likewise, it is important that
+drivers \em{do not} accept feature bits they do not know how to handle
+(even though devices are not supposed to offer them in the first place,
+according to this version of the specification.) The preferred way for
+handling reserved and unexpected features is that the driver ignores
+them.
+
+In particular, this is
+especially important for features limited to specific transports, as
+enabling these for more transports in future versions of the
+specification is highly likely to require changing the behaviour from
+drivers and devices.  Drivers and devices supporting multiple
+transports need to carefully maintain per-transport lists of allowed
+features.
+
 \drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device /

Feature Bits} The driver MUST NOT accept a feature which the device did not offer, and MUST NOT accept a feature which requires another feature which was not accepted.

\devicenormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device /
Feature Bits} The device MUST NOT offer a feature which requires another feature which was not offered. The device SHOULD accept any valid subset of

features the driver accepts, otherwise it MUST fail to set the FEATURES_OK \field{device status} bit when the driver writes it.

content.tex +11 -0

+The device MUST NOT offer feature bits corresponding to features it
+would not support if accepted by the driver (even if the driver is
+prohibited from accepting the feature bits by the specification); for
+the sake of clarity, this refers to feature bits not described in this
+specification, reserved feature bits and feature bits reserved or not
+supported for the specific transport or the specific device type, but
+this does not preclude devices written to a future version of this
+specification from offering such feature bits should such a
+specification have a provision for devices to support the corresponding
+features.
+
 If a device has successfully negotiated a set of features  at least once (by

accepting the FEATURES_OK \field{device status} bit during device initialization), then it SHOULD
--
MST

Quoted 8 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented.
Do it now.

Quoted 3 lines

Suggested-by Halil Pasic <[email protected]>

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

---

It may be little painful, but in 10 patch series, it is worth having per patch change log.
Because I do not know my reviewed by tag in [1] was dropped because of a big change, if so what was it or it was simply missed out.

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

Quoted 35 lines

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

content.tex +0 -0

index 0e474dd..0c2d917 100644
@@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a

Virtio Device / Feature B In particular, new fields in the device configuration space are indicated by offering a new feature bit.

content.tex +19 -0

+To keep the feature negotiation mechanism extensible, it is important
+that devices \em{do not} offer any feature bits that they would not be
+able to handle if the driver accepted them (even though drivers are not
+supposed to accept them in the first place even if offered, according
+to this version of the specification.) Likewise, it is important that
+drivers \em{do not} accept feature bits they do not know how to handle
+(even though devices are not supposed to offer them in the first place,
+according to this version of the specification.) The preferred way for
+handling reserved and unexpected features is that the driver ignores
+them.
+
+In particular, this is
+especially important for features limited to specific transports, as
+enabling these for more transports in future versions of the
+specification is highly likely to require changing the behaviour from
+drivers and devices.  Drivers and devices supporting multiple
+transports need to carefully maintain per-transport lists of allowed
+features.
+
 \drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device /

Feature Bits} The driver MUST NOT accept a feature which the device did not offer, and MUST NOT accept a feature which requires another feature which was not accepted.

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B

Quoted 8 lines

content.tex +7 -0

+The driver MUST validate the feature bits offered by the device.
+The driver MUST ignore and MUST NOT accept any feature bit that is
+\begin{itemize} \item not described in this specification, \item marked
+as reserved, \item not supported for the specific transport, \item not
+defined for the device type.
+\end{itemize}
+
 The driver SHOULD go into backwards compatibility mode  if the device does

The grammar tool is suggesting s/backwards/backward.

content.tex

Quoted 6 lines

not offer a feature it understands, otherwise MUST set the FAILED \field{device status} bit and cease initialization.

content.tex +3 -0

+By contrast, the driver MUST NOT fail solely because a feature it does
+not understand has been offered by the device.
+

Above line can be written without introducing "has been" given we have only two entities (driver, dev) to care about.

By contrast, the driver MUST NOT fail solely because it does not understand the feature offered by the device.

Quoted 3 lines

\devicenormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device /
Feature Bits} The device MUST NOT offer a feature which requires another feature which was not offered. The device SHOULD accept any valid subset of

Extra white space between feature and which, subset and of.

Quoted 3 lines

features the driver accepts, otherwise it MUST fail to set the FEATURES_OK \field{device status} bit when the driver writes it.

Extra white space between the FEATURE_OK

Quoted 12 lines

content.tex +11 -0

+The device MUST NOT offer feature bits corresponding to features it
+would not support if accepted by the driver (even if the driver is
+prohibited from accepting the feature bits by the specification); for
+the sake of clarity, this refers to feature bits not described in this
+specification, reserved feature bits and feature bits reserved or not
+supported for the specific transport or the specific device type, but
+this does not preclude devices written to a future version of this
+specification from offering such feature bits should such a
+specification have a provision for devices to support the corresponding
+features.
+
 If a device has successfully negotiated a set of features  at least once (by

Quoted 4 lines

accepting the FEATURES_OK \field{device status} bit during device initialization), then it SHOULD
--
MST

Quoted 15 lines · Michael S. Tsirkin

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented. Do it now.

Suggested-by Halil Pasic <[email protected]>

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

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

content.tex +0 -0

index 0e474dd..0c2d917 100644

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 121

Quoted 8 lines

content.tex +5 -0

@@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 In particular, new fields in the device configuration space are
 indicated by offering a new feature bit.
 
+To keep the feature negotiation mechanism extensible, it is important
+that devices \em{do not} offer any feature bits that they would not be
+able to handle if the driver accepted them (even though drivers are not
+supposed to accept them in the first place even if offered, according to
+this version of the specification.) 

I find this (the bit in parenthesis) confusing.

Why are drivers not supposed to accept features that they have been offered, given that they can't know that the device cannot handle the feature that it just offered?

Is this alluding to the later section:

Quoted 3 lines

feature bits not described in this specification, reserved feature bits and feature bits reserved or not supported for the specific transport or the specific device type

?

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B

Quoted 12 lines

Likewise, it is important that

content.tex +11 -0

+drivers \em{do not} accept feature bits they do not know how to handle
+(even though devices are not supposed to offer them in the first place,
+according to this version of the specification.) The preferred way for
+handling reserved and unexpected features is that the driver ignores
+them.
+
+In particular, this is
+especially important for features limited to specific transports,
+as enabling these for more transports in future versions of the
+specification is highly likely to require changing the behaviour
+from drivers and devices.

"require changed behaviour from drivers and devices".

Quoted 49 lines

Drivers and devices supporting

content.tex +26 -0

+multiple transports need to carefully maintain per-transport
+lists of allowed features.
+
 \drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The driver MUST NOT accept a feature which the device did not offer,
 and MUST NOT accept a feature which requires another feature which was
 not accepted.
 
+The driver MUST validate the feature bits offered by the device.
+The driver MUST ignore and MUST NOT accept any feature bit that is
+\begin{itemize}
+\item not described in this specification,
+\item marked as reserved,
+\item not supported for the specific transport,
+\item not defined for the device type.
+\end{itemize}
+
 The driver SHOULD go into backwards compatibility mode
 if the device does not offer a feature it understands, otherwise MUST
 set the FAILED \field{device status} bit and cease initialization.
 
+By contrast, the driver MUST NOT fail solely because a feature
+it does not understand has been offered by the device.
+
 \devicenormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The device MUST NOT offer a feature which requires another feature
 which was not offered.  The device SHOULD accept any valid subset
 of features the driver accepts, otherwise it MUST fail to set the
 FEATURES_OK \field{device status} bit when the driver writes it.
 
+The device MUST NOT offer feature bits corresponding to features
+it would not support if accepted by the driver (even if the
+driver is prohibited from accepting the feature bits by the
+specification); for the sake of clarity, this refers to feature
+bits not described in this specification, reserved feature bits
+and feature bits reserved or not supported for the specific
+transport or the specific device type, but this does not preclude
+devices written to a future version of this specification from
+offering such feature bits should such a specification have a
+provision for devices to support the corresponding features.
+
 If a device has successfully negotiated a set of features
 at least once (by accepting the FEATURES_OK \field{device
 status} bit during device initialization), then it SHOULD

Signature · 1 line

MST

Quoted 4 lines

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

Quoted 34 lines · David Edmondson

Quoted 15 lines · Michael S. Tsirkin

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented. Do it now.

Suggested-by Halil Pasic <[email protected]>

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

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

content.tex +0 -0

index 0e474dd..0c2d917 100644

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 121

Quoted 8 lines

content.tex +5 -0

@@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 In particular, new fields in the device configuration space are
 indicated by offering a new feature bit.
 
+To keep the feature negotiation mechanism extensible, it is important
+that devices \em{do not} offer any feature bits that they would not be
+able to handle if the driver accepted them (even though drivers are not
+supposed to accept them in the first place even if offered, according to
+this version of the specification.) 

I find this (the bit in parenthesis) confusing.

Why are drivers not supposed to accept features that they have been offered, given that they can't know that the device cannot handle the feature that it just offered?

Is this alluding to the later section:

Quoted 5 lines

Quoted 3 lines

feature bits not described in this specification, reserved feature bits and feature bits reserved or not supported for the specific transport or the specific device type

?

exactly. how would you put this better? given an example?

(even though drivers are not supposed to accept unrecognised features in the first place even if offered, according to the specification)

Quoted 68 lines

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B

Quoted 12 lines

Likewise, it is important that

content.tex +11 -0

+drivers \em{do not} accept feature bits they do not know how to handle
+(even though devices are not supposed to offer them in the first place,
+according to this version of the specification.) The preferred way for
+handling reserved and unexpected features is that the driver ignores
+them.
+
+In particular, this is
+especially important for features limited to specific transports,
+as enabling these for more transports in future versions of the
+specification is highly likely to require changing the behaviour
+from drivers and devices.

"require changed behaviour from drivers and devices".

Quoted 53 lines

Drivers and devices supporting

content.tex +26 -0

+multiple transports need to carefully maintain per-transport
+lists of allowed features.
+
 \drivernormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The driver MUST NOT accept a feature which the device did not offer,
 and MUST NOT accept a feature which requires another feature which was
 not accepted.
 
+The driver MUST validate the feature bits offered by the device.
+The driver MUST ignore and MUST NOT accept any feature bit that is
+\begin{itemize}
+\item not described in this specification,
+\item marked as reserved,
+\item not supported for the specific transport,
+\item not defined for the device type.
+\end{itemize}
+
 The driver SHOULD go into backwards compatibility mode
 if the device does not offer a feature it understands, otherwise MUST
 set the FAILED \field{device status} bit and cease initialization.
 
+By contrast, the driver MUST NOT fail solely because a feature
+it does not understand has been offered by the device.
+
 \devicenormative{\subsection}{Feature Bits}{Basic Facilities of a Virtio Device / Feature Bits}
 The device MUST NOT offer a feature which requires another feature
 which was not offered.  The device SHOULD accept any valid subset
 of features the driver accepts, otherwise it MUST fail to set the
 FEATURES_OK \field{device status} bit when the driver writes it.
 
+The device MUST NOT offer feature bits corresponding to features
+it would not support if accepted by the driver (even if the
+driver is prohibited from accepting the feature bits by the
+specification); for the sake of clarity, this refers to feature
+bits not described in this specification, reserved feature bits
+and feature bits reserved or not supported for the specific
+transport or the specific device type, but this does not preclude
+devices written to a future version of this specification from
+offering such feature bits should such a specification have a
+provision for devices to support the corresponding features.
+
 If a device has successfully negotiated a set of features
 at least once (by accepting the FEATURES_OK \field{device
 status} bit during device initialization), then it SHOULD

Signature · 7 lines

MST


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

Quoted 35 lines · Michael S. Tsirkin

Quoted 34 lines · David Edmondson

Quoted 15 lines · Michael S. Tsirkin

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented. Do it now.

Suggested-by Halil Pasic <[email protected]>

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

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

content.tex +0 -0

index 0e474dd..0c2d917 100644

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 121

Quoted 8 lines

content.tex +5 -0

@@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 In particular, new fields in the device configuration space are
 indicated by offering a new feature bit.
 
+To keep the feature negotiation mechanism extensible, it is important
+that devices \em{do not} offer any feature bits that they would not be
+able to handle if the driver accepted them (even though drivers are not
+supposed to accept them in the first place even if offered, according to
+this version of the specification.) 

I find this (the bit in parenthesis) confusing.

Why are drivers not supposed to accept features that they have been offered, given that they can't know that the device cannot handle the feature that it just offered?

Is this alluding to the later section:

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

feature bits not described in this specification, reserved feature bits and feature bits reserved or not supported for the specific transport or the specific device type

?

exactly. how would you put this better? given an example?

Perhaps it would be enough to say:

Quoted 2 lines

(even though drivers are not supposed to accept unrecognised features in the first place even if offered, according to the specification)

"Unrecognised" is intended as a shorthand for the whole "not described, reserved, ...". Maybe "unrecognised or reserved"?

Quoted 45 lines · David Edmondson

Quoted 35 lines · Michael S. Tsirkin

Quoted 34 lines · David Edmondson

Quoted 15 lines · Michael S. Tsirkin

Feature negotiation forms the basis of forward compatibility guarantees of virtio but has never been properly documented. Do it now.

Suggested-by Halil Pasic <[email protected]>

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

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

content.tex +0 -0

index 0e474dd..0c2d917 100644

content.tex \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B · line 121

Quoted 8 lines

content.tex +5 -0

@@ -114,21 +114,63 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 In particular, new fields in the device configuration space are
 indicated by offering a new feature bit.
 
+To keep the feature negotiation mechanism extensible, it is important
+that devices \em{do not} offer any feature bits that they would not be
+able to handle if the driver accepted them (even though drivers are not
+supposed to accept them in the first place even if offered, according to
+this version of the specification.) 

I find this (the bit in parenthesis) confusing.

Why are drivers not supposed to accept features that they have been offered, given that they can't know that the device cannot handle the feature that it just offered?

Is this alluding to the later section:

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

feature bits not described in this specification, reserved feature bits and feature bits reserved or not supported for the specific transport or the specific device type

?

exactly. how would you put this better? given an example?

Quoted 7 lines

Perhaps it would be enough to say:

Quoted 2 lines

(even though drivers are not supposed to accept unrecognised features in the first place even if offered, according to the specification)

"Unrecognised" is intended as a shorthand for the whole "not described, reserved, ...". Maybe "unrecognised or reserved"?

Hm, what about

"even though drivers are not supposed to accept any unspecified, reserved, or unsupported features even if offered..."

?

I'm not sure how we can make this both short and descriptive enough...

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

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

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

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

Note: one can argue both ways whether the new device group handling functionality (this and following patches) is closer to a new device type or a new transport type.

However, I expect that we will add more features in the near future. To facilitate this as much as possible of the text is located in the new admin chapter.

I did my best to minimize transport-specific text.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 51 insertions(+)
 create mode 100644 admin.tex

admin.tex new file +27 -0

new file mode 100644
index 0000000..3dc47be
@@ -0,0 +1,49 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device /
+Device groups}
+
+It is occasionally useful to have a device control a group of other
+devices. Terminology used in such cases:
+
+\begin{description}
+\item[Device group]
+        or just group, includes zero or more devices.
+\item[Owner device]
+        or owner, the device controlling the group.
+\item[Member device]
+        a device within a group. The owner device itself is not
+	a member of the group.
+\item[Member identifier]
+        each member has this identifier, unique within the group
+	and used to address it through the owner device.
+\item[Group type identifier]
+	specifies what kind of member devices there are in a
+	group, how is the member identifier is interpreted
+	and what kind of control the owner has.
+	A given owner can control a single group of a given type,
+	thus the type and the owner together identify the group.
+	\footnote{Even though some group types only support
+			specific transports, group type identifiers
+			are global rather than transport-specific -
+			we don't expect a flood of new group types.}

\end{description}

admin.tex new file +22 -0

+
+The following group types, and their identifiers, are currently specified):
+\begin{description}
+\item[SR-IOV group type (0x1)]
+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as the owner and includes all
+its SR-IOV virtual functions (VFs) as members (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+The PF device itself is not a member of the group.
+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value from 0x1 to 0xFFFF
+and equals the SR-IOV VF number of the member device (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+Both owner and member devices for this group type use the Virtio PCI
+transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
+\end{description}
+
+

content.tex +0 -0

index 0c2d917..ffe45c4 100644
@@ -491,6 +491,8 @@ \section{Exporting Objects}\label{sec:Basic Facilities of

a Virtio Device / Expo types. It is RECOMMENDED that devices generate version 4 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.

+\input{admin.tex}
+
\chapter{General Initialization And Device Operation}\label{sec:General
Initialization And Device Operation}

We start with an overview of device initialization, then expand on the
--
MST

Quoted 22 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

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

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

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

Note: one can argue both ways whether the new device group handling functionality (this and following patches) is closer to a new device type or a new transport type.

However, I expect that we will add more features in the near future. To facilitate this as much as possible of the text is located in the new admin chapter.

I did my best to minimize transport-specific text.

Quoted 8 lines

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 51 insertions(+)
 create mode 100644 admin.tex

You missed addressing the comments of [1].
Can you please fix them?

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

Quoted 64 lines

admin.tex new file +27 -0

new file mode 100644
index 0000000..3dc47be
@@ -0,0 +1,49 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device /
+Device groups}
+
+It is occasionally useful to have a device control a group of other
+devices. Terminology used in such cases:
+
+\begin{description}
+\item[Device group]
+        or just group, includes zero or more devices.
+\item[Owner device]
+        or owner, the device controlling the group.
+\item[Member device]
+        a device within a group. The owner device itself is not
+	a member of the group.
+\item[Member identifier]
+        each member has this identifier, unique within the group
+	and used to address it through the owner device.
+\item[Group type identifier]
+	specifies what kind of member devices there are in a
+	group, how is the member identifier is interpreted
+	and what kind of control the owner has.
+	A given owner can control a single group of a given type,
+	thus the type and the owner together identify the group.
+	\footnote{Even though some group types only support
+			specific transports, group type identifiers
+			are global rather than transport-specific -
+			we don't expect a flood of new group types.}

\end{description}

admin.tex new file +22 -0

+
+The following group types, and their identifiers, are currently specified):
+\begin{description}
+\item[SR-IOV group type (0x1)]
+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as the owner and includes all
+its SR-IOV virtual functions (VFs) as members (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+The PF device itself is not a member of the group.
+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value from 0x1 to 0xFFFF
+and equals the SR-IOV VF number of the member device (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+Both owner and member devices for this group type use the Virtio PCI
+transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
+\end{description}
+
+

content.tex +0 -0

index 0c2d917..ffe45c4 100644
@@ -491,6 +491,8 @@ \section{Exporting Objects}\label{sec:Basic Facilities of

a Virtio Device / Expo types. It is RECOMMENDED that devices generate version 4 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.

Quoted 8 lines

+\input{admin.tex}
+
\chapter{General Initialization And Device Operation}\label{sec:General
Initialization And Device Operation}

We start with an overview of device initialization, then expand on the
--
MST

+The following group types, and their identifiers, are currently specified):

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

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

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

Note: one can argue both ways whether the new device group handling functionality (this and following patches) is closer
to a new device type or a new transport type.

However, I expect that we will add more features in the near future. To facilitate this as much as possible of the text is located in the new admin chapter.

I did my best to minimize transport-specific text.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex   | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 content.tex |  2 ++
 2 files changed, 51 insertions(+)
 create mode 100644 admin.tex

admin.tex new file +9 -0

new file mode 100644
index 0000000..3dc47be
@@ -0,0 +1,49 @@
+\section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device groups}
+
+It is occasionally useful to have a device control a group of
+other devices. Terminology used in such cases:
+
+\begin{description}
+\item[Device group]
+        or just group, includes zero or more devices.
+\item[Owner device]

admin.tex new file +20 -0

+\begin{description}
+\item[SR-IOV group type (0x1)]
+This device group has a PCI Single Root I/O Virtualization
+(SR-IOV) physical function (PF) device as the owner and includes
+all its SR-IOV virtual functions (VFs) as members (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+The PF device itself is not a member of the group.
+
+The group type identifier for this group is 0x1.
+
+A member identifier for this group can have a value from 0x1 to 0xFFFF
+and equals the SR-IOV VF number of the member device (see
+\hyperref[intro:PCIe]{[PCIe]}).
+
+Both owner and member devices for this group type use the Virtio
+PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
+\end{description}
+
+

content.tex +2 -0

index 0c2d917..ffe45c4 100644
@@ -491,6 +491,8 @@ \section{Exporting Objects}\label{sec:Basic Facilities of a Virtio Device / Expo
 types. It is RECOMMENDED that devices generate version 4
 UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
 
+\input{admin.tex}
+
 \chapter{General Initialization And Device Operation}\label{sec:General Initialization And Device Operation}
 
 We start with an overview of device initialization, then expand on the

Signature · 7 lines

MST


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

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00 & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND & command
error: no additional information \\
+\hline
+0x01 & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or
invalid \field{opcode} \\
+\hline
+0x02 & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or
invalid field within \field{command_specific_data} \\
+\hline
+0x03 & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or

invalid \field{group_type} \\
+\hline
+0x04 & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER & unsupported or
invalid \field{group_member_id} \\
+\hline
+0x05-0xFFFF & - & reserved for future use \\
+\hline
+\end{tabular}

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

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

admin.tex +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Quoted 10 lines · Parav Pandit

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

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

admin.tex +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Quoted 24 lines · Stefan Hajnoczi

Quoted 10 lines · Parav Pandit

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

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

admin.tex +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Quoted 4 lines

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to be seen.

Quoted 5 lines

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Really most drivers just want to propagate errno to userspace. All the detailed reporting is for sure well intentional but in the end it is at best printed into log - end to end people just end up with a switch statement converting these to errno codes.
So we are passing them from device and this way there will be some uniformity.

Quoted 47 lines · Michael S. Tsirkin

Quoted 24 lines · Stefan Hajnoczi

Quoted 10 lines · Parav Pandit

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

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

admin.tex +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Quoted 4 lines

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to be seen.

Quoted 5 lines

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Quoted 7 lines

Really most drivers just want to propagate errno to userspace. All the detailed reporting is for sure well intentional but in the end it is at best printed into log - end to end people just end up with a switch statement converting these to errno codes.
So we are passing them from device and this way there will be some uniformity.

Please clarify the rationale in the spec. I don't agree with it, as explained in my earlier email, but as long as its documented, people can try to follow it.

Stefan

From: Max Gurtovoy <[email protected]>
Sent: Monday, March 6, 2023 6:05 AM

Quoted 56 lines · Michael S. Tsirkin

Quoted 55 lines · Stefan Hajnoczi

Quoted 53 lines · Michael S. Tsirkin

Quoted 24 lines · Stefan Hajnoczi

Quoted 10 lines · Parav Pandit

Review

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

(quoted diff) +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK,
+\field{status_qialifier} is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following
+table describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Quoted 4 lines

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to be seen.

Quoted 5 lines

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Really most drivers just want to propagate errno to userspace. All the detailed reporting is for sure well intentional but in the end it is at best printed into log - end to end people just end up with a switch statement converting these to errno codes.
So we are passing them from device and this way there will be some uniformity.

Quoted 8 lines

Quoted 5 lines

Please clarify the rationale in the spec. I don't agree with it, as explained in my earlier email, but as long as its documented, people can try to follow it.

Stefan

It's 2:2 for now, you are with Parav, me and Cornelia like it :) Or I will try to document it better.

I don't understand this status_qualifier as well and it wasn't included in my original patch set.
I vote for "status" that describe generic status codes and "command_specific_error" that should be inspected by the driver only if "status" is set to "VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR". We discussed this so many times before (and already agreed IIRC) and adding this new qualifiers mechanism sounds not right to me and not intuitive for device and driver developers.

I suggest:

1. VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE
2. VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
3. VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP
4. VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER
5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

Quoted 61 lines · Michael S. Tsirkin

Quoted 60 lines · Stefan Hajnoczi

Quoted 47 lines · Michael S. Tsirkin

Quoted 24 lines · Stefan Hajnoczi

Quoted 10 lines · Parav Pandit

Review

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

(quoted diff) +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Quoted 4 lines

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to be seen.

Quoted 5 lines

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Quoted 7 lines

Really most drivers just want to propagate errno to userspace. All the detailed reporting is for sure well intentional but in the end it is at best printed into log - end to end people just end up with a switch statement converting these to errno codes.
So we are passing them from device and this way there will be some uniformity.

Please clarify the rationale in the spec. I don't agree with it, as explained in my earlier email, but as long as its documented, people can try to follow it.

Quoted 4 lines

Quoted 1 line

Stefan

It's 2:2 for now, you are with Parav, me and Cornelia like it :) Or I will try to document it better.

I don't understand this status_qualifier as well and it wasn't included in my original patch set.

I vote for "status" that describe generic status codes and "command_specific_error" that should be inspected by the driver only if "status" is set to "VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR". We discussed this so many times before (and already agreed IIRC) and adding this new qualifiers mechanism sounds not right to me and not intuitive for device and driver developers.

I suggest:

1. VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE
2. VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
3. VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP
4. VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER
5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

Quoted 3 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, March 7, 2023 7:34 PM
COMMAND_SPECIFIC_ERR is just way too much detail - commands generally just should not fail it's a quality of implementation issue.

Quoted 5 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, March 7, 2023 7:34 PM
COMMAND_SPECIFIC_ERR is just way too much detail - commands generally just should not fail it's a quality of implementation issue.

I disagree partially.

1. Hw device != sw_hypervisor device.
Device may fail or error out that may need want sw driver to retry. Like Stefan's example, it may need to return timeout/retry intermittently. Doesnt means the device is broken at that point.

2. A device implementation may not have imposed a certain locking scheme to synchronize VF enablement with VF provisioning. ENODEV can reflect two commands not synchronized.

So Boolean 0 = success, 22 = error is not the right way to craft the spec.
Many times, those sub-error codes are good indications of what may have gone wrong in the field. Useful for the quality issue you pointed out to debug.

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, March 7, 2023 8:06 PM

Quoted 6 lines · Parav Pandit

Quoted 5 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Tuesday, March 7, 2023 7:34 PM COMMAND_SPECIFIC_ERR is just way too much detail - commands generally just should not fail it's a quality of implementation issue.

Quoted 8 lines

I disagree partially.

1. Hw device != sw_hypervisor device.
Device may fail or error out that may need want sw driver to retry. Like Stefan's example, it may need to return timeout/retry intermittently. Doesnt means the device is broken at that point.

2. A device implementation may not have imposed a certain locking scheme

to synchronize VF enablement with VF provisioning.

Quoted 2 lines

ENODEV can reflect two commands not synchronized.

Quoted 2 lines

So Boolean 0 = success, 22 = error is not the right way to craft the spec. Many times, those sub-error codes are good indications of what may have

gone wrong in the field.

Quoted 1 line

Useful for the quality issue you pointed out to debug.

Maybe, but I think we can just leave this stuff for later. Too much hand-waving, when we add commands that actually need this kind of ability that is when we will add the relevant error codes.

Quoted 64 lines · Max Gurtovoy

Quoted 61 lines · Michael S. Tsirkin

Quoted 60 lines · Stefan Hajnoczi

Quoted 47 lines · Michael S. Tsirkin

Quoted 24 lines · Stefan Hajnoczi

Quoted 10 lines · Parav Pandit

Review

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

(quoted diff) +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Quoted 4 lines

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to be seen.

Quoted 5 lines

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Quoted 7 lines

Really most drivers just want to propagate errno to userspace. All the detailed reporting is for sure well intentional but in the end it is at best printed into log - end to end people just end up with a switch statement converting these to errno codes.
So we are passing them from device and this way there will be some uniformity.

Please clarify the rationale in the spec. I don't agree with it, as explained in my earlier email, but as long as its documented, people can try to follow it.

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Stefan

It's 2:2 for now, you are with Parav, me and Cornelia like it :) Or I will try to document it better.

I don't understand this status_qualifier as well and it wasn't included in my original patch set.

Sounds like you feel I should drop your S.O.B - is this the complaint? I wanted to give attribution since I started with that but sure, no problem.

Quoted 9 lines

I vote for "status" that describe generic status codes and "command_specific_error" that should be inspected by the driver only if "status" is set to "VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR". We discussed this so many times before (and already agreed IIRC) and adding this new qualifiers mechanism sounds not right to me and not intuitive for device and driver developers.

I suggest:

Quoted 6 lines

1. VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE
2. VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
3. VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP
4. VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER
5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

I don't think it's a good idea, we'll have to agree to disagree.

The point is simple. We can have a detailed virtio specific error. Nice for debugging most drivers won't know what to do with it. This is the status_qualifier.
Very detailed but
generally drivers will just have a giant switch statement translating it to a simple error code for userspace.

So to save everyone work, we also added "status"
a generic kind of error class that is easy to pass to userspace with a small switch statement.

COMMAND_SPECIFIC_ERR is just way too much detail - commands generally just should not fail it's a quality of implementation issue.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

I don't think it's a good idea, we'll have to agree to disagree.

Ok.
can you explain why isn't this a good idea please ?

Quoted 3 lines · Max Gurtovoy

Quoted 3 lines

Quoted 2 lines

5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

Quoted 4 lines

Quoted 1 line

I don't think it's a good idea, we'll have to agree to disagree.

Ok.
can you explain why isn't this a good idea please ?

Pointless complexity for spec, devices and drivers. In the end drivers don't really do anything with all this detailed info.

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Max Gurtovoy

Quoted 4 lines

Quoted 2 lines

5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

I don't think it's a good idea, we'll have to agree to disagree.

Ok.
can you explain why isn't this a good idea please ?

Quoted 3 lines

Pointless complexity for spec, devices and drivers. In the end drivers don't really do anything with all this detailed info.

Pointless according to your own opinion. As I mentioned, other specifications and devices work this way. I didn't invent it.

All drivers in SW world ? All drivers in Linux ? or Virtio drivers ?

Quoted 10 lines · Max Gurtovoy

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Max Gurtovoy

Quoted 4 lines

Quoted 2 lines

5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

I don't think it's a good idea, we'll have to agree to disagree.

Ok.
can you explain why isn't this a good idea please ?

Quoted 8 lines

Quoted 3 lines

Pointless complexity for spec, devices and drivers. In the end drivers don't really do anything with all this detailed info.

Pointless according to your own opinion. As I mentioned, other specifications and devices work this way. I didn't invent it.

All drivers in SW world ? All drivers in Linux ? or Virtio drivers ?

FWIW, I've spent enough time translating detailed-but-not-really-that-useful error status codes to -EINVAL for s390-specific things...

The only thing that might make sense to transmit to a consumer would be the difference between "this thing won't work" and "this thing might work if you retry later" (if the consumer is even in a place to retry.)

All drivers in SW world ? All drivers in Linux ? or Virtio drivers ?

Quoted 69 lines · Michael S. Tsirkin

Quoted 68 lines · Max Gurtovoy

Quoted 65 lines · Michael S. Tsirkin

Quoted 63 lines · Stefan Hajnoczi

Quoted 49 lines · Michael S. Tsirkin

Quoted 26 lines · Stefan Hajnoczi

Quoted 11 lines · Parav Pandit

Review

Quoted 8 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

+When \field{status} is VIRTIO_ADMIN_STATUS_OK,
\field{status_qialifier}

(quoted diff) +2 -0

+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 7 lines

Quoted 3 lines

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

Please define a unique constant for each error condition that can occur instead of sharing catch-all errno constants between multiple error conditions. If a driver wants to squash them together into an errno, that's fine, but I think doing this at the hardware interface level is just propagating the mistakes of errnos.

Quoted 4 lines

Only status_qualifier is needed and the vague status field can be dropped. It's not clear to me why adding EAGAIN, ENOMEM, EBUSY, and ENODEV is useful. They have no meaning to the driver, only the status_qualifier really indicates what is going on.

At a high level at the moment we have only two cases:
- ok
- invalid input supplied by driver

maybe we will have more reasons for a failure - remains to be seen.

Quoted 5 lines

I'm sure you guys have discussed this previously, but please provide rationale in the spec because it looks weird to someone with fresh eyes.

Stefan

Quoted 7 lines

Really most drivers just want to propagate errno to userspace. All the detailed reporting is for sure well intentional but in the end it is at best printed into log - end to end people just end up with a switch statement converting these to errno codes.
So we are passing them from device and this way there will be some uniformity.

Please clarify the rationale in the spec. I don't agree with it, as explained in my earlier email, but as long as its documented, people can try to follow it.

Stefan

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

It's 2:2 for now, you are with Parav, me and Cornelia like it :) Or I will try to document it better.

I don't understand this status_qualifier as well and it wasn't included in my original patch set.

Sounds like you feel I should drop your S.O.B - is this the complaint? I wanted to give attribution since I started with that but sure, no problem.

Quoted 10 lines

Quoted 10 lines

I vote for "status" that describe generic status codes and "command_specific_error" that should be inspected by the driver only if "status" is set to "VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR". We discussed this so many times before (and already agreed IIRC) and adding this new qualifiers mechanism sounds not right to me and not intuitive for device and driver developers.

I suggest:

Quoted 8 lines

Quoted 6 lines

1. VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE
2. VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD
3. VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP
4. VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER
5. VIRTIO_ADMIN_STATUS_COMMAND_SPECIFIC_ERR (for more info read the command_specific_error field).

I don't think it's a good idea, we'll have to agree to disagree.

Ok.
can you explain why isn't this a good idea please ?
remember that this is very common logic in many specifications and many HW device implementation.

Quoted 8 lines

The point is simple. We can have a detailed virtio specific error. Nice for debugging most drivers won't know what to do with it. This is the status_qualifier.
Very detailed but
generally drivers will just have a giant switch statement translating it to a simple error code for userspace.

how giant is this switch statement ?
NVMe handles fine with this logic. Maybe you can take a look on that to get some more ideas of implementing this kind of things.

Why do we need to add some qualifiers logic into our devices ?

Quoted 3 lines

So to save everyone work, we also added "status"
a generic kind of error class that is easy to pass to userspace with a small switch statement.

if you want something that is easy just pass OK = 0 and not_OK = 1 or != 0. Not need for qualifiers.
In you case the qualifiers is just an extension for the status.

Quoted 3 lines

COMMAND_SPECIFIC_ERR is just way too much detail - commands generally just should not fail it's a quality of implementation issue.

it's the way many specifications work.
And this is the correct way for future extensions IMO. Each new command may have it's own namespace of errors extensions.

why we defer such essential decisions for the future ?
We already introduces 2-3 admin features in the past (MSIX, feature bits config, num_queues config, etc..) - maybe we can learn from it ?

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

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: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
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/

please use "Co-developed-by:" for this patch

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

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

admin.tex +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

And with that, VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER moves under ENODEV error code.

Quoted 14 lines

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00 & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND & command
error: no additional information \\
+\hline
+0x01 & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or
invalid \field{opcode} \\
+\hline
+0x02 & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or
invalid field within \field{command_specific_data} \\
+\hline
+0x03 & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or

Quoted 8 lines

invalid \field{group_type} \\
+\hline
+0x04 & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER & unsupported or
invalid \field{group_member_id} \\
+\hline
+0x05-0xFFFF & - & reserved for future use \\
+\hline
+\end{tabular}

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 6:47 PM

Quoted 2 lines

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

We'll add them when there's a use-case.

Quoted 2 lines

And with that, VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER moves under

ENODEV error code.

No I think that belongs in in EINVAL - command is malformed.

ENODEV is more "The wrong type of device was given to a function that expects a particular sort of device."

Quoted 10 lines · Parav Pandit

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

Quoted 7 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

admin.tex +3 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}
+is reserved and set to zero by the device.
+

s/status_qialifier/status_qualifier
Missed from v10 of Feb.

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL, the following table
+describes possible \field{status_qialifier} values:

s/status_qialifier/status_qualifier

Can you please add other useful error codes in addition to the EINVAL? Few that we are needed EAGAIN, ENOMEM, EBUSY, ENODEV.

We'll add them when there's a use-case.

Quoted 2 lines

And with that, VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER moves under ENODEV error code.

No I think that belongs in in EINVAL - command is malformed. ENODEV is more "The wrong type of device was given to a function that expects a particular sort of device."

Quoted 22 lines

Quoted 22 lines

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00 & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND & command
error: no additional information \\
+\hline
+0x01 & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE & unsupported or
invalid \field{opcode} \\
+\hline
+0x02 & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD & unsupported or
invalid field within \field{command_specific_data} \\
+\hline
+0x03 & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP & unsupported or
invalid \field{group_type} \\
+\hline
+0x04 & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER & unsupported or
invalid \field{group_member_id} \\
+\hline
+0x05-0xFFFF & - & reserved for future use \\
+\hline
+\end{tabular}

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644

admin.tex +45 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:

admin.tex +14 -0

+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\

admin.tex +13 -0

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER    & unsupported or invalid \field{group_member_id} \\

Quoted 21 lines · Michael S. Tsirkin

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 

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

Quoted 3 lines

admin.tex +2 -0

+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of

I notice that the terminology is simply "the driver". "Owner driver" and "group member driver" might be clearer because there will be two (possibly different) drivers involved.

Quoted 51 lines

admin.tex +51 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\

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

Quoted 4 lines

admin.tex +4 -0

+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}

I thought all commands are "group administration commands" but this table makes it look like they are just a subset (0x0000 - 0x7FFF) of group administration commands, which is a paradox.

Quoted 14 lines

admin.tex +14 -0

+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\

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

Quoted 10 lines

admin.tex +10 -0

+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}

s/qialifier/qualifier/

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

Quoted 4 lines

admin.tex +4 -0

+is reserved and set to zero by the device.
+
+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:

s/qialifier/qualifier/

Quoted 13 lines

admin.tex +13 -0

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER    & unsupported or invalid \field{group_member_id} \\

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

Quoted 10 lines

admin.tex +10 -0

+\hline
+0x05-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}
+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

On more thing:

Does the owner device see commands in order but may complete them in any order?

I think this information might be useful just to make it clear that driver authors shouldn't make assumptions about ordering and completion order, e.g. pipelining a bunch of dependent commands doesn't work because the first command is not necessarily completed before the second command is started.

Quoted 22 lines · Stefan Hajnoczi

Quoted 21 lines · Michael S. Tsirkin

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 

Quoted 7 lines

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

Quoted 3 lines

admin.tex +2 -0

+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of

I notice that the terminology is simply "the driver". "Owner driver" and "group member driver" might be clearer because there will be two (possibly different) drivers involved.

Hmm I don't really want to repeat owner everywhere.
I will clarify that in this section simple "driver" and "device" are owner, "member device" and "member driver" is always called explicitly.

Quoted 51 lines

Quoted 51 lines

admin.tex +51 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\

Quoted 8 lines

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

Quoted 4 lines

admin.tex +4 -0

+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}

I thought all commands are "group administration commands" but this table makes it look like they are just a subset (0x0000 - 0x7FFF) of group administration commands, which is a paradox.

Well the rest are reserved, maybe we will have more command types who knows. No?

Quoted 60 lines

Quoted 14 lines

admin.tex +14 -0

+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\

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

Quoted 10 lines

admin.tex +10 -0

+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}

s/qialifier/qualifier/

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

Quoted 4 lines

admin.tex +4 -0

+is reserved and set to zero by the device.
+
+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:

s/qialifier/qualifier/

Quoted 13 lines

admin.tex +13 -0

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER    & unsupported or invalid \field{group_member_id} \\

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

Quoted 10 lines

admin.tex +10 -0

+\hline
+0x05-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}
+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

On more thing:

Quoted 8 lines

Does the owner device see commands in order but may complete them in any order?

I think this information might be useful just to make it clear that driver authors shouldn't make assumptions about ordering and completion order, e.g. pipelining a bunch of dependent commands doesn't work because the first command is not necessarily completed before the second command is started.

I think this is discussed when we discuss sending commands through vq. in this section it's just about commands generally whatever the way to send them to device. no?

Quoted 31 lines · Michael S. Tsirkin

Quoted 22 lines · Stefan Hajnoczi

Quoted 21 lines · Michael S. Tsirkin

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 

Quoted 7 lines

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

Quoted 3 lines

admin.tex +2 -0

+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of

I notice that the terminology is simply "the driver". "Owner driver" and "group member driver" might be clearer because there will be two (possibly different) drivers involved.

Quoted 3 lines

Hmm I don't really want to repeat owner everywhere.
I will clarify that in this section simple "driver" and "device" are owner, "member device" and "member driver" is always called explicitly.

Sounds good.

Quoted 60 lines

Quoted 59 lines

Quoted 45 lines

admin.tex +45 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:

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

Quoted 10 lines

admin.tex +10 -0

+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}

I thought all commands are "group administration commands" but this table makes it look like they are just a subset (0x0000 - 0x7FFF) of group administration commands, which is a paradox.

Quoted 2 lines

Well the rest are reserved, maybe we will have more command types who knows. No?

I see. Does that mean the reserved commands don't need to be in the same format as struct virtio_admin_cmd?

The entire section is called "Group administration commands" but I get the impression it's talking both about admin virtqueue commands in general and specifically about group administration commands.

Is it possible to structure this as follows:

Admin Commands

    ...common stuff...
    Group Administration Commands (0x0000-0x7fff)
      ...specific to group administration commands...
    Reserved (0x8000-0xffff)

Quoted 4 lines

Quoted 4 lines

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

Quoted 4 lines

admin.tex +4 -0

+\hline \hline
+0x0000 - 0x7FFF & - &  commands using struct virtio_admin_cmd    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\

Quoted 69 lines

Quoted 68 lines

Quoted 14 lines

admin.tex +14 -0

+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\

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

Quoted 10 lines

admin.tex +10 -0

+\hline \hline
+00   & VIRTIO_ADMIN_STATUS_OK    & successful completion  \\
+\hline
+22   & VIRTIO_ADMIN_STATUS_EINVAL    & invalid command \\
+\hline
+other   & -    & group administration command error  \\
+\hline
+\end{tabular}
+
+When \field{status} is VIRTIO_ADMIN_STATUS_OK, \field{status_qialifier}

s/qialifier/qualifier/

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

Quoted 4 lines

admin.tex +4 -0

+is reserved and set to zero by the device.
+
+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:

s/qialifier/qualifier/

Quoted 13 lines

admin.tex +13 -0

+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER    & unsupported or invalid \field{group_member_id} \\

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

Quoted 10 lines

admin.tex +10 -0

+\hline
+0x05-0xFFFF   & -    & reserved for future use \\
+\hline
+\end{tabular}
+
+Each command uses a different \field{command_specific_data} and
+\field{command_specific_result} structures and the length of
+\field{command_specific_data} and \field{command_specific_result}
+depends on these structures and is described separately or is
+implicit in the structure description.

On more thing:

Does the owner device see commands in order but may complete them in any order?

I think this information might be useful just to make it clear that driver authors shouldn't make assumptions about ordering and completion order, e.g. pipelining a bunch of dependent commands doesn't work because the first command is not necessarily completed before the second command is started.

Quoted 3 lines

I think this is discussed when we discuss sending commands through vq. in this section it's just about commands generally whatever the way to send them to device. no?

Okay. I'm reading this patch series linearly and the question popped into my mind here. Glad it's already covered elsewhere.

Stefan

Quoted 98 lines · Stefan Hajnoczi

Quoted 31 lines · Michael S. Tsirkin

Quoted 22 lines · Stefan Hajnoczi

Quoted 21 lines · Michael S. Tsirkin

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 

Quoted 7 lines

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

Quoted 3 lines

admin.tex +2 -0

+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of

I notice that the terminology is simply "the driver". "Owner driver" and "group member driver" might be clearer because there will be two (possibly different) drivers involved.

Quoted 3 lines

Hmm I don't really want to repeat owner everywhere.
I will clarify that in this section simple "driver" and "device" are owner, "member device" and "member driver" is always called explicitly.

Sounds good.

Quoted 60 lines

Quoted 59 lines

Quoted 45 lines

admin.tex +45 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:

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

Quoted 10 lines

admin.tex +10 -0

+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}

I thought all commands are "group administration commands" but this table makes it look like they are just a subset (0x0000 - 0x7FFF) of group administration commands, which is a paradox.

Quoted 5 lines

Quoted 2 lines

Well the rest are reserved, maybe we will have more command types who knows. No?

I see. Does that mean the reserved commands don't need to be in the same format as struct virtio_admin_cmd?

I am not good in predicting future ...

Quoted 11 lines

The entire section is called "Group administration commands" but I get the impression it's talking both about admin virtqueue commands in general and specifically about group administration commands.

Is it possible to structure this as follows:

Admin Commands

    ...common stuff...
    Group Administration Commands (0x0000-0x7fff)
      ...specific to group administration commands...
    Reserved (0x8000-0xffff)

What if I just write it like this:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

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

Quoted 4 lines

admin.tex +4 -0

+\hline \hline
+0x0000 - 0x7FFF & - &  commands using struct virtio_admin_cmd    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\

will this address the comment?

Quoted 128 lines · Michael S. Tsirkin

Quoted 98 lines · Stefan Hajnoczi

Quoted 31 lines · Michael S. Tsirkin

Quoted 22 lines · Stefan Hajnoczi

Quoted 21 lines · Michael S. Tsirkin

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 

Quoted 7 lines

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

Quoted 3 lines

admin.tex +2 -0

+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of

I notice that the terminology is simply "the driver". "Owner driver" and "group member driver" might be clearer because there will be two (possibly different) drivers involved.

Quoted 3 lines

Hmm I don't really want to repeat owner everywhere.
I will clarify that in this section simple "driver" and "device" are owner, "member device" and "member driver" is always called explicitly.

Sounds good.

Quoted 60 lines

Quoted 59 lines

Quoted 45 lines

admin.tex +45 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:

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

Quoted 10 lines

admin.tex +10 -0

+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}

I thought all commands are "group administration commands" but this table makes it look like they are just a subset (0x0000 - 0x7FFF) of group administration commands, which is a paradox.

Quoted 5 lines

Quoted 2 lines

Well the rest are reserved, maybe we will have more command types who knows. No?

I see. Does that mean the reserved commands don't need to be in the same format as struct virtio_admin_cmd?

I am not good in predicting future ...

Quoted 11 lines

The entire section is called "Group administration commands" but I get the impression it's talking both about admin virtqueue commands in general and specifically about group administration commands.

Is it possible to structure this as follows:

Admin Commands

    ...common stuff...
    Group Administration Commands (0x0000-0x7fff)
      ...specific to group administration commands...
    Reserved (0x8000-0xffff)

What if I just write it like this:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

admin.tex +4 -0

+\hline \hline
+0x0000 - 0x7FFF & - &  commands using struct virtio_admin_cmd    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\

Quoted 1 line

will this address the comment?

Yes, thanks.

Stefan

Quoted 129 lines · Fri, Mar 03, 2023 at 09:23:14PM CET, [email protected] wrote:

Quoted 128 lines · Michael S. Tsirkin

Quoted 98 lines · Stefan Hajnoczi

Quoted 31 lines · Michael S. Tsirkin

Quoted 22 lines · Stefan Hajnoczi

Quoted 21 lines · Michael S. Tsirkin

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +0 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 

Quoted 7 lines

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

Quoted 3 lines

admin.tex +2 -0

+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of

I notice that the terminology is simply "the driver". "Owner driver" and "group member driver" might be clearer because there will be two (possibly different) drivers involved.

Quoted 3 lines

Hmm I don't really want to repeat owner everywhere.
I will clarify that in this section simple "driver" and "device" are owner, "member device" and "member driver" is always called explicitly.

Sounds good.

Quoted 60 lines

Quoted 59 lines

Quoted 45 lines

admin.tex +45 -0

+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}
+
+All the group administration commands are of the following form:
+
+\begin{lstlisting}
+struct virtio_admin_cmd {
+        /* Device-readable part */
+        le16 opcode;
+        /*
+         * 1 - SR-IOV
+         * 2 - 65535 reserved
+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:

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

Quoted 10 lines

admin.tex +10 -0

+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}

I thought all commands are "group administration commands" but this table makes it look like they are just a subset (0x0000 - 0x7FFF) of group administration commands, which is a paradox.

Quoted 5 lines

Quoted 2 lines

Well the rest are reserved, maybe we will have more command types who knows. No?

I see. Does that mean the reserved commands don't need to be in the same format as struct virtio_admin_cmd?

I am not good in predicting future ...

Quoted 11 lines

The entire section is called "Group administration commands" but I get the impression it's talking both about admin virtqueue commands in general and specifically about group administration commands.

Is it possible to structure this as follows:

Admin Commands

    ...common stuff...
    Group Administration Commands (0x0000-0x7fff)
      ...specific to group administration commands...
    Reserved (0x8000-0xffff)

What if I just write it like this:

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

admin.tex +4 -0

+\hline \hline
+0x0000 - 0x7FFF & - &  commands using struct virtio_admin_cmd    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\

Quoted 3 lines

Quoted 1 line

will this address the comment?

Yes, thanks.

What is the reason for having this table? I fail to see it.

Quoted 3 lines

Stefan

This introduces a general structure for group administration commands, used to control device groups through their owner.

Following patches will introduce specific commands and an interface for submitting these commands to the owner.

Signed-off-by Max Gurtovoy <[email protected]>

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

---
 admin.tex        | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 introduction.tex |   3 ++
 2 files changed, 111 insertions(+)

admin.tex +9 -0

index 3dc47be..7e28b77 100644
@@ -46,4 +46,112 @@ \section{Device groups}\label{sec:Basic Facilities of a Virtio Device / Device g
 PCI transport (see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus}).
 \end{description}
 
+\subsection{Group administration commands}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
+The driver sends group administration commands to the owner device of
+a group to control member devices of the group.
+This mechanism can
+be used, for example, to configure a member device before it is
+initialized by its driver.
+\footnote{The term "administration" is intended to be interpreted
+widely to include any kind of control. See specific commands
+for detail.}

admin.tex +52 -0

+         */
+        le16 group_type;
+        /* unused, reserved for future extensions */
+        u8 reserved1[12];
+        le64 group_member_id;
+        u8 command_specific_data[];
+
+        /* Device-writable part */
+        le16 status;
+        le16 status_qualifier;
+        /* unused, reserved for future extensions */
+        u8 reserved2[4];
+        u8 command_specific_result[];
+};
+\end{lstlisting}
+
+For all commands, \field{opcode}, \field{group_type} and if
+necessary \field{group_member_id} and \field{command_specific_data} are
+set by the driver, and the owner device sets \field{status} and if
+needed \field{status_qualifier} and
+\field{command_specific_result}.
+
+Generally, any unused device-readable fields are set to zero by the driver
+and ignored by the device.  Any unused device-writeable fields are set to zero
+by the device and ignored by the driver.
+
+\field{opcode} specifies the command. The valid
+values for \field{opcode} can be found in the following table:
+
+\begin{tabular}{|l|l|}
+\hline
+opcode & Name & Command Description \\
+\hline \hline
+0x0000 - 0x7FFF & - &  Group administration commands    \\
+\hline
+0x8000 - 0xFFFF & - & Reserved    \\
+\hline
+\end{tabular}
+
+The \field{group_type} specifies the group type identifier.
+The \field{group_member_id} specifies the member identifier within the group.
+See section \ref{sec:Introduction / Terminology / Device group}
+for the definition of the group type identifier and group member
+identifier.
+
+The following table describes possible \field{status} values;
+to simplify common implementations, they are intentionally
+matching common \hyperref[intro:errno]{Linux error names and numbers}:
+
+\begin{tabular}{|l|l|l|}
+\hline
+Status (decimal) & Name & Description \\

admin.tex +17 -0

+is reserved and set to zero by the device.
+
+When \field{status} is VIRTIO_ADMIN_STATUS_EINVAL,
+the following table describes possible \field{status_qialifier} values:
+\begin{tabular}{|l|l|l|}
+\hline
+Status & Name & Description \\
+\hline \hline
+0x00   & VIRTIO_ADMIN_STATUS_Q_INVALID_COMMAND   & command error: no additional information  \\
+\hline
+0x01   & VIRTIO_ADMIN_STATUS_Q_INVALID_OPCODE    & unsupported or invalid \field{opcode}  \\
+\hline
+0x02   & VIRTIO_ADMIN_STATUS_Q_INVALID_FIELD    & unsupported or invalid field within \field{command_specific_data}  \\
+\hline
+0x03   & VIRTIO_ADMIN_STATUS_Q_INVALID_GROUP    & unsupported or invalid \field{group_type} \\
+\hline
+0x04   & VIRTIO_ADMIN_STATUS_Q_INVALID_MEMBER    & unsupported or invalid \field{group_member_id} \\

introduction.tex +3 -0

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

Signature · 7 lines

MST


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

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

+>>>>>>> 0edc690... admin: introduce virtio admin virtqueues

Quoted 5 lines

From: Michael S. Tsirkin <[email protected]>
Sent: Thursday, March 2, 2023 8:05 AM

+>>>>>>> 0edc690... admin: introduce virtio admin virtqueues

I requested to fix this in v10 of Feb, it was in some other patch. This hunk shifted here.

Review

Quoted 3 lines

(quoted diff) +3 -0

+  \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device
+ exposes one or more  administration virtqueues.
+

Please remove extra white space between more and administration.

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

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

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

Signed-off-by Max Gurtovoy <[email protected]>

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

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

admin.tex +8 -0

index 7e28b77..3ffac2e 100644
@@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{command_specific_data} and \field{command_specific_result}
 depends on these structures and is described separately or is
 implicit in the structure description.
+
+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
+
+An administration virtqueue of an owner device is used to submit
+group administration commands. An owner device can have more
+than one administration virtqueue.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one

admin.tex +46 -0

+
+Administration virtqueues are used as follows:
+\begin{itemize}
+\item The driver submits the command using the \field{struct virtio_admin_cmd}
+structure using a buffer consisting of two parts: a device-readable one followed by a
+device-writable one.
+\item the device-readable part includes fields from \field{opcode}
+through \field{command_specific_data}.
+\item the device-writeable buffer includes fields from \field{status}
+through \field{command_specific_result} inclusive.
+\end{itemize}
+
+For each command, this specification describes a distinct
+format structure used for \field{command_specific_data} and
+\field{command_specific_result}, the length of these fields
+depends on the command.
+
+However, to ensure forward compatibility
+\begin{itemize}
+\item drivers are allowed to submit buffers that are longer
+than what the device expects
+(that is, longer than the length of
+\field{opcode} through \field{command_specific_data}).
+This allows the driver to maintain
+a single format structure even if some structure fields are
+unused by the device.
+\item drivers are allowed to submit buffers that are shorter
+than what the device expects
+(that is, shorter than the length of \field{status} through
+\field{command_specific_result}). This allows the device to maintain
+a single format structure even if some structure fields are
+unused by the driver.
+\end{itemize}
+
+The device compares the length of each part (device-readable and
+device-writeable) of the buffer as submitted by driver to what it
+expects and then silently truncates the structures to either the
+length submitted by the driver, or the length described in this
+specification, whichever is shorter.  The device silently ignores
+any data falling outside the shorter of the two lengths. Any
+missing fields are interpreted as set to zero.
+
+Similarly, the driver compares the used buffer length
+of the buffer to what it expects and then silently
+truncates the structure to the used buffer length.
+The driver silently ignores any data falling outside

content.tex +5 -2

index ffe45c4..c8647c9 100644
@@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 \begin{description}
 \item[0 to 23, and 50 to 127] Feature bits for the specific device type
 
-\item[24 to 40] Feature bits reserved for extensions to the queue and
+\item[24 to 41] Feature bits reserved for extensions to the queue and
   feature negotiation mechanisms
 
-\item[41 to 49, and 128 and above] Feature bits reserved for future extensions.
+\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}

@@ -7682,6 +7682,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   that the driver can reset a queue individually.
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
+  \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
+  administration virtqueues.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

Signature · 1 line

MST

Quoted 9 lines · Michael S. Tsirkin

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

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

Is this referring to the existing virtio-net, virtio-scsi, etc control virtqueues?

I see the admin virtqueue as the virtqueue equivalent to transport feature bits. The admin queue does nothing device type-specific (net, scsi, etc) and instead focusses on transport and core virtio tasks.

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

Quoted 34 lines

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

Signed-off-by Max Gurtovoy <[email protected]>

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

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

admin.tex +16 -0

index 7e28b77..3ffac2e 100644
@@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{command_specific_data} and \field{command_specific_result}
 depends on these structures and is described separately or is
 implicit in the structure description.
+
+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
+
+An administration virtqueue of an owner device is used to submit
+group administration commands. An owner device can have more
+than one administration virtqueue.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
+or more adminstration virtqueues. The number and locations of the
+administration virtqueues are exposed by the owner device in a transport
+specific manner.
+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 175

Quoted 2 lines

admin.tex +2 -0

+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Does "they are processed by the device in the order in which they are queued" mean only 1 admin command can be running at any given time and therefore they will complete in order? This would allow pipelining dependent commands but prevent long-running commands because the virtqueue is blocked while executing a command.

Quoted 50 lines

admin.tex +50 -0

+
+Administration virtqueues are used as follows:
+\begin{itemize}
+\item The driver submits the command using the \field{struct virtio_admin_cmd}
+structure using a buffer consisting of two parts: a device-readable one followed by a
+device-writable one.
+\item the device-readable part includes fields from \field{opcode}
+through \field{command_specific_data}.
+\item the device-writeable buffer includes fields from \field{status}
+through \field{command_specific_result} inclusive.
+\end{itemize}
+
+For each command, this specification describes a distinct
+format structure used for \field{command_specific_data} and
+\field{command_specific_result}, the length of these fields
+depends on the command.
+
+However, to ensure forward compatibility
+\begin{itemize}
+\item drivers are allowed to submit buffers that are longer
+than what the device expects
+(that is, longer than the length of
+\field{opcode} through \field{command_specific_data}).
+This allows the driver to maintain
+a single format structure even if some structure fields are
+unused by the device.
+\item drivers are allowed to submit buffers that are shorter
+than what the device expects
+(that is, shorter than the length of \field{status} through
+\field{command_specific_result}). This allows the device to maintain
+a single format structure even if some structure fields are
+unused by the driver.
+\end{itemize}
+
+The device compares the length of each part (device-readable and
+device-writeable) of the buffer as submitted by driver to what it
+expects and then silently truncates the structures to either the
+length submitted by the driver, or the length described in this
+specification, whichever is shorter.  The device silently ignores
+any data falling outside the shorter of the two lengths. Any
+missing fields are interpreted as set to zero.
+
+Similarly, the driver compares the used buffer length
+of the buffer to what it expects and then silently
+truncates the structure to the used buffer length.
+The driver silently ignores any data falling outside
+the used buffer length reported by the device.  Any missing
+fields are interpreted as set to zero.
+
+This simplifies driver and device implementations since the

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 231

Quoted 6 lines

admin.tex +6 -0

+driver/device can simply maintain a single large structure (such
+as a C structure) for a command and its result. As new versions
+of the specification are designed, new fields can be added to the
+tail of a structure, with the driver/device using the full
+structure without concern for versioning.
+>>>>>>> 0edc690... admin: introduce virtio admin virtqueues

A merge conflict line crept into the patch?

Quoted 26 lines

content.tex +5 -2

index ffe45c4..c8647c9 100644
@@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 \begin{description}
 \item[0 to 23, and 50 to 127] Feature bits for the specific device type
 
-\item[24 to 40] Feature bits reserved for extensions to the queue and
+\item[24 to 41] Feature bits reserved for extensions to the queue and
   feature negotiation mechanisms
 
-\item[41 to 49, and 128 and above] Feature bits reserved for future extensions.
+\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}

@@ -7682,6 +7682,9 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   that the driver can reset a queue individually.
   See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
 
+  \item[VIRTIO_F_ADMIN_VQ(41)] This feature indicates that the device exposes one or more
+  administration virtqueues.
+
 \end{description}
 

Quoted 4 lines

content.tex +0 -0

 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}

Signature · 1 line

MST

Quoted 14 lines · Stefan Hajnoczi

Quoted 9 lines · Michael S. Tsirkin

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

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

Is this referring to the existing virtio-net, virtio-scsi, etc control virtqueues?

Quoted 8 lines

I see the admin virtqueue as the virtqueue equivalent to transport feature bits. The admin queue does nothing device type-specific (net, scsi, etc) and instead focusses on transport and core virtio tasks.

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

yes this commit log is outdated. referred to original proposal by Max which also planned to replace cvq. will update.

Quoted 34 lines

Quoted 34 lines

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

Signed-off-by Max Gurtovoy <[email protected]>

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

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

admin.tex +16 -0

index 7e28b77..3ffac2e 100644
@@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{command_specific_data} and \field{command_specific_result}
 depends on these structures and is described separately or is
 implicit in the structure description.
+
+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
+
+An administration virtqueue of an owner device is used to submit
+group administration commands. An owner device can have more
+than one administration virtqueue.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
+or more adminstration virtqueues. The number and locations of the
+administration virtqueues are exposed by the owner device in a transport
+specific manner.
+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands

Quoted 8 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 175

Quoted 2 lines

admin.tex +2 -0

+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Does "they are processed by the device in the order in which they are queued" mean only 1 admin command can be running at any given time and therefore they will complete in order? This would allow pipelining dependent commands but prevent long-running commands because the virtqueue is blocked while executing a command.

we have multiple vqs for that.

Quoted 72 lines · Michael S. Tsirkin

Quoted 14 lines · Stefan Hajnoczi

Quoted 9 lines · Michael S. Tsirkin

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

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

Is this referring to the existing virtio-net, virtio-scsi, etc control virtqueues?

Quoted 8 lines

I see the admin virtqueue as the virtqueue equivalent to transport feature bits. The admin queue does nothing device type-specific (net, scsi, etc) and instead focusses on transport and core virtio tasks.

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

yes this commit log is outdated. referred to original proposal by Max which also planned to replace cvq. will update.

Quoted 34 lines

Quoted 34 lines

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

Signed-off-by Max Gurtovoy <[email protected]>

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

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

admin.tex +16 -0

index 7e28b77..3ffac2e 100644
@@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{command_specific_data} and \field{command_specific_result}
 depends on these structures and is described separately or is
 implicit in the structure description.
+
+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
+
+An administration virtqueue of an owner device is used to submit
+group administration commands. An owner device can have more
+than one administration virtqueue.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
+or more adminstration virtqueues. The number and locations of the
+administration virtqueues are exposed by the owner device in a transport
+specific manner.
+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands

Quoted 8 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 175

Quoted 2 lines

admin.tex +2 -0

+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Does "they are processed by the device in the order in which they are queued" mean only 1 admin command can be running at any given time and therefore they will complete in order? This would allow pipelining dependent commands but prevent long-running commands because the virtqueue is blocked while executing a command.

Quoted 1 line

we have multiple vqs for that.

This reminds me of the async challenges with QEMU's QMP monitor.

Will it ever be possible to abort an in-flight command? I guess the abort command would need to be submitted on another virtqueue, but how do you identify the in-flight command that you wish to cancel?

Please clarify the blocking semantics in the spec because it wasn't clear to me.

Thanks,
Stefan

Quoted 81 lines · Stefan Hajnoczi

Quoted 72 lines · Michael S. Tsirkin

Quoted 14 lines · Stefan Hajnoczi

Quoted 9 lines · Michael S. Tsirkin

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

Currently virtio specification defines control virtqueue to manipulate features and configuration of the device it operates on. However, control virtqueue commands are device type specific, which makes it very difficult to extend for device agnostic commands.

To support this requirement in a more generic way, this patch introduces a new admin virtqueue interface.

Is this referring to the existing virtio-net, virtio-scsi, etc control virtqueues?

Quoted 8 lines

I see the admin virtqueue as the virtqueue equivalent to transport feature bits. The admin queue does nothing device type-specific (net, scsi, etc) and instead focusses on transport and core virtio tasks.

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

yes this commit log is outdated. referred to original proposal by Max which also planned to replace cvq. will update.

Quoted 34 lines

Quoted 34 lines

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

Signed-off-by Max Gurtovoy <[email protected]>

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

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

admin.tex +16 -0

index 7e28b77..3ffac2e 100644
@@ -155,3 +155,77 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{command_specific_data} and \field{command_specific_result}
 depends on these structures and is described separately or is
 implicit in the structure description.
+
+\section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Device / Administration Virtqueues}
+
+An administration virtqueue of an owner device is used to submit
+group administration commands. An owner device can have more
+than one administration virtqueue.
+
+If VIRTIO_F_ADMIN_VQ has been negotiated, an owner device exposes one
+or more adminstration virtqueues. The number and locations of the
+administration virtqueues are exposed by the owner device in a transport
+specific manner.
+
+The driver submits commands by queueing them to an arbitrary
+administration virtqueue, and they are processed by the
+device in the order in which they are queued. It is the
+responsibility of the driver to ensure ordering for commands

Quoted 8 lines

admin.tex \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti · line 175

Quoted 2 lines

admin.tex +2 -0

+placed on different administration virtqueues, because they will
+be executed with no order constraints.

Does "they are processed by the device in the order in which they are queued" mean only 1 admin command can be running at any given time and therefore they will complete in order? This would allow pipelining dependent commands but prevent long-running commands because the virtqueue is blocked while executing a command.

Quoted 1 line

we have multiple vqs for that.

This reminds me of the async challenges with QEMU's QMP monitor.

Will it ever be possible to abort an in-flight command? I guess the abort command would need to be submitted on another virtqueue, but how do you identify the in-flight command that you wish to cancel?

Quoted 5 lines

Please clarify the blocking semantics in the spec because it wasn't clear to me.

Thanks,
Stefan

I don't really get what does "blocking" mean. Nothing is required to block I think.
I guess for abort it is still in order but a different vq should not be needed.

Maybe we can use ID of buffer to cancel commands?

1. driver submits command as ID = 1
2. driver submits abort as ID 2
3. device sees the abort and cancels ID 2
4. device uses buffer 1
5. device uses buffer 3 - abort is complete

Device gets commands and processes them in order. It's just like scsi:
multiple queues, if you break it you get to keep both pieces.

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Quoted 1 line

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

They rely on multi-queue for abort operations:

In virtio-scsi the abort requests (VIRTIO_SCSI_T_TMF_ABORT_TASK) are sent on the control virtqueue. The the request identifier namespace is shared across all virtqueues so it's possible to abort a request that was submitted to any command virtqueue.

NVMe also follows the same design where abort commands are sent on the Admin Submission Queue instead of an I/O Submission Queue. It's possible to identify NVMe requests by <Submission Queue ID, Command Identifier>.

virtio-blk doesn't support aborting requests.

I think the logic behind this design is that if a queue gets stuck processing long-running requests, then the device should not be forced to perform lookahead in the queue to find abort commands. A separate control/admin queue is used for the abort requests.

Stefan

Quoted 17 lines · Michael S. Tsirkin

Quoted 15 lines · Stefan Hajnoczi

Quoted 11 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Quoted 2 lines

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Michael S. Tsirkin

Quoted 15 lines · Stefan Hajnoczi

Quoted 11 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Quoted 7 lines

Quoted 2 lines

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

I see. I think it's a good design to follow then.

I'll just copy
The driver queues requests to an arbitrary request queue, and they are used by the device on that same queue. It is the responsibility of the driver to ensure strict request ordering for commands placed on different queues, because they will be consumed with no order constraints.

replacing "request" with "admin".

Quoted 36 lines · Michael S. Tsirkin

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Michael S. Tsirkin

Quoted 15 lines · Stefan Hajnoczi

Quoted 11 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Quoted 7 lines

Quoted 2 lines

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

I see. I think it's a good design to follow then.

I'll just copy
The driver queues requests to an arbitrary request queue, and they are used by the device on that same queue. It is the responsibility of the driver to ensure strict request ordering for commands placed on different queues, because they will be consumed with no order constraints.

Quoted 1 line

replacing "request" with "admin".

That sounds like it's only about multi-queue because it says "... for commands placed on different queues". What I mentioned about a write followed by a read quest also applies within a single queue.

Can you clarify the semantics in the single queue case?

Stefan

Quoted 29 lines · Michael S. Tsirkin

Quoted 18 lines · Stefan Hajnoczi

Quoted 17 lines · Michael S. Tsirkin

Quoted 15 lines · Stefan Hajnoczi

Quoted 11 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Quoted 7 lines

Quoted 2 lines

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

I see. I think it's a good design to follow then.

Quoted 8 lines

I'll just copy
The driver queues requests to an arbitrary request queue, and they are used by the device on that same queue. It is the responsibility of the driver to ensure strict request ordering for commands placed on different queues, because they will be consumed with no order constraints.

replacing "request" with "admin".

That sounds like it's only about multi-queue because it says "... for commands placed on different queues". What I mentioned about a write followed by a read quest also applies within a single queue.

Can you clarify the semantics in the single queue case?

Stefan

Quoted 21 lines · Stefan Hajnoczi

Quoted 19 lines · Michael S. Tsirkin

Quoted 11 lines · Stefan Hajnoczi

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Quoted 4 lines

Quoted 1 line

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

Quoted 4 lines

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

I see. I think it's a good design to follow then.

I'll just copy
The driver queues requests to an arbitrary request queue, and they are used by the device on that same queue. It is the responsibility of the driver to ensure strict request ordering for commands placed on different queues, because they will be consumed with no order constraints.

replacing "request" with "admin".

Signature · 18 lines

MST


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

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

Subscribe: [email protected]
Unsubscribe: [email protected]
List help: [email protected]
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/

Quoted 22 lines · Mon, Mar 06, 2023 at 07:37:31PM CET, [email protected] wrote:

Quoted 21 lines · Stefan Hajnoczi

Quoted 19 lines · Michael S. Tsirkin

Quoted 11 lines · Stefan Hajnoczi

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Quoted 4 lines

Quoted 1 line

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

Quoted 6 lines

Quoted 4 lines

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

I see. I think it's a good design to follow then.

Hmm, is it suitable to have this approach for configuration interface? Storage device is a different beast, having parallel reads and writes makes complete sense for performance.

->read a req
->read b req
->read c req
<-read a rep
<-read b rep
<-read c rep

There is no dependency, even between writes.

But in case of configuration, does not make any sense to me. Why is it needed? To pass the burden of consistency of configuration to driver sounds odd at least.

I sense there is no concete idea about what the "admin virtqueue" should serve for exactly.

Quoted 41 lines · Jiri Pirko

Quoted 22 lines · Mon, Mar 06, 2023 at 07:37:31PM CET, [email protected] wrote:

Quoted 21 lines · Stefan Hajnoczi

Quoted 19 lines · Michael S. Tsirkin

Quoted 11 lines · Stefan Hajnoczi

Quoted 10 lines · Michael S. Tsirkin

Quoted 8 lines · Stefan Hajnoczi

What happens if a command takes 1 second to complete, is the device allowed to process the next command from the virtqueue during this time, possibly completing it before the first command?

This requires additional clarification in the spec because "they are processed by the device in the order in which they are queued" does not explain whether commands block the virtqueue (in order completion) or not (out of order completion).

Quoted 4 lines

Quoted 1 line

Oh I begin to see. Hmm how does e.g. virtio scsi handle this?

virtio-scsi, virtio-blk, and NVMe requests may complete out of order. Several may be processed by the device at the same time.

Let's say I submit a write followed by read - is read guaranteed to return an up to date info?

Quoted 6 lines

Quoted 4 lines

In general, no. The driver must wait for the write completion before submitting the read if it wants consistency.

Stefan

I see. I think it's a good design to follow then.

Hmm, is it suitable to have this approach for configuration interface? Storage device is a different beast, having parallel reads and writes makes complete sense for performance.

->read a req
->read b req
->read c req
<-read a rep
<-read b rep
<-read c rep

Quoted 8 lines

There is no dependency, even between writes.

But in case of configuration, does not make any sense to me. Why is it needed? To pass the burden of consistency of configuration to driver sounds odd at least.

I sense there is no concete idea about what the "admin virtqueue" should serve for exactly.

It's useful for long-running commands because they prevent other commands from executing.

An example I've given is that deleting a group member might require waiting for the group member's I/O activity to finish. If that I/O activity cannot be cancelled instantaneously, then it could take an unbounded amount of time to delete the group member. The device would be unable to process futher admin commands.

Group member creation might have similar issues if it involves acquiring remote resources (e.g. connecting to a Ceph cluster or allocating ports on a distributed network switch). It can be impossible to defer resource

acquisition/initialization because because VIRTIO devices must be available as soon as the driver can see them (i.e. how do populate Configuration Space fields if you don't have the details of the resource yet?).

So I have raised two questions:

1. What are the admin queue command completion semantics: in-order or out-of-order command completion?

2. Will there be long-running commands and how will we deal with them when they hang?

Stefan