Re: [PATCH v2 4/4] Add CCW configuration field "indirect_num" to vq_info_block

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Donnerstag, 10. März 2022 17:09:25 CET Cornelia Huck wrote:
> On Thu, Mar 10 2022, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Feb 21, 2022 at 06:01:41PM +0100, Christian Schoenebeck wrote:
> >> This new CCW 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
> >> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> >> ---
> >> 
> >>  content.tex | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >> 
> >> diff --git a/content.tex b/content.tex
> >> index a3baf4d..d400ea7 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -2599,6 +2599,7 @@ \subsubsection{Configuring a
> >> Virtqueue}\label{sec:Virtio Transport Options / Vir>> 
> >>          be16 num;
> >>          be64 driver;
> >>          be64 device;
> >> 
> >> +        be32 indirect_num;
> >> 
> >>  };
> >>  \end{lstlisting}
> >> 
> >> @@ -2607,6 +2608,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}.>> 
> >> +If VIRTIO_RING_F_INDIRECT_SIZE has been negotiated then
> >> \field{indirect_num} +reflects the maximum length of indirect descriptor
> >> tables for queue +\field{index}.
> > 
> > I think the transfer direction of CCW_CMD_SET_VQ struct vq_info_block is
> > driver-to-device. So it allows the driver to set the Queue Indirect
> > Size, but how does the driver query the device's maximum Queue Indirect
> > Size value?
> 
> [cc:ing Halil in case he has any further comments]
> 
> You're right, CCW_CMD_SET_VQ + vq_info_block is driver-to-device. The
> driver will obtain information about a queue via CCW_CMD_READ_VQ_CONF +
> vq_config_block, so a max_indirect_num field needs to be added there as
> well, I think.
> 
> Moreover, we're changing the length of the ccw payload. Extending at the
> end is generally fine, but the device and the driver need to agree on
> what the expected payload is. We basically have two options here:
> 
> * Make it depend on the feature bit being negotiated. This works because
>   virtqueue discovery needs to be done only after feature negotiation
>   has completed. However, this will get a bit awkward if we need to add
>   another field depending on a new feature bit: negotiating that
>   hypothetical feature would imply that the indirect num fields would be
>   present, but not valid, if the indirect feature had not been
>   negotiated. Not a showstopper, but looks a bit odd.
> * Tie it to a new ccw revision (3) and make offering the feature bit
>   dependant upon revision 3 or later being negotiated. This has the
>   advantage that ccw revisions always build on each other (so no
>   awkwardness for future extension) and the drawback of introducing
>   another transport-specific prereq.
> 
> If we can live with the possible awkwardness of future extensions, tying
> the size of the structures to feature bits might be the preferable way.

Really? My intuitive pick would rather be option 2 (CCW revision). But I'll go 
for whatever the common opinion is on this CCW issue.

Best regards,
Christian Schoenebeck