All of lore.kernel.org
 help / color / mirror / Atom feed
* [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size
@ 2022-02-21 16:22 Christian Schoenebeck
  2022-02-21 16:25 ` [virtio-comment] [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE Christian Schoenebeck
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-21 16:22 UTC (permalink / raw)
  To: virtio-comment
  Cc: Cornelia Huck, Stefan Hajnoczi, Greg Kurz, Dominique Martinet

This is a follow-up of:
[1/2]: https://lists.oasis-open.org/archives/virtio-comment/202112/msg00034.html
[2/2]: https://lists.oasis-open.org/archives/virtio-comment/202112/msg00035.html

These 4 patches introduce a more fine graded control over the maximum length
of indirect descriptor tables.

Associated Github Task:
https://github.com/oasis-tcs/virtio-spec/issues/122

Latest Version (all patches):
https://github.com/cschoenebeck/virtio-spec/commits/long_indirect_descr

v1 -> v2:

  * Update reserved bits section [patch 1].

  * Add requirement to device normative section [patch 1].

  * Use the general new term "Queue Indirect Size" where possible instead of
    mentioning a bus specific field name [patch 1].

  * Simplified phrasing of requirements in split ring section [patch 1].

  * Move bus-independent changes from patch 2 to patch 1.

  * There is only one indirect descriptor table allowed per buffer
    (round-trip message), so use the term "per indirect descriptor table" for
    the semantic of "Queue Indirect Size" in patch 2.

  * Simplify phrasing of requirements in patch 2.

  * MMIO: First shot [NEW patch 3].

  * CCW: First shot [NEW patch 4].

Christian Schoenebeck (4):
  Add VIRTIO_RING_F_INDIRECT_SIZE
  Add PCI configuration field "queue_indirect_size"
  Add MMIO configuration register "QueueIndirectNum"
  Add CCW configuration field "indirect_num" to vq_info_block

 content.tex     | 77 ++++++++++++++++++++++++++++++++++++++++++++++---
 packed-ring.tex |  2 +-
 split-ring.tex  |  8 +++--
 3 files changed, 80 insertions(+), 7 deletions(-)

-- 
2.30.2





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

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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [virtio-comment] [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE
  2022-02-21 16:22 [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
@ 2022-02-21 16:25 ` Christian Schoenebeck
  2022-03-10 14:57   ` Stefan Hajnoczi
  2022-02-21 16:27 ` [PATCH v2 2/4] Add PCI configuration field "queue_indirect_size" Christian Schoenebeck
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-21 16:25 UTC (permalink / raw)
  To: virtio-comment
  Cc: Cornelia Huck, Stefan Hajnoczi, Greg Kurz, Dominique Martinet

This new feature flag allows to decouple the maximum amount of
descriptors in indirect descriptor tables from the Queue Size.

The new term "Queue Indirect Size" is introduced for this purpose,
which is a bus specific configuration whose negotiation is further
specified with subsequent patches.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 content.tex     | 32 ++++++++++++++++++++++++++++++--
 packed-ring.tex |  2 +-
 split-ring.tex  |  8 ++++++--
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/content.tex b/content.tex
index c6f116c..685525d 100644
--- a/content.tex
+++ b/content.tex
@@ -99,10 +99,10 @@ \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature B
 \begin{description}
 \item[0 to 23, and 50 to 127] Feature bits for the specific device type
 
-\item[24 to 40] Feature bits reserved for extensions to the queue and
+\item[24 to 41] Feature bits reserved for extensions to the queue and
   feature negotiation mechanisms
 
-\item[41 to 49, and 128 and above] Feature bits reserved for future extensions.
+\item[42 to 49, and 128 and above] Feature bits reserved for future extensions.
 \end{description}
 
 \begin{note}
@@ -1051,6 +1051,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 present either a value of 0 or a power of 2 in
 \field{queue_size}.
 
+If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated, the device MUST provide the
+Queue Indirect Size supported by device, which is a transport specific
+configuration. It MUST allow the driver to set a lower value.
+
 \drivernormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
 
 The driver MUST NOT write to \field{device_feature}, \field{num_queues}, \field{config_generation}, \field{queue_notify_off} or \field{queue_notify_data}.
@@ -6847,6 +6851,30 @@ \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_RING_F_INDIRECT_SIZE(41)] This feature indicates that the
+  Queue Indirect Size, i.e. the maximum amount of descriptors in indirect
+  descriptor tables, is independent from the Queue Size.
+
+  Without this feature, the Queue Size limits the length of the descriptor
+  chain, including indirect descriptor tables as in \ref{sec:Basic Facilities of
+  a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
+  Descriptors}, i.e. both the maximum amount of slots in the vring and the
+  actual bulk data size transmitted per vring slot.
+
+  With this feature enabled, the Queue Size only limits the maximum amount
+  of slots in the vring, but does not limit the actual bulk data size
+  being transmitted when indirect descriptors are used. Decoupling these
+  two configuration parameters this way not only allows much larger bulk data
+  being transferred per vring slot, but also avoids complicated synchronization
+  mechanisms if the device only supports a very small amount of vring slots. Due
+  to the 16-bit size of a descriptor's "next" field there is still an absolute
+  limit of $2^{16}$ descriptors per indirect descriptor table. However the
+  actual maximum amount supported by either device or driver might be less,
+  and therefore the bus specific Queue Indirect Size value MUST additionally
+  be negotiated if VIRTIO_RING_F_INDIRECT_SIZE was negotiated to subsequently
+  negotiate the actual amount of maximum indirect descriptors supported
+  by both sides.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
diff --git a/packed-ring.tex b/packed-ring.tex
index a9e6c16..e26d112 100644
--- a/packed-ring.tex
+++ b/packed-ring.tex
@@ -195,7 +195,7 @@ \subsection{Scatter-Gather Support}
 The device limits the number of descriptors in a list through a
 transport-specific and/or device-specific value. If not limited,
 the maximum number of descriptors in a list is the virt queue
