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

[Cornelia Huck <cohuck@redhat.com>]

On Fri, Mar 11 2022, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> 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.

Either would work; that's why I'd like to have Halil's opinion as well.