On 6/12/25 2:50 AM, Jason Wang wrote:

On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni [off-list ref] wrote:

quoted

@@ -272,22 +272,22 @@ static int virtio_dev_probe(struct device *_d)
        int err, i;
        struct virtio_device *dev = dev_to_virtio(_d);
        struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
-       u64 device_features;
-       u64 driver_features;
+       u64 device_features[VIRTIO_FEATURES_DWORDS];
+       u64 driver_features[VIRTIO_FEATURES_DWORDS];
        u64 driver_features_legacy;

        /* We have a driver! */
        virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);

        /* Figure out what features the device supports. */
-       device_features = dev->config->get_features(dev);
+       virtio_get_features(dev, device_features);

        /* Figure out what features the driver supports. */
-       driver_features = 0;
+       virtio_features_zero(driver_features);
        for (i = 0; i < drv->feature_table_size; i++) {
                unsigned int f = drv->feature_table[i];
-               BUG_ON(f >= 64);
-               driver_features |= (1ULL << f);
+               BUG_ON(f >= VIRTIO_FEATURES_MAX);
+               virtio_features_set_bit(driver_features, f);

Instead of doing BUG_ON here, could we just stop at 128 bits?

I think it would be nice to have a sanity check to ensure the driver
code is sync with the core. What about a WARN_ON_ONCE?

quoted

@@ -121,6 +124,8 @@ struct virtio_config_ops {
        void (*del_vqs)(struct virtio_device *);
        void (*synchronize_cbs)(struct virtio_device *);
        u64 (*get_features)(struct virtio_device *vdev);
+       void (*get_extended_features)(struct virtio_device *vdev,
+                                     u64 *features);

I think it would be better to add a size to simplify the future extension.

Note that the array size is implied by the virtio-features definition.
Future extensions will be obtained by increasing the
VIRTIO_FEATURES_DWORD define, with no other change to the code.

I think a length here would be redundant.

quoted

+static inline bool virtio_features_equal(const u64 *f1, const u64 *f2)
+{
+       u64 diff = 0;
+       int i;
+
+       for (i = 0; i < VIRTIO_FEATURES_DWORDS; ++i)
+               diff |= f1[i] ^ f2[i];
+       return !!diff;

Nit: we can return false early here.

I can do in in the next revision.

[same disclaimer here: I'm traveling for the whole week, my replies will
be rare and delayed]

/P