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 <da...@redhat.com> wrote:
> >
> > > For virito-balloon, we should probably do the following:
> > >
> > > From 38e340c2bb53c2a7cc7c675f5dfdd44ecf7701d9 Mon Sep 17 00:00:00 2001
> > > From: David Hildenbrand <da...@redhat.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 <da...@redhat.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 :)
Now I am beginning to think we should leave the spec alone
and fix the drivers ... Ugh ....
>
> Is that really "negotiated" or is it "the device offers the feature X" ?
>
> --
> Cheers,
>
> David / dhildenb
