[PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Christian Schoenebeck]

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

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

diff --git a/content.tex b/content.tex
index 5d112af..0aa4842 100644
--- a/content.tex
+++ b/content.tex
@@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   transport specific.
   For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
 
+  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the
+  maximum amount of descriptors in indirect descriptor tables is independent
+  from the Queue Size.
+
+  If this feature bit is not negotiated, then a driver MUST NOT create a
+  descriptor chain longer than the Queue Size, which then also applies to
+  indirect descriptor tables as in \ref{sec:Basic Facilities of a Virtio
+  Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
+  Descriptors}. Which means without this feature, the Queue Size limits
+  both the maximum amount of slots in the vring, as well as the actual
+  bulk data size being 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.
+
 \end{description}
 
 \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
diff --git a/split-ring.tex b/split-ring.tex
index bfef62d..ae2aeb8 100644
--- a/split-ring.tex
+++ b/split-ring.tex
@@ -269,7 +269,7 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
 one table per descriptor).
 
 A driver MUST NOT create a descriptor chain longer than the Queue Size of
-the device.
+the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
 
 A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
 in \field{flags}.
-- 
2.20.1

[PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

This new common 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    | 25 ++++++++++++++++++++++++-
 split-ring.tex |  3 +++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/content.tex b/content.tex
index 0aa4842..e3cfcae 100644
--- a/content.tex
+++ b/content.tex
@@ -859,6 +859,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         le64 queue_driver;              /* read-write */
         le64 queue_device;              /* read-write */
         le16 queue_notify_data;         /* read-only for driver */
+        le32 queue_indirect_size;       /* read-write */
 };
 \end{lstlisting}
 
@@ -938,6 +939,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
         may benefit from providing another value, for example an internal virtqueue
         identifier, or an internal offset related to the virtqueue number.
         \end{note}
+
+\item[\field{queue_indirect_size}]
+        This field is used to negotiate the maximum amount of descriptors per
+        vring slot 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 supported number of descriptors per
+        vring slot. 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}
@@ -1003,6 +1014,12 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
 
 The driver MUST NOT write a 0 to \field{queue_enable}.
 
+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
@@ -6712,7 +6729,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
   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.
+  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 common configuration field "queue_indirect_size" MUST exist
+  if VIRTIO_RING_F_INDIRECT_SIZE was negotiated to subsequently
+  negotiate the actual amount of maximum indirect descriptors supported
+  by both sides, as described in \ref{sec:Virtio Transport Options / Virtio
+  Over PCI Bus / PCI Device Layout / Common configuration structure layout}.
 
 \end{description}
 
diff --git a/split-ring.tex b/split-ring.tex
index ae2aeb8..d8f66c1 100644
--- a/split-ring.tex
+++ b/split-ring.tex
@@ -270,6 +270,9 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
 
 A driver MUST NOT create a descriptor chain longer than the Queue Size of
 the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
+Furthermore if VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then the number
+of descriptors per vring slot MUST NOT exceed the value negotiated by common
+configuration field "queue_indirect_size".
 
 A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
 in \field{flags}.
-- 
2.20.1

[PATCH 0/2] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size

[Christian Schoenebeck]

This is a follow-up of:
[0/2]: https://lists.oasis-open.org/archives/virtio-comment/202111/msg00089.html
[1/2]: https://lists.oasis-open.org/archives/virtio-comment/202112/msg00005.html
[2/2]: https://lists.oasis-open.org/archives/virtio-comment/202112/msg00006.html

These two 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

Changes:

  * VIRTIO_RING_F_LARGE_INDIRECT_DESC -> VIRTIO_RING_F_INDIRECT_SIZE
    [patch 1], [patch 2]

  * queue_indirect_size field: 16 bit -> 32 bit [patch 2]

  * Semantic change: "per indirect descriptor table" -> "per vring slot".
    [patch 2]

  * Revised first paragraph of VIRTIO_RING_F_INDIRECT_SIZE(40) to be more clear
    that it is not only about exceeding the queue size
    ("This feature indicates ... "). [patch 1]

  * Drop mentioning "revision 3". [patch 2]

  * revised commit log [patch 1]

  * Grammar: "... but does not limit the actual bulk data size ...". [patch 1]

  * Grammar: "... if the device ...". [patch 1]

  * queue_indirect_size field may only be used if VIRTIO_RING_F_INDIRECT_SIZE
    was used. [patch 2]

  * Drop usage of SHOULD/MUST in common description. [patch 2]

  * Grammar: had been -> was [patch 2]

  * Add requirements for field 'queue_indirect_size' to driver normative
    section. [patch 2]

Christian Schoenebeck (2):
  Add VIRTIO_RING_F_INDIRECT_SIZE
  Add common configuration field "queue_indirect_size"

 content.tex    | 44 ++++++++++++++++++++++++++++++++++++++++++++
 split-ring.tex |  5 ++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

-- 
2.20.1

[virtio-comment] Re: [PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Cornelia Huck]

On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This new feature flag allows to decouple the maximum amount of
> descriptors in indirect descriptor tables from the Queue Size.
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  content.tex    | 21 +++++++++++++++++++++
>  split-ring.tex |  2 +-
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/content.tex b/content.tex
> index 5d112af..0aa4842 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    transport specific.
>    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>  
> +  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the
> +  maximum amount of descriptors in indirect descriptor tables is independent
> +  from the Queue Size.
> +
> +  If this feature bit is not negotiated, then a driver MUST NOT create a

"MUST NOT" does only belong into normative sections... maybe you can
reword the last sentence in this paragraph a bit and move the "MUST NOT"
part into a normative section?

> +  descriptor chain longer than the Queue Size, which then also applies to
> +  indirect descriptor tables as in \ref{sec:Basic Facilities of a Virtio
> +  Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
> +  Descriptors}. Which means without this feature, the Queue Size limits
> +  both the maximum amount of slots in the vring, as well as the actual
> +  bulk data size being 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.
> +
>  \end{description}
>  
>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> diff --git a/split-ring.tex b/split-ring.tex
> index bfef62d..ae2aeb8 100644
> --- a/split-ring.tex
> +++ b/split-ring.tex
> @@ -269,7 +269,7 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
>  one table per descriptor).
>  
>  A driver MUST NOT create a descriptor chain longer than the Queue Size of
> -the device.
> +the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.

This might already be enough of a normative statement, though.

>  
>  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
>  in \field{flags}.


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

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

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

[virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Cornelia Huck]

On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> This new common 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    | 25 ++++++++++++++++++++++++-
>  split-ring.tex |  3 +++
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/content.tex b/content.tex
> index 0aa4842..e3cfcae 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          le64 queue_driver;              /* read-write */
>          le64 queue_device;              /* read-write */
>          le16 queue_notify_data;         /* read-only for driver */
> +        le32 queue_indirect_size;       /* read-write */

[Note for below: this "common" configuration structure layout is
actually PCI-specific, it is only common between the different device
types.]

>  };
>  \end{lstlisting}
>  
> @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          may benefit from providing another value, for example an internal virtqueue
>          identifier, or an internal offset related to the virtqueue number.
>          \end{note}
> +
> +\item[\field{queue_indirect_size}]
> +        This field is used to negotiate the maximum amount of descriptors per
> +        vring slot 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 supported number of descriptors per
> +        vring slot. 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}
> @@ -1003,6 +1014,12 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>  
>  The driver MUST NOT write a 0 to \field{queue_enable}.
>  
> +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.

Maybe

"If the driver's maximum number of descriptors per vring slot is lower
than the maximum value reported by the device, it SHOULD write that
number to \field{queue_indirect_size}."

?

> +
> +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
> @@ -6712,7 +6729,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    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.
> +  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 common configuration field "queue_indirect_size" MUST exist
> +  if VIRTIO_RING_F_INDIRECT_SIZE was negotiated to subsequently
> +  negotiate the actual amount of maximum indirect descriptors supported
> +  by both sides, as described in \ref{sec:Virtio Transport Options / Virtio
> +  Over PCI Bus / PCI Device Layout / Common configuration structure layout}.
>  
>  \end{description}
>  
> diff --git a/split-ring.tex b/split-ring.tex
> index ae2aeb8..d8f66c1 100644
> --- a/split-ring.tex
> +++ b/split-ring.tex
> @@ -270,6 +270,9 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic Facilities of a Virtio Devi
>  
>  A driver MUST NOT create a descriptor chain longer than the Queue Size of
>  the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
> +Furthermore if VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then the number
> +of descriptors per vring slot MUST NOT exceed the value negotiated by common
> +configuration field "queue_indirect_size".

As mentioned above, the "common configuration layout" is actually
PCI-specific; other transports will use different mechanism. Maybe it
would make sense to reword the whole paragraph and add it in patch 1?

"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 vring slot MUST NOT exceed the negotiated Queue Indirect
Size."

I also wonder whether we need a device normative statement as well,
something like:

"With VIRTIO_RING_F_INDIRECT_SIZE, the device MUST provide the maximum
Queue Indirect Size for the number of descriptors per vring slot. It
MUST allow the driver to set a lower value."

Maybe we also need to mention that the mechanism is transport specific?

Also, this is only for split ring; does packed ring need any updates?

>  
>  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
>  in \field{flags}.


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

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

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

[virtio-comment] Re: [PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Christian Schoenebeck]

On Dienstag, 14. Dezember 2021 17:59:51 CET Cornelia Huck wrote:
> On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > This new feature flag allows to decouple the maximum amount of
> > descriptors in indirect descriptor tables from the Queue Size.
> > 
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  content.tex    | 21 +++++++++++++++++++++
> >  split-ring.tex |  2 +-
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/content.tex b/content.tex
> > index 5d112af..0aa4842 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> > Feature Bits}> 
> >    transport specific.
> >    For more details about driver notifications over PCI see
> >    \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific
> >    Initialization And Device Operation / Available Buffer Notifications}.> 
> > +  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the
> > +  maximum amount of descriptors in indirect descriptor tables is
> > independent +  from the Queue Size.
> > +
> > +  If this feature bit is not negotiated, then a driver MUST NOT create a
> 
> "MUST NOT" does only belong into normative sections... maybe you can
> reword the last sentence in this paragraph a bit and move the "MUST NOT"
> part into a normative section?