-size.
+size unless the VIRTIO_RING_F_INDIRECT_SIZE feature has been negotiated.
 
 \subsection{Next Flag: Descriptor Chaining}
 \label{sec:Packed Virtqueues / Next Flag: Descriptor Chaining}
diff --git a/split-ring.tex b/split-ring.tex
index de94038..eaa90c3 100644
--- a/split-ring.tex
+++ b/split-ring.tex
@@ -268,8 +268,12 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
 set the VIRTQ_DESC_F_INDIRECT flag within an indirect descriptor (ie. only
 one table per descriptor).
 
-A driver MUST NOT create a descriptor chain longer than the Queue Size of
-the device.
+If VIRTIO_RING_F_INDIRECT_SIZE has not been negotiated, the driver MUST
+NOT create a descriptor chain longer than the Queue Size of the device.
+
+If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated, the number of
+descriptors per indirect descriptor table MUST NOT exceed the negotiated
+Queue Indirect Size.
 
 A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
 in \field{flags}.
-- 
2.30.2





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

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

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 2/4] Add PCI configuration field "queue_indirect_size"
  2022-02-21 16:22 [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
  2022-02-21 16:25 ` [virtio-comment] [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE Christian Schoenebeck
@ 2022-02-21 16:27 ` Christian Schoenebeck
  2022-03-10 15:04   ` [virtio-comment] " Stefan Hajnoczi
  2022-02-21 16:58 ` [PATCH v2 3/4] Add MMIO configuration register "QueueIndirectNum" Christian Schoenebeck
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-21 16:27 UTC (permalink / raw)
  To: virtio-comment
  Cc: Cornelia Huck, Stefan Hajnoczi, Greg Kurz, Dominique Martinet

This new PCI configuration field allows to negotiate a more fine
graded maximum lenght of indirect descriptor chains.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 content.tex | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/content.tex b/content.tex
index 685525d..da57d5d 100644
--- a/content.tex
+++ b/content.tex
@@ -902,6 +902,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         le64 queue_device;              /* read-write */
         le16 queue_notify_data;         /* read-only for driver */
         le16 queue_reset;               /* read-write */
+        le32 queue_indirect_size;       /* read-write */
 };
 \end{lstlisting}
 
@@ -987,6 +988,17 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         This field exists only if VIRTIO_F_RING_RESET has been
         negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 
+\item[\field{queue_indirect_size}]
+        This field is used to negotiate the maximum number of descriptors per
+        indirect descriptor table as in \ref{sec:Basic Facilities of a Virtio
+        Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
+        Descriptors} if and only if the VIRTIO_RING_F_INDIRECT_SIZE feature has
+        been negotiated.
+
+        The device specifies its maximum number of descriptors per indirect
+        descriptor table. If the driver requires fewer descriptors, it writes
+        its lower value to inform the device of the reduced resource
+        requirements.
 \end{description}
 
 \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
@@ -1081,6 +1093,12 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 values to different values than those that were used before the queue reset.
 (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}).
 
+The driver SHOULD write to \field{queue_indirect_size} if its maximum number of
+descriptors per vring slot is lower than that reported by the device.
+
+The driver MUST NOT write a higher value to \field{queue_indirect_size} than the
+one it reads from the device.
+
 \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability}
 
 The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG
-- 
2.30.2





