Re: [PATCH vhost v4 1/6] virtio_balloon: remove the dependence where names[] is null

[David Hildenbrand <david@redhat.com>]

On 22.03.24 20:16, Daniel Verkamp wrote:
> On Thu, Mar 21, 2024 at 3:16 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>>
>> Currently, the init_vqs function within the virtio_balloon driver relies
>> on the condition that certain names array entries are null in order to
>> skip the initialization of some virtual queues (vqs). This behavior is
>> unique to this part of the codebase. In an upcoming commit, we plan to
>> eliminate this dependency by removing the function entirely. Therefore,
>> with this change, we are ensuring that the virtio_balloon no longer
>> depends on the aforementioned function.
> 
> This is a behavior change, and I believe means that the driver no
> longer follows the spec [1].
> 
> For example, the spec says that virtqueue 4 is reporting_vq, and
> reporting_vq only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set,
> but there is no mention of its virtqueue number changing if other
> features are not set. If a device/driver combination negotiates
> VIRTIO_BALLOON_F_PAGE_REPORTING but not VIRTIO_BALLOON_F_STATS_VQ or
> VIRTIO_BALLOON_F_FREE_PAGE_HINT, my reading of the specification is
> that reporting_vq should still be vq number 4, and vq 2 and 3 should
> be unused. This patch would make the reporting_vq use vq 2 instead in
> this case.
> 
> If the new behavior is truly intended, then the spec does not match
> reality, and it would need to be changed first (IMO); however,
> changing the spec would mean that any devices implemented correctly
> per the previous spec would now be wrong, so some kind of mechanism
> for detecting the new behavior would be warranted, e.g. a new
> non-device-specific virtio feature flag.
> 
> I have brought this up previously on the virtio-comment list [2], but
> it did not receive any satisfying answers at that time.

Rings a bell, but staring at this patch, I thought that there would be
no behavioral change. Maybe I missed it :/

I stared at virtio_ccw_find_vqs(), and it contains:

	for (i = 0; i < nvqs; ++i) {
		if (!names[i]) {
			vqs[i] = NULL;
			continue;
		}

		vqs[i] = virtio_ccw_setup_vq(vdev, queue_idx++, callbacks[i],
					     names[i], ctx ? ctx[i] : false,
					     ccw);
		if (IS_ERR(vqs[i])) {
			ret = PTR_ERR(vqs[i]);
			vqs[i] = NULL;
			goto out;
		}
	}

We increment queue_idx only if an entry was not NULL. SO I thought no
behavioral change? (at least on s390x :) )

It's late here in Germany, so maybe I'm missing something.

-- 
Cheers,

David / dhildenb