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

[David Hildenbrand]

On 04.04.25 16:00, Halil Pasic wrote:
> On Fri, 4 Apr 2025 15:48:49 +0200
> David Hildenbrand <david@xxxxxxxxxx> wrote:
>
> > > 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"
> >
> >
> > Is that really "negotiated" or is it "the device offers the feature X"
> > ?
>
> It is offered. And this is precisely why I'm so keen on having a precise
> wording here.

Yes, me too. The current phrasing in the spec is not clear.

Linux similarly checks 
virtio_has_feature()->virtio_check_driver_offered_feature().

> Usually for compatibility one needs negotiated. Because the feature
> negotiation is mostly about compatibility. I.e. the driver should be
> able to say, hey I don't know about that feature, and get compatible
> behavior. If for example VIRTIO_BALLOON_F_FREE_PAGE_HINT and
> VIRTIO_BALLOON_F_PAGE_REPORTING are both offered but only
> VIRTIO_BALLOON_F_PAGE_REPORTING is negotiated. That would make reporting_vq
> jump to +1 compared to the case where VIRTIO_BALLOON_F_FREE_PAGE_HINT is
> not offered. Which is IMHO no good, because for the features that the
> driver is going to reject in most of the cases it should not matter if
> it was offered or not.

Yes. The key part is that we may only add new features to the tail of 
our feature list; maybe we should document that as well.

I agree that a driver that implements VIRTIO_BALLOON_F_PAGE_REPORTING 
*must* be aware that VIRTIO_BALLOON_F_FREE_PAGE_HINT exists. So queue 
existence is not about feature negotiation but about features being 
offered from the device.

... which is a bit the same behavior as with fixed-assigned numbers a 
bit. VIRTIO_BALLOON_F_PAGE_REPORTING was documented as "4" because 
VIRTIO_BALLOON_F_FREE_PAGE_HINT was documented to be "3" -- IOW, it 
already existed in the spec.

Not perfect, but AFAIKS, not horrible.

(as Linux supports all these features, it's easy. A driver that only 
supports some features has to calculate the queue index manually based 
on the offered features)

> @MST: Please correct me if I'm wrong!
>
> Regards,
> Halil


--
Cheers,

David / dhildenb