^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 3/4] Add MMIO configuration register "QueueIndirectNum"
  2022-02-21 16:22 [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
  2022-02-21 16:25 ` [virtio-comment] [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE Christian Schoenebeck
  2022-02-21 16:27 ` [PATCH v2 2/4] Add PCI configuration field "queue_indirect_size" Christian Schoenebeck
@ 2022-02-21 16:58 ` Christian Schoenebeck
  2022-03-10 15:23   ` Stefan Hajnoczi
  2022-02-21 17:01 ` [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block Christian Schoenebeck
  2022-03-09 14:58 ` [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-21 16:58 UTC (permalink / raw)
  To: virtio-comment
  Cc: Cornelia Huck, Stefan Hajnoczi, Greg Kurz, Dominique Martinet

This new MMIO configuration register allows to negotiate a more fine
graded maximum lenght of indirect descriptor chains.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 content.tex | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/content.tex b/content.tex
index da57d5d..a3baf4d 100644
--- a/content.tex
+++ b/content.tex
@@ -2225,8 +2225,9 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
   \hline
   \mmioreg{QueueSel}{Virtual queue index}{0x030}{W}{%
     Writing to this register selects the virtual queue that the
-    following operations on the \field{QueueNumMax}, \field{QueueNum}, \field{QueueAlign}
-    and \field{QueuePFN} registers apply to. The index
+    following operations on the \field{QueueNumMax}, \field{QueueNum},
+    \field{QueueAlign}, \field{QueuePFN} and \field{QueueIndirectNum} registers
+    apply to. The index
     number of the first queue is zero (0x0). 
 .
   }
@@ -2282,6 +2283,19 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
     Also see \ref{sec:General Initialization And Device Operation / Device Initialization}~\nameref{sec:General Initialization And Device Operation / Device Initialization}.
   }
   \hline
+  \mmioreg{QueueIndirectNum}{Virtual queue indirect size}{0x074}{RW}{%
+    This register is used to negotiate the maximum number of descriptors per
+    indirect descriptor table as in \ref{sec:Basic Facilities of a Virtio
+    Device / Virtqueues / The Virtqueue Descriptor Table / Indirect Descriptors}
+    if and only if the VIRTIO_RING_F_INDIRECT_SIZE feature has been negotiated.
+
+    The device specifies its maximum number of descriptors per indirect
+    descriptor table. If the driver requires fewer descriptors, it writes
+    its lower value to inform the device of the reduced resource requirements.
+
+    This applies to the queue selected by writing to \field{QueueSel}.
+  }
+  \hline
   \mmioreg{Config}{Configuration space}{0x100+}{RW}{}
   \hline
 \end{longtable}
@@ -2314,6 +2328,10 @@ \subsection{Legacy interface}\label{sec:Virtio Transport Options / Virtio Over M
 \item Notify the device about the queue size by writing the size to
    \field{QueueNum}.
 
+\item If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated, read the maximum
+   length of indirect descriptor tables by reading \field{QueueIndirectNum}.
+   Write to lower this value if necessary.
+
 \item Notify the device about the used alignment by writing its value
    in bytes to \field{QueueAlign}.
 
-- 
2.30.2





^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-02-21 16:22 [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
                   ` (2 preceding siblings ...)
  2022-02-21 16:58 ` [PATCH v2 3/4] Add MMIO configuration register "QueueIndirectNum" Christian Schoenebeck
@ 2022-02-21 17:01 ` Christian Schoenebeck
  2022-03-10 15:26   ` Stefan Hajnoczi
  2022-03-09 14:58 ` [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-02-21 17:01 UTC (permalink / raw)
  To: virtio-comment
  Cc: Cornelia Huck, Stefan Hajnoczi, Greg Kurz, Dominique Martinet

This new CCW configuration field allows to negotiate a more fine
graded maximum lenght of indirect descriptor chains.

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
---
 content.tex | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/content.tex b/content.tex
index a3baf4d..d400ea7 100644
--- a/content.tex
+++ b/content.tex
@@ -2599,6 +2599,7 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
         be16 num;
         be64 driver;
         be64 device;
+        be32 indirect_num;
 };
 \end{lstlisting}
 
@@ -2607,6 +2608,10 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
 available area and used area for queue \field{index}, respectively. The actual
 virtqueue size (number of allocated buffers) is transmitted in \field{num}.
 
+If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then \field{indirect_num}
+reflects the maximum length of indirect descriptor tables for queue
+\field{index}.
+
 \devicenormative{\paragraph}{Configuring a Virtqueue}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Configuring a Virtqueue}
 
 \field{res0} is reserved and MUST be ignored by the device.
-- 
2.30.2





^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size
  2022-02-21 16:22 [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
                   ` (3 preceding siblings ...)
  2022-02-21 17:01 ` [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block Christian Schoenebeck
@ 2022-03-09 14:58 ` Christian Schoenebeck
  2022-03-10 15:35   ` Stefan Hajnoczi
  4 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-03-09 14:58 UTC (permalink / raw)
  To: virtio-comment
  Cc: Cornelia Huck, Stefan Hajnoczi, Greg Kurz, Dominique Martinet,
	Michael S. Tsirkin

On Montag, 21. Februar 2022 17:22:18 CET Christian Schoenebeck wrote:
> This is a follow-up of:
> [1/2]:
> https://lists.oasis-open.org/archives/virtio-comment/202112/msg00034.html
> [2/2]:
> https://lists.oasis-open.org/archives/virtio-comment/202112/msg00035.html
> 
> These 4 patches introduce a more fine graded control over the maximum length
> of indirect descriptor tables.
> 
> Associated Github Task:
> https://github.com/oasis-tcs/virtio-spec/issues/122
> 
> Latest Version (all patches):
> https://github.com/cschoenebeck/virtio-spec/commits/long_indirect_descr

Ping.

Best regards,
Christian Schoenebeck

> v1 -> v2:
> 
>   * Update reserved bits section [patch 1].
> 
>   * Add requirement to device normative section [patch 1].
> 
>   * Use the general new term "Queue Indirect Size" where possible instead of
> mentioning a bus specific field name [patch 1].
> 
>   * Simplified phrasing of requirements in split ring section [patch 1].
> 
>   * Move bus-independent changes from patch 2 to patch 1.
> 
>   * There is only one indirect descriptor table allowed per buffer
>     (round-trip message), so use the term "per indirect descriptor table"
> for the semantic of "Queue Indirect Size" in patch 2.
> 
>   * Simplify phrasing of requirements in patch 2.
> 
>   * MMIO: First shot [NEW patch 3].
> 
>   * CCW: First shot [NEW patch 4].
> 
> Christian Schoenebeck (4):
>   Add VIRTIO_RING_F_INDIRECT_SIZE
>   Add PCI configuration field "queue_indirect_size"
>   Add MMIO configuration register "QueueIndirectNum"
>   Add CCW configuration field "indirect_num" to vq_info_block
> 
>  content.tex     | 77 ++++++++++++++++++++++++++++++++++++++++++++++---
>  packed-ring.tex |  2 +-
>  split-ring.tex  |  8 +++--
>  3 files changed, 80 insertions(+), 7 deletions(-)





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

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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE
  2022-02-21 16:25 ` [virtio-comment] [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE Christian Schoenebeck
@ 2022-03-10 14:57   ` Stefan Hajnoczi
  2022-03-10 16:20     ` Christian Schoenebeck
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2022-03-10 14:57 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

On Mon, Feb 21, 2022 at 05:25:00PM +0100, Christian Schoenebeck wrote:
> @@ -6847,6 +6851,30 @@ \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_RING_F_INDIRECT_SIZE(41)] This feature indicates that the
> +  Queue Indirect Size, i.e. the maximum amount of descriptors in indirect
> +  descriptor tables, is independent from the Queue Size.
> +
> +  Without this feature, the Queue Size limits the length of the descriptor
> +  chain, including indirect descriptor tables as in \ref{sec:Basic Facilities of
> +  a Virtio Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
> +  Descriptors}, i.e. both the maximum amount of slots in the vring and the
> +  actual bulk data size transmitted per vring slot.
> +
> +  With this feature enabled, the Queue Size only limits the maximum amount
> +  of slots in the vring, but does not limit the actual bulk data size
> +  being transmitted when indirect descriptors are used. Decoupling these
> +  two configuration parameters this way not only allows much larger bulk data
> +  being transferred per vring slot, but also avoids complicated synchronization
> +  mechanisms if the device only supports a very small amount of vring slots. Due
> +  to the 16-bit size of a descriptor's "next" field there is still an absolute
> +  limit of $2^{16}$ descriptors per indirect descriptor table. However the
> +  actual maximum amount supported by either device or driver might be less,
> +  and therefore the bus specific Queue Indirect Size value MUST additionally

s/bus/transport/

Otherwise:
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [virtio-comment] [PATCH v2 2/4] Add PCI configuration field "queue_indirect_size"
  2022-02-21 16:27 ` [PATCH v2 2/4] Add PCI configuration field "queue_indirect_size" Christian Schoenebeck
@ 2022-03-10 15:04   ` Stefan Hajnoczi
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2022-03-10 15:04 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet

[-- Attachment #1: Type: text/plain, Size: 465 bytes --]

On Mon, Feb 21, 2022 at 05:27:10PM +0100, Christian Schoenebeck wrote:
> This new PCI configuration field allows to negotiate a more fine
> graded maximum lenght of indirect descriptor chains.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  content.tex | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/4] Add MMIO configuration register "QueueIndirectNum"
  2022-02-21 16:58 ` [PATCH v2 3/4] Add MMIO configuration register "QueueIndirectNum" Christian Schoenebeck
@ 2022-03-10 15:23   ` Stefan Hajnoczi
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2022-03-10 15:23 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Mon, Feb 21, 2022 at 05:58:48PM +0100, Christian Schoenebeck wrote:
> This new MMIO configuration register allows to negotiate a more fine
> graded maximum lenght of indirect descriptor chains.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  content.tex | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-02-21 17:01 ` [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block Christian Schoenebeck
@ 2022-03-10 15:26   ` Stefan Hajnoczi
  2022-03-10 16:09     ` [virtio-comment] " Cornelia Huck
  2022-03-10 16:09     ` Christian Schoenebeck
  0 siblings, 2 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2022-03-10 15:26 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet

[-- Attachment #1: Type: text/plain, Size: 1501 bytes --]

On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:
> This new CCW configuration field allows to negotiate a more fine
> graded maximum lenght of indirect descriptor chains.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  content.tex | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index a3baf4d..d400ea7 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
>          be16 num;
>          be64 driver;
>          be64 device;
> +        be32 indirect_num;
>  };
>  \end{lstlisting}
>  
> @@ -2607,6 +2608,10 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
>  available area and used area for queue \field{index}, respectively. The actual
>  virtqueue size (number of allocated buffers) is transmitted in \field{num}.
>  
> +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then \field{indirect_num}
> +reflects the maximum length of indirect descriptor tables for queue
> +\field{index}.

I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
driver-to-device. So it allows the driver to set the Queue Indirect
Size, but how does the driver query the device's maximum Queue Indirect
Size value?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size
  2022-03-09 14:58 ` [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
@ 2022-03-10 15:35   ` Stefan Hajnoczi
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Hajnoczi @ 2022-03-10 15:35 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet,
	Michael S. Tsirkin

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

On Wed, Mar 09, 2022 at 03:58:56PM +0100, Christian Schoenebeck wrote:
> On Montag, 21. Februar 2022 17:22:18 CET Christian Schoenebeck wrote:
> > This is a follow-up of:
> > [1/2]:
> > https://lists.oasis-open.org/archives/virtio-comment/202112/msg00034.html
> > [2/2]:
> > https://lists.oasis-open.org/archives/virtio-comment/202112/msg00035.html
> > 
> > These 4 patches introduce a more fine graded control over the maximum length
> > of indirect descriptor tables.
> > 
> > Associated Github Task:
> > https://github.com/oasis-tcs/virtio-spec/issues/122
> > 
> > Latest Version (all patches):
> > https://github.com/cschoenebeck/virtio-spec/commits/long_indirect_descr
> 
> Ping.

Sorry for the delay.

This looks very close. I posted a CCW question but it might just be my
lack of familiarity with CCW :).

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [virtio-comment] Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-03-10 15:26   ` Stefan Hajnoczi
@ 2022-03-10 16:09     ` Cornelia Huck
  2022-03-11  8:53       ` Christian Schoenebeck
  2022-03-10 16:09     ` Christian Schoenebeck
  1 sibling, 1 reply; 18+ messages in thread
From: Cornelia Huck @ 2022-03-10 16:09 UTC (permalink / raw)
  To: Stefan Hajnoczi, Christian Schoenebeck
  Cc: virtio-comment, Greg Kurz, Dominique Martinet, Halil Pasic

On Thu, Mar 10 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:
>> This new CCW configuration field allows to negotiate a more fine
>> graded maximum lenght of indirect descriptor chains.
>> 
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> ---
>>  content.tex | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/content.tex b/content.tex
>> index a3baf4d..d400ea7 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
>>          be16 num;
>>          be64 driver;
>>          be64 device;
>> +        be32 indirect_num;
>>  };
>>  \end{lstlisting}
>>  
>> @@ -2607,6 +2608,10 @@ \subsubsection{Configuring a Virtqueue}\label{sec:Virtio Transport Options / Vir
>>  available area and used area for queue \field{index}, respectively. The actual
>>  virtqueue size (number of allocated buffers) is transmitted in \field{num}.
>>  
>> +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then \field{indirect_num}
>> +reflects the maximum length of indirect descriptor tables for queue
>> +\field{index}.
>
> I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
> driver-to-device. So it allows the driver to set the Queue Indirect
> Size, but how does the driver query the device's maximum Queue Indirect
> Size value?

[cc:ing Halil in case he has any further comments]

You're right, CCW_CMD_SET_VQ + vq_info_block is driver-to-device. The
driver will obtain information about a queue via CCW_CMD_READ_VQ_CONF +
vq_config_block, so a max_indirect_num field needs to be added there as
well, I think.

Moreover, we're changing the length of the ccw payload. Extending at the
end is generally fine, but the device and the driver need to agree on
what the expected payload is. We basically have two options here:

* Make it depend on the feature bit being negotiated. This works because
  virtqueue discovery needs to be done only after feature negotiation
  has completed. However, this will get a bit awkward if we need to add
  another field depending on a new feature bit: negotiating that
  hypothetical feature would imply that the indirect num fields would be
  present, but not valid, if the indirect feature had not been
  negotiated. Not a showstopper, but looks a bit odd.
* Tie it to a new ccw revision (3) and make offering the feature bit
  dependant upon revision 3 or later being negotiated. This has the
  advantage that ccw revisions always build on each other (so no
  awkwardness for future extension) and the drawback of introducing
  another transport-specific prereq.

If we can live with the possible awkwardness of future extensions, tying
the size of the structures to feature bits might be the preferable way.


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

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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-03-10 15:26   ` Stefan Hajnoczi
  2022-03-10 16:09     ` [virtio-comment] " Cornelia Huck
@ 2022-03-10 16:09     ` Christian Schoenebeck
  1 sibling, 0 replies; 18+ messages in thread
From: Christian Schoenebeck @ 2022-03-10 16:09 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet

On Donnerstag, 10. März 2022 16:26:46 CET Stefan Hajnoczi wrote:
> On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:
> > This new CCW configuration field allows to negotiate a more fine
> > graded maximum lenght of indirect descriptor chains.
> > 
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

I'll fix those accidentally added double SOBs BTW.

> > ---
> > 
> >  content.tex | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/content.tex b/content.tex
> > index a3baf4d..d400ea7 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a
> > Virtqueue}\label{sec:Virtio Transport Options / Vir> 
> >          be16 num;
> >          be64 driver;
> >          be64 device;
> > 
> > +        be32 indirect_num;
> > 
> >  };
> >  \end{lstlisting}
> > 
> > @@ -2607,6 +2608,10 @@ \subsubsection{Configuring a
> > Virtqueue}\label{sec:Virtio Transport Options / Vir> 
> >  available area and used area for queue \field{index}, respectively. The
> >  actual virtqueue size (number of allocated buffers) is transmitted in
> >  \field{num}.> 
> > +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then
> > \field{indirect_num} +reflects the maximum length of indirect descriptor
> > tables for queue +\field{index}.
> 
> I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
> driver-to-device. So it allows the driver to set the Queue Indirect
> Size, but how does the driver query the device's maximum Queue Indirect
> Size value?

Ah, seems you are right Stefan. I assumed this was similar to PCI.

So the way to go on CCW would be an additional (equally named?) field in 
struct vq_info_block (CCW_CMD_READ_VQ_CONF) for device -> driver?

Cornelia?

Best regards,
Christian Schoenebeck



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE
  2022-03-10 14:57   ` Stefan Hajnoczi
@ 2022-03-10 16:20     ` Christian Schoenebeck
  0 siblings, 0 replies; 18+ messages in thread
From: Christian Schoenebeck @ 2022-03-10 16:20 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: virtio-comment, Cornelia Huck, Greg Kurz, Dominique Martinet

On Donnerstag, 10. März 2022 15:57:28 CET Stefan Hajnoczi wrote:
> On Mon, Feb 21, 2022 at 05:25:00PM +0100, Christian Schoenebeck wrote:
> > @@ -6847,6 +6851,30 @@ \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_RING_F_INDIRECT_SIZE(41)] This feature indicates that the
> > +  Queue Indirect Size, i.e. the maximum amount of descriptors in indirect
> > +  descriptor tables, is independent from the Queue Size.
> > +
> > +  Without this feature, the Queue Size limits the length of the
> > descriptor
> > +  chain, including indirect descriptor tables as in \ref{sec:Basic
> > Facilities of +  a Virtio Device / Virtqueues / The Virtqueue Descriptor
> > Table / Indirect +  Descriptors}, i.e. both the maximum amount of slots
> > in the vring and the +  actual bulk data size transmitted per vring slot.
> > +
> > +  With this feature enabled, the Queue Size only limits the maximum
> > amount
> > +  of slots in the vring, but does not limit the actual bulk data size
> > +  being transmitted when indirect descriptors are used. Decoupling these
> > +  two configuration parameters this way not only allows much larger bulk
> > data +  being transferred per vring slot, but also avoids complicated
> > synchronization +  mechanisms if the device only supports a very small
> > amount of vring slots. Due +  to the 16-bit size of a descriptor's "next"
> > field there is still an absolute +  limit of $2^{16}$ descriptors per
> > indirect descriptor table. However the +  actual maximum amount supported
> > by either device or driver might be less, +  and therefore the bus
> > specific Queue Indirect Size value MUST additionally
> s/bus/transport/
> 
> Otherwise:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

I'll change that in v3. Thanks!

Best regards,
Christian Schoenebeck



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-03-10 16:09     ` [virtio-comment] " Cornelia Huck
@ 2022-03-11  8:53       ` Christian Schoenebeck
  2022-03-11  9:21         ` [virtio-comment] " Cornelia Huck
  0 siblings, 1 reply; 18+ messages in thread
From: Christian Schoenebeck @ 2022-03-11  8:53 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Stefan Hajnoczi, virtio-comment, Greg Kurz, Dominique Martinet,
	Halil Pasic

On Donnerstag, 10. März 2022 17:09:25 CET Cornelia Huck wrote:
> On Thu, Mar 10 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:
> >> This new CCW configuration field allows to negotiate a more fine
> >> graded maximum lenght of indirect descriptor chains.
> >> 
> >> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> >> ---
> >> 
> >>  content.tex | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/content.tex b/content.tex
> >> index a3baf4d..d400ea7 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a
> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
> >>          be16 num;
> >>          be64 driver;
> >>          be64 device;
> >> 
> >> +        be32 indirect_num;
> >> 
> >>  };
> >>  \end{lstlisting}
> >> 
> >> @@ -2607,6 +2608,10 @@ \subsubsection{Configuring a
> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
> >>  available area and used area for queue \field{index}, respectively. The
> >>  actual virtqueue size (number of allocated buffers) is transmitted in
> >>  \field{num}.>> 
> >> +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then
> >> \field{indirect_num} +reflects the maximum length of indirect descriptor
> >> tables for queue +\field{index}.
> > 
> > I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
> > driver-to-device. So it allows the driver to set the Queue Indirect
> > Size, but how does the driver query the device's maximum Queue Indirect
> > Size value?
> 
> [cc:ing Halil in case he has any further comments]
> 
> You're right, CCW_CMD_SET_VQ + vq_info_block is driver-to-device. The
> driver will obtain information about a queue via CCW_CMD_READ_VQ_CONF +
> vq_config_block, so a max_indirect_num field needs to be added there as
> well, I think.
> 
> Moreover, we're changing the length of the ccw payload. Extending at the
> end is generally fine, but the device and the driver need to agree on
> what the expected payload is. We basically have two options here:
> 
> * Make it depend on the feature bit being negotiated. This works because
>   virtqueue discovery needs to be done only after feature negotiation
>   has completed. However, this will get a bit awkward if we need to add
>   another field depending on a new feature bit: negotiating that
>   hypothetical feature would imply that the indirect num fields would be
>   present, but not valid, if the indirect feature had not been
>   negotiated. Not a showstopper, but looks a bit odd.
> * Tie it to a new ccw revision (3) and make offering the feature bit
>   dependant upon revision 3 or later being negotiated. This has the
>   advantage that ccw revisions always build on each other (so no
>   awkwardness for future extension) and the drawback of introducing
>   another transport-specific prereq.
> 
> If we can live with the possible awkwardness of future extensions, tying
> the size of the structures to feature bits might be the preferable way.

Really? My intuitive pick would rather be option 2 (CCW revision). But I'll go 
for whatever the common opinion is on this CCW issue.

Best regards,
Christian Schoenebeck



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [virtio-comment] Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-03-11  8:53       ` Christian Schoenebeck
@ 2022-03-11  9:21         ` Cornelia Huck
  2022-03-11 11:15           ` Halil Pasic
  0 siblings, 1 reply; 18+ messages in thread
From: Cornelia Huck @ 2022-03-11  9:21 UTC (permalink / raw)
  To: Christian Schoenebeck
  Cc: Stefan Hajnoczi, virtio-comment, Greg Kurz, Dominique Martinet,
	Halil Pasic

On Fri, Mar 11 2022, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 10. März 2022 17:09:25 CET Cornelia Huck wrote:
>> On Thu, Mar 10 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> > On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:
>> >> This new CCW configuration field allows to negotiate a more fine
>> >> graded maximum lenght of indirect descriptor chains.
>> >> 
>> >> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
>> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
>> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> >> ---
>> >> 
>> >>  content.tex | 5 +++++
>> >>  1 file changed, 5 insertions(+)
>> >> 
>> >> diff --git a/content.tex b/content.tex
>> >> index a3baf4d..d400ea7 100644
>> >> --- a/content.tex
>> >> +++ b/content.tex
>> >> @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a
>> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
>> >>          be16 num;
>> >>          be64 driver;
>> >>          be64 device;
>> >> 
>> >> +        be32 indirect_num;
>> >> 
>> >>  };
>> >>  \end{lstlisting}
>> >> 
>> >> @@ -2607,6 +2608,10 @@ \subsubsection{Configuring a
>> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
>> >>  available area and used area for queue \field{index}, respectively. The
>> >>  actual virtqueue size (number of allocated buffers) is transmitted in
>> >>  \field{num}.>> 
>> >> +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then
>> >> \field{indirect_num} +reflects the maximum length of indirect descriptor
>> >> tables for queue +\field{index}.
>> > 
>> > I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
>> > driver-to-device. So it allows the driver to set the Queue Indirect
>> > Size, but how does the driver query the device's maximum Queue Indirect
>> > Size value?
>> 
>> [cc:ing Halil in case he has any further comments]
>> 
>> You're right, CCW_CMD_SET_VQ + vq_info_block is driver-to-device. The
>> driver will obtain information about a queue via CCW_CMD_READ_VQ_CONF +
>> vq_config_block, so a max_indirect_num field needs to be added there as
>> well, I think.
>> 
>> Moreover, we're changing the length of the ccw payload. Extending at the
>> end is generally fine, but the device and the driver need to agree on
>> what the expected payload is. We basically have two options here:
>> 
>> * Make it depend on the feature bit being negotiated. This works because
>>   virtqueue discovery needs to be done only after feature negotiation
>>   has completed. However, this will get a bit awkward if we need to add
>>   another field depending on a new feature bit: negotiating that
>>   hypothetical feature would imply that the indirect num fields would be
>>   present, but not valid, if the indirect feature had not been
>>   negotiated. Not a showstopper, but looks a bit odd.
>> * Tie it to a new ccw revision (3) and make offering the feature bit
>>   dependant upon revision 3 or later being negotiated. This has the
>>   advantage that ccw revisions always build on each other (so no
>>   awkwardness for future extension) and the drawback of introducing
>>   another transport-specific prereq.
>> 
>> If we can live with the possible awkwardness of future extensions, tying
>> the size of the structures to feature bits might be the preferable way.
>
> Really? My intuitive pick would rather be option 2 (CCW revision). But I'll go 
> for whatever the common opinion is on this CCW issue.

Either would work; that's why I'd like to have Halil's opinion as well.


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

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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-03-11  9:21         ` [virtio-comment] " Cornelia Huck
@ 2022-03-11 11:15           ` Halil Pasic
  2022-03-11 12:09             ` [virtio-comment] " Christian Schoenebeck
  0 siblings, 1 reply; 18+ messages in thread
From: Halil Pasic @ 2022-03-11 11:15 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Christian Schoenebeck, Stefan Hajnoczi, virtio-comment,
	Greg Kurz, Dominique Martinet, Halil Pasic

On Fri, 11 Mar 2022 10:21:54 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Fri, Mar 11 2022, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> 
> > On Donnerstag, 10. März 2022 17:09:25 CET Cornelia Huck wrote:  
> >> On Thu, Mar 10 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:  
> >> > On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:  
> >> >> This new CCW configuration field allows to negotiate a more fine
> >> >> graded maximum lenght of indirect descriptor chains.
> >> >> 
> >> >> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> >> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com
> >> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> >> >> ---
> >> >> 
> >> >>  content.tex | 5 +++++
> >> >>  1 file changed, 5 insertions(+)
> >> >> 
> >> >> diff --git a/content.tex b/content.tex
> >> >> index a3baf4d..d400ea7 100644
> >> >> --- a/content.tex
> >> >> +++ b/content.tex
> >> >> @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a
> >> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
> >> >>          be16 num;
> >> >>          be64 driver;
> >> >>          be64 device;
> >> >> 
> >> >> +        be32 indirect_num;
> >> >> 
> >> >>  };
> >> >>  \end{lstlisting}
> >> >> 
> >> >> @@ -2607,6 +2608,10 @@ \subsubsection{Configuring a
> >> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
> >> >>  available area and used area for queue \field{index}, respectively. The
> >> >>  actual virtqueue size (number of allocated buffers) is transmitted in  
> >> >>  \field{num}.>>   
> >> >> +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then
> >> >> \field{indirect_num} +reflects the maximum length of indirect descriptor
> >> >> tables for queue +\field{index}.  
> >> > 
> >> > I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
> >> > driver-to-device. So it allows the driver to set the Queue Indirect
> >> > Size, but how does the driver query the device's maximum Queue Indirect
> >> > Size value?  
> >> 
> >> [cc:ing Halil in case he has any further comments]
> >> 
> >> You're right, CCW_CMD_SET_VQ + vq_info_block is driver-to-device. The
> >> driver will obtain information about a queue via CCW_CMD_READ_VQ_CONF +
> >> vq_config_block, so a max_indirect_num field needs to be added there as
> >> well, I think.
> >> 
> >> Moreover, we're changing the length of the ccw payload. Extending at the
> >> end is generally fine, but the device and the driver need to agree on
> >> what the expected payload is. We basically have two options here:
> >> 
> >> * Make it depend on the feature bit being negotiated. This works because
> >>   virtqueue discovery needs to be done only after feature negotiation
> >>   has completed. However, this will get a bit awkward if we need to add
> >>   another field depending on a new feature bit: negotiating that
> >>   hypothetical feature would imply that the indirect num fields would be
> >>   present, but not valid, if the indirect feature had not been
> >>   negotiated. Not a showstopper, but looks a bit odd.

I think can get very ugly. Present but not valid is not the problem in
my opinion. In fact we would do the same: if the feature is not
negotiated, the field is invalid. But making the layout and size depend
on a feature is tricky, because one can't tell: if feature x is
negotiated the layout is that and that for the reasons you pointed out.

> >> * Tie it to a new ccw revision (3) and make offering the feature bit
> >>   dependant upon revision 3 or later being negotiated. This has the
> >>   advantage that ccw revisions always build on each other (so no
> >>   awkwardness for future extension) and the drawback of introducing
> >>   another transport-specific prereq.

I think this is inherent. If the transport does not support the new
CCW layout there is no way we can make this work. AFAIR the transport
can just fence the feature if the transport prereq is not met.

> >> 
> >> If we can live with the possible awkwardness of future extensions, tying
> >> the size of the structures to feature bits might be the preferable way.  
> >
> > Really? My intuitive pick would rather be option 2 (CCW revision). But I'll go 
> > for whatever the common opinion is on this CCW issue.  
> 
> Either would work; that's why I'd like to have Halil's opinion as well.

Halil was catching up with the history :) Sorry for being late to the
party.

I tend to agree with Christian: this impacts when a CCW should be
accepted or rejected, and is thus not much unlike adding a new ccw.
Furthermore I don't think mix and match (which is kind of the default for
feature bits) works well for layout of structs.

