[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <88d8f2d2-7b8a-458f-8fc4-c31964996817@redhat.com>
Date: Mon, 7 Apr 2025 15:13:28 +0200
From: David Hildenbrand <david@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Halil Pasic <pasic@...ux.ibm.com>, linux-kernel@...r.kernel.org,
linux-s390@...r.kernel.org, virtualization@...ts.linux.dev,
kvm@...r.kernel.org, Chandra Merla <cmerla@...hat.com>,
Stable@...r.kernel.org, Cornelia Huck <cohuck@...hat.com>,
Thomas Huth <thuth@...hat.com>, Eric Farman <farman@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>, Vasily Gorbik <gor@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Sven Schnelle <svens@...ux.ibm.com>, Wei Wang <wei.w.wang@...el.com>,
Daniel Verkamp <dverkamp@...omium.org>
Subject: Re: [PATCH v1] s390/virtio_ccw: don't allocate/assign airqs for
non-existing queues
On 07.04.25 11:13, David Hildenbrand wrote:
> On 07.04.25 11:11, David Hildenbrand wrote:
>> On 07.04.25 10:58, Michael S. Tsirkin wrote:
>>> On Mon, Apr 07, 2025 at 10:54:00AM +0200, David Hildenbrand wrote:
>>>> On 07.04.25 10:49, Michael S. Tsirkin wrote:
>>>>> On Mon, Apr 07, 2025 at 10:44:21AM +0200, David Hildenbrand wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Whoever adds new feat_X *must be aware* about all previous features,
>>>>>>>> otherwise we'd be reusing feature bits and everything falls to pieces.
>>>>>>>
>>>>>>>
>>>>>>> The knowledge is supposed be limited to which feature bit to use.
>>>>>>
>>>>>> I think we also have to know which virtqueue bits can be used, right?
>>>>>>
>>>>>
>>>>> what are virtqueue bits? vq number?
>>>>
>>>> Yes, sorry.
>>>
>>> I got confused myself, it's vq index actually now, we made the spec
>>> consistent with that terminology. used to be number/index
>>> interchangeably.
>>>
>>>> Assume cross-vm as an example. It would make use of virtqueue indexes 5+6
>>>> with their VIRTIO_BALLOON_F_WS_REPORTING.
>>>
>>>
>>> crossvm guys really should have reserved the feature bit even if they
>>> did not bother specifying it. Let's reserve it now at least?
>>
>> Along with the virtqueue indices, right?
>>
>> Note that there was
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02503.html
>>
>> and
>>
>> https://groups.oasis-open.org/communities/community-home/digestviewer/viewthread?GroupId=3973&MessageKey=afb07613-f56c-4d40-8981-2fad1c723998&CommunityKey=2f26be99-3aa1-48f6-93a5-018dce262226&hlmlt=VT
>>
>> But it only was RFC, and as the QEMU implementation didn't materialize,
>> nobody seemed to care ...
>
> Heh, but that one said:
>
> +\item[ VIRTIO_BALLOON_F_WS_REPORTING(6) ] The device has support for
> Working Set
>
> Which does not seem to reflect reality ...
>
I dug a bit more into cross-vm, because that one seems to be the only
one out there that does not behave like everybody else I found (maybe good,
maybe bad :) ).
1) There was temporarily even another feature (VIRTIO_BALLOON_F_EVENTS_VQ)
and another queue.
It got removed from cross-vm in:
commit 9ba634b82b55ba762dc8724676b2cf9419460145
Author: Daniel Verkamp <dverkamp@...omium.org>
Date: Thu Jul 11 11:29:52 2024 -0700
devices: virtio-balloon: remove event queue support
VIRTIO_BALLOON_F_EVENTS_VQ was part of a proposed virtio spec change.
It is not currently supported by upstream Linux, so removing this should
have no effect except for guest kernels that had CHROMIUM patches
applied.
The virtqueue indexes for the ws-related queues are decremented to fill
the hole left by the removal of the event VQ; these are non-standard as
well, so they do not have virtqueue indexes assigned in the virtio spec,
but the proposed spec extension did actually use vq indexes 5 and 6.
BUG=b:214864326
2) cross-vm is aware of the upstream Linux driver
They thought your fix would go upstream; it didn't.
commit a2fa119e759d0238a42ff15a9aff0dfd122afebd
Author: Daniel Verkamp <dverkamp@...omium.org>
Date: Wed Jul 10 16:16:28 2024 -0700
devices: virtio-balloon: warn about queue index mismatches
The Linux kernel virtio-balloon driver spec non-compliance related to
queue numbering is being fixed; add some diagnostics to our device that
help to check if everything is working as expected.
<https://lore.kernel.org/virtualization/CACGkMEsg0+vpav1Fo8JF1isq4Ef8t4_CFN1scyztDO8bXzRLBQ@mail.gmail.com/T/>
Additionally, replace the num_expected_queues() function with per-queue
checking to avoid the need for the duplicate feature checks and queue
count calculation; each pop_queue() call will be checked using the `?`
operator and return a more useful error message if a particular queue is
missing.
BUG=None
TEST=crosvm run --balloon-page-reporting ...
IIRC, in that commit they switched to the "spec" behavior.
That's when they started hard-coding the queue indexes.
CCing Daniel. All Linux versions should be incompatible with cross-vmm regarding free page reporting.
How is that handled?
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists