[v1] s390/virtio_ccw: don't allocate/assign airqs for non-existing queues

Commit Message

David Hildenbrand April 2, 2025, 8:36 p.m. UTC

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.

However, in virtio_ccw_register_adapter_ind()->get_airq_indicator() we
will not ignore these "non-existing queues", but instead assign an airq
indicator to them.

Besides never releasing them in virtio_ccw_drop_indicators() (because
there is no virtqueue), the bigger issue seems to be that there will be a
disagreement between the device and the Linux guest about the airq
indicator to be used for notifying a queue, because the indicator bit
for adapter I/O interrupt is derived from the queue index.

The virtio spec states under "Setting Up Two-Stage Queue Indicators":

	... indicator contains the guest address of an area wherein the
	indicators for the devices are contained, starting at bit_nr, one
	bit per virtqueue of the device.

And further in "Notification via Adapter I/O Interrupts":

	For notifying the driver of virtqueue buffers, the device sets the
	bit in the guest-provided indicator area at the corresponding
	offset.

For example, QEMU uses in virtio_ccw_notify() the queue index (passed as
"vector") to select the relevant indicator bit. If a queue does not exist,
it does not have a corresponding indicator bit assigned, because it
effectively doesn't have a queue index.

Using a virtio-balloon-ccw device under QEMU with free-page-hinting
disabled ("free-page-hint=off") but free-page-reporting enabled
("free-page-reporting=on") will result in free page reporting
not working as expected: in the virtio_balloon driver, we'll be stuck
forever in virtballoon_free_page_report()->wait_event(), because the
waitqueue will not be woken up as the notification from the device is
lost: it would use the wrong indicator bit.

Free page reporting stops working and we get splats (when configured to
detect hung wqs) like:

 INFO: task kworker/1:3:463 blocked for more than 61 seconds.
       Not tainted 6.14.0 #4
 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
 task:kworker/1:3 [...]
 Workqueue: events page_reporting_process
 Call Trace:
  [<000002f404e6dfb2>] __schedule+0x402/0x1640
  [<000002f404e6f22e>] schedule+0x3e/0xe0
  [<000002f3846a88fa>] virtballoon_free_page_report+0xaa/0x110 [virtio_balloon]
  [<000002f40435c8a4>] page_reporting_process+0x2e4/0x740
  [<000002f403fd3ee2>] process_one_work+0x1c2/0x400
  [<000002f403fd4b96>] worker_thread+0x296/0x420
  [<000002f403fe10b4>] kthread+0x124/0x290
  [<000002f403f4e0dc>] __ret_from_fork+0x3c/0x60
  [<000002f404e77272>] ret_from_fork+0xa/0x38

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.

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.

[1] https://lore.kernel.org/all/[e-mail]/

Fixes: a229989d975e ("virtio: don't allocate vqs when names[i] = NULL")
Reported-by: Chandra Merla <[e-mail]>
Cc: <[e-mail]>
Cc: Cornelia Huck <[e-mail]>
Cc: Thomas Huth <[e-mail]>
Cc: Halil Pasic <[e-mail]>
Cc: Eric Farman <[e-mail]>
Cc: Heiko Carstens <[e-mail]>
Cc: Vasily Gorbik <[e-mail]>
Cc: Alexander Gordeev <[e-mail]>
Cc: Christian Borntraeger <[e-mail]>
Cc: Sven Schnelle <[e-mail]>
Cc: "Michael S. Tsirkin" <[e-mail]>
Cc: Wei Wang <[e-mail]>
Signed-off-by: David Hildenbrand <[e-mail]>
---
 drivers/s390/virtio/virtio_ccw.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Comments

Thomas Huth April 3, 2025, 9:44 a.m. UTC | #1

