On 03.04.25 16:18, Halil Pasic wrote:
On Wed, 2 Apr 2025 22:36:21 +0200
David Hildenbrand <da...@redhat.com> wrote:
If we finds a vq without a name in our input array in
virtio_ccw_find_vqs(), we treat it as "non-existing" and set the vq pointer
to NULL; we will not call virtio_ccw_setup_vq() to allocate/setup a vq.
Consequently, we create only a queue if it actually exists (name != NULL)
and assign an incremental queue index to each such existing queue.
First and foremost: thank you for addressing this! I have to admit, I'm
still plagued by some cognitive dissonance here. Please bear with me.
For starters the commit message of a229989d975e ("virtio: don't
allocate vqs when names[i] = NULL") goes like this:
"""
virtio: don't allocate vqs when names[i] = NULL
Some vqs may not need to be allocated when their related feature bits
are disabled. So callers may pass in such vqs with "names = NULL".
Then we skip such vq allocations.
"""
In my reading it does not talk about "non-existent" queues, but queues
that do not need to be allocated. This could make sense for something
like virtio-net where controlq 2N is with N being max_virtqueue_pairs.
I guess for the guest it could make sense to not set up some of the
queues initially, but those, I guess would be perfectly existent queues
spec-wise and we would expect the index of controlq being 2N. And the
queues that don't get set up initially can get set up later. At least
this is my naive understanding at the moment.
Now apparently there is a different case where queues may or may not
exist, but we would, for some reason like to have the non-existent
queues in the array, because for an other set of features negotiated
those queues would actually exist and occupy and index. Frankly
I don't fully comprehend it at the moment, but I will have another look
at the code and at the spec.
So lookign at the spec for virtio-ballon I see:
5.5.2 Virtqueues
0
inflateq
1
deflateq
2
statsq
3
free_page_vq
4
reporting_vq
statsq only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
free_page_vq only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
reporting_vq only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.
Which is IMHO weird. I used to think about the number in front of the
name as the virtqueue index. But based on this patch I'm wondering if
that is compatible with the approach of this patch.
What does for example mean if we have VIRTIO_BALLOON_F_STATS_VQ not
offered, VIRTIO_BALLOON_F_FREE_PAGE_HINT offered but not negotiated
and VIRTIO_BALLOON_F_PAGE_REPORTING negotiated.
One reading of the things is that statq is does not exist for sure,
free_page_vq is a little tricky because "is set" is not precise enough,
and reporting_vq exists for sure. And in your reading of the spec, if
I understood you correctly and we assume that free_page_vq does not
exist, it has index 2. But I from the top of my head, I don't know why
interpreting the spec like it reporting_vq has index 4 and indexes 2
and 3 are not mapped to existing-queues would be considered wrong.
And even if we do want reportig_vq to have index 2, the virtio-balloon
code could still give us an array where reportig_vq is at index 2. Why
not?
Sorry this ended up being a very long rant. the bottom line is that, I
lack conceptual clarity on where the problem exactly is and how it needs
to be addressed. I would like to understand this properly before moving
forward.
I would suggest you take a look at [1] I added below, and the disconnect
between the spec and what Linux + QEMU actually implemented.
In reality (with QEMU), reporting_vq sits either on index 3 or 4,
depending on the existence of VIRTIO_BALLOON_F_FREE_PAGE_HINT.
[..]
There was recently a discussion [1] whether the "holes" should be
treated differently again, effectively assigning also non-existing
queues a queue index: that should also fix the issue, but requires other
workarounds to not break existing setups.
Sorry I have to have a look at that discussion. Maybe it will answer
some my questions.
Yes, I think so.
Let's fix it without affecting existing setups for now by properly ignoring
the non-existing queues, so the indicator bits will match the queue
indexes.
Just one question. My understanding is that the crux is that Linux
and QEMU (or the driver and the device) disagree at which index
reporting_vq is actually sitting. Is that right?
I thought I made it clear: this is only about the airq indicator bit.
That's where both disagree.
Not the actual queue index (see above).
--
Cheers,
David / dhildenb
