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

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Freitag, 18. März 2022 17:10:34 CET Cornelia Huck wrote:
> On Fri, Mar 18 2022, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > 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:
> >> > +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*."
> 
> Hm, I was coming from the driver's perspective here. But "undefined"
> probably captures "junk in there, do not use" better.

Like written to Halil moments ago, I would prefer "undefined" but I have no 
strong opininon on it.

> Ack on your other points.
> 
> >> 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?
> 
> The "max_indirect_num" goes with the "max_num": this is the maximum
> value that the device supports (the "maximum maximum", in a way :).
> "indirect_num" is the actual upper limit while the device is in use.

Ok, I understand your motivation now, your interpretation aspect for this was:

	device's value >= driver's value

Another interpretation aspect would be though from driver PoV: 'what is the 
maximum bulk size I could send to device?', and in this case you would call 
both fields 'max_indirect_num'.

Therefore I would prefer using the same name 'max_indirect_num' on both 
structs, to also make it clear they are about negotiating the same thing.

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