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

[Stefan Hajnoczi <stefanha@redhat.com>]

On Fri, Dec 03, 2021 at 04:03:26PM +0100, Christian Schoenebeck wrote:
> On Donnerstag, 2. Dezember 2021 17:21:05 CET Stefan Hajnoczi wrote:
> > 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.
> 
> I don't understand which sections you see as "drivernormative" and which ones 
> not. Which precise sections in the spec do you want those comments to go to?

The spec is structured so that each section contains:

- Description
  \section{Foo}\label{sec:Bar / Foo}

- Driver requirements sub-section (optional)
  \drivernormative{\subsection}{Foo}{Bar / Foo}

- Device requirements sub-section (optional)
  \devicenormative{\subsection}{Foo}{Bar / Foo}

The driver and device requirements are kept separate from the
description, so normative statements (SHOULD, MUST, etc) are not
supposed to be used in the description text. Each driver and device
requirements sub-section is referenced from conformance.tex, a single
chapter at the end of the spec that lists all the normative
sub-sections.

Here is where I would put the spec changes:

- In "Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout" (4.1.4.3 Common configuration structure layout in VIRTIO 1.1):

  \item[\field{queue_indirect_size}] 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.

- In the \drivernormative paragraph for "Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout" (4.1.4.3.2 Driver Requirements: Common configuration structure layout in VIRTIO 1.1):

  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.

Stefan

Attachment: signature.asc
Description: PGP signature