On 02/04/2025 22.36, David Hildenbrand 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.
> 
> However, in virtio_ccw_register_adapter_ind()->get_airq_indicator() we
> will not ignore these "non-existing queues", but instead assign an airq
> indicator to them.
> 
> Besides never releasing them in virtio_ccw_drop_indicators() (because
> there is no virtqueue), the bigger issue seems to be that there will be a
> disagreement between the device and the Linux guest about the airq
> indicator to be used for notifying a queue, because the indicator bit
> for adapter I/O interrupt is derived from the queue index.
> 
> The virtio spec states under "Setting Up Two-Stage Queue Indicators":
> 
> 	... indicator contains the guest address of an area wherein the
> 	indicators for the devices are contained, starting at bit_nr, one
> 	bit per virtqueue of the device.
> 
> And further in "Notification via Adapter I/O Interrupts":
> 
> 	For notifying the driver of virtqueue buffers, the device sets the
> 	bit in the guest-provided indicator area at the corresponding
> 	offset.
> 
> For example, QEMU uses in virtio_ccw_notify() the queue index (passed as
> "vector") to select the relevant indicator bit. If a queue does not exist,
> it does not have a corresponding indicator bit assigned, because it
> effectively doesn't have a queue index.
> 
> Using a virtio-balloon-ccw device under QEMU with free-page-hinting
> disabled ("free-page-hint=off") but free-page-reporting enabled
> ("free-page-reporting=on") will result in free page reporting
> not working as expected: in the virtio_balloon driver, we'll be stuck
> forever in virtballoon_free_page_report()->wait_event(), because the
> waitqueue will not be woken up as the notification from the device is
> lost: it would use the wrong indicator bit.
> 
> Free page reporting stops working and we get splats (when configured to
> detect hung wqs) like:
> 
>   INFO: task kworker/1:3:463 blocked for more than 61 seconds.
>         Not tainted 6.14.0 #4
>   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>   task:kworker/1:3 [...]
>   Workqueue: events page_reporting_process
>   Call Trace:
>    [<000002f404e6dfb2>] __schedule+0x402/0x1640
>    [<000002f404e6f22e>] schedule+0x3e/0xe0
>    [<000002f3846a88fa>] virtballoon_free_page_report+0xaa/0x110 [virtio_balloon]
>    [<000002f40435c8a4>] page_reporting_process+0x2e4/0x740
>    [<000002f403fd3ee2>] process_one_work+0x1c2/0x400
>    [<000002f403fd4b96>] worker_thread+0x296/0x420
>    [<000002f403fe10b4>] kthread+0x124/0x290
>    [<000002f403f4e0dc>] __ret_from_fork+0x3c/0x60
>    [<000002f404e77272>] ret_from_fork+0xa/0x38
> 
> 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.
> 
> 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.
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
> Fixes: a229989d975e ("virtio: don't allocate vqs when names[i] = NULL")
> Reported-by: Chandra Merla <[e-mail]>
> Cc: <[e-mail]>
> Cc: Cornelia Huck <[e-mail]>
> Cc: Thomas Huth <[e-mail]>
> Cc: Halil Pasic <[e-mail]>
> Cc: Eric Farman <[e-mail]>
> Cc: Heiko Carstens <[e-mail]>
> Cc: Vasily Gorbik <[e-mail]>
> Cc: Alexander Gordeev <[e-mail]>
> Cc: Christian Borntraeger <[e-mail]>
> Cc: Sven Schnelle <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Wei Wang <[e-mail]>
> Signed-off-by: David Hildenbrand <[e-mail]>
> ---
>   drivers/s390/virtio/virtio_ccw.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)

Thanks for tackling this, David!

I tested the patch and the problems are gone now, indeed!

Tested-by: Thomas Huth <[e-mail]>
Reviewed-by: Thomas Huth <[e-mail]>

Cornelia Huck April 3, 2025, 12:45 p.m. UTC | #2