But let me take a step back, and ask some more general questions?
1) What is the objective behind the mechanism which can be used by the
driver to tell the device its maximum. I believe the indirect descriptor
table is always allocated by the guest, so the guest has no reason to
fear larger than its max. The only thing I can think of is some pools
of buffers allocated and maintained by the device, where each buffer
is the size of max payload. Is this what we have in mind here?
2) Not so long ago we had a discussion about introducing a common (device
agnostic) configuration space (a.k.a. misc config). The indirect table
size might be a good candidate for that as well. We essentially want
the very same functionality for all the transports, it is just that
we have no vehicle at the moment to do this in an uniform way. Opinions?

Regards,
Halil


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [virtio-comment] Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block
  2022-03-11 11:15           ` Halil Pasic
@ 2022-03-11 12:09             ` Christian Schoenebeck
  0 siblings, 0 replies; 18+ messages in thread
From: Christian Schoenebeck @ 2022-03-11 12:09 UTC (permalink / raw)
  To: Halil Pasic
  Cc: Cornelia Huck, Stefan Hajnoczi, virtio-comment, Greg Kurz,
	Dominique Martinet

On Freitag, 11. März 2022 12:15:40 CET Halil Pasic wrote:
> But let me take a step back, and ask some more general questions?
> 1) What is the objective behind the mechanism which can be used by the
> driver to tell the device its maximum. I believe the indirect descriptor
> table is always allocated by the guest, so the guest has no reason to
> fear larger than its max. The only thing I can think of is some pools
> of buffers allocated and maintained by the device, where each buffer
> is the size of max payload. Is this what we have in mind here?

