On Fri, Jul 18, 2025 at 10:52:30AM +0200, Paolo Abeni wrote:
If the driver uses any of the extended features (i.e. 64 or above),
store the extended features range (64-127 bits).

At load time, let legacy features initialize the full features range
and pass it to the set helper; sub-states loading will have filled-up
the extended part as needed.

This is one of the few spots that need explicitly to know and set
in stone the extended features array size; add a build bug to prevent
breaking the migration should such size change again in the future:
more serialization plumbing will be needed.

Signed-off-by: Paolo Abeni <pab...@redhat.com>
---
v2 -> v3:
- 128bit_features state load/stores only the high bits
- consolidate the load implementation to use a single set
  helper for 128/64/32 bits features
- _array -> _ex

v1 -> v2:
- uint128_t -> u64[2]
---
hw/virtio/virtio.c | 88 ++++++++++++++++++++++++++++++----------------
1 file changed, 57 insertions(+), 31 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 2ab1d20769..2817d3a893 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2954,6 +2954,24 @@ static const VMStateDescription vmstate_virtio_disabled 
= {
    }
};

+static bool virtio_128bit_features_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    return virtio_features_use_extended(vdev->host_features_ex);

Related to live-migration, IIUC if we disable the new features in the 10.0 machine type, this should allow the migration (from a new qemu using the old machine time to an old qemu not supporting extended features), since those features should not be set in vdev->host_features_ex, and this will return `false`.

Right?

Thanks,
Stefano

+}
+
+static const VMStateDescription vmstate_virtio_128bit_features = {
+    .name = "virtio/128bit_features",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_128bit_features_needed,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT64(guest_features_ex[1], VirtIODevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
static const VMStateDescription vmstate_virtio = {
    .name = "virtio",
    .version_id = 1,
@@ -2963,6 +2981,7 @@ static const VMStateDescription vmstate_virtio = {
    },
    .subsections = (const VMStateDescription * const []) {
        &vmstate_virtio_device_endian,
+        &vmstate_virtio_128bit_features,
        &vmstate_virtio_64bit_features,
        &vmstate_virtio_virtqueues,
        &vmstate_virtio_ringsize,
@@ -3059,23 +3078,28 @@ const VMStateInfo  virtio_vmstate_info = {
    .put = virtio_device_put,
};

-static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
+static int virtio_set_features_nocheck(VirtIODevice *vdev, const uint64_t *val)
{
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
-    bool bad = (val & ~(vdev->host_features)) != 0;
+    uint64_t tmp[VIRTIO_FEATURES_DWORDS];
+    bool bad;
+
+    bad = virtio_features_andnot(tmp, val, vdev->host_features_ex);
+    virtio_features_and(tmp, val, vdev->host_features_ex);

-    val &= vdev->host_features;
    if (k->set_features) {
-        k->set_features(vdev, val);
+        bad = bad || virtio_features_use_extended(tmp);
+        k->set_features(vdev, tmp[0]);
    }
-    vdev->guest_features = val;
+
+    virtio_features_copy(vdev->guest_features_ex, tmp);
    return bad ? -1 : 0;
}

typedef struct VirtioSetFeaturesNocheckData {
    Coroutine *co;
    VirtIODevice *vdev;
-    uint64_t val;
+    uint64_t val[VIRTIO_FEATURES_DWORDS];
    int ret;
} VirtioSetFeaturesNocheckData;

@@ -3088,14 +3112,15 @@ static void virtio_set_features_nocheck_bh(void *opaque)
}

static int coroutine_mixed_fn
-virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, uint64_t val)
+virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev,
+                                     const uint64_t *val)
{
    if (qemu_in_coroutine()) {
        VirtioSetFeaturesNocheckData data = {
            .co = qemu_coroutine_self(),
            .vdev = vdev,
-            .val = val,
        };
+        virtio_features_copy(data.val, val);
        aio_bh_schedule_oneshot(qemu_get_current_aio_context(),
                                virtio_set_features_nocheck_bh, &data);
        qemu_coroutine_yield();
@@ -3107,6 +3132,7 @@ virtio_set_features_nocheck_maybe_co(VirtIODevice *vdev, 
uint64_t val)

int virtio_set_features(VirtIODevice *vdev, uint64_t val)
{
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int ret;
    /*
     * The driver must not attempt to set features after feature negotiation
@@ -3122,7 +3148,8 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
                      __func__, vdev->name);
    }

-    ret = virtio_set_features_nocheck(vdev, val);
+    virtio_features_from_u64(features, val);
+    ret = virtio_set_features_nocheck(vdev, features);
    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
        /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
        int i;
@@ -3145,6 +3172,7 @@ void virtio_reset(void *opaque)
{
    VirtIODevice *vdev = opaque;
    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
+    uint64_t features[VIRTIO_FEATURES_DWORDS];
    int i;

    virtio_set_status(vdev, 0);
@@ -3171,7 +3199,8 @@ void virtio_reset(void *opaque)
    vdev->start_on_kick = false;
    vdev->started = false;
    vdev->broken = false;
-    virtio_set_features_nocheck(vdev, 0);
+    virtio_features_clear(features);
+    virtio_set_features_nocheck(vdev, features);
    vdev->queue_sel = 0;
    vdev->status = 0;
    vdev->disabled = false;
@@ -3254,7 +3283,7 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
     * Note: devices should always test host features in future - don't create
     * new dependencies like this.
     */
-    vdev->guest_features = features;
+    virtio_features_from_u64(vdev->guest_features_ex, features);

    config_len = qemu_get_be32(f);

@@ -3333,26 +3362,23 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
version_id)
        vdev->device_endian = virtio_default_endian();
    }

-    if (virtio_64bit_features_needed(vdev)) {
-        /*
-         * Subsection load filled vdev->guest_features.  Run them
-         * through virtio_set_features to sanity-check them against
-         * host_features.
-         */
-        uint64_t features64 = vdev->guest_features;
-        if (virtio_set_features_nocheck_maybe_co(vdev, features64) < 0) {
-            error_report("Features 0x%" PRIx64 " unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features64, vdev->host_features);
-            return -1;
-        }
-    } else {
-        if (virtio_set_features_nocheck_maybe_co(vdev, features) < 0) {
-            error_report("Features 0x%x unsupported. "
-                         "Allowed features: 0x%" PRIx64,
-                         features, vdev->host_features);
-            return -1;
-        }
+    /*
+     * Avoid silently breaking migration should the feature space increase
+     * even more in the (far away) future
+     */
+    QEMU_BUILD_BUG_ON(VIRTIO_FEATURES_DWORDS != 2);
+
+    /*
+     * guest_features_ex is fully initialized with u32 features and upper
+     * bits have been filled as needed by the later load.
+     */
+    if (virtio_set_features_nocheck_maybe_co(vdev,
+                                             vdev->guest_features_ex) < 0) {
+        error_report("Features 0x" VIRTIO_FEATURES_FMT " unsupported. "
+                     "Allowed features: 0x" VIRTIO_FEATURES_FMT,
+                     VIRTIO_FEATURES_PR(vdev->guest_features_ex),
+                     VIRTIO_FEATURES_PR(vdev->host_features_ex));
+        return -1;
    }

    if (!virtio_device_started(vdev, vdev->status) &&
--
2.50.0



Reply via email to