I lack creativity in this case. Suggestions appreciated. On doubt I would just 
delete the entire paragraph here ("If this feature ... transmitted per vring 
slot.").

> > +  descriptor chain longer than the Queue Size, which then also applies to
> > +  indirect descriptor tables as in \ref{sec:Basic Facilities of a Virtio
> > +  Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
> > +  Descriptors}. Which means without this feature, the Queue Size limits
> > +  both the maximum amount of slots in the vring, as well as the actual
> > +  bulk data size being 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.
> > +
> > 
> >  \end{description}
> >  
> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > 
> > diff --git a/split-ring.tex b/split-ring.tex
> > index bfef62d..ae2aeb8 100644
> > --- a/split-ring.tex
> > +++ b/split-ring.tex
> > @@ -269,7 +269,7 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic
> > Facilities of a Virtio Devi> 
> >  one table per descriptor).
> >  
> >  A driver MUST NOT create a descriptor chain longer than the Queue Size of
> > 
> > -the device.
> > +the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
> 
> This might already be enough of a normative statement, though.
> 
> >  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
> >  in \field{flags}.



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

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

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

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
> On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > This new common 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    | 25 ++++++++++++++++++++++++-
> >  split-ring.tex |  3 +++
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/content.tex b/content.tex
> > index 0aa4842..e3cfcae 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport> 
> >          le64 queue_driver;              /* read-write */
> >          le64 queue_device;              /* read-write */
> >          le16 queue_notify_data;         /* read-only for driver */
> > 
> > +        le32 queue_indirect_size;       /* read-write */
> 
> [Note for below: this "common" configuration structure layout is
> actually PCI-specific, it is only common between the different device
> types.]

True

> >  };
> >  \end{lstlisting}
> > 
> > @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport> 
> >          may benefit from providing another value, for example an internal
> >          virtqueue
> >          identifier, or an internal offset related to the virtqueue
> >          number.
> >          \end{note}
> > 
> > +
> > +\item[\field{queue_indirect_size}]
> > +        This field is used to negotiate the maximum amount of descriptors
> > per +        vring slot 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 supported number of descriptors
> > per +        vring slot. 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}> 
> > @@ -1003,6 +1014,12 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport> 
> >  The driver MUST NOT write a 0 to \field{queue_enable}.
> > 
> > +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.
> Maybe
> 
> "If the driver's maximum number of descriptors per vring slot is lower
> than the maximum value reported by the device, it SHOULD write that
> number to \field{queue_indirect_size}."
> 
> ?

I actually just used Stefan's wording here and extended it with semantically
required components, i.e. "vring slot" and "\field{queue_indirect_size}":
https://lists.oasis-open.org/archives/virtio-comment/202112/msg00006.html

For me both versions are fine.

> > +
> > +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
> > 
> > @@ -6712,7 +6729,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> > Feature Bits}> 
> >    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.
> > +  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 common configuration field
> > "queue_indirect_size" MUST exist +  if VIRTIO_RING_F_INDIRECT_SIZE was
> > negotiated to subsequently
> > +  negotiate the actual amount of maximum indirect descriptors supported
> > +  by both sides, as described in \ref{sec:Virtio Transport Options /
> > Virtio +  Over PCI Bus / PCI Device Layout / Common configuration
> > structure layout}.> 
> >  \end{description}
> > 
> > diff --git a/split-ring.tex b/split-ring.tex
> > index ae2aeb8..d8f66c1 100644
> > --- a/split-ring.tex
> > +++ b/split-ring.tex
> > @@ -270,6 +270,9 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic
> > Facilities of a Virtio Devi> 
> >  A driver MUST NOT create a descriptor chain longer than the Queue Size of
> >  the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
> > 
> > +Furthermore if VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then the
> > number +of descriptors per vring slot MUST NOT exceed the value
> > negotiated by common +configuration field "queue_indirect_size".
> 
> As mentioned above, the "common configuration layout" is actually
> PCI-specific; other transports will use different mechanism. Maybe it
> would make sense to reword the whole paragraph and add it in patch 1?
> 
> "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 vring slot MUST NOT exceed the negotiated Queue Indirect
> Size."

That simplifies the statements, yes.

About the term "Queue Indirect Size": I understand your point about the field
being PCI specific, but for somebody who just reads the virtio spec for the
first time, how would you know what "Queue Indirect Size" means?

> I also wonder whether we need a device normative statement as well,
> something like:
> 
> "With VIRTIO_RING_F_INDIRECT_SIZE, the device MUST provide the maximum
> Queue Indirect Size for the number of descriptors per vring slot. It
> MUST allow the driver to set a lower value."

Makes sense.

> Maybe we also need to mention that the mechanism is transport specific?

I wouldn't do that. Instead this would probably be extended for other
transports in future appropriately.

> Also, this is only for split ring; does packed ring need any updates?

I have not reviewed the packed ring as much as I did the split ring, so I
could not say reliably all the parts that shall be updated for the packed
ring. There are some obvious parts like:

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

However the question is, would anybody want large descriptor chains with the
packaged ring in the first place? If I understand it correctly, the benefits
of the packed ring over the split ring only manifest for devices that
interchange a very large number of rather small bulk data (e.g. network
devices), no?

> >  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
> >  in \field{flags}.

Re: [virtio-comment] Re: [PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Cornelia Huck]

On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 14. Dezember 2021 17:59:51 CET Cornelia Huck wrote:
>> On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > This new feature flag allows to decouple the maximum amount of
>> > descriptors in indirect descriptor tables from the Queue Size.
>> > 
>> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
>> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> > ---
>> > 
>> >  content.tex    | 21 +++++++++++++++++++++
>> >  split-ring.tex |  2 +-
>> >  2 files changed, 22 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/content.tex b/content.tex
>> > index 5d112af..0aa4842 100644
>> > --- a/content.tex
>> > +++ b/content.tex
>> > @@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
>> > Feature Bits}> 
>> >    transport specific.
>> >    For more details about driver notifications over PCI see
>> >    \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific
>> >    Initialization And Device Operation / Available Buffer Notifications}.> 
>> > +  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the
>> > +  maximum amount of descriptors in indirect descriptor tables is
>> > independent +  from the Queue Size.
>> > +
>> > +  If this feature bit is not negotiated, then a driver MUST NOT create a
>> 
>> "MUST NOT" does only belong into normative sections... maybe you can
>> reword the last sentence in this paragraph a bit and move the "MUST NOT"
>> part into a normative section?
>
> I lack creativity in this case. Suggestions appreciated. On doubt I would just 
> delete the entire paragraph here ("If this feature ... transmitted per vring 
> slot.").

Maybe

"Without this feature bit, the queue size limits the length of the
descriptor chain, including indirect descriptor tables, i.e. both the
maximum amound of slots in the vring and the actual bulk data size
transmitted per vring slot."

>
>> > +  descriptor chain longer than the Queue Size, which then also applies to
>> > +  indirect descriptor tables as in \ref{sec:Basic Facilities of a Virtio
>> > +  Device / Virtqueues / The Virtqueue Descriptor Table / Indirect
>> > +  Descriptors}. Which means without this feature, the Queue Size limits
>> > +  both the maximum amount of slots in the vring, as well as the actual
>> > +  bulk data size being 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.

I think the second paragraph can stay like this.

>> > +
>> > 
>> >  \end{description}
>> >  
>> >  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
>> > 
>> > diff --git a/split-ring.tex b/split-ring.tex
>> > index bfef62d..ae2aeb8 100644
>> > --- a/split-ring.tex
>> > +++ b/split-ring.tex
>> > @@ -269,7 +269,7 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic
>> > Facilities of a Virtio Devi> 
>> >  one table per descriptor).
>> >  
>> >  A driver MUST NOT create a descriptor chain longer than the Queue Size of
>> > 
>> > -the device.
>> > +the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
>> 
>> This might already be enough of a normative statement, though.
>> 
>> >  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT
>> >  in \field{flags}.


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

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

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

[virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Cornelia Huck]

