Re: [PATCH v3 4/4] Add CCW configuration field "indirect_num"

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 17. März 2022 15:12:42 CET Cornelia Huck wrote:
> On Wed, Mar 16 2022, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > This new CCW configuration field allows to negotiate a more fine
> > graded maximum lenght of indirect descriptor chains.
> > 
> > Bump CCW virtio revision to 3 and make the existence of this new
> > field "indirect_num" dependant on revision 3.
> > 
> > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/122
> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  content.tex | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> (...)
> 
> > @@ -2581,12 +2583,17 @@ \subsubsection{Configuring a
> > Virtqueue}\label{sec:Virtio Transport Options / Vir> 
> >  struct vq_config_block {
> >  
> >          be16 index;
> >          be16 max_num;
> > 
> > +        be32 indirect_num; /* since virtio-ccw rev. 3 */
> 
> Maybe make it max_indirect_num (to mirror max_num?)

Right, that name appears indeed more appropriate here.

> >  };
> >  \end{lstlisting}
> >  
> >  The requested number of buffers for queue \field{index} is returned in
> >  \field{max_num}.
> > 
> > +Since revision 3, \field{indirect_num} exists, which is supposed to
> > reflect the +Queue Indirect Size (i.e. the maximum length of indirect
> > descriptor tables) +supported by device for this queue.
> 
> It still depends on the feature flag, though.
> 
> Maybe
> 
> "If revision 2 or lower is set, struct vq_config_block extends up to
> \field{max_num}.
> 
> If revision 3 or higher is set, struct vq_config_block extends to
> \field{max_indirect_num}. If VIRTIO_RING_F_INDIRECT_SIZE is negotiated,
> \field{indirect_max_num} contains the maximum Queue Indirect Size
> (i.e. the maximum length of indirect descriptor tables) supported by the
> device for this queue; otherwise, its contents are unpredictable."

Maybe rather something like this instead:

"Actual size of struct vq_config_block depends on the virtio-ccw revision.

If revision 2 or lower is set, struct vq_config_block extends up to
*including* \field{max_num}.

If revision 3 or higher is set, struct vq_config_block extends up to
*including* \field{max_indirect_num}. If VIRTIO_RING_F_INDIRECT_SIZE is 
negotiated, \field{indirect_max_num} contains the maximum Queue Indirect Size
(i.e. the maximum length of indirect descriptor tables) supported by the
device for this queue; otherwise, its contents are *undefined*."
 
> > +
> > 
> >  Afterwards, CCW_CMD_SET_VQ is issued by the driver to inform the
> >  device about the location used for its queue. The transmitted
> >  structure is
> > 
> > @@ -2599,6 +2606,7 @@ \subsubsection{Configuring a
> > Virtqueue}\label{sec:Virtio Transport Options / Vir> 
> >          be16 num;
> >          be64 driver;
> >          be64 device;
> > 
> > +        be32 indirect_num; /* since virtio-ccw rev. 3 */
> > 
> >  };
> >  \end{lstlisting}
> > 
> > @@ -2607,6 +2615,10 @@ \subsubsection{Configuring a
> > Virtqueue}\label{sec:Virtio Transport Options / Vir> 
> >  available area and used area for queue \field{index}, respectively. The
> >  actual virtqueue size (number of allocated buffers) is transmitted in
> >  \field{num}.> 
> > +Since revision 3, \field{indirect_num} exists, which is supposed to
> > reflect the +Queue Indirect Size (i.e. the maximum length of indirect
> > descriptor tables) +supported by driver for this queue.
> 
> Similar to above:
> 
> "If revision 2 or lower is set, struct vq_info_block extends up to
> \field{device}.
> 
> If revision 3 or higher is set, struct vq_info_block extends to
> \field{indirect_num}. If VIRTIO_RING_F_INDIRECT_SIZE is negotiated,
> the Queue Indirect Size (i.e. the maximum length of indirect descriptor
> tables) supported by the driver for this queue is transmitted in
> \field{indirect_num}."

Ditto.

> > +
> > 
> >  \devicenormative{\paragraph}{Configuring a Virtqueue}{Virtio Transport
> >  Options / Virtio over channel I/O / Device Initialization / Configuring
> >  a Virtqueue}
> >  
> >  \field{res0} is reserved and MUST be ignored by the device.
> 
> We should include a device normative statement:
> 
> "The device MUST NOT offer VIRTIO_RING_F_INDIRECT_SIZE, unless at least
> revision 3 is set, and therefore \field{max_indirect_num} respectively
> \field{indirect_num} are available."

Oh, you are suggesting two different names for those two fields? 
"max_indirect_num" vs. "indirect_num". If yes, why?

Otherwise about adding that normative statement in general here: sure, makes 
sense.

Best regards,
Christian Schoenebeck