On Wed, Apr 02 2025, David Hildenbrand <[e-mail]> 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.
>
> However, in virtio_ccw_register_adapter_ind()->get_airq_indicator() we
> will not ignore these "non-existing queues", but instead assign an airq
> indicator to them.
>
> Besides never releasing them in virtio_ccw_drop_indicators() (because
> there is no virtqueue), the bigger issue seems to be that there will be a
> disagreement between the device and the Linux guest about the airq
> indicator to be used for notifying a queue, because the indicator bit
> for adapter I/O interrupt is derived from the queue index.
>
> The virtio spec states under "Setting Up Two-Stage Queue Indicators":
>
> 	... indicator contains the guest address of an area wherein the
> 	indicators for the devices are contained, starting at bit_nr, one
> 	bit per virtqueue of the device.
>
> And further in "Notification via Adapter I/O Interrupts":
>
> 	For notifying the driver of virtqueue buffers, the device sets the
> 	bit in the guest-provided indicator area at the corresponding
> 	offset.
>
> For example, QEMU uses in virtio_ccw_notify() the queue index (passed as
> "vector") to select the relevant indicator bit. If a queue does not exist,
> it does not have a corresponding indicator bit assigned, because it
> effectively doesn't have a queue index.
>
> Using a virtio-balloon-ccw device under QEMU with free-page-hinting
> disabled ("free-page-hint=off") but free-page-reporting enabled
> ("free-page-reporting=on") will result in free page reporting
> not working as expected: in the virtio_balloon driver, we'll be stuck
> forever in virtballoon_free_page_report()->wait_event(), because the
> waitqueue will not be woken up as the notification from the device is
> lost: it would use the wrong indicator bit.
>
> Free page reporting stops working and we get splats (when configured to
> detect hung wqs) like:
>
>  INFO: task kworker/1:3:463 blocked for more than 61 seconds.
>        Not tainted 6.14.0 #4
>  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>  task:kworker/1:3 [...]
>  Workqueue: events page_reporting_process
>  Call Trace:
>   [<000002f404e6dfb2>] __schedule+0x402/0x1640
>   [<000002f404e6f22e>] schedule+0x3e/0xe0
>   [<000002f3846a88fa>] virtballoon_free_page_report+0xaa/0x110 [virtio_balloon]
>   [<000002f40435c8a4>] page_reporting_process+0x2e4/0x740
>   [<000002f403fd3ee2>] process_one_work+0x1c2/0x400
>   [<000002f403fd4b96>] worker_thread+0x296/0x420
>   [<000002f403fe10b4>] kthread+0x124/0x290
>   [<000002f403f4e0dc>] __ret_from_fork+0x3c/0x60
>   [<000002f404e77272>] ret_from_fork+0xa/0x38
>
> 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.
>
> 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.
>
> [1] https://lore.kernel.org/all/[e-mail]/
>
> Fixes: a229989d975e ("virtio: don't allocate vqs when names[i] = NULL")
> Reported-by: Chandra Merla <[e-mail]>
> Cc: <[e-mail]>
> Cc: Cornelia Huck <[e-mail]>
> Cc: Thomas Huth <[e-mail]>
> Cc: Halil Pasic <[e-mail]>
> Cc: Eric Farman <[e-mail]>
> Cc: Heiko Carstens <[e-mail]>
> Cc: Vasily Gorbik <[e-mail]>
> Cc: Alexander Gordeev <[e-mail]>
> Cc: Christian Borntraeger <[e-mail]>
> Cc: Sven Schnelle <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Wei Wang <[e-mail]>
> Signed-off-by: David Hildenbrand <[e-mail]>
> ---
>  drivers/s390/virtio/virtio_ccw.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

Reviewed-by: Cornelia Huck <[e-mail]>

[I assume that one of the IBM folks can simply pick this up?]

Michael S. Tsirkin April 3, 2025, 12:57 p.m. UTC | #3