On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
>> On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > This new common 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    | 25 ++++++++++++++++++++++++-
>> >  split-ring.tex |  3 +++
>> >  2 files changed, 27 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/content.tex b/content.tex
>> > index 0aa4842..e3cfcae 100644
>> > --- a/content.tex
>> > +++ b/content.tex
>> > @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure
>> > layout}\label{sec:Virtio Transport> 
>> >          le64 queue_driver;              /* read-write */
>> >          le64 queue_device;              /* read-write */
>> >          le16 queue_notify_data;         /* read-only for driver */
>> > 
>> > +        le32 queue_indirect_size;       /* read-write */
>> 
>> [Note for below: this "common" configuration structure layout is
>> actually PCI-specific, it is only common between the different device
>> types.]
>
> True
>
>> >  };
>> >  \end{lstlisting}
>> > 
>> > @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure
>> > layout}\label{sec:Virtio Transport> 
>> >          may benefit from providing another value, for example an internal
>> >          virtqueue
>> >          identifier, or an internal offset related to the virtqueue
>> >          number.
>> >          \end{note}
>> > 
>> > +
>> > +\item[\field{queue_indirect_size}]
>> > +        This field is used to negotiate the maximum amount of descriptors
>> > per +        vring slot 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 supported number of descriptors
>> > per +        vring slot. 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}> 
>> > @@ -1003,6 +1014,12 @@ \subsubsection{Common configuration structure
>> > layout}\label{sec:Virtio Transport> 
>> >  The driver MUST NOT write a 0 to \field{queue_enable}.
>> > 
>> > +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.
>> Maybe
>> 
>> "If the driver's maximum number of descriptors per vring slot is lower
>> than the maximum value reported by the device, it SHOULD write that
>> number to \field{queue_indirect_size}."
>> 
>> ?
>
> I actually just used Stefan's wording here and extended it with semantically
> required components, i.e. "vring slot" and "\field{queue_indirect_size}":
> https://lists.oasis-open.org/archives/virtio-comment/202112/msg00006.html
>
> For me both versions are fine.

No strong opinion here, feel free to keep it like this if you prefer.

>> > diff --git a/split-ring.tex b/split-ring.tex
>> > index ae2aeb8..d8f66c1 100644
>> > --- a/split-ring.tex
>> > +++ b/split-ring.tex
>> > @@ -270,6 +270,9 @@ \subsubsection{Indirect Descriptors}\label{sec:Basic
>> > Facilities of a Virtio Devi> 
>> >  A driver MUST NOT create a descriptor chain longer than the Queue Size of
>> >  the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
>> > 
>> > +Furthermore if VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then the
>> > number +of descriptors per vring slot MUST NOT exceed the value
>> > negotiated by common +configuration field "queue_indirect_size".
>> 
>> As mentioned above, the "common configuration layout" is actually
>> PCI-specific; other transports will use different mechanism. Maybe it
>> would make sense to reword the whole paragraph and add it in patch 1?
>> 
>> "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 vring slot MUST NOT exceed the negotiated Queue Indirect
>> Size."
>
> That simplifies the statements, yes.
>
> About the term "Queue Indirect Size": I understand your point about the field
> being PCI specific, but for somebody who just reads the virtio spec for the
> first time, how would you know what "Queue Indirect Size" means?

If we follow my suggestion below, it is described in the device
normative statement. (Not sure if we actually do the same for Queue Size
already? That's a similar case.)

>
>> I also wonder whether we need a device normative statement as well,
>> something like:
>> 
>> "With VIRTIO_RING_F_INDIRECT_SIZE, the device MUST provide the maximum
>> Queue Indirect Size for the number of descriptors per vring slot. It
>> MUST allow the driver to set a lower value."
>
> Makes sense.
>
>> Maybe we also need to mention that the mechanism is transport specific?
>
> I wouldn't do that. Instead this would probably be extended for other
> transports in future appropriately.

The mechanics, not providing the value... something like

"The maximum Queue Indirect Size is provided by the device in a
transport-specific way."

(There should already be some precedence for other values.)

>
>> Also, this is only for split ring; does packed ring need any updates?
>
> I have not reviewed the packed ring as much as I did the split ring, so I
> could not say reliably all the parts that shall be updated for the packed
> ring. There are some obvious parts like:
>
> 2.7.5 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."
>
> However the question is, would anybody want large descriptor chains with the
> packaged ring in the first place? If I understand it correctly, the benefits
> of the packed ring over the split ring only manifest for devices that
> interchange a very large number of rather small bulk data (e.g. network
> devices), no?

If we think that the feature does not make sense for packed ring, they
should probably conflict with each other. Otherwise, I think we need at
least a statement that the higher limit does not take effect for packed
ring, or touch all the places where it would be relevant.

What do others think?


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

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

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

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
> >> On Tue, Dec 14 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> 
wrote:
> >> > This new common 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    | 25 ++++++++++++++++++++++++-
> >> >  split-ring.tex |  3 +++
> >> >  2 files changed, 27 insertions(+), 1 deletion(-)
> >> > 
> >> > diff --git a/content.tex b/content.tex
> >> > index 0aa4842..e3cfcae 100644
> >> > --- a/content.tex
> >> > +++ b/content.tex
> >> > @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure
> >> > layout}\label{sec:Virtio Transport>
> >> > 
> >> >          le64 queue_driver;              /* read-write */
> >> >          le64 queue_device;              /* read-write */
> >> >          le16 queue_notify_data;         /* read-only for driver */
> >> > 
> >> > +        le32 queue_indirect_size;       /* read-write */
> >> 
> >> [Note for below: this "common" configuration structure layout is
> >> actually PCI-specific, it is only common between the different device
> >> types.]
> > 
> > True
> > 
> >> >  };
> >> >  \end{lstlisting}
> >> > 
> >> > @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure
> >> > layout}\label{sec:Virtio Transport>
> >> > 
> >> >          may benefit from providing another value, for example an
> >> >          internal
> >> >          virtqueue
> >> >          identifier, or an internal offset related to the virtqueue
> >> >          number.
> >> >          \end{note}
> >> > 
> >> > +
> >> > +\item[\field{queue_indirect_size}]
> >> > +        This field is used to negotiate the maximum amount of
> >> > descriptors
> >> > per +        vring slot 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 supported number of
> >> > descriptors
> >> > per +        vring slot. 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}>
> >> > 
> >> > @@ -1003,6 +1014,12 @@ \subsubsection{Common configuration structure
> >> > layout}\label{sec:Virtio Transport>
> >> > 
> >> >  The driver MUST NOT write a 0 to \field{queue_enable}.
> >> > 
> >> > +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.
> >> 
> >> Maybe
> >> 
> >> "If the driver's maximum number of descriptors per vring slot is lower
> >> than the maximum value reported by the device, it SHOULD write that
> >> number to \field{queue_indirect_size}."
> >> 
> >> ?
> > 
> > I actually just used Stefan's wording here and extended it with
> > semantically required components, i.e. "vring slot" and
> > "\field{queue_indirect_size}":
> > https://lists.oasis-open.org/archives/virtio-comment/202112/msg00006.html
> > 
> > For me both versions are fine.
> 
> No strong opinion here, feel free to keep it like this if you prefer.
> 
> >> > diff --git a/split-ring.tex b/split-ring.tex
> >> > index ae2aeb8..d8f66c1 100644
> >> > --- a/split-ring.tex
> >> > +++ b/split-ring.tex
> >> > @@ -270,6 +270,9 @@ \subsubsection{Indirect
> >> > Descriptors}\label{sec:Basic
> >> > Facilities of a Virtio Devi>
> >> > 
> >> >  A driver MUST NOT create a descriptor chain longer than the Queue Size
> >> >  of
> >> >  the device unless VIRTIO_RING_F_INDIRECT_SIZE has been negotiated.
> >> > 
> >> > +Furthermore if VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then
> >> > the
> >> > number +of descriptors per vring slot MUST NOT exceed the value
> >> > negotiated by common +configuration field "queue_indirect_size".
> >> 
> >> As mentioned above, the "common configuration layout" is actually
> >> PCI-specific; other transports will use different mechanism. Maybe it
> >> would make sense to reword the whole paragraph and add it in patch 1?
> >> 
> >> "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 vring slot MUST NOT exceed the negotiated Queue Indirect
> >> Size."
> > 
> > That simplifies the statements, yes.
> > 
> > About the term "Queue Indirect Size": I understand your point about the
> > field being PCI specific, but for somebody who just reads the virtio spec
> > for the first time, how would you know what "Queue Indirect Size" means?
> 
> If we follow my suggestion below, it is described in the device
> normative statement. (Not sure if we actually do the same for Queue Size
> already? That's a similar case.)
> 
> >> I also wonder whether we need a device normative statement as well,
> >> something like:
> >> 
> >> "With VIRTIO_RING_F_INDIRECT_SIZE, the device MUST provide the maximum
> >> Queue Indirect Size for the number of descriptors per vring slot. It
> >> MUST allow the driver to set a lower value."
> > 
> > Makes sense.
> > 
> >> Maybe we also need to mention that the mechanism is transport specific?
> > 
> > I wouldn't do that. Instead this would probably be extended for other
> > transports in future appropriately.
> 
> The mechanics, not providing the value... something like
> 
> "The maximum Queue Indirect Size is provided by the device in a
> transport-specific way."
> 
> (There should already be some precedence for other values.)
> 
> >> Also, this is only for split ring; does packed ring need any updates?
> > 
> > I have not reviewed the packed ring as much as I did the split ring, so I
> > could not say reliably all the parts that shall be updated for the packed
> > ring. There are some obvious parts like:
> > 
> > 2.7.5 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."
> > 
> > However the question is, would anybody want large descriptor chains with
> > the packaged ring in the first place? If I understand it correctly, the
> > benefits of the packed ring over the split ring only manifest for devices
> > that interchange a very large number of rather small bulk data (e.g.
> > network devices), no?
> 
> If we think that the feature does not make sense for packed ring, they
> should probably conflict with each other. Otherwise, I think we need at
> least a statement that the higher limit does not take effect for packed
> ring, or touch all the places where it would be relevant.
> 
> What do others think?

It would indeed be very useful if other people express their opinion about 
this issue (packed ring scenario) as well before I continue on this issue.

Probably the fact that my patches never made it through to the list were not 
necessarily supporting this. Should I contact somebody regarding this ML 
issue? Do members of the other ML also read this virtio-comment list?

I tried to compensate the current situation by updating the corresponding 
issue description on Github in a very defailed and verbose way:
https://github.com/oasis-tcs/virtio-spec/issues/122

If nobody replies early January, I would suggest to continue by ignoring the 
packed ring. Because if somebody wants this for packed ring in future, this 
can still be added to the specs without breaking things, because this feature 
is negotiated per queue, not for the entire device.

Best regards,
Christian Schoenebeck

[virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Cornelia Huck]

On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
>> On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
>> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
>> >> Also, this is only for split ring; does packed ring need any updates?
>> > 
>> > I have not reviewed the packed ring as much as I did the split ring, so I
>> > could not say reliably all the parts that shall be updated for the packed
>> > ring. There are some obvious parts like:
>> > 
>> > 2.7.5 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."
>> > 
>> > However the question is, would anybody want large descriptor chains with
>> > the packaged ring in the first place? If I understand it correctly, the
>> > benefits of the packed ring over the split ring only manifest for devices
>> > that interchange a very large number of rather small bulk data (e.g.
>> > network devices), no?
>> 
>> If we think that the feature does not make sense for packed ring, they
>> should probably conflict with each other. Otherwise, I think we need at
>> least a statement that the higher limit does not take effect for packed
>> ring, or touch all the places where it would be relevant.
>> 
>> What do others think?
>
> It would indeed be very useful if other people express their opinion about 
> this issue (packed ring scenario) as well before I continue on this issue.
>
> Probably the fact that my patches never made it through to the list were not 
> necessarily supporting this. Should I contact somebody regarding this ML 
> issue? Do members of the other ML also read this virtio-comment list?

Yes, this situation is very unsatisfactory :( (I have contacted the
people running this list, but there have not yet been any fixes...)

Not sure which other lists would be appropriate to cc: -- maybe
virtualization@lists.linux-foundation.org, but that one also suffers
from DKIM issues :(

>
> I tried to compensate the current situation by updating the corresponding 
> issue description on Github in a very defailed and verbose way:
> https://github.com/oasis-tcs/virtio-spec/issues/122

Thanks. Hopefully me quoting this makes it more visible (I tried to
quote more than I usually would in my other replies already...)

Just to feature it more prominently for people who collapse quotes:

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

>
> If nobody replies early January, I would suggest to continue by ignoring the 
> packed ring. Because if somebody wants this for packed ring in future, this 
> can still be added to the specs without breaking things, because this feature 
> is negotiated per queue, not for the entire device.

The problem is that we need to specify what is supposed to happen if
packed ring *and* this feature are negotiated. If we do not want to add
statements for the packed ring case, my suggestion would be
- make packed ring and this feature mutually exclusive
- add a new feature bit that works with packed ring later, if we think
  it is useful


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

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

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

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> >> On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> 
wrote:
> >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
> >> >> Also, this is only for split ring; does packed ring need any updates?
> >> > 
> >> > I have not reviewed the packed ring as much as I did the split ring, so
> >> > I
> >> > could not say reliably all the parts that shall be updated for the
> >> > packed
> >> > ring. There are some obvious parts like:
> >> > 
> >> > 2.7.5 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."
> >> > 
> >> > However the question is, would anybody want large descriptor chains
> >> > with
> >> > the packaged ring in the first place? If I understand it correctly, the
> >> > benefits of the packed ring over the split ring only manifest for
> >> > devices
> >> > that interchange a very large number of rather small bulk data (e.g.
> >> > network devices), no?
> >> 
> >> If we think that the feature does not make sense for packed ring, they
> >> should probably conflict with each other. Otherwise, I think we need at
> >> least a statement that the higher limit does not take effect for packed
> >> ring, or touch all the places where it would be relevant.
> >> 
> >> What do others think?
> > 
> > It would indeed be very useful if other people express their opinion about
> > this issue (packed ring scenario) as well before I continue on this issue.
> > 
> > Probably the fact that my patches never made it through to the list were
> > not necessarily supporting this. Should I contact somebody regarding this
> > ML issue? Do members of the other ML also read this virtio-comment list?
> Yes, this situation is very unsatisfactory :( (I have contacted the
> people running this list, but there have not yet been any fixes...)

Only my emails with patches are refused by the list. All my other emails are 
accepted. So not sure if the cause is really DKIM or something else. Maybe the 
admins can suggest a workaround for me.

> Not sure which other lists would be appropriate to cc: -- maybe
> virtualization@lists.linux-foundation.org, but that one also suffers
> from DKIM issues :(

What I thought was wether subscribers of virtio-dev would typically read 
virtio-comment as well. Because AFAICS people who more frequently deal with 
virtio for their companies rather seem to post to virtio-dev.

> > I tried to compensate the current situation by updating the corresponding
> > issue description on Github in a very defailed and verbose way:
> > https://github.com/oasis-tcs/virtio-spec/issues/122
> 
> Thanks. Hopefully me quoting this makes it more visible (I tried to
> quote more than I usually would in my other replies already...)
> 
> Just to feature it more prominently for people who collapse quotes:
> 
> https://github.com/oasis-tcs/virtio-spec/issues/122
> 
> > If nobody replies early January, I would suggest to continue by ignoring
> > the packed ring. Because if somebody wants this for packed ring in
> > future, this can still be added to the specs without breaking things,
> > because this feature is negotiated per queue, not for the entire device.
> 
> The problem is that we need to specify what is supposed to happen if
> packed ring *and* this feature are negotiated. If we do not want to add
> statements for the packed ring case, my suggestion would be
> - make packed ring and this feature mutually exclusive
> - add a new feature bit that works with packed ring later, if we think
>   it is useful

Mja, I have to correct myself on that: I wrote in my previous email that this 
was negotiated per queue. That's false of course as all virtio features are 
currently negotiated for the entire device.

So you are right, if this new indirect size feature was negotiated then it 
would apply to both a) split rings and b) packed rings a device might have. 
Which is unfortunate.

Stefan, you are aware about this circumstance as well, right? Because I 
remember we originally had a discussion on qemu-devel where you wanted to have 
this configurable per queue, not per device.

Best regards,
Christian Schoenebeck

Re: [PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Stefan Hajnoczi]

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

On Tue, Dec 14, 2021 at 04:13:10PM +0100, Christian Schoenebeck wrote:
> This new feature flag allows to decouple the maximum amount of
> descriptors in indirect descriptor tables from the Queue Size.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  content.tex    | 21 +++++++++++++++++++++
>  split-ring.tex |  2 +-
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 5d112af..0aa4842 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    transport specific.
>    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>  
> +  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the

Another feature bit was added in the meantime:

  \item[VIRTIO_F_RING_RESET(40)] This feature indicates
                            ^^

Please use 41 and update the text that says:

  \item[24 to 40] 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.

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

[virtio-comment] Re: [PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Cornelia Huck]

On Mon, Jan 24 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:

> On Tue, Dec 14, 2021 at 04:13:10PM +0100, Christian Schoenebeck wrote:
>> This new feature flag allows to decouple the maximum amount of
>> descriptors in indirect descriptor tables from the Queue Size.
>> 
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
>> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
>> ---
>>  content.tex    | 21 +++++++++++++++++++++
>>  split-ring.tex |  2 +-
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>> 
>> diff --git a/content.tex b/content.tex
>> index 5d112af..0aa4842 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>    transport specific.
>>    For more details about driver notifications over PCI see \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Available Buffer Notifications}.
>>  
>> +  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the
>
> Another feature bit was added in the meantime:
>
>   \item[VIRTIO_F_RING_RESET(40)] This feature indicates
>                             ^^
>
> Please use 41 and update the text that says:
>
>   \item[24 to 40] 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.

Indeed... I'm wondering whether we could automate updating this with
some clever variables?


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

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

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

[virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Stefan Hajnoczi]

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

On Wed, Jan 05, 2022 at 02:52:46PM +0100, Christian Schoenebeck wrote:
> On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> > On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> > >> On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> 
> wrote:
> > >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
> > >> >> Also, this is only for split ring; does packed ring need any updates?
> > >> > 
> > >> > I have not reviewed the packed ring as much as I did the split ring, so
> > >> > I
> > >> > could not say reliably all the parts that shall be updated for the
> > >> > packed
> > >> > ring. There are some obvious parts like:
> > >> > 
> > >> > 2.7.5 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."
> > >> > 
> > >> > However the question is, would anybody want large descriptor chains
> > >> > with
> > >> > the packaged ring in the first place? If I understand it correctly, the
> > >> > benefits of the packed ring over the split ring only manifest for
> > >> > devices
> > >> > that interchange a very large number of rather small bulk data (e.g.
> > >> > network devices), no?
> > >> 
> > >> If we think that the feature does not make sense for packed ring, they
> > >> should probably conflict with each other. Otherwise, I think we need at
> > >> least a statement that the higher limit does not take effect for packed
> > >> ring, or touch all the places where it would be relevant.
> > >> 
> > >> What do others think?
> > > 
> > > It would indeed be very useful if other people express their opinion about
> > > this issue (packed ring scenario) as well before I continue on this issue.
> > > 
> > > Probably the fact that my patches never made it through to the list were
> > > not necessarily supporting this. Should I contact somebody regarding this
> > > ML issue? Do members of the other ML also read this virtio-comment list?
> > Yes, this situation is very unsatisfactory :( (I have contacted the
> > people running this list, but there have not yet been any fixes...)
> 
> Only my emails with patches are refused by the list. All my other emails are 
> accepted. So not sure if the cause is really DKIM or something else. Maybe the 
> admins can suggest a workaround for me.
> 
> > Not sure which other lists would be appropriate to cc: -- maybe
> > virtualization@lists.linux-foundation.org, but that one also suffers
> > from DKIM issues :(
> 
> What I thought was wether subscribers of virtio-dev would typically read 
> virtio-comment as well. Because AFAICS people who more frequently deal with 
> virtio for their companies rather seem to post to virtio-dev.
> 
> > > I tried to compensate the current situation by updating the corresponding
> > > issue description on Github in a very defailed and verbose way:
> > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > 
> > Thanks. Hopefully me quoting this makes it more visible (I tried to
> > quote more than I usually would in my other replies already...)
> > 
> > Just to feature it more prominently for people who collapse quotes:
> > 
> > https://github.com/oasis-tcs/virtio-spec/issues/122
> > 
> > > If nobody replies early January, I would suggest to continue by ignoring
> > > the packed ring. Because if somebody wants this for packed ring in
> > > future, this can still be added to the specs without breaking things,
> > > because this feature is negotiated per queue, not for the entire device.
> > 
> > The problem is that we need to specify what is supposed to happen if
> > packed ring *and* this feature are negotiated. If we do not want to add
> > statements for the packed ring case, my suggestion would be
> > - make packed ring and this feature mutually exclusive
> > - add a new feature bit that works with packed ring later, if we think
> >   it is useful
> 
> Mja, I have to correct myself on that: I wrote in my previous email that this 
> was negotiated per queue. That's false of course as all virtio features are 
> currently negotiated for the entire device.
> 
> So you are right, if this new indirect size feature was negotiated then it 
> would apply to both a) split rings and b) packed rings a device might have. 
> Which is unfortunate.
>
> Stefan, you are aware about this circumstance as well, right? Because I 
> remember we originally had a discussion on qemu-devel where you wanted to have 
> this configurable per queue, not per device.

Regarding packed virtqueues, there are multiple reasons for using them.
Recently someone asked about reducing complexity in VIRTIO
implementations and one of the suggestions that Michael Tsirkin and I
both made independently was to use the packed layout instead of split
layout. I don't think we should assume packed virtqueues are only used
in scenarios that don't need INDIRECT_SIZE.

Extending INDIRECT_SIZE to packed virtqueues looks straightforward and
does not require many changes to the Packed Virtqueues section.

The reason I pushed for per-virtqueue Indirect Size values is because
multi-virtqueue devices have specific purposes for each virtqueue. The
Queue Size field is per-virtqueue already across all transports. This
way a device with control virtqueue can allocate fewer resources
(smaller Queue Size and Indirect Size) to it than to the data path
virtqueues that carry I/O data buffers.

I don't think it's necessary to support INDIRECT_SIZE on only a subset
of a device's virtqueues. If the device doesn't want to "enable"
INDIRECT_SIZE on a specific virtqueue it should just report Queue Size
as the Indirect Size value?

Stefan

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

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Stefan Hajnoczi]

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

On Tue, Dec 14, 2021 at 04:13:17PM +0100, Christian Schoenebeck wrote:
> This new common 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    | 25 ++++++++++++++++++++++++-
>  split-ring.tex |  3 +++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 0aa4842..e3cfcae 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          le64 queue_driver;              /* read-write */
>          le64 queue_device;              /* read-write */
>          le16 queue_notify_data;         /* read-only for driver */
> +        le32 queue_indirect_size;       /* read-write */

I see no other unaligned fields in this struct, so I think a le16
padding field is needed for safety:

           le64 queue_device;              /* read-write */
           le16 queue_notify_data;         /* read-only for driver */
  +        le16 reserved;                  /* no access */
  +        le32 queue_indirect_size;       /* read-write */

>  };
>  \end{lstlisting}
>  
> @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
>          may benefit from providing another value, for example an internal virtqueue
>          identifier, or an internal offset related to the virtqueue number.
>          \end{note}
> +
> +\item[\field{queue_indirect_size}]
> +        This field is used to negotiate the maximum amount of descriptors per
> +        vring slot 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 supported number of descriptors per
> +        vring slot. If the driver requires fewer descriptors, it writes its
> +        lower value to inform the device of the reduced resource requirements.

"vring slot" is a little vague, it means "indirect descriptor
table"? The two paragraphs could use "maximum number of descriptors per
indirect descriptor table" instead of referring to vring slots to imply
indirect descriptor tables.

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

Re: [PATCH 0/2] Add VIRTIO_RING_F_INDIRECT_SIZE and queue_indirect_size

[Stefan Hajnoczi]

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

On Tue, Dec 14, 2021 at 04:13:23PM +0100, Christian Schoenebeck wrote:
> This is a follow-up of:
> [0/2]: https://lists.oasis-open.org/archives/virtio-comment/202111/msg00089.html
> [1/2]: https://lists.oasis-open.org/archives/virtio-comment/202112/msg00005.html
> [2/2]: https://lists.oasis-open.org/archives/virtio-comment/202112/msg00006.html
> 
> These two patches introduce a more fine graded control over the maximum length
> of indirect descriptor tables.

Sorry for the long delay, I am very behind on reviews!

Stefan

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

Re: [PATCH 1/2] Add VIRTIO_RING_F_INDIRECT_SIZE

[Christian Schoenebeck]

On Montag, 24. Januar 2022 14:14:07 CET Cornelia Huck wrote:
> On Mon, Jan 24 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Tue, Dec 14, 2021 at 04:13:10PM +0100, Christian Schoenebeck wrote:
> >> This new feature flag allows to decouple the maximum amount of
> >> descriptors in indirect descriptor tables from the Queue Size.
> >> 
> >> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> >> ---

Adding Dominique Martinet on CC from now on, for being informed about progress 
on this virtio issue for Linux kernel side.

> >> 
> >>  content.tex    | 21 +++++++++++++++++++++
> >>  split-ring.tex |  2 +-
> >>  2 files changed, 22 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/content.tex b/content.tex
> >> index 5d112af..0aa4842 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -6693,6 +6693,27 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved
> >> Feature Bits}>> 
> >>    transport specific.
> >>    For more details about driver notifications over PCI see
> >>    \ref{sec:Virtio Transport Options / Virtio Over PCI Bus /
> >>    PCI-specific Initialization And Device Operation / Available Buffer
> >>    Notifications}.>> 
> >> +  \item[VIRTIO_RING_F_INDIRECT_SIZE(40)] This feature indicates that the
> > 
> > Another feature bit was added in the meantime:
> >   \item[VIRTIO_F_RING_RESET(40)] This feature indicates
> >   
> >                             ^^
> > 
> > Please use 41 and update the text that says:
> >   \item[24 to 40] 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.

Sure, I'll take care of that. As I am always rebasing to latest git version of 
the spec, I guess I would have realized it anyway.

> Indeed... I'm wondering whether we could automate updating this with
> some clever variables?

Well, for LaTeX there is

	\newcommand{\newCommandName}{1234}

and

	\def\variable{1234}

Both solutions have pros and cons. But not sure I would want to rack my brain 
on that. I just manually update the feature bit number for now :)

Best regards,
Christian Schoenebeck

Re: [virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Montag, 24. Januar 2022 14:39:28 CET Stefan Hajnoczi wrote:
> On Wed, Jan 05, 2022 at 02:52:46PM +0100, Christian Schoenebeck wrote:
> > On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> > > On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> 
wrote:
> > > > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> > > >> On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com>
> > 
> > wrote:
> > > >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
> > > >> >> Also, this is only for split ring; does packed ring need any
> > > >> >> updates?
> > > >> > 
> > > >> > I have not reviewed the packed ring as much as I did the split
> > > >> > ring, so
> > > >> > I
> > > >> > could not say reliably all the parts that shall be updated for the
> > > >> > packed
> > > >> > ring. There are some obvious parts like:
> > > >> > 
> > > >> > 2.7.5 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."
> > > >> > 
> > > >> > However the question is, would anybody want large descriptor chains
> > > >> > with
> > > >> > the packaged ring in the first place? If I understand it correctly,
> > > >> > the
> > > >> > benefits of the packed ring over the split ring only manifest for
> > > >> > devices
> > > >> > that interchange a very large number of rather small bulk data
> > > >> > (e.g.
> > > >> > network devices), no?
> > > >> 
> > > >> If we think that the feature does not make sense for packed ring,
> > > >> they
> > > >> should probably conflict with each other. Otherwise, I think we need
> > > >> at
> > > >> least a statement that the higher limit does not take effect for
> > > >> packed
> > > >> ring, or touch all the places where it would be relevant.
> > > >> 
> > > >> What do others think?
> > > > 
> > > > It would indeed be very useful if other people express their opinion
> > > > about
> > > > this issue (packed ring scenario) as well before I continue on this
> > > > issue.
> > > > 
> > > > Probably the fact that my patches never made it through to the list
> > > > were
> > > > not necessarily supporting this. Should I contact somebody regarding
> > > > this
> > > > ML issue? Do members of the other ML also read this virtio-comment
> > > > list?
> > > 
> > > Yes, this situation is very unsatisfactory :( (I have contacted the
> > > people running this list, but there have not yet been any fixes...)
> > 
> > Only my emails with patches are refused by the list. All my other emails
> > are accepted. So not sure if the cause is really DKIM or something else.
> > Maybe the admins can suggest a workaround for me.
> > 
> > > Not sure which other lists would be appropriate to cc: -- maybe
> > > virtualization@lists.linux-foundation.org, but that one also suffers
> > > from DKIM issues :(
> > 
> > What I thought was wether subscribers of virtio-dev would typically read
> > virtio-comment as well. Because AFAICS people who more frequently deal
> > with
> > virtio for their companies rather seem to post to virtio-dev.
> > 
> > > > I tried to compensate the current situation by updating the
> > > > corresponding
> > > > issue description on Github in a very defailed and verbose way:
> > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > 
> > > Thanks. Hopefully me quoting this makes it more visible (I tried to
> > > quote more than I usually would in my other replies already...)
> > > 
> > > Just to feature it more prominently for people who collapse quotes:
> > > 
> > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > 
> > > > If nobody replies early January, I would suggest to continue by
> > > > ignoring
> > > > the packed ring. Because if somebody wants this for packed ring in
> > > > future, this can still be added to the specs without breaking things,
> > > > because this feature is negotiated per queue, not for the entire
> > > > device.
> > > 
> > > The problem is that we need to specify what is supposed to happen if
> > > packed ring *and* this feature are negotiated. If we do not want to add
> > > statements for the packed ring case, my suggestion would be
> > > - make packed ring and this feature mutually exclusive
> > > - add a new feature bit that works with packed ring later, if we think
> > > 
> > >   it is useful
> > 
> > Mja, I have to correct myself on that: I wrote in my previous email that
> > this was negotiated per queue. That's false of course as all virtio
> > features are currently negotiated for the entire device.
> > 
> > So you are right, if this new indirect size feature was negotiated then it
> > would apply to both a) split rings and b) packed rings a device might
> > have.
> > Which is unfortunate.
> > 
> > Stefan, you are aware about this circumstance as well, right? Because I
> > remember we originally had a discussion on qemu-devel where you wanted to
> > have this configurable per queue, not per device.
> 
> Regarding packed virtqueues, there are multiple reasons for using them.
> Recently someone asked about reducing complexity in VIRTIO
> implementations and one of the suggestions that Michael Tsirkin and I
> both made independently was to use the packed layout instead of split
> layout. I don't think we should assume packed virtqueues are only used
> in scenarios that don't need INDIRECT_SIZE.
> 
> Extending INDIRECT_SIZE to packed virtqueues looks straightforward and
> does not require many changes to the Packed Virtqueues section.

Yes I agree, it does make sense to apply this new INDIRECT_SIZE feature to 
packed queues as well.

> The reason I pushed for per-virtqueue Indirect Size values is because
> multi-virtqueue devices have specific purposes for each virtqueue. The
> Queue Size field is per-virtqueue already across all transports. This
> way a device with control virtqueue can allocate fewer resources
> (smaller Queue Size and Indirect Size) to it than to the data path
> virtqueues that carry I/O data buffers.
> 
> I don't think it's necessary to support INDIRECT_SIZE on only a subset
> of a device's virtqueues. If the device doesn't want to "enable"
> INDIRECT_SIZE on a specific virtqueue it should just report Queue Size
> as the Indirect Size value?

Or another idea: what about adding this new indirect size field to the struct 
that holds the queue size field already, then this would become transport-
independent (i.e. the discussed PCI config field would be unnecessary), and it 
would be a feature configurable per queue. Or is that struct inappropriate for 
feature negotiation purposes?

Best regards,
Christian Schoenebeck

Re: [virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Stefan Hajnoczi]

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

On Mon, Jan 24, 2022 at 03:31:57PM +0100, Christian Schoenebeck wrote:
> On Montag, 24. Januar 2022 14:39:28 CET Stefan Hajnoczi wrote:
> > On Wed, Jan 05, 2022 at 02:52:46PM +0100, Christian Schoenebeck wrote:
> > > On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> > > > On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com> 
> wrote:
> > > > > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> > > > >> On Wed, Dec 15 2021, Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > 
> > > wrote:
> > > > >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck wrote:
> > > > >> >> Also, this is only for split ring; does packed ring need any
> > > > >> >> updates?
> > > > >> > 
> > > > >> > I have not reviewed the packed ring as much as I did the split
> > > > >> > ring, so
> > > > >> > I
> > > > >> > could not say reliably all the parts that shall be updated for the
> > > > >> > packed
> > > > >> > ring. There are some obvious parts like:
> > > > >> > 
> > > > >> > 2.7.5 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."
> > > > >> > 
> > > > >> > However the question is, would anybody want large descriptor chains
> > > > >> > with
> > > > >> > the packaged ring in the first place? If I understand it correctly,
> > > > >> > the
> > > > >> > benefits of the packed ring over the split ring only manifest for
> > > > >> > devices
> > > > >> > that interchange a very large number of rather small bulk data
> > > > >> > (e.g.
> > > > >> > network devices), no?
> > > > >> 
> > > > >> If we think that the feature does not make sense for packed ring,
> > > > >> they
> > > > >> should probably conflict with each other. Otherwise, I think we need
> > > > >> at
> > > > >> least a statement that the higher limit does not take effect for
> > > > >> packed
> > > > >> ring, or touch all the places where it would be relevant.
> > > > >> 
> > > > >> What do others think?
> > > > > 
> > > > > It would indeed be very useful if other people express their opinion
> > > > > about
> > > > > this issue (packed ring scenario) as well before I continue on this
> > > > > issue.
> > > > > 
> > > > > Probably the fact that my patches never made it through to the list
> > > > > were
> > > > > not necessarily supporting this. Should I contact somebody regarding
> > > > > this
> > > > > ML issue? Do members of the other ML also read this virtio-comment
> > > > > list?
> > > > 
> > > > Yes, this situation is very unsatisfactory :( (I have contacted the
> > > > people running this list, but there have not yet been any fixes...)
> > > 
> > > Only my emails with patches are refused by the list. All my other emails
> > > are accepted. So not sure if the cause is really DKIM or something else.
> > > Maybe the admins can suggest a workaround for me.
> > > 
> > > > Not sure which other lists would be appropriate to cc: -- maybe
> > > > virtualization@lists.linux-foundation.org, but that one also suffers
> > > > from DKIM issues :(
> > > 
> > > What I thought was wether subscribers of virtio-dev would typically read
> > > virtio-comment as well. Because AFAICS people who more frequently deal
> > > with
> > > virtio for their companies rather seem to post to virtio-dev.
> > > 
> > > > > I tried to compensate the current situation by updating the
> > > > > corresponding
> > > > > issue description on Github in a very defailed and verbose way:
> > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > 
> > > > Thanks. Hopefully me quoting this makes it more visible (I tried to
> > > > quote more than I usually would in my other replies already...)
> > > > 
> > > > Just to feature it more prominently for people who collapse quotes:
> > > > 
> > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > 
> > > > > If nobody replies early January, I would suggest to continue by
> > > > > ignoring
> > > > > the packed ring. Because if somebody wants this for packed ring in
> > > > > future, this can still be added to the specs without breaking things,
> > > > > because this feature is negotiated per queue, not for the entire
> > > > > device.
> > > > 
> > > > The problem is that we need to specify what is supposed to happen if
> > > > packed ring *and* this feature are negotiated. If we do not want to add
> > > > statements for the packed ring case, my suggestion would be
> > > > - make packed ring and this feature mutually exclusive
> > > > - add a new feature bit that works with packed ring later, if we think
> > > > 
> > > >   it is useful
> > > 
> > > Mja, I have to correct myself on that: I wrote in my previous email that
> > > this was negotiated per queue. That's false of course as all virtio
> > > features are currently negotiated for the entire device.
> > > 
> > > So you are right, if this new indirect size feature was negotiated then it
> > > would apply to both a) split rings and b) packed rings a device might
> > > have.
> > > Which is unfortunate.
> > > 
> > > Stefan, you are aware about this circumstance as well, right? Because I
> > > remember we originally had a discussion on qemu-devel where you wanted to
> > > have this configurable per queue, not per device.
> > 
> > Regarding packed virtqueues, there are multiple reasons for using them.
> > Recently someone asked about reducing complexity in VIRTIO
> > implementations and one of the suggestions that Michael Tsirkin and I
> > both made independently was to use the packed layout instead of split
> > layout. I don't think we should assume packed virtqueues are only used
> > in scenarios that don't need INDIRECT_SIZE.
> > 
> > Extending INDIRECT_SIZE to packed virtqueues looks straightforward and
> > does not require many changes to the Packed Virtqueues section.
> 
> Yes I agree, it does make sense to apply this new INDIRECT_SIZE feature to 
> packed queues as well.
> 
> > The reason I pushed for per-virtqueue Indirect Size values is because
> > multi-virtqueue devices have specific purposes for each virtqueue. The
> > Queue Size field is per-virtqueue already across all transports. This
> > way a device with control virtqueue can allocate fewer resources
> > (smaller Queue Size and Indirect Size) to it than to the data path
> > virtqueues that carry I/O data buffers.
> > 
> > I don't think it's necessary to support INDIRECT_SIZE on only a subset
> > of a device's virtqueues. If the device doesn't want to "enable"
> > INDIRECT_SIZE on a specific virtqueue it should just report Queue Size
> > as the Indirect Size value?
> 
> Or another idea: what about adding this new indirect size field to the struct 
> that holds the queue size field already, then this would become transport-
> independent (i.e. the discussed PCI config field would be unnecessary), and it 
> would be a feature configurable per queue. Or is that struct inappropriate for 
> feature negotiation purposes?

There is no transport-independent struct that contains Queue Size. It is
handled by struct virtio_pci_common_cfg->queue_size, the MMIO QueueNum
register, and the CCW struct vq_config_block->max_num and struct
vq_info_block->num fields.

Stefan

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

Re: [virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Montag, 24. Januar 2022 17:41:09 CET Stefan Hajnoczi wrote:
> On Mon, Jan 24, 2022 at 03:31:57PM +0100, Christian Schoenebeck wrote:
> > On Montag, 24. Januar 2022 14:39:28 CET Stefan Hajnoczi wrote:
> > > On Wed, Jan 05, 2022 at 02:52:46PM +0100, Christian Schoenebeck wrote:
> > > > On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> > > > > On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com>
> > 
> > wrote:
> > > > > > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> > > > > >> On Wed, Dec 15 2021, Christian Schoenebeck
> > > > > >> <qemu_oss@crudebyte.com>
> > > > 
> > > > wrote:
> > > > > >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck 
wrote:
> > > > > >> >> Also, this is only for split ring; does packed ring need any
> > > > > >> >> updates?
> > > > > >> > 
> > > > > >> > I have not reviewed the packed ring as much as I did the split
> > > > > >> > ring, so
> > > > > >> > I
> > > > > >> > could not say reliably all the parts that shall be updated for
> > > > > >> > the
> > > > > >> > packed
> > > > > >> > ring. There are some obvious parts like:
> > > > > >> > 
> > > > > >> > 2.7.5 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."
> > > > > >> > 
> > > > > >> > However the question is, would anybody want large descriptor
> > > > > >> > chains
> > > > > >> > with
> > > > > >> > the packaged ring in the first place? If I understand it
> > > > > >> > correctly,
> > > > > >> > the
> > > > > >> > benefits of the packed ring over the split ring only manifest
> > > > > >> > for
> > > > > >> > devices
> > > > > >> > that interchange a very large number of rather small bulk data
> > > > > >> > (e.g.
> > > > > >> > network devices), no?
> > > > > >> 
> > > > > >> If we think that the feature does not make sense for packed ring,
> > > > > >> they
> > > > > >> should probably conflict with each other. Otherwise, I think we
> > > > > >> need
> > > > > >> at
> > > > > >> least a statement that the higher limit does not take effect for
> > > > > >> packed
> > > > > >> ring, or touch all the places where it would be relevant.
> > > > > >> 
> > > > > >> What do others think?
> > > > > > 
> > > > > > It would indeed be very useful if other people express their
> > > > > > opinion
> > > > > > about
> > > > > > this issue (packed ring scenario) as well before I continue on
> > > > > > this
> > > > > > issue.
> > > > > > 
> > > > > > Probably the fact that my patches never made it through to the
> > > > > > list
> > > > > > were
> > > > > > not necessarily supporting this. Should I contact somebody
> > > > > > regarding
> > > > > > this
> > > > > > ML issue? Do members of the other ML also read this virtio-comment
> > > > > > list?
> > > > > 
> > > > > Yes, this situation is very unsatisfactory :( (I have contacted the
> > > > > people running this list, but there have not yet been any fixes...)
> > > > 
> > > > Only my emails with patches are refused by the list. All my other
> > > > emails
> > > > are accepted. So not sure if the cause is really DKIM or something
> > > > else.
> > > > Maybe the admins can suggest a workaround for me.
> > > > 
> > > > > Not sure which other lists would be appropriate to cc: -- maybe
> > > > > virtualization@lists.linux-foundation.org, but that one also suffers
> > > > > from DKIM issues :(
> > > > 
> > > > What I thought was wether subscribers of virtio-dev would typically
> > > > read
> > > > virtio-comment as well. Because AFAICS people who more frequently deal
> > > > with
> > > > virtio for their companies rather seem to post to virtio-dev.
> > > > 
> > > > > > I tried to compensate the current situation by updating the
> > > > > > corresponding
> > > > > > issue description on Github in a very defailed and verbose way:
> > > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > 
> > > > > Thanks. Hopefully me quoting this makes it more visible (I tried to
> > > > > quote more than I usually would in my other replies already...)
> > > > > 
> > > > > Just to feature it more prominently for people who collapse quotes:
> > > > > 
> > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > 
> > > > > > If nobody replies early January, I would suggest to continue by
> > > > > > ignoring
> > > > > > the packed ring. Because if somebody wants this for packed ring in
> > > > > > future, this can still be added to the specs without breaking
> > > > > > things,
> > > > > > because this feature is negotiated per queue, not for the entire
> > > > > > device.
> > > > > 
> > > > > The problem is that we need to specify what is supposed to happen if
> > > > > packed ring *and* this feature are negotiated. If we do not want to
> > > > > add
> > > > > statements for the packed ring case, my suggestion would be
> > > > > - make packed ring and this feature mutually exclusive
> > > > > - add a new feature bit that works with packed ring later, if we
> > > > > think
> > > > > 
> > > > >   it is useful
> > > > 
> > > > Mja, I have to correct myself on that: I wrote in my previous email
> > > > that
> > > > this was negotiated per queue. That's false of course as all virtio
> > > > features are currently negotiated for the entire device.
> > > > 
> > > > So you are right, if this new indirect size feature was negotiated
> > > > then it
> > > > would apply to both a) split rings and b) packed rings a device might
> > > > have.
> > > > Which is unfortunate.
> > > > 
> > > > Stefan, you are aware about this circumstance as well, right? Because
> > > > I
> > > > remember we originally had a discussion on qemu-devel where you wanted
> > > > to
> > > > have this configurable per queue, not per device.
> > > 
> > > Regarding packed virtqueues, there are multiple reasons for using them.
> > > Recently someone asked about reducing complexity in VIRTIO
> > > implementations and one of the suggestions that Michael Tsirkin and I
> > > both made independently was to use the packed layout instead of split
> > > layout. I don't think we should assume packed virtqueues are only used
> > > in scenarios that don't need INDIRECT_SIZE.
> > > 
> > > Extending INDIRECT_SIZE to packed virtqueues looks straightforward and
> > > does not require many changes to the Packed Virtqueues section.
> > 
> > Yes I agree, it does make sense to apply this new INDIRECT_SIZE feature to
> > packed queues as well.
> > 
> > > The reason I pushed for per-virtqueue Indirect Size values is because
> > > multi-virtqueue devices have specific purposes for each virtqueue. The
> > > Queue Size field is per-virtqueue already across all transports. This
> > > way a device with control virtqueue can allocate fewer resources
> > > (smaller Queue Size and Indirect Size) to it than to the data path
> > > virtqueues that carry I/O data buffers.
> > > 
> > > I don't think it's necessary to support INDIRECT_SIZE on only a subset
> > > of a device's virtqueues. If the device doesn't want to "enable"
> > > INDIRECT_SIZE on a specific virtqueue it should just report Queue Size
> > > as the Indirect Size value?
> > 
> > Or another idea: what about adding this new indirect size field to the
> > struct that holds the queue size field already, then this would become
> > transport- independent (i.e. the discussed PCI config field would be
> > unnecessary), and it would be a feature configurable per queue. Or is
> > that struct inappropriate for feature negotiation purposes?
> 
> There is no transport-independent struct that contains Queue Size. It is
> handled by struct virtio_pci_common_cfg->queue_size, the MMIO QueueNum
> register, and the CCW struct vq_config_block->max_num and struct
> vq_info_block->num fields.

Aaah, I just realized I was completely confused about
struct virtio_pci_common_cfg: so far I assumed this were all entirely device 
global settings.

The respective queue is first selected via 'queue_select' field and then the 
subsequent fields reflect the selected queue. Now I got it.

Ok fine, then the current draft actually already negotiated the indirect size 
for each queue independently already, without me even being aware. ;-)

I guess I know all I need then for the next round of patches. Maybe I also 
come along for changes for MMIO and CCW side, we'll see.

Thanks!

Best regards,
Christian Schoenebeck

Re: [virtio-comment] Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Stefan Hajnoczi]

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

On Tue, Jan 25, 2022 at 08:05:47PM +0100, Christian Schoenebeck wrote:
> On Montag, 24. Januar 2022 17:41:09 CET Stefan Hajnoczi wrote:
> > On Mon, Jan 24, 2022 at 03:31:57PM +0100, Christian Schoenebeck wrote:
> > > On Montag, 24. Januar 2022 14:39:28 CET Stefan Hajnoczi wrote:
> > > > On Wed, Jan 05, 2022 at 02:52:46PM +0100, Christian Schoenebeck wrote:
> > > > > On Montag, 3. Januar 2022 14:21:13 CET Cornelia Huck wrote:
> > > > > > On Wed, Dec 29 2021, Christian Schoenebeck <qemu_oss@crudebyte.com>
> > > 
> > > wrote:
> > > > > > > On Donnerstag, 23. Dezember 2021 12:03:50 CET Cornelia Huck wrote:
> > > > > > >> On Wed, Dec 15 2021, Christian Schoenebeck
> > > > > > >> <qemu_oss@crudebyte.com>
> > > > > 
> > > > > wrote:
> > > > > > >> > On Dienstag, 14. Dezember 2021 18:20:28 CET Cornelia Huck 
> wrote:
> > > > > > >> >> Also, this is only for split ring; does packed ring need any
> > > > > > >> >> updates?
> > > > > > >> > 
> > > > > > >> > I have not reviewed the packed ring as much as I did the split
> > > > > > >> > ring, so
> > > > > > >> > I
> > > > > > >> > could not say reliably all the parts that shall be updated for
> > > > > > >> > the
> > > > > > >> > packed
> > > > > > >> > ring. There are some obvious parts like:
> > > > > > >> > 
> > > > > > >> > 2.7.5 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."
> > > > > > >> > 
> > > > > > >> > However the question is, would anybody want large descriptor
> > > > > > >> > chains
> > > > > > >> > with
> > > > > > >> > the packaged ring in the first place? If I understand it
> > > > > > >> > correctly,
> > > > > > >> > the
> > > > > > >> > benefits of the packed ring over the split ring only manifest
> > > > > > >> > for
> > > > > > >> > devices
> > > > > > >> > that interchange a very large number of rather small bulk data
> > > > > > >> > (e.g.
> > > > > > >> > network devices), no?
> > > > > > >> 
> > > > > > >> If we think that the feature does not make sense for packed ring,
> > > > > > >> they
> > > > > > >> should probably conflict with each other. Otherwise, I think we
> > > > > > >> need
> > > > > > >> at
> > > > > > >> least a statement that the higher limit does not take effect for
> > > > > > >> packed
> > > > > > >> ring, or touch all the places where it would be relevant.
> > > > > > >> 
> > > > > > >> What do others think?
> > > > > > > 
> > > > > > > It would indeed be very useful if other people express their
> > > > > > > opinion
> > > > > > > about
> > > > > > > this issue (packed ring scenario) as well before I continue on
> > > > > > > this
> > > > > > > issue.
> > > > > > > 
> > > > > > > Probably the fact that my patches never made it through to the
> > > > > > > list
> > > > > > > were
> > > > > > > not necessarily supporting this. Should I contact somebody
> > > > > > > regarding
> > > > > > > this
> > > > > > > ML issue? Do members of the other ML also read this virtio-comment
> > > > > > > list?
> > > > > > 
> > > > > > Yes, this situation is very unsatisfactory :( (I have contacted the
> > > > > > people running this list, but there have not yet been any fixes...)
> > > > > 
> > > > > Only my emails with patches are refused by the list. All my other
> > > > > emails
> > > > > are accepted. So not sure if the cause is really DKIM or something
> > > > > else.
> > > > > Maybe the admins can suggest a workaround for me.
> > > > > 
> > > > > > Not sure which other lists would be appropriate to cc: -- maybe
> > > > > > virtualization@lists.linux-foundation.org, but that one also suffers
> > > > > > from DKIM issues :(
> > > > > 
> > > > > What I thought was wether subscribers of virtio-dev would typically
> > > > > read
> > > > > virtio-comment as well. Because AFAICS people who more frequently deal
> > > > > with
> > > > > virtio for their companies rather seem to post to virtio-dev.
> > > > > 
> > > > > > > I tried to compensate the current situation by updating the
> > > > > > > corresponding
> > > > > > > issue description on Github in a very defailed and verbose way:
> > > > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > > 
> > > > > > Thanks. Hopefully me quoting this makes it more visible (I tried to
> > > > > > quote more than I usually would in my other replies already...)
> > > > > > 
> > > > > > Just to feature it more prominently for people who collapse quotes:
> > > > > > 
> > > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > > 
> > > > > > > If nobody replies early January, I would suggest to continue by
> > > > > > > ignoring
> > > > > > > the packed ring. Because if somebody wants this for packed ring in
> > > > > > > future, this can still be added to the specs without breaking
> > > > > > > things,
> > > > > > > because this feature is negotiated per queue, not for the entire
> > > > > > > device.
> > > > > > 
> > > > > > The problem is that we need to specify what is supposed to happen if
> > > > > > packed ring *and* this feature are negotiated. If we do not want to
> > > > > > add
> > > > > > statements for the packed ring case, my suggestion would be
> > > > > > - make packed ring and this feature mutually exclusive
> > > > > > - add a new feature bit that works with packed ring later, if we
> > > > > > think
> > > > > > 
> > > > > >   it is useful
> > > > > 
> > > > > Mja, I have to correct myself on that: I wrote in my previous email
> > > > > that
> > > > > this was negotiated per queue. That's false of course as all virtio
> > > > > features are currently negotiated for the entire device.
> > > > > 
> > > > > So you are right, if this new indirect size feature was negotiated
> > > > > then it
> > > > > would apply to both a) split rings and b) packed rings a device might
> > > > > have.
> > > > > Which is unfortunate.
> > > > > 
> > > > > Stefan, you are aware about this circumstance as well, right? Because
> > > > > I
> > > > > remember we originally had a discussion on qemu-devel where you wanted
> > > > > to
> > > > > have this configurable per queue, not per device.
> > > > 
> > > > Regarding packed virtqueues, there are multiple reasons for using them.
> > > > Recently someone asked about reducing complexity in VIRTIO
> > > > implementations and one of the suggestions that Michael Tsirkin and I
> > > > both made independently was to use the packed layout instead of split
> > > > layout. I don't think we should assume packed virtqueues are only used
> > > > in scenarios that don't need INDIRECT_SIZE.
> > > > 
> > > > Extending INDIRECT_SIZE to packed virtqueues looks straightforward and
> > > > does not require many changes to the Packed Virtqueues section.
> > > 
> > > Yes I agree, it does make sense to apply this new INDIRECT_SIZE feature to
> > > packed queues as well.
> > > 
> > > > The reason I pushed for per-virtqueue Indirect Size values is because
> > > > multi-virtqueue devices have specific purposes for each virtqueue. The
> > > > Queue Size field is per-virtqueue already across all transports. This
> > > > way a device with control virtqueue can allocate fewer resources
> > > > (smaller Queue Size and Indirect Size) to it than to the data path
> > > > virtqueues that carry I/O data buffers.
> > > > 
> > > > I don't think it's necessary to support INDIRECT_SIZE on only a subset
> > > > of a device's virtqueues. If the device doesn't want to "enable"
> > > > INDIRECT_SIZE on a specific virtqueue it should just report Queue Size
> > > > as the Indirect Size value?
> > > 
> > > Or another idea: what about adding this new indirect size field to the
> > > struct that holds the queue size field already, then this would become
> > > transport- independent (i.e. the discussed PCI config field would be
> > > unnecessary), and it would be a feature configurable per queue. Or is
> > > that struct inappropriate for feature negotiation purposes?
> > 
> > There is no transport-independent struct that contains Queue Size. It is
> > handled by struct virtio_pci_common_cfg->queue_size, the MMIO QueueNum
> > register, and the CCW struct vq_config_block->max_num and struct
> > vq_info_block->num fields.
> 
> Aaah, I just realized I was completely confused about
> struct virtio_pci_common_cfg: so far I assumed this were all entirely device 
> global settings.
> 
> The respective queue is first selected via 'queue_select' field and then the 
> subsequent fields reflect the selected queue. Now I got it.
> 
> Ok fine, then the current draft actually already negotiated the indirect size 
> for each queue independently already, without me even being aware. ;-)
> 
> I guess I know all I need then for the next round of patches. Maybe I also 
> come along for changes for MMIO and CCW side, we'll see.

