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

[Stefan Hajnoczi <stefanha@redhat.com>]

On Mon, Nov 29, 2021 at 02:23:01PM +0100, Christian Schoenebeck wrote:
> This new common configuration field allows to negotiate a more fine
> graded numeric maximum length 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    | 37 ++++++++++++++++++++++++++++++++++++-
>  split-ring.tex |  5 +++++
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/content.tex b/content.tex
> index 34fa23e..25861f3 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 */
> +        le16 queue_indirect_size;       /* read-write */
>  };
>  \end{lstlisting}
>  
> @@ -938,6 +939,34 @@ \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 was added in revision 3 and MUST exist if revision 3
> +        has been negotiated. This field is used to negotiate the maximum
> +        amount of indirect descriptors per indirect descriptor table as in
> +        \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / The
> +        Virtqueue Descriptor Table / Indirect Descriptors}.
> +
> +        The device specifies its supported maximum amount here first,
> +        subsequently driver may read and then SHOULD write this field to
> +        lower the value if driver's maximum amount of indirect descriptors
> +        ever being emplaced by driver per vring slot is less than what
> +        device supports. However driver MUST NOT increase this value.

may/SHOULD/MUST NOT statements need to go into drivernormative or
devicenormative sections.

I suggest phrasing it so functionality is described here and normative
sections don't repeat what was already covered:

  The device specifies its maximum supported number of descriptors per
  indirect descriptor table. If the driver requires fewer descriptors,
  it writes its lower value to inform the device of the reduced resource
  requirements.

The drivernormative section would say:

- The driver SHOULD write the field if its maximum number of descriptors
  per indirect descriptor table is lower than that reported by the
  device.
- The driver MUST NOT write a higher value than the one it reads from
  the device.

> +
> +        Two different semantics evolve for the value of this field, depending on
> +        whether VIRTIO_RING_F_LARGE_INDIRECT_DESC (see section
> +        \ref{sec:Reserved Feature Bits}) has been negotiated:
> +
> +        If VIRTIO_RING_F_LARGE_INDIRECT_DESC has not been negotiated, then the
> +        effective maximum amount of indirect descriptors per indirect descriptor
> +        table is min(Queue Size, \field{queue_indirect_size}). So in this case
> +        this field allows driver to indicate if its supported maximum amount of
> +        indirect descriptors is less than Queue Size.

I expected the !VIRTIO_RING_F_LARGE_INDIRECT_DESC case to preserve
existing behavior to avoid breaking existing drivers/devices. An
existing driver that honors Queue Size and doesn't know about
queue_indirect_size now violates the spec when a new device reports a
queue_indirect_size value less than Queue Size.

I expected drivers to only check queue_indirect_size when
VIRTIO_RING_F_LARGE_INDIRECT_DESC is negotiated.

> +
> +        If VIRTIO_RING_F_LARGE_INDIRECT_DESC has been negotiated, then the
> +        effective maximum amount of indirect descriptors per indirect descriptor
> +        table is directly and solely reflected by \field{queue_indirect_size}
> +        field here.
>  \end{description}
>  
>  \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout}
> @@ -6712,7 +6741,13 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>    being transferred per vring slot, but also avoids complicated synchronization
>    mechanisms if 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_LARGE_INDIRECT_DESC had been negotiated to subsequently

s/had been/was/

Attachment: signature.asc
Description: PGP signature