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

[David Hildenbrand <david@redhat.com>]

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@redhat.com> 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