Awesome!

Stefan

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

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Montag, 24. Januar 2022 14:53:45 CET Stefan Hajnoczi wrote:
> On Tue, Dec 14, 2021 at 04:13:17PM +0100, Christian Schoenebeck wrote:
> > This new common 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    | 25 ++++++++++++++++++++++++-
> >  split-ring.tex |  3 +++
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/content.tex b/content.tex

Stefan, while preparing the next version of patches for this issue (#122), I 
noticed that I missed your email here ...

> > index 0aa4842..e3cfcae 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport> 
> >          le64 queue_driver;              /* read-write */
> >          le64 queue_device;              /* read-write */
> >          le16 queue_notify_data;         /* read-only for driver */
> > 
> > +        le32 queue_indirect_size;       /* read-write */
> 
> I see no other unaligned fields in this struct, so I think a le16
> padding field is needed for safety:
> 
>            le64 queue_device;              /* read-write */
>            le16 queue_notify_data;         /* read-only for driver */
>   +        le16 reserved;                  /* no access */
>   +        le32 queue_indirect_size;       /* read-write */
> 

That's no longer needed as there's now the new 'queue_reset' field, i.e.:

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}

> >  };
> >  \end{lstlisting}
> > 
> > @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport> 
> >          may benefit from providing another value, for example an internal
> >          virtqueue
> >          identifier, or an internal offset related to the virtqueue
> >          number.
> >          \end{note}
> > 
> > +
> > +\item[\field{queue_indirect_size}]
> > +        This field is used to negotiate the maximum amount of descriptors
> > per +        vring slot 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 supported number of descriptors
> > per +        vring slot. If the driver requires fewer descriptors, it
> > writes its +        lower value to inform the device of the reduced
> > resource requirements.
> "vring slot" is a little vague, it means "indirect descriptor
> table"? The two paragraphs could use "maximum number of descriptors per
> indirect descriptor table" instead of referring to vring slots to imply
> indirect descriptor tables.