As described at the end on the associated virtio-spec github issue [1]:

  "
  Linux drivers

  Since torvalds/linux@44ed808 the Linux kernel ignores if individual drivers
  exceed the Queue Size (and violating the specs). Likewise there are some
  devices which use device specific config to negotiate a max. bulk data size 
  being lower than the Queue Size. The proposed spec changes would address 
  both use cases in a clean way.

  QEMU

  QEMU tolerates as well if guests drivers exceed the Queue Size. However
  currently it has a hard coded limit of max. 1024 memory segments:
  #define VIRTQUEUE_MAX_SIZE 1024
  Which limits the max. buik data size per virtio message to slightly below
  4MB
  "

So ATM when you run a Linux driver that exceeds QEMU's current limit of 1024
segments, you would get the following fatal QEMU error [2]:

  qemu-system-x86_64: virtio: too many write descriptors in indirect table

QEMU currently allocates the required space for the descriptors on the stack
and uses that hard coded value for the allocation size. Now even if you would
change that (which is a delicate bunch of code), you would still break
compatibility with Linux guests running on older QEMU hosts.

[1] https://github.com/oasis-tcs/virtio-spec/issues/122
[2] https://lore.kernel.org/netdev/cover.1640870037.git.linux_oss@crudebyte.com/

> 2) Not so long ago we had a discussion about introducing a common (device
> agnostic) configuration space (a.k.a. misc config). The indirect table
> size might be a good candidate for that as well. We essentially want
> the very same functionality for all the transports, it is just that
> we have no vehicle at the moment to do this in an uniform way. Opinions?
> 
> Regards,
> Halil

