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

[Halil Pasic <pasic@linux.ibm.com>]

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! 

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().

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.

> 
> > 
> > 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.

I don't agree with the comparison.  One obviously needs to avoid fatal
collisions when extending the spec, and has to consider prior art.

But ideally not implemented  or fenced optional features A should have no
impact to implemented optional or not optional features B -- unless the
features are actually interdependent, but then the spec would prohibit
the combo of having B but not A. And IMHO exactly this would have been
the advantage of fixed-assigned numbers: you may not care if the other
queueues exist or not. 

Also like cloud-hypervisor has decided that they are going only to
support VIRTIO_BALLOON_F_REPORTING some weird OS could in theory
decide that they only care about VIRTIO_BALLOON_F_REPORTING. In that
setting having to look at VIRTIO_BALLOON_F_STATS_VQ and 
VIRTIO_BALLOON_F_FREE_PAGE_HINT are offered is weird. But that is all water
under the bridge. We have to respect what is out there in the field.

> 
> Not perfect, but AFAIKS, not horrible.

It is like it is. QEMU does queue exist if the corresponding feature
is offered by the device, and that is what we have to live with.

Yes, I agree we should make the spec reflect reality!

> 
> (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)

As I've tried to explain above, not implementing/accepting optional
features and then implementing/accepting a newer feature is problematic
with the current code. Supporting some features would work only as
supporting all features up to X.

Regards,
Halil