The intended phrasing was intended to reflect that "Queue Indirect Size" is 
meant to be the *sum* of all indirect descriptors:
https://lists.oasis-open.org/archives/virtio-comment/202112/msg00008.html

Just to avoid that I am missing something here; as of now, there can only be 
two indirect tables per round-trip message, right?

Best regards,
Christian Schoenebeck

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Stefan Hajnoczi]

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

On Sat, Feb 19, 2022 at 05:36:24PM +0100, Christian Schoenebeck wrote:
> On Montag, 24. Januar 2022 14:53:45 CET Stefan Hajnoczi wrote:
> > On Tue, Dec 14, 2021 at 04:13:17PM +0100, Christian Schoenebeck wrote:
> > > This new common 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    | 25 ++++++++++++++++++++++++-
> > >  split-ring.tex |  3 +++
> > >  2 files changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/content.tex b/content.tex
> 
> Stefan, while preparing the next version of patches for this issue (#122), I 
> noticed that I missed your email here ...
> 
> > > index 0aa4842..e3cfcae 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -859,6 +859,7 @@ \subsubsection{Common configuration structure
> > > layout}\label{sec:Virtio Transport> 
> > >          le64 queue_driver;              /* read-write */
> > >          le64 queue_device;              /* read-write */
> > >          le16 queue_notify_data;         /* read-only for driver */
> > > 
> > > +        le32 queue_indirect_size;       /* read-write */
> > 
> > I see no other unaligned fields in this struct, so I think a le16
> > padding field is needed for safety:
> > 
> >            le64 queue_device;              /* read-write */
> >            le16 queue_notify_data;         /* read-only for driver */
> >   +        le16 reserved;                  /* no access */
> >   +        le32 queue_indirect_size;       /* read-write */
> > 
> 
> That's no longer needed as there's now the new 'queue_reset' field, i.e.:
> 
> 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}
> 
> > >  };
> > >  \end{lstlisting}
> > > 
> > > @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure
> > > layout}\label{sec:Virtio Transport> 
> > >          may benefit from providing another value, for example an internal
> > >          virtqueue
> > >          identifier, or an internal offset related to the virtqueue
> > >          number.
> > >          \end{note}
> > > 
> > > +
> > > +\item[\field{queue_indirect_size}]
> > > +        This field is used to negotiate the maximum amount of descriptors
> > > per +        vring slot 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 supported number of descriptors
> > > per +        vring slot. If the driver requires fewer descriptors, it
> > > writes its +        lower value to inform the device of the reduced
> > > resource requirements.
> > "vring slot" is a little vague, it means "indirect descriptor
> > table"? The two paragraphs could use "maximum number of descriptors per
> > indirect descriptor table" instead of referring to vring slots to imply
> > indirect descriptor tables.
> 
> The intended phrasing was intended to reflect that "Queue Indirect Size" is 
> meant to be the *sum* of all indirect descriptors:
> https://lists.oasis-open.org/archives/virtio-comment/202112/msg00008.html
> 
> Just to avoid that I am missing something here; as of now, there can only be 
> two indirect tables per round-trip message, right?

No, just one indirect descriptor table. An indirect descriptor table may
contain both IN and OUT descriptors so it can handle a round-trip
message.

2.6.5.3.1 Driver Requirements: Indirect Descriptors says:

  A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT in flags.

It's unclear whether this statement is referring to 1) one descriptor,
2) to all descriptors in a buffer, or 3) to all descriptors ever made
available by the driver. QEMU's hw/virtio.c:virtqueue_pop() shows that
the interpretation is #2. So if a buffer uses an indirect descriptor
table then it has exactly 1 descriptor in the virtqueue's descriptor
table.

Stefan

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

Re: [PATCH 2/2] Add common configuration field "queue_indirect_size"

[Christian Schoenebeck]

On Montag, 21. Februar 2022 11:33:45 CET Stefan Hajnoczi wrote:
> On Sat, Feb 19, 2022 at 05:36:24PM +0100, Christian Schoenebeck wrote:
> > On Montag, 24. Januar 2022 14:53:45 CET Stefan Hajnoczi wrote:
> > > On Tue, Dec 14, 2021 at 04:13:17PM +0100, Christian Schoenebeck wrote:
> > > > @@ -938,6 +939,16 @@ \subsubsection{Common configuration structure
> > > > layout}\label{sec:Virtio Transport>
> > > > 
> > > >          may benefit from providing another value, for example an
> > > >          internal
> > > >          virtqueue
> > > >          identifier, or an internal offset related to the virtqueue
> > > >          number.
> > > >          \end{note}
> > > > 
> > > > +
> > > > +\item[\field{queue_indirect_size}]
> > > > +        This field is used to negotiate the maximum amount of
> > > > descriptors
> > > > per +        vring slot 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 supported number of
> > > > descriptors
> > > > per +        vring slot. If the driver requires fewer descriptors, it
> > > > writes its +        lower value to inform the device of the reduced
> > > > resource requirements.
> > > 
> > > "vring slot" is a little vague, it means "indirect descriptor
> > > table"? The two paragraphs could use "maximum number of descriptors per
> > > indirect descriptor table" instead of referring to vring slots to imply
> > > indirect descriptor tables.
> > 
> > The intended phrasing was intended to reflect that "Queue Indirect Size"
> > is
> > meant to be the *sum* of all indirect descriptors:
> > https://lists.oasis-open.org/archives/virtio-comment/202112/msg00008.html
> > 
> > Just to avoid that I am missing something here; as of now, there can only
> > be two indirect tables per round-trip message, right?
> 
> No, just one indirect descriptor table. An indirect descriptor table may
> contain both IN and OUT descriptors so it can handle a round-trip
> message.

Ok, you are right. I thought it would pick (per round-trip message) 2 pre-
allocated descriptors (directly from the queue), and that those 2 descriptors 
would then point to a separate indirect table each.

But it is like you said: just one pre-allocated descriptor, pointing to 
exactly one indirect table, and inside that indirect table there are first the 
indirect descriptors (pages) for device<-driver request message, followed by 
indirect descriptors (pages) for device->driver response message. And this 
order (request first, response second) is implied by the QEMU code as well.

The names for "in" and "out" in the code are a bit confusing BTW.

> 2.6.5.3.1 Driver Requirements: Indirect Descriptors says:
> 
>   A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT in
> flags.

Yes, I was aware about that and that should have made it clear to me already. 
I had a calculation error while debugging the code which mislead me.

> It's unclear whether this statement is referring to 1) one descriptor,
> 2) to all descriptors in a buffer, or 3) to all descriptors ever made
> available by the driver. QEMU's hw/virtio.c:virtqueue_pop() shows that
> the interpretation is #2. So if a buffer uses an indirect descriptor
> table then it has exactly 1 descriptor in the virtqueue's descriptor
> table.
> 
> Stefan

Yes, it is 2).

Ok, I'll change the phrasing as you suggested to "per indirect descriptor 
table" and will send a new version this week.

Thanks!

Best regards,
Christian Schoenebeck