I need to leave that to other people to comment on, as I am completely unaware
about these plans. Sounds like a long-term plan to me though.

Best regards,
Christian Schoenebeck



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

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

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


^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-03-11 12:09 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-21 16:22 [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
2022-02-21 16:25 ` [virtio-comment] [PATCH v2 1/4] Add VIRTIO_RING_F_INDIRECT_SIZE Christian Schoenebeck
2022-03-10 14:57   ` Stefan Hajnoczi
2022-03-10 16:20     ` Christian Schoenebeck
2022-02-21 16:27 ` [PATCH v2 2/4] Add PCI configuration field "queue_indirect_size" Christian Schoenebeck
2022-03-10 15:04   ` [virtio-comment] " Stefan Hajnoczi
2022-02-21 16:58 ` [PATCH v2 3/4] Add MMIO configuration register "QueueIndirectNum" Christian Schoenebeck
2022-03-10 15:23   ` Stefan Hajnoczi
2022-02-21 17:01 ` [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block Christian Schoenebeck
2022-03-10 15:26   ` Stefan Hajnoczi
2022-03-10 16:09     ` [virtio-comment] " Cornelia Huck
2022-03-11  8:53       ` Christian Schoenebeck
2022-03-11  9:21         ` [virtio-comment] " Cornelia Huck
2022-03-11 11:15           ` Halil Pasic
2022-03-11 12:09             ` [virtio-comment] " Christian Schoenebeck
2022-03-10 16:09     ` Christian Schoenebeck
2022-03-09 14:58 ` [virtio-comment] [PATCH v2 0/4] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size Christian Schoenebeck
2022-03-10 15:35   ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.