On Wed, Apr 02, 2025 at 10:36:21PM +0200, David Hildenbrand 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.
> 
> However, in virtio_ccw_register_adapter_ind()->get_airq_indicator() we
> will not ignore these "non-existing queues", but instead assign an airq
> indicator to them.
> 
> Besides never releasing them in virtio_ccw_drop_indicators() (because
> there is no virtqueue), the bigger issue seems to be that there will be a
> disagreement between the device and the Linux guest about the airq
> indicator to be used for notifying a queue, because the indicator bit
> for adapter I/O interrupt is derived from the queue index.
> 
> The virtio spec states under "Setting Up Two-Stage Queue Indicators":
> 
> 	... indicator contains the guest address of an area wherein the
> 	indicators for the devices are contained, starting at bit_nr, one
> 	bit per virtqueue of the device.
> 
> And further in "Notification via Adapter I/O Interrupts":
> 
> 	For notifying the driver of virtqueue buffers, the device sets the
> 	bit in the guest-provided indicator area at the corresponding
> 	offset.
> 
> For example, QEMU uses in virtio_ccw_notify() the queue index (passed as
> "vector") to select the relevant indicator bit. If a queue does not exist,
> it does not have a corresponding indicator bit assigned, because it
> effectively doesn't have a queue index.
> 
> Using a virtio-balloon-ccw device under QEMU with free-page-hinting
> disabled ("free-page-hint=off") but free-page-reporting enabled
> ("free-page-reporting=on") will result in free page reporting
> not working as expected: in the virtio_balloon driver, we'll be stuck
> forever in virtballoon_free_page_report()->wait_event(), because the
> waitqueue will not be woken up as the notification from the device is
> lost: it would use the wrong indicator bit.
> 
> Free page reporting stops working and we get splats (when configured to
> detect hung wqs) like:
> 
>  INFO: task kworker/1:3:463 blocked for more than 61 seconds.
>        Not tainted 6.14.0 #4
>  "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>  task:kworker/1:3 [...]
>  Workqueue: events page_reporting_process
>  Call Trace:
>   [<000002f404e6dfb2>] __schedule+0x402/0x1640
>   [<000002f404e6f22e>] schedule+0x3e/0xe0
>   [<000002f3846a88fa>] virtballoon_free_page_report+0xaa/0x110 [virtio_balloon]
>   [<000002f40435c8a4>] page_reporting_process+0x2e4/0x740
>   [<000002f403fd3ee2>] process_one_work+0x1c2/0x400
>   [<000002f403fd4b96>] worker_thread+0x296/0x420
>   [<000002f403fe10b4>] kthread+0x124/0x290
>   [<000002f403f4e0dc>] __ret_from_fork+0x3c/0x60
>   [<000002f404e77272>] ret_from_fork+0xa/0x38
> 
> 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.
> 
> 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.
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
> Fixes: a229989d975e ("virtio: don't allocate vqs when names[i] = NULL")
> Reported-by: Chandra Merla <[e-mail]>
> Cc: <[e-mail]>
> Cc: Cornelia Huck <[e-mail]>
> Cc: Thomas Huth <[e-mail]>
> Cc: Halil Pasic <[e-mail]>
> Cc: Eric Farman <[e-mail]>
> Cc: Heiko Carstens <[e-mail]>
> Cc: Vasily Gorbik <[e-mail]>
> Cc: Alexander Gordeev <[e-mail]>
> Cc: Christian Borntraeger <[e-mail]>
> Cc: Sven Schnelle <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Wei Wang <[e-mail]>
> Signed-off-by: David Hildenbrand <[e-mail]>

Acked-by: Michael S. Tsirkin <[e-mail]>


feel free to merge.

