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.