lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4450ec71-8a8f-478c-a66e-b53d858beb02@redhat.com>
Date: Mon, 7 Apr 2025 09:18:21 +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>
Subject: Re: [PATCH v1] s390/virtio_ccw: don't allocate/assign airqs for
 non-existing queues

On 06.04.25 20:42, Michael S. Tsirkin wrote:
> On Fri, Apr 04, 2025 at 03:48:49PM +0200, David Hildenbrand wrote:
>> On 04.04.25 15:36, Halil Pasic wrote:
>>> On Fri, 4 Apr 2025 12:55:09 +0200
>>> David Hildenbrand <david@...hat.com> wrote:
>>>
>>>> For virito-balloon, we should probably do the following:
>>>>
>>>>    From 38e340c2bb53c2a7cc7c675f5dfdd44ecf7701d9 Mon Sep 17 00:00:00 2001
>>>> From: David Hildenbrand <david@...hat.com>
>>>> Date: Fri, 4 Apr 2025 12:53:16 +0200
>>>> Subject: [PATCH] virtio-balloon: Fix queue index assignment for
>>>>     non-existing queues
>>>>
>>>> Signed-off-by: David Hildenbrand <david@...hat.com>
>>>> ---
>>>>     device-types/balloon/description.tex | 22 ++++++++++++++++------
>>>>     1 file changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/device-types/balloon/description.tex b/device-types/balloon/description.tex
>>>> index a1d9603..a7396ff 100644
>>>> --- a/device-types/balloon/description.tex
>>>> +++ b/device-types/balloon/description.tex
>>>> @@ -16,6 +16,21 @@ \subsection{Device ID}\label{sec:Device Types / Memory Balloon Device / Device I
>>>>       5
>>>>     \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtqueues}
>>>> +
>>>> +\begin{description}
>>>> +\item[inflateq] Exists unconditionally.
>>>> +\item[deflateq] Exists unconditionally.
>>>> +\item[statsq] Only exists if VIRTIO_BALLOON_F_STATS_VQ is set.
>>>> +\item[free_page_vq] Only exists if VIRTIO_BALLOON_F_FREE_PAGE_HINT is set.
>>>> +\item[reporting_vq] Only exists if VIRTIO_BALLOON_F_PAGE_REPORTING is set.
>>>
>>> s/is set/is negotiated/?
>>>
>>> I think we should stick to "feature is offered" and "feature is
>>> negotiated".
>>>
>>>> +\end{description}
>>>> +
>>>> +\begin{note}
>>>> +Virtqueue indexes are assigned sequentially for existing queues, starting
>>>> +with index 0; consequently, if a virtqueue does not exist, it does not get
>>>> +an index assigned. Assuming all virtqueues exist for a device, the indexes
>>>> +are:
>>>> +
>>>>     \begin{description}
>>>>     \item[0] inflateq
>>>>     \item[1] deflateq
>>>> @@ -23,12 +38,7 @@ \subsection{Virtqueues}\label{sec:Device Types / Memory Balloon Device / Virtque
>>>>     \item[3] free_page_vq
>>>>     \item[4] reporting_vq
>>>>     \end{description}
>>>> -
>>>> -  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.
>>>> +\end{note}
>>>>     \subsection{Feature bits}\label{sec:Device Types / Memory Balloon Device / Feature bits}
>>>>     \begin{description}
>>>
>>> Sounds good to me! But I'm still a little confused by the "holes". What
>>> confuses me is that i can think of at least 2 distinct types of "holes":
>>> 1) Holes that can be filled later. The queue conceptually exists, but
>>>      there is no need to back it with any resources for now because it is
>>>      dormant (it can be seen a hole in comparison to queues that need to
>>>     materialize -- vring, notifiers, ...)
>>> 2) Holes that can not be filled without resetting the device: i.e. if
>>>      certain features are not negotiated, then a queue X does not exist,
>>>      but subsequent queues retain their index.
>>
>> I think it is not about "negotiated", that might be the wrong terminology.
>>
>> E.g., in QEMU virtio_balloon_device_realize() we define the virtqueues
>> (virtio_add_queue()) if virtio_has_feature(s->host_features).
>>
>> That is, it's independent of a feature negotiation (IIUC), it's static for
>> the device --  "host_features"
> 
> 
> No no that is a bad idea. Breaks forward compatibility.
> 
> Oh my. I did not realize. It is really broken hopelessly.
> 
> Because, note, the guest looks at the guest features :)

Can you elaborate why?

statsq = 2

free_page_vq = statsq + host_offered_feat(VIRTIO_BALLOON_F_STATS_VQ)

reporting_vq = free_page_vq + 
host_offered_feat(VIRTIO_BALLOON_F_FREE_PAGE_HINT)


Independent of any upcoming features. And if a new feature defines a new 
virtqueue

new_vq = reporting_vq +  host_offered_feat(VIRTIO_BALLOON_F_PAGE_REPORTING)

We only have to make sure in the spec that these calculations always hold.

Querying of the host offered features already happens as part of 
determining the actual guest usable feature (driver_offered & host_offered).


> Now I am beginning to think we should leave the spec alone
> and fix the drivers ... Ugh ....

We could always say that starting with feature X, queue indexes are 
fixed again. E.g., VIRTIO_BALLOON_F_X would have it's virtqueue fixed at 
index 5, independent of the other (older) features where the virtqueue 
indexes are determined like today.

Won't make the implementation easier, though, I'm afraid.

(I also thought about a way to query the virtqueue index for a feature, 
but that's probably overengineering)

-- 
Cheers,

David / dhildenb


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