Re: VIRTIO_RING_F_INDIRECT_SIZE status

[Christian Schoenebeck <qemu_oss@crudebyte.com>]

On Wednesday, March 1, 2023 2:57:57 PM CET Stefan Hajnoczi wrote:
> On Wed, Mar 01, 2023 at 01:55:14PM +0100, Christian Schoenebeck wrote:
> > On Tuesday, February 28, 2023 1:05:21 PM CET Michael S. Tsirkin wrote:
> > > On Tue, Feb 28, 2023 at 12:49:00PM +0100, Christian Schoenebeck wrote:
> > > > On Monday, February 27, 2023 6:41:12 PM CET Michael S. Tsirkin wrote:
> > > > > On Mon, Feb 27, 2023 at 05:13:12PM +0100, Christian Schoenebeck wrote:
> > > > > > On Monday, February 27, 2023 4:45:45 PM CET Stefan Hajnoczi wrote:
> > > > > > > On Mon, Feb 27, 2023 at 02:53:55PM +0000, Afsa, Baptiste wrote:
> > > > > > > > The issue that we have now, is that this limitation does not seem to be enforced
> > > > > > > > in Linux virtio drivers today. I came across:
> > > > > > > > 
> > > > > > > > https://github.com/oasis-tcs/virtio-spec/issues/122
> > > > > > > > 
> > > > > > > > which looks like a good base for us to build upon, but I'm not sure what is the
> > > > > > > > status with this issue. Do you know whether there is any plan regarding this?
> > > > > > > 
> > > > > > > CCing Christian regarding extending queue size limits.
> > > > > > 
> > > > > > Status on this issue: it was suspended in May last year. AFAICR Michael
> > > > > > expressed the need to give some more thought about it, and a new virtio spec
> > > > > > release was just ahead at that time.
> > > > > > 
> > > > > > Michael, suggestions how to bring this forward?
> > > > > > 
> > > > > > Best regards,
> > > > > > Christian Schoenebeck
> > > > > > 
> > > > > 
> > > > > How about a summary letter listing various options, pros and cons?
> > > > 
> > > > Actually I had submitted a draft for this feature (latest version linked
> > > > above), and AFAICR Michael was the only person who expressed concerns from
> > > > design perspective. Comments by other people were already just aboout precise
> > > > wording, but not about the design itself.
> > > > 
> > > > We stopped the discussion at the point where Michael expressed the need to
> > > > think more about it, but as his expressed concerns were a bit vague, I still
> > > > don't see how I could bring this issue forward.
> > > > 
> > > > Best regards,
> > > > Christian Schoenebeck
> > > > 
> > > 
> > > 
> > > So the last time there were two things:
> > > 
> > > 
> > > 1. bugs introduced during the packed ring work. For example:
> > > 
> > > 	> 
> > > 	> I don't think so:
> > > 	> 
> > > 	>   2.6.5.3.1 Driver Requirements: Indirect Descriptors
> > > 	>   ..
> > > 	>   "A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT and VIRTQ_DESC_F_NEXT in
> > > 	>   flags."
> > > 	> So as far as I can see it, the amount of direct descriptors is currently
> > > 	> always exactly one if an indirect table is used.
> > > 	> 
> > > 	> Best regards,
> > > 	> Christian Schoenebeck
> > > 	> 
> > > 
> > > 	Oh. You are right.  Weirdly this text is not in packed-ring.tex - I
> > > 	think this and a bunch of other cases like this are an oversight,
> > > 	however we need to fix them first before adding features that assume
> > > 	they are fixed ...
> > > 
> > > we need to get them fixed before we poke at core ring otherwise it's a
> > > mess - maybe the TC will accept just fixing this
> > > without a feature bit, or maybe people will feel a feature bit
> > > is required.
> > 
> > It's been a while. Looking at v1.2 it says:
> > 
> >   2.8 Packed Virtqueues
> >   ...
> >   2.8.5 Scatter-Gather Support [1]
> >   ...
> >   While unusual (most implementations either create all lists solely using   
> >   non-indirect descriptors, or always use a single indirect element), if both 
> >   features have been negotiated, mixing indirect and non-indirect descriptors 
> >   in a ring is valid, as long as each list only contains descriptors of a 
> >   given type.
> > 
> >   [1] https://docs.oasis-open.org/virtio/virtio/v1.2/cs01/virtio-v1.2-cs01.html#x1-770005
> > 
> > To avoid misapprehensions: the way I understand it, same restrictions apply to
> > packed queues as split queues, in the sense that you may neither chain several
> > tables in a single message, nor multi-level nest tables, nor mix a list of
> > direct descriptors and indirect descriptors on the same level within one
> > message. So the explicit exception described here, only means you may use
> > *one* indirect table in one message, while using chained direct descriptors in
> > another message. But that's it, right?
> 
> Hi Christian,
> Yes, for packed virtqueues it is forbidden to mix normal and indirect
> descriptors in a single buffer. Further support for this interpretation
> is in 2.8.19 Driver Requirements: Indirect Descriptors:
> 
>   A driver MUST NOT write direct descriptors with VIRTQ_DESC_F_INDIRECT
>   set in a scatter-gather list linked by VIRTQ_DESC_F_NEXT. flags.
> 
> VIRTQ_DESC_F_INDIRECT cannot be set if the descriptor chain uses
> VIRTQ_DESC_F_NEXT, therefore it's not possible to mix normal and
> indirect descriptors in a single buffer.
> 
> However, the Split Virtqueues section documents an exception where
> mixing normal and indirect descriptors in a single buffer is allowed,
> but only in a specific order. 2.7.5.3.2 Device Requirements: Indirect
> Descriptors says:
> 
>   The device MUST handle the case of zero or more normal chained
>   descriptors followed by a single descriptor with
>   flags&VIRTQ_DESC_F_INDIRECT. Note: While unusual (most implementations
>   either create a chain solely using non-indirect descriptors, or use a
>   single indirect element), such a layout is valid.
> 
> So what degree of mixing normal and indirect descriptors is allowed
> depends on the virtqueue format.

Oh, good catch! I was not aware about this device requirement for split
queues. I thought 2.7.5.3.1 "A driver MUST NOT set both VIRTQ_DESC_F_INDIRECT
and VIRTQ_DESC_F_NEXT in flags" would rule that mixed case out, but right, if
the indirect descriptor is last in the buffer then 2.7.5.3.1 is not violated.

Which makes Michael's suggestion even mandatory.

Out of pure curiosity: is this mixed case actively used by somebody?

> 
> Stefan
> 
> > > 2. Given this is a lot of work I am trying to find a way to
> > > make the impact bigger. In particular to cover the use-case
> > > of limiting s/g to 1k while making queues deeper (with
> > > or without indirect). For this I proposed:
> > > 
> > > 	So I think that given this, we can limit the total number
> > > 	of non-indirect descriptors, including non-indirect ones
> > > 	in a chain + all the ones in indirect pointer table if any,
> > > 	and excluding the indirect descriptor itself, and this
> > > 	will address the issue you are describing here, right?
> > > 
> > > people seemed to be ok with this idea?
> > 
> > IIUIC it would not make a difference from design perspective from what I
> > proposed, as virtio currently neither allows to mix, chain or mult-level nest
> > indirect descriptor tables within a single message), and hence it would just
> > boil down to adjusting the wording. So yes, it would therefore cover my
> > intended use case.
> > 
> > Best regards,
> > Christian Schoenebeck
> > 
> > 
>