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

[David Hildenbrand]

On 04.04.25 18:49, David Hildenbrand wrote:
> On 04.04.25 17:39, Halil Pasic wrote:
> > On Fri, 4 Apr 2025 16:17:14 +0200
> > David Hildenbrand <david@xxxxxxxxxx> wrote:
> >
> > > > 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().
> >
> > Careful, that is a *driver* offered and not a *device* offered!
>
> Right, I was pointing at the usage of the term "offered".
> virtio_check_driver_offered_feature(). (but was also confused about that
> function)
>
> virtio_has_feature() is clearer: "helper to determine if this device has
> this feature."
>
> The way it's currently implemented is that it's essentially "device has
> this feature and we know about it (->feature_table)"
>
> > We basically mandate that one can only check for a feature F with
> > virtio_has_feature() such that it is either in drv->feature_table or in
> > drv->feature_table_legacy.
> >
> > AFAICT *device_features* obtained via dev->config->get_features(dev)
> > isn't even saved but is only used for binary and-ing it with the
> > driver_features to obtain the negotiated features.
> >
> > That basically means that if I was, for the sake of fun do
> >
> > --- a/drivers/virtio/virtio_balloon.c
> > +++ b/drivers/virtio/virtio_balloon.c
> > @@ -1197,7 +1197,6 @@ static unsigned int features[] = {
> >           VIRTIO_BALLOON_F_MUST_TELL_HOST,
> >           VIRTIO_BALLOON_F_STATS_VQ,
> >           VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> > -       VIRTIO_BALLOON_F_FREE_PAGE_HINT,
> >           VIRTIO_BALLOON_F_PAGE_POISON,
> >           VIRTIO_BALLOON_F_REPORTING,
> >    };
> >
> > I would end up with virtio_check_driver_offered_feature() calling
> > BUG().
>
> Right.
>
> > That basically means that Linux mandates implementing all previous
> > features regardless whether does are supposed to be optional ones or
> > not. Namely if you put the feature into drv->feature_table it will
> > get negotiated.
> >
> > Which is not nice IMHO.
>
> I think the validate() callbacks allows for fixing that up.
>
> Like us unconditionally clearing VIRTIO_F_ACCESS_PLATFORM (I know,
> that's a transport feature and a bit different for this reason).
>
> ... not that I think the current way of achieving that is nice :)

Thinking again, that won't work, because it would also make 
virtio_has_feature() say that the device does not have that feature.

So yeah, virtio_has_feature() is confusing and the documentation does 
not quite match.

Would need a change/cleanup to handle such features that we don't 
implement but still want to check if they are offered.

--
Cheers,

David / dhildenb