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

[David Hildenbrand]

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@xxxxxxxxxx> wrote:
> > >
> > > > For virito-balloon, we should probably do the following:
> > > >
> > > >    From 38e340c2bb53c2a7cc7c675f5dfdd44ecf7701d9 Mon Sep 17 00:00:00 2001
> > > > From: David Hildenbrand <david@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >     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