On Mon, Mar 25, 2024 at 5:44 PM Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
>
> On Mon, Mar 25 2024, Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> > On Fri, 22 Mar 2024 22:02:27 +0100, David Hildenbrand <david@xxxxxxxxxx> wrote:
> >> On 22.03.24 20:16, Daniel Verkamp wrote:
> >> > On Thu, Mar 21, 2024 at 3:16 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> 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.
>
> I had missed it back then, but now that I read it, I realize that we
> really have a bit of a mess here :/
>
> >>
> >> 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 :) )
>
> The code for pci behaves in the same way.
>
> >>
> >> It's late here in Germany, so maybe I'm missing something.
> >
> > I think we've encountered a tricky issue. Currently, all transports handle queue
> > id by incrementing them in order, without skipping any queue id. So, I'm quite
> > surprised that my changes would affect the spec. The fact that the
> > 'names' value is null is just a small trick in the Linux kernel implementation
> > and should not have an impact on the queue id.
> >
> > I believe that my recent modification will not affect the spec. So, let's
> > consider the issues with this patch set separately for now. Regarding the Memory
> > Balloon Device, it has been operational for many years, and perhaps we should
> > add to the spec that if a certain vq does not exist, then subsequent vqs will
> > take over its id.
>
> The changes here do not really seem to affect the spec issue that Daniel
> had noted, unless I'm reading the code wrong.
Spec seems to be wrong here:
5.5.2 Virtqueues
0 inflateq 1 deflateq 2 statsq 3 free_page_vq4 r eporting_vq
And this is the Qemu implementation:
5.5.2 Virtqueues
s->ivq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
s->dvq = virtio_add_queue(vdev, 128, virtio_balloon_handle_output);
s->svq = virtio_add_queue(vdev, 128, virtio_balloon_receive_stats);
if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) {
s->free_page_vq = virtio_add_queue(vdev, VIRTQUEUE_MAX_SIZE,
virtio_balloon_handle_free_page_vq);
precopy_add_notifier(&s->free_page_hint_notify);
object_ref(OBJECT(s->iothread));
s->free_page_bh =
aio_bh_new_guarded(iothread_get_aio_context(s->iothread),
virtio_ballloon_get_free_page_hints, s,
&dev->mem_reentrancy_guard);
}
if (virtio_has_feature(s->host_features, VIRTIO_BALLOON_F_REPORTING)) {
s->reporting_vq = virtio_add_queue(vdev, 32,
virtio_balloon_handle_report);
}
We need to fix it.
>
> However, we should try to address the spec mess, where we have at least
> some of the most popular/important implementations behaving differently
> than the spec describes... I would suggest to discuss that on the virtio
> lists -- but they are still dead, and at this point I'm just hoping
> they'll come back eventually :/
>
Thanks