> ---
>  drivers/s390/virtio/virtio_ccw.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index 21fa7ac849e5c..4904b831c0a75 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -302,11 +302,17 @@ static struct airq_info *new_airq_info(int index)
>  static unsigned long *get_airq_indicator(struct virtqueue *vqs[], int nvqs,
>  					 u64 *first, void **airq_info)
>  {
> -	int i, j;
> +	int i, j, queue_idx, highest_queue_idx = -1;
>  	struct airq_info *info;
>  	unsigned long *indicator_addr = NULL;
>  	unsigned long bit, flags;
>  
> +	/* Array entries without an actual queue pointer must be ignored. */
> +	for (i = 0; i < nvqs; i++) {
> +		if (vqs[i])
> +			highest_queue_idx++;
> +	}
> +
>  	for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
>  		mutex_lock(&airq_areas_lock);
>  		if (!airq_areas[i])
> @@ -316,7 +322,7 @@ static unsigned long *get_airq_indicator(struct virtqueue *vqs[], int nvqs,
>  		if (!info)
>  			return NULL;
>  		write_lock_irqsave(&info->lock, flags);
> -		bit = airq_iv_alloc(info->aiv, nvqs);
> +		bit = airq_iv_alloc(info->aiv, highest_queue_idx + 1);
>  		if (bit == -1UL) {
>  			/* Not enough vacancies. */
>  			write_unlock_irqrestore(&info->lock, flags);
> @@ -325,8 +331,10 @@ static unsigned long *get_airq_indicator(struct virtqueue *vqs[], int nvqs,
>  		*first = bit;
>  		*airq_info = info;
>  		indicator_addr = info->aiv->vector;
> -		for (j = 0; j < nvqs; j++) {
> -			airq_iv_set_ptr(info->aiv, bit + j,
> +		for (j = 0, queue_idx = 0; j < nvqs; j++) {
> +			if (!vqs[j])
> +				continue;
> +			airq_iv_set_ptr(info->aiv, bit + queue_idx++,
>  					(unsigned long)vqs[j]);
>  		}
>  		write_unlock_irqrestore(&info->lock, flags);
> -- 
> 2.48.1

Christian Borntraeger April 3, 2025, 1:12 p.m. UTC | #4

Am 02.04.25 um 22:36 schrieb David Hildenbrand:
[...]>
> Fixes: a229989d975e ("virtio: don't allocate vqs when names[i] = NULL")
> Reported-by: Chandra Merla <[e-mail]>
> Cc: <[e-mail]>
> Cc: Cornelia Huck <[e-mail]>
> Cc: Thomas Huth <[e-mail]>
> Cc: Halil Pasic <[e-mail]>
> Cc: Eric Farman <[e-mail]>
> Cc: Heiko Carstens <[e-mail]>
> Cc: Vasily Gorbik <[e-mail]>
> Cc: Alexander Gordeev <[e-mail]>
> Cc: Christian Borntraeger <[e-mail]>
> Cc: Sven Schnelle <[e-mail]>
> Cc: "Michael S. Tsirkin" <[e-mail]>
> Cc: Wei Wang <[e-mail]>
> Signed-off-by: David Hildenbrand <[e-mail]>

Acked-by: Christian Borntraeger <[e-mail]>

I will apply this to our internal s390 tree and let it sit for a
day to get CI coverage.

Alexander, Vasily, Heiko,
please then schedule for the s390/fixes branch if there is no CI fallout.

Halil Pasic April 3, 2025, 2:18 p.m. UTC | #5

On Wed,  2 Apr 2025 22:36:21 +0200
David Hildenbrand <[e-mail]> 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.

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

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

Regards,
Halil

David Hildenbrand April 3, 2025, 2:28 p.m. UTC | #6

On 03.04.25 16:18, Halil Pasic wrote:
> On Wed,  2 Apr 2025 22:36:21 +0200
> David Hildenbrand <[e-mail]> 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).

Michael S. Tsirkin April 3, 2025, 2:35 p.m. UTC | #7

On Thu, Apr 03, 2025 at 04:18:36PM +0200, Halil Pasic wrote:
> On Wed,  2 Apr 2025 22:36:21 +0200
> David Hildenbrand <[e-mail]> 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

Indeed. Unfortunately, no one at all implemented this properly as
per spec :(.

Balloon is the worst offender here but other devices are broken
too in some configurations.


Given it has been like this for many years I'm inclined in this
instance to fix the spec not the implementations.




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


Yep. I tried that, and it is basically a terrible mess :(.


> > 
> 
> Sorry I have to have a look at that discussion. Maybe it will answer
> some my questions.
> 
> > 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?
> 
> Regards,


> Halil

Halil Pasic April 4, 2025, 4:02 a.m. UTC | #8

On Thu, 3 Apr 2025 10:35:33 -0400
"Michael S. Tsirkin" <[e-mail]> wrote:

> On Thu, Apr 03, 2025 at 04:18:36PM +0200, Halil Pasic wrote:
> > On Wed,  2 Apr 2025 22:36:21 +0200
[..]
> > 
> > 5.5.2 Virtqueues
> > 
> > 0
> >     inflateq 
> > 1
> >     deflateq 
> > 2
> >     statsq 
> > 3
> >     free_page_vq 
> > 4
> >     reporting_vq  
> 
> Indeed. Unfortunately, no one at all implemented this properly as
> per spec :(.
> 
> Balloon is the worst offender here but other devices are broken
> too in some configurations.
> 
> 
> Given it has been like this for many years I'm inclined in this
> instance to fix the spec not the implementations.
> 

I understand! For me at least knowing if we are going to change the
spec or the implementations is pretty important. It would probably
make sense to spin a patch for the spec, just for the unlikely case that
somebody did end up implementing this by the spec and wants to protest.

I think, a consequence of this design is that all queues need to be
created and allocated at initialization time.

I mean in the spec we have something like 
"""
5.1.6.5.6.1 Driver Requirements: Automatic receive steering in multiqueue mode
The driver MUST configure the virtqueues before enabling them with the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command. 
"""

For example one could want to hotplug 2 more vCPUs and still have a
queue-pair per cpu (and a controlq). Those 2 extra queue-pairs could
in theory be allocated on-demand instead of having to allocate memory
for the rings for all the queues corresponding to the maxed out setup.
I've had a look at the Linux virtio-net and it does allocate all the
queues up front.

Also with this design, I believe we would effectively prohibit "holes".
Now if holes are prohibited, IMHO it does not make a whole lot of
sense for the virtio_find_vqs() to support holes. Because the holes
and a fair amount of complexity. Of course that would make sense as a
cleanup on top.

Regards,
Halil

Halil Pasic April 4, 2025, 4:36 a.m. UTC | #9

On Thu, 3 Apr 2025 16:28:31 +0200
David Hildenbrand <[e-mail]> wrote:

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

I did some more research including having a look at that discussion. Let
me try to sum up how did we end up here.

Before commit a229989d975e ("virtio: don't allocate vqs when names[i] =
NULL") the kernel behavior used to be in spec, but QEMU and possibly
other hypervisor were out of spec and things did not work.

Possibly because of the complexity of fixing the hypervisor(s) commit
a229989d975e ("virtio: don't allocate vqs when names[i] = NULL") opted
for changing the guest side so that it does not fit the spec but fits
the hypervisor(s). It unfortunately also broke notifiers (for the with
holes) scenario for virtio-ccw only.

Now we had another look at this, and have concluded that fixing the
hypervisor(s) and fixing the kernel, and making sure that the fixed
kernel can tolerate the old broken hypervisor(s) is way to complicated
if possible at all. So we decided to give the spec a reality check and
fix the notifier bit assignment for virtio-ccw which is broken beyond
doubt if we accept that the correct virtqueue index is the one that the
hypervisor(s) use and not the one that the spec says they should use.

With the spec fixed, the whole notion of "holes" will be something that
does not make sense any more. With that the merit of the kernel interface
virtio_find_vqs() supporting "holes" is quite questionable. Now we need
it because the drivers within the Linux kernel still think of the queues
in terms of the current spec, i.e. they try to have the "holes" as
mandated by the spec, and the duty of making it work with the broken
device implementations falls to the transports.

Under the assumption that the spec is indeed going to be fixed:

Reviewed-by: Halil Pasic <[e-mail]>

Regards,
Halil

Michael S. Tsirkin April 4, 2025, 5:33 a.m. UTC | #10

On Fri, Apr 04, 2025 at 06:02:04AM +0200, Halil Pasic wrote:
> On Thu, 3 Apr 2025 10:35:33 -0400
> "Michael S. Tsirkin" <[e-mail]> wrote:
> 
> > On Thu, Apr 03, 2025 at 04:18:36PM +0200, Halil Pasic wrote:
> > > On Wed,  2 Apr 2025 22:36:21 +0200
> [..]
> > > 
> > > 5.5.2 Virtqueues
> > > 
> > > 0
> > >     inflateq 
> > > 1
> > >     deflateq 
> > > 2
> > >     statsq 
> > > 3
> > >     free_page_vq 
> > > 4
> > >     reporting_vq  
> > 
> > Indeed. Unfortunately, no one at all implemented this properly as
> > per spec :(.
> > 
> > Balloon is the worst offender here but other devices are broken
> > too in some configurations.
> > 
> > 
> > Given it has been like this for many years I'm inclined in this
> > instance to fix the spec not the implementations.
> > 
> 
> I understand! For me at least knowing if we are going to change the
> spec or the implementations is pretty important. It would probably
> make sense to spin a patch for the spec, just for the unlikely case that
> somebody did end up implementing this by the spec and wants to protest.
> 
> I think, a consequence of this design is that all queues need to be
> created and allocated at initialization time.

Why? after feature negotiation.

> I mean in the spec we have something like 
> """
> 5.1.6.5.6.1 Driver Requirements: Automatic receive steering in multiqueue mode
> The driver MUST configure the virtqueues before enabling them with the VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET command. 
> """
> 
> For example one could want to hotplug 2 more vCPUs and still have a
> queue-pair per cpu (and a controlq). Those 2 extra queue-pairs could
> in theory be allocated on-demand instead of having to allocate memory
> for the rings for all the queues corresponding to the maxed out setup.
> I've had a look at the Linux virtio-net and it does allocate all the
> queues up front.
> 
> Also with this design, I believe we would effectively prohibit "holes".

For existing devices at least.

> Now if holes are prohibited, IMHO it does not make a whole lot of
> sense for the virtio_find_vqs() to support holes. Because the holes
> and a fair amount of complexity. Of course that would make sense as a
> cleanup on top.
> 
> Regards,
> Halil

Patch

diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index 21fa7ac849e5c..4904b831c0a75 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -302,11 +302,17 @@  static struct airq_info *new_airq_info(int index)
 static unsigned long *get_airq_indicator(struct virtqueue *vqs[], int nvqs,
 					 u64 *first, void **airq_info)
 {
-	int i, j;
+	int i, j, queue_idx, highest_queue_idx = -1;
 	struct airq_info *info;
 	unsigned long *indicator_addr = NULL;
 	unsigned long bit, flags;
 
+	/* Array entries without an actual queue pointer must be ignored. */
+	for (i = 0; i < nvqs; i++) {
+		if (vqs[i])
+			highest_queue_idx++;
+	}
+
 	for (i = 0; i < MAX_AIRQ_AREAS && !indicator_addr; i++) {
 		mutex_lock(&airq_areas_lock);
 		if (!airq_areas[i])
@@ -316,7 +322,7 @@  static unsigned long *get_airq_indicator(struct virtqueue *vqs[], int nvqs,
 		if (!info)
 			return NULL;
 		write_lock_irqsave(&info->lock, flags);
-		bit = airq_iv_alloc(info->aiv, nvqs);
+		bit = airq_iv_alloc(info->aiv, highest_queue_idx + 1);
 		if (bit == -1UL) {
 			/* Not enough vacancies. */
 			write_unlock_irqrestore(&info->lock, flags);
@@ -325,8 +331,10 @@  static unsigned long *get_airq_indicator(struct virtqueue *vqs[], int nvqs,
 		*first = bit;
 		*airq_info = info;
 		indicator_addr = info->aiv->vector;
-		for (j = 0; j < nvqs; j++) {
-			airq_iv_set_ptr(info->aiv, bit + j,
+		for (j = 0, queue_idx = 0; j < nvqs; j++) {
+			if (!vqs[j])
+				continue;
+			airq_iv_set_ptr(info->aiv, bit + queue_idx++,
 					(unsigned long)vqs[j]);
 		}
 		write_unlock_irqrestore(&info